Bug 117129

Summary: [GTK] Parameters 'inResult' and 'resolver' from function 'webkit_dom_document_evaluate' should be allowed to be NULL
Product: WebKit Reporter: Diego Pino <dpino>
Component: BindingsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Diego Pino 2013-06-02 14:38:42 PDT
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.
Comment 1 Diego Pino 2013-06-02 15:09:33 PDT
Created attachment 203548 [details]
Patch
Comment 2 WebKit Commit Bot 2013-06-02 15:12:14 PDT
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.
Comment 3 Diego Pino 2013-06-02 15:15:34 PDT
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 4 Xan Lopez 2013-06-04 08:22:10 PDT
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.
Comment 5 Xan Lopez 2013-06-04 08:23:09 PDT
(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.
Comment 6 Diego Pino 2013-06-04 11:34:50 PDT
Created attachment 203715 [details]
Patch
Comment 7 Diego Pino 2013-06-05 13:50:37 PDT
On the last patch I forgot to include the Changelog :P
Comment 8 Diego Pino 2013-06-05 13:51:14 PDT
Created attachment 203880 [details]
Patch
Comment 9 Diego Pino 2013-06-06 03:55:01 PDT
Created attachment 203921 [details]
Patch
Comment 10 Diego Pino 2013-06-06 03:56:09 PDT
Modified 'ParamCanBeNull' function. Return 'false' if 'functionName' is not defined.
Comment 11 Xan Lopez 2013-06-06 04:19:56 PDT
Comment on attachment 203921 [details]
Patch

LGTM.
Comment 12 WebKit Commit Bot 2013-06-06 04:21:16 PDT
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
Comment 13 Xan Lopez 2013-06-06 04:22:49 PDT
Ah, I think you actually need to remove the No new tests (OOPS!) thing :) (and we actually are adding tests anyway).
Comment 14 Diego Pino 2013-06-06 04:41:57 PDT
Created attachment 203923 [details]
Patch
Comment 15 Xan Lopez 2013-06-06 06:17:07 PDT
Comment on attachment 203923 [details]
Patch

Looks good!
Comment 16 WebKit Commit Bot 2013-06-06 06:39:26 PDT
Comment on attachment 203923 [details]
Patch

Clearing flags on attachment: 203923

Committed r151267: <http://trac.webkit.org/changeset/151267>
Comment 17 WebKit Commit Bot 2013-06-06 06:39:29 PDT
All reviewed patches have been landed.  Closing bug.