Bug 156853 – Remove WebCore::Dictionary and use WebCore::JSDictionary directly instead

• Component: WebCore JavaScript
• Status: RESOLVED DUPLICATE

[Darin Adler]

Remove WebCore::Dictionary and use WebCore::JSDictionary directly instead

[Darin Adler]

Created attachment 277011 [details]
Patch

[Darin Adler]

Comment on attachment 277011 [details]
Patch

Missing some includes of JSCJSValueInlines.h, I think.

[Build Bot]

Comment on attachment 277011 [details]
Patch

Attachment 277011 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1200338

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 277013 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

Created attachment 277217 [details]
Patch

[Build Bot]

Comment on attachment 277217 [details]
Patch

Attachment 277217 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1216171

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 277225 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 277217 [details]
Patch

Attachment 277217 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1216175

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 277227 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Alexey Proskuryakov]

Comment on attachment 277217 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277217&action=review

> Source/WebCore/ChangeLog:21
> +        Longer term the direction should be to remove almost all direct use of
> +        JSDictionary in the C++ DOM and move it to the bindings. I have some specific
> +        ideas of how to do this; we should use have the bindings translate the
> +        dictionaries into structs defined in the C++ DOM.

From a discussion when I was implementing WebCrypto, I remember that having a separate class for Dictionary was the first step in this direction.

I can't compare the approaches deeply enough, really interested in Sam's take.

[Build Bot]

Comment on attachment 277217 [details]
Patch

Attachment 277217 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1216193

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 277228 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Build Bot]

Comment on attachment 277217 [details]
Patch

Attachment 277217 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/1216194

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 277229 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Darin Adler]

(In reply to comment #10)
> > +        Longer term the direction should be to remove almost all direct use of
> > +        JSDictionary in the C++ DOM and move it to the bindings. I have some specific
> > +        ideas of how to do this; we should use have the bindings translate the
> > +        dictionaries into structs defined in the C++ DOM.
> 
> From a discussion when I was implementing WebCrypto, I remember that having
> a separate class for Dictionary was the first step in this direction.

My approach will involve a structure for each dictionary, defined in the C++ DOM, and bindings translating the dictionary to the structure before calling the C++ DOM. You’ll start to see it in patches over the next weeks or months and we can discuss it either then or before then.

[Chris Dumez]

(In reply to comment #15)
> (In reply to comment #10)
> > > +        Longer term the direction should be to remove almost all direct use of
> > > +        JSDictionary in the C++ DOM and move it to the bindings. I have some specific
> > > +        ideas of how to do this; we should use have the bindings translate the
> > > +        dictionaries into structs defined in the C++ DOM.
> > 
> > From a discussion when I was implementing WebCrypto, I remember that having
> > a separate class for Dictionary was the first step in this direction.
> 
> My approach will involve a structure for each dictionary, defined in the C++
> DOM, and bindings translating the dictionary to the structure before calling
> the C++ DOM. You’ll start to see it in patches over the next weeks or months
> and we can discuss it either then or before then.

The one issue I see using a struct is that Dictionaries passed in from JS often have very few of their members defined. Dictionaries are usually used a bags of options/settings and the JS often specifies just one of them.

The issue with using a struct for this is that it will always have all its members. Likely, most of the members will be using WTF::Optional<> since dictionary members are almost always optional. It may look a bit awkward to use a struct for this.

The nice thing about converting to a struct in the bindings is that it will take care of all the input validation and type conversion for us. This part is great.

[Darin Adler]

(In reply to comment #16)
> Likely, most of the members will be using WTF::Optional<> since
> dictionary members are almost always optional.

Unless there is a default value for that item in the dictionary. The situation is quite similar to method arguments.

[Chris Dumez]

(In reply to comment #17)
> (In reply to comment #16)
> > Likely, most of the members will be using WTF::Optional<> since
> > dictionary members are almost always optional.
> 
> Unless there is a default value for that item in the dictionary. The
> situation is quite similar to method arguments.

Good point, I did not think about this.

[Darin Adler]

Still good to have both WebCore::Dictionary and WebCore::JSDictionary. We need to delete one or the other. In the mean time, we removed many uses of WebCore::Dictionary.

[Darin Adler]

*** This bug has been marked as a duplicate of bug 165641 ***