Bug 117129 – [GTK] Parameters 'inResult' and 'resolver' from function 'webkit_dom_document_evaluate' should be allowed to be NULL

• Component: Bindings
• Status: RESOLVED FIXED

[Diego Pino]

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.

[Diego Pino]

Created attachment 203548 [details]
Patch

[WebKit Commit Bot]

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.

[Diego Pino]

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.

[Xan Lopez]

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.

[Xan Lopez]

(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.

[Diego Pino]

Created attachment 203715 [details]
Patch

[Diego Pino]

On the last patch I forgot to include the Changelog :P

[Diego Pino]

Created attachment 203880 [details]
Patch

[Diego Pino]

Created attachment 203921 [details]
Patch

[Diego Pino]

Modified 'ParamCanBeNull' function. Return 'false' if 'functionName' is not defined.

[Xan Lopez]

Comment on attachment 203921 [details]
Patch

LGTM.

[WebKit Commit Bot]

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

[Xan Lopez]

Ah, I think you actually need to remove the No new tests (OOPS!) thing :) (and we actually are adding tests anyway).

[Diego Pino]

Created attachment 203923 [details]
Patch

[Xan Lopez]

Comment on attachment 203923 [details]
Patch

Looks good!

[WebKit Commit Bot]

Comment on attachment 203923 [details]
Patch

Clearing flags on attachment: 203923

Committed r151267: <http://trac.webkit.org/changeset/151267>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.