Bug 154224 - Sort, deduplicate & comment JSDOMWindowCustom getOwnPropertySlot
Summary: Sort, deduplicate & comment JSDOMWindowCustom getOwnPropertySlot
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-13 22:43 PST by Gavin Barraclough
Modified: 2016-02-15 11:57 PST (History)
2 users (show)

See Also:


Attachments
WIP (21.75 KB, patch)
2016-02-13 22:44 PST, Gavin Barraclough
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (776.48 KB, application/zip)
2016-02-13 23:30 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (962.42 KB, application/zip)
2016-02-13 23:36 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-yosemite (841.39 KB, application/zip)
2016-02-13 23:38 PST, Build Bot
no flags Details
Fix (25.05 KB, patch)
2016-02-14 01:26 PST, Gavin Barraclough
cdumez: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2016-02-13 22:43:06 PST
There's a lot going on here, mixed up in an inconsistent order, with duplication between (& within!) functions. Organizing this a little better will make further behavioral changes easier.
Comment 1 Gavin Barraclough 2016-02-13 22:44:22 PST
Created attachment 271303 [details]
WIP
Comment 2 Build Bot 2016-02-13 23:30:04 PST
Comment on attachment 271303 [details]
WIP

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

New failing tests:
fast/dom/Window/get-set-properties.html
fast/dom/Window/window-property-shadowing.html
accessibility/mac/test-convenience-methods.html
Comment 3 Build Bot 2016-02-13 23:30:07 PST
Created attachment 271304 [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
Comment 4 Build Bot 2016-02-13 23:36:15 PST
Comment on attachment 271303 [details]
WIP

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

New failing tests:
fast/dom/Window/get-set-properties.html
fast/dom/Window/window-property-shadowing.html
accessibility/mac/test-convenience-methods.html
Comment 5 Build Bot 2016-02-13 23:36:17 PST
Created attachment 271305 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 6 Build Bot 2016-02-13 23:38:49 PST
Comment on attachment 271303 [details]
WIP

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

New failing tests:
fast/dom/Window/get-set-properties.html
fast/dom/Window/window-property-shadowing.html
accessibility/mac/test-convenience-methods.html
Comment 7 Build Bot 2016-02-13 23:38:51 PST
Created attachment 271306 [details]
Archive of layout-test-results from ews112 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 8 Gavin Barraclough 2016-02-14 01:26:15 PST
Created attachment 271307 [details]
Fix
Comment 9 Chris Dumez 2016-02-14 15:56:49 PST
Comment on attachment 271307 [details]
Fix

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

Nice clean up.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:96
> +    // Allow access to toString() cross-domain, but always Object.prototype.toString.

We now allow toString() for frameless windows, is this intentional?

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:214
> +    Document* document = frame.document();

auto*

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:216
> +        HTMLDocument& htmlDocument = downcast<HTMLDocument>(*document);

auto&

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:217
> +        AtomicStringImpl* atomicPropertyName = propertyName.publicName();

auto*

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:223
> +                namedItem = toJS(exec, thisObject->globalObject(), WTF::getPtr(collection));

We prefer collection.ptr(). WTF::getPtr() is mostly used in the generated bindings where we don't know what type we are getting as input.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:242
> +    Optional<unsigned> index = parseIndex(propertyName);

Could be inside the if()

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:246
> +    JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(object);

auto*

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:247
> +    Frame* frame = thisObject->wrapped().frame();

auto*

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:323
> +    Frame* frame = thisObject->wrapped().frame();

auto*
Comment 10 Gavin Barraclough 2016-02-15 11:56:32 PST
(In reply to comment #9)
> Comment on attachment 271307 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271307&action=review
> 
> Nice clean up.
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:96
> > +    // Allow access to toString() cross-domain, but always Object.prototype.toString.
> 
> We now allow toString() for frameless windows, is this intentional?

Yes  –there was this old FIXME:
    // FIXME: This doesn't fully match Firefox, which allows at least toString in addition to those.

So I made it so. Would be easy switch if we wanted.

In general, it seems crazy to me that we have a whole third path handling frameless access, and that the properties just magically disappear off the global object when the frame is closed. We probably don't gain anything anyway (since functions & accessors can now all be pulled off into local vars, so I doubt this buys us anything anymore. If not spec defined, I think we should probably consider removing this behviour.

> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:214
> > +    Document* document = frame.document();
> 
> auto*
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:216
> > +        HTMLDocument& htmlDocument = downcast<HTMLDocument>(*document);
> 
> auto&
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:217
> > +        AtomicStringImpl* atomicPropertyName = propertyName.publicName();
> 
> auto*
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:223
> > +                namedItem = toJS(exec, thisObject->globalObject(), WTF::getPtr(collection));
> 
> We prefer collection.ptr(). WTF::getPtr() is mostly used in the generated
> bindings where we don't know what type we are getting as input.
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:242
> > +    Optional<unsigned> index = parseIndex(propertyName);
> 
> Could be inside the if()
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:246
> > +    JSDOMWindow* thisObject = jsCast<JSDOMWindow*>(object);
> 
> auto*
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:247
> > +    Frame* frame = thisObject->wrapped().frame();
> 
> auto*
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:323
> > +    Frame* frame = thisObject->wrapped().frame();
> 
> auto*

All fixed.

Committed revision 196583.