Bug 56573 - [Chromium] Expose the shadow DOM to DumpRenderTree JS tests.
Summary: [Chromium] Expose the shadow DOM to DumpRenderTree JS tests.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 53020 57415
Blocks: 54634
  Show dependency treegraph
 
Reported: 2011-03-17 10:44 PDT by Ami Fischman
Modified: 2011-03-30 10:50 PDT (History)
4 users (show)

See Also:


Attachments
First crack at exposing shadowRoot to JS in DRT. (163.24 KB, patch)
2011-03-17 10:59 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Extracted just chromium DRT shadowRoot addition from previous attachment. (6.82 KB, patch)
2011-03-28 13:36 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Added missing comma in .gyp file. (7.09 KB, patch)
2011-03-28 14:11 PDT, Ami Fischman
no flags Details | Formatted Diff | Diff
Patch (7.81 KB, patch)
2011-03-30 10:19 PDT, Hajime Morrita
dglazkov: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ami Fischman 2011-03-17 10:44:26 PDT
Expose the shadow DOM to DumpRenderTree JS tests.
Comment 1 Ami Fischman 2011-03-17 10:59:14 PDT
Created attachment 86071 [details]
First crack at exposing shadowRoot to JS in DRT.
Comment 2 Dimitri Glazkov (Google) 2011-03-18 09:28:28 PDT
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.
Comment 3 Ami Fischman 2011-03-18 10:16:36 PDT
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 :)
Comment 4 Dimitri Glazkov (Google) 2011-03-18 12:14:53 PDT
(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!!!
Comment 5 Hajime Morrita 2011-03-28 13:28:21 PDT
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?
Comment 6 Ami Fischman 2011-03-28 13:36:38 PDT
Created attachment 87192 [details]
Extracted just chromium DRT shadowRoot addition from previous attachment.
Comment 7 WebKit Review Bot 2011-03-28 13:40:25 PDT
Attachment 87192 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8278217
Comment 8 Ami Fischman 2011-03-28 14:11:44 PDT
Created attachment 87202 [details]
Added missing comma in .gyp file.
Comment 9 Tony Chang 2011-03-28 14:19:34 PDT
(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).
Comment 10 Adam Barth 2011-03-28 16:25:16 PDT
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.
Comment 11 Ami Fischman 2011-03-28 16:28:29 PDT
Thanks Tony & Adam.  
morrita@ said he'll take a crack at doing it the right way soon.
Comment 12 Hajime Morrita 2011-03-30 10:19:37 PDT
Created attachment 87562 [details]
Patch
Comment 13 Hajime Morrita 2011-03-30 10:20:25 PDT
> morrita@ said he'll take a crack at doing it the right way soon.
Now updated the stolen patch ;-)
Comment 14 Dimitri Glazkov (Google) 2011-03-30 10:21:55 PDT
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
Comment 15 Hajime Morrita 2011-03-30 10:50:47 PDT
Committed r82469: <http://trac.webkit.org/changeset/82469>