According to the spec http://www.w3.org/TR/DOM-Level-3-XPath/xpath.html Parameters 'inResult' and 'resolver' from function 'webkit_dom_document_evaluate' can be NULL. This bug was reported for the parameter 'inResult' in webk.it/42115 and a temporary fix that skipped the validation of this paramater was applied. Since there was no validation, the parameter was allowed to be NULL.
Created attachment 203548 [details] Patch
Attachment 203548 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/tests/testdomdocument.c']" exit_code: 1 Source/WebCore/ChangeLog:5: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testdomdocument.c" Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
The patch refactors the code of the temporary fix and moves the validation of nullability of parameters to the GetGReturnMacro function. A hash table keeps track of the parameters that are allowed to be NULL indexed by function name. This solution is more extensable, and it will allow to add more parameters which can be NULL in the future. Finally, a test for function 'dom_document_evaluate' was added. The test passes both parameter 'inResult' and 'resolver' as NULL.
Comment on attachment 203548 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=203548&action=review Looks pretty good in general, just a few small comments. >> Source/WebCore/ChangeLog:5 >> + * Add test for function 'webkit_dom_document_evaluate' > > Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] I think the style bot is complaining because no text should appear above the title/uri combo. > Source/WebKit/gtk/ChangeLog:5 > + Add test for function 'webkit_dom_document_evaluate' Although no warning here, so not sure. Still this seems wrong. > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:49 > +my $can_be_null_params = { I think methods have to be in camel case. > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:864 > + } I think you can leave the original line and just prepend the !$paramName || thing if ParamCanBeNull is true, right? > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:885 > + } Can this actually happen? > Source/WebKit/gtk/tests/testdomdocument.c:227 > + 0, NULL, NULL); I'd just leave this in the previous line.
(In reply to comment #4) > > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:49 > > +my $can_be_null_params = { > > I think methods have to be in camel case. Of course I meant variable names here.
Created attachment 203715 [details] Patch
On the last patch I forgot to include the Changelog :P
Created attachment 203880 [details] Patch
Created attachment 203921 [details] Patch
Modified 'ParamCanBeNull' function. Return 'false' if 'functionName' is not defined.
Comment on attachment 203921 [details] Patch LGTM.
Comment on attachment 203921 [details] Patch Rejecting attachment 203921 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 203921, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!. Full output: http://webkit-queues.appspot.com/results/712640
Ah, I think you actually need to remove the No new tests (OOPS!) thing :) (and we actually are adding tests anyway).
Created attachment 203923 [details] Patch
Comment on attachment 203923 [details] Patch Looks good!
Comment on attachment 203923 [details] Patch Clearing flags on attachment: 203923 Committed r151267: <http://trac.webkit.org/changeset/151267>
All reviewed patches have been landed. Closing bug.