Bug 43134

Created attachment 62864 [details] Patch (depends on 43130)

Attachment 62864 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3559627

Comment on attachment 62864 [details] Patch (depends on 43130) i think you might need to modify JSValueToNative in CodeGeneratorV8.pm to make the patch build in chromium. for [Callback] parameters, i think you would want to return "$value". WebCore/bindings/scripts/CodeGenerator.pm:363 + $ret =~ s/^eXCLUSIVE/exclusiveFlag/ if $ret =~ /^eXCLUSIVE$/; i think isCreate() or isCreateFlag() would be a better name for a boolean getter. same for exclusiveFlag(). also, you should add a test for these cases to WebCore/bindings/scripts/test/TestObj.idl, and change the expected outputs in WebCore/bindings/scripts/test/{JS|V8|...}/. Please make sure WebKitTools/Scripts/run-bindings-tests passes. WebCore/storage/DOMFileSystem.cpp:35 + #include "DOMFileSystem.h" i believe the common pattern is: #include "config.h" #include "DOMFileSystem.h" #if ENABLE(FILE_SYSTEM) WebCore/storage/DOMFileSystem.cpp:38 + #include "EntryCallback.h" looks like this include is not yet needed. WebCore/storage/DOMFileSystem.cpp:39 + #include "ErrorCallback.h" ditto. WebCore/storage/DOMFileSystem.h:44 + class ErrorCallback; these 2 classes don't need to be forward-declared just yet. WebCore/storage/DOMFileSystem.h:45 + class ScriptExecutionContext; ditto. WebCore/storage/Entry.cpp:34 + #include "Entry.h" same comment about moving this outside #if ENABLE(FILE_SYSTEM) WebCore/storage/Entry.cpp:50 + m_name = fullPath.substring(index); should there be a "else m_name = fullPath;" here? WebCore/storage/Entry.h:34 + #include "DOMFileSystem.h" either remove this include, or the forward-declaration below. WebCore/storage/ErrorCallback.h:43 + class ErrorCallback : public ThreadSafeShared<ErrorCallback> { this callback is ThreadSafeShared, and all other callbacks are RefCounted. is this intended? WebCore/storage/Flags.h:50 + void setEXCLUSIVE(bool exclusive) { m_exclusive = exclusive; } you should modify WK_ucfirst in CodeGenerator.pm the same way you modified WK_lcfirst, and convert setCREATE() and setEXCLUSIVE() to setCreate() and setExclusive(). WebCore/storage/Metadata.h:53 + double m_modificationTime; empty line before this field.

Created attachment 62914 [details] Patch

Thanks for your careful review! I fixed the issues you pointed out. (In reply to comment #3) > (From update of attachment 62864 [details]) > i think you might need to modify JSValueToNative in CodeGeneratorV8.pm to make the patch build in chromium. for [Callback] parameters, i think you would want to return "$value". I have another proposing patch (issue 43130 - I've cc'ed you) that hopefully fixes this. (I'll look for alternative solution if it's not the right solution.) > WebCore/bindings/scripts/CodeGenerator.pm:363 > + $ret =~ s/^eXCLUSIVE/exclusiveFlag/ if $ret =~ /^eXCLUSIVE$/; > i think isCreate() or isCreateFlag() would be a better name for a boolean getter. same for exclusiveFlag(). also, you should add a test for these cases to WebCore/bindings/scripts/test/TestObj.idl, and change the expected outputs in WebCore/bindings/scripts/test/{JS|V8|...}/. Please make sure WebKitTools/Scripts/run-bindings-tests passes. > > WebCore/storage/DOMFileSystem.cpp:35 > + #include "DOMFileSystem.h" > i believe the common pattern is: > > #include "config.h" > #include "DOMFileSystem.h" > > #if ENABLE(FILE_SYSTEM) > > WebCore/storage/DOMFileSystem.cpp:38 > + #include "EntryCallback.h" > looks like this include is not yet needed. > > WebCore/storage/DOMFileSystem.cpp:39 > + #include "ErrorCallback.h" > ditto. > > WebCore/storage/DOMFileSystem.h:44 > + class ErrorCallback; > these 2 classes don't need to be forward-declared just yet. > > WebCore/storage/DOMFileSystem.h:45 > + class ScriptExecutionContext; > ditto. > > WebCore/storage/Entry.cpp:34 > + #include "Entry.h" > same comment about moving this outside #if ENABLE(FILE_SYSTEM) > > WebCore/storage/Entry.cpp:50 > + m_name = fullPath.substring(index); > should there be a "else m_name = fullPath;" here? > > WebCore/storage/Entry.h:34 > + #include "DOMFileSystem.h" > either remove this include, or the forward-declaration below. > > WebCore/storage/ErrorCallback.h:43 > + class ErrorCallback : public ThreadSafeShared<ErrorCallback> { > this callback is ThreadSafeShared, and all other callbacks are RefCounted. is this intended? Good catch... it was a mistake. > WebCore/storage/Flags.h:50 > + void setEXCLUSIVE(bool exclusive) { m_exclusive = exclusive; } > you should modify WK_ucfirst in CodeGenerator.pm the same way you modified WK_lcfirst, and convert setCREATE() and setEXCLUSIVE() to setCreate() and setExclusive(). > > WebCore/storage/Metadata.h:53 > + double m_modificationTime; > empty line before this field.

Comment on attachment 62914 [details] Patch r=me, but please address the comments below, and make sure you can build webkit and chromium before you submit the patch. WebCore/bindings/scripts/CodeGenerator.pm:348 + $ret =~ s/^CREATE/Create/ if $ret =~ /^CREATE$/; now that i look at this code, i think we can do a little better: instead of adding special cases for CREATE and EXCLUSIVE, we can do the same trick for all uppercase strings. basically, i think we should just add 'return ucfirst(lc($param)) if $param eq uc($param);' at the beginning of this function. sorry i didn't think about this earlier. WebCore/bindings/scripts/CodeGenerator.pm:367 + $ret =~ s/^cREATE/isCreate/ if $ret =~ /^cREATE$/; same thing here, i think it's ok to assume for now that all uppercase attributes are boolean: return "is" . ucfirst(lc($param)) if $param eq uc($param); WebCore/bindings/scripts/test/TestObj.idl:48 + attribute bool CREATE; s/bool/boolean/. otherwise, we'll get lots of non-sense includes like "JSbool.h" and "V8bool.h".

Thanks for reviewing. (In reply to comment #6) > (From update of attachment 62914 [details]) > r=me, but please address the comments below, and make sure you can build webkit and chromium before you submit the patch. > > WebCore/bindings/scripts/CodeGenerator.pm:348 > + $ret =~ s/^CREATE/Create/ if $ret =~ /^CREATE$/; > now that i look at this code, i think we can do a little better: instead of adding special cases for CREATE and EXCLUSIVE, we can do the same trick for all uppercase strings. > basically, i think we should just add 'return ucfirst(lc($param)) if $param eq uc($param);' at the beginning of this function. Yeah it'll be cleaner - I didn't do so out of fear to break something, but it looks ok as you say (grepping confirmed that). I'll change like that. I'll also fix bool in idl and make sure they can be built on both platforms before submitting.

Committed r64414: <http://trac.webkit.org/changeset/64414>

http://trac.webkit.org/changeset/64414 might have broken Chromium Mac Release