Bug 56573

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

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.

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 :)

(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!!!

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?

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

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

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

(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 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.

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

Created attachment 87562 [details] Patch

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

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

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