Bug 56573 – [Chromium] Expose the shadow DOM to DumpRenderTree JS tests.

• Component: New Bugs
• Status: RESOLVED FIXED

[Ami Fischman]

Expose the shadow DOM to DumpRenderTree JS tests.

[Ami Fischman]

Created attachment 86071 [details]
First crack at exposing shadowRoot to JS in DRT.

[Dimitri Glazkov (Google)]

Comment on attachment 86071 [details]
First crack at exposing shadowRoot to JS in DRT.

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

I see three excellent patches here:
1) One that removes whitespace
2) One that introduces layoutTestController.shadowRoot
3) One that changes video-controls-in-media-document to not use pixel tests.

And hopefully, I will flip over HTMLMediaElement to use proper shadow DOM before you finish 3. If not, you will need to keep nagging me! :)

> Source/WebCore/rendering/MediaControlElements.cpp:-72
> -    setShadowHost(mediaElement);

Unfortunately, this is not yet ready for flip-over to use the new shadow DOM. Please watch bug 53020, where I am trying to just that.

> Source/WebCore/rendering/MediaControlElements.cpp:89
> +    mediaElement->setShadowRoot(element);

Ditto. This is not the right spot to set the shadow root.

> Source/WebCore/rendering/MediaControlElements.cpp:97
> +    shadowHost()->setShadowRoot(0);

Ditto.

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:176
> +    bindMethod("shadowRoot", &LayoutTestController::shadowRoot);

This is great, but please don't forget about other ports in the final patch. They will be sad and cry all night if you don't visit them.

[Ami Fischman]

Thanks for the (p)review, Dimitri!
Questions:
1) Is whitespace-removal considered worthy of its own CL in webkit-land?  What removal happened was automatically done by my editor setup, and I was skittish about it, since I couldn't find a policy about whether WS removal was considered helpful, harmful, or neutral.

2) For the other ports of DumpRenderTree: I hacked on chromium until I had something that worked, but I don't really understand the relationship between the different LTC classes.  Is the full list:
./Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h
./Tools/DumpRenderTree/LayoutTestController.h
./Tools/DumpRenderTree/chromium/LayoutTestController.h
?  If yes, can you point me at the equivalents of convertNPVariantToV8Object for the first two?  (and hints about how I might test any changes I make, esp. on a linux machine :)).

3) By your #3 did you mean you thought I still had changes to make to the v-c-i-m-d.html test, or just that checking in the change I already have for it is going to depend on the previous items (and your change)?

I look forward to you landing 53020 :)

[Dimitri Glazkov (Google)]

(In reply to comment #3)
> Thanks for the (p)review, Dimitri!
> Questions:
> 1) Is whitespace-removal considered worthy of its own CL in webkit-land?  What removal happened was automatically done by my editor setup, and I was skittish about it, since I couldn't find a policy about whether WS removal was considered helpful, harmful, or neutral.
> 
> 2) For the other ports of DumpRenderTree: I hacked on chromium until I had something that worked, but I don't really understand the relationship between the different LTC classes.  Is the full list:
> ./Tools/WebKitTestRunner/InjectedBundle/LayoutTestController.h
> ./Tools/DumpRenderTree/LayoutTestController.h
> ./Tools/DumpRenderTree/chromium/LayoutTestController.h
> ?  If yes, can you point me at the equivalents of convertNPVariantToV8Object for the first two?  (and hints about how I might test any changes I make, esp. on a linux machine :)).

Get a mac! :)

But you also should be able to build gtk port on Linux: https://trac.webkit.org/wiki/BuildingGtk

> 
> 3) By your #3 did you mean you thought I still had changes to make to the v-c-i-m-d.html test, or just that checking in the change I already have for it is going to depend on the previous items (and your change)?
> 

The latter. You already have a great patch there.

> I look forward to you landing 53020 :)

ME TOO!!!

[Hajime Morrita]

Hi Ami, thank you for pushing this forward ;-)

I think it might be better to use WebBinding and WebElement instead of V8Element and WebCore::Element
because accessing WebCore classes directly can be considered as a layering violation.
We need some more WebElement and WebBindings API to do it though....

Dimitri, how do you think?

[Ami Fischman]

Created attachment 87192 [details]
Extracted just chromium DRT shadowRoot addition from previous attachment.

[WebKit Review Bot]

Attachment 87192 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8278217

[Ami Fischman]

Created attachment 87202 [details]
Added missing comma in .gyp file.

[Tony Chang]

(In reply to comment #8)
> Created an attachment (id=87202) [details]
> Added missing comma in .gyp file.

Morita's feedback is correct.  DRT shouldn't reach into WebCore directly, it should call into WebKit, which in turn can call into WebCore.  If the actual work is done in WebCore, it allows all the ports to share an implementation (although you still have to write the code in WebKit to call into WebCore for all ports).

[Adam Barth]

Comment on attachment 87202 [details]
Added missing comma in .gyp file.

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

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:660
> +    Element* hostElement = V8Element::toNative(v8value);
> +    Element* shadowRootElement = toElement(hostElement->shadowRoot());

This are WebCore types.  DRT needs to restrict itself to WebKit types.

[Ami Fischman]

Thanks Tony & Adam.  
morrita@ said he'll take a crack at doing it the right way soon.

[Hajime Morrita]

Created attachment 87562 [details]
Patch

[Hajime Morrita]

> morrita@ said he'll take a crack at doing it the right way soon.
Now updated the stolen patch ;-)

[Dimitri Glazkov (Google)]

Comment on attachment 87562 [details]
Patch

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

ok.

> Source/WebKit/chromium/ChangeLog:9
> +        - WebBingins::makeNode() to convert WebNode to a JS object, and

Bindings

[Hajime Morrita]

Committed r82469: <http://trac.webkit.org/changeset/82469>