Bug 117129 - [GTK] Parameters 'inResult' and 'resolver' from function 'webkit_dom_document_evaluate' should be allowed to be NULL
Summary: [GTK] Parameters 'inResult' and 'resolver' from function 'webkit_dom_document...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-02 14:38 PDT by Diego Pino
Modified: 2013-06-06 06:39 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.23 KB, patch)
2013-06-02 15:09 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (5.53 KB, patch)
2013-06-04 11:34 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (7.75 KB, patch)
2013-06-05 13:51 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (7.84 KB, patch)
2013-06-06 03:55 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (7.81 KB, patch)
2013-06-06 04:41 PDT, Diego Pino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.