Bug 184653 - Add bindings code for RemoteDOMWindow
Summary: Add bindings code for RemoteDOMWindow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 184515
  Show dependency treegraph
 
Reported: 2018-04-16 09:13 PDT by Chris Dumez
Modified: 2018-04-17 10:00 PDT (History)
7 users (show)

See Also:


Attachments
Patch (55.69 KB, patch)
2018-04-16 10:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-sierra (526.29 KB, application/zip)
2018-04-16 11:43 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews116 for mac-sierra (1.45 MB, application/zip)
2018-04-16 11:44 PDT, EWS Watchlist
no flags Details
Patch (56.99 KB, patch)
2018-04-16 11:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (57.58 KB, patch)
2018-04-16 12:06 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (57.68 KB, patch)
2018-04-16 13:27 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (58.52 KB, patch)
2018-04-17 08:52 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-04-16 09:13:33 PDT
Add bindings code for RemoteDOMWindow.
Comment 1 Chris Dumez 2018-04-16 10:48:24 PDT
Created attachment 338014 [details]
Patch
Comment 2 EWS Watchlist 2018-04-16 11:43:06 PDT
Comment on attachment 338014 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 3 EWS Watchlist 2018-04-16 11:43:07 PDT
Created attachment 338018 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 4 EWS Watchlist 2018-04-16 11:44:09 PDT
Comment on attachment 338014 [details]
Patch

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

Number of test failures exceeded the failure limit.
Comment 5 EWS Watchlist 2018-04-16 11:44:11 PDT
Created attachment 338019 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 6 Chris Dumez 2018-04-16 11:49:16 PDT
Created attachment 338021 [details]
Patch
Comment 7 Chris Dumez 2018-04-16 12:06:52 PDT
Created attachment 338023 [details]
Patch
Comment 8 Chris Dumez 2018-04-16 13:27:15 PDT
Created attachment 338030 [details]
Patch
Comment 9 Ryosuke Niwa 2018-04-16 21:23:27 PDT
Comment on attachment 338030 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:82
> +template <IsRemoteDOMWindow isRemoteDOMWindow>

I think I'd prefer having DOMWindowType / DOMWindowLocation enum with Remote and Local as values.

> Source/WebCore/bindings/js/JSRemoteDOMWindowBase.h:37
> +class WEBCORE_EXPORT JSRemoteDOMWindowBase : public JSDOMGlobalObject {

Why is this class introduced / needed? Please explain it in the change log.
Comment 10 Chris Dumez 2018-04-17 08:46:35 PDT
(In reply to Ryosuke Niwa from comment #9)
> Comment on attachment 338030 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338030&action=review
> 
> > Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:82
> > +template <IsRemoteDOMWindow isRemoteDOMWindow>
> 
> I think I'd prefer having DOMWindowType / DOMWindowLocation enum with Remote
> and Local as values.

OK.

> 
> > Source/WebCore/bindings/js/JSRemoteDOMWindowBase.h:37
> > +class WEBCORE_EXPORT JSRemoteDOMWindowBase : public JSDOMGlobalObject {
> 
> Why is this class introduced / needed? Please explain it in the change log.

I will add it to the ChangeLog. Basically, JSRemoteDOMWindow needs to be a JSGlobalObject because:
1. a JSProxy's target needs to be a JSGlobalObject currently
2. The 'structure()->setGlobalObject(vm, &window);' call in JSDOMWindowProxy::setWindow(VM&, JSDOMGlobalObject&) which requires a JSGlobalObject

Ideally, this wouldn't be the case in the future but this would require some code refactoring.

Our DOM global objects normally subclass JSDOMGlobalObject so I decided to subclass JSDOMGlobalObject, which brings some things our bindings code expect.
However, subclassing JSDOMGlobalObject directly is problematic because it does not hold the m_wrapped implementation pointer. To address this issue, all our our DOM global objects have a JS*Base base class which subclasses JSDOMGlobalObject and stores the m_wrapped implementation pointer. I followed the same pattern here.

Again, ideally, in the future, JSRemoteDOMWindow wouldn't need to be a GlobalObject at all and we would be able to get rid of all this code.
Comment 11 Chris Dumez 2018-04-17 08:52:31 PDT
Created attachment 338121 [details]
Patch
Comment 12 WebKit Commit Bot 2018-04-17 09:58:51 PDT
The commit-queue encountered the following flaky tests while processing attachment 338121 [details]:

imported/w3c/web-platform-tests/dom/nodes/Document-constructor.html bug 184700 (authors: cdumez@apple.com and youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2018-04-17 09:59:40 PDT
Comment on attachment 338121 [details]
Patch

Clearing flags on attachment: 338121

Committed r230715: <https://trac.webkit.org/changeset/230715>
Comment 14 WebKit Commit Bot 2018-04-17 09:59:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2018-04-17 10:00:27 PDT
<rdar://problem/39493862>