Bug 91564

Summary: Element wants to have userAgentShadowRoot()
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Shinya Kawanaka <shinyak>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, eric.carlson, feature-media-reviews, shinyak, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 82313    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Hajime Morrita 2012-07-17 17:28:47 PDT
This is a follow up t bug 77936.
userAgentShadowRoot() could have an assertion to type debug field.
Also, it will make ua-shadow related code more readable.
Comment 1 Shinya Kawanaka 2012-07-17 18:53:36 PDT
I think Element should have this method, right?

A lot of code use the following pattern
  shadow()->oldestShadowRoot()->...
But we should be able to do:
  userAgentShadowRoot()->...
Comment 2 Shinya Kawanaka 2012-07-17 22:22:56 PDT
Created attachment 152919 [details]
Patch
Comment 3 Early Warning System Bot 2012-07-17 22:37:55 PDT
Comment on attachment 152919 [details]
Patch

Attachment 152919 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13269368
Comment 4 Early Warning System Bot 2012-07-17 22:40:24 PDT
Comment on attachment 152919 [details]
Patch

Attachment 152919 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13273405
Comment 5 WebKit Review Bot 2012-07-17 22:42:38 PDT
Comment on attachment 152919 [details]
Patch

Attachment 152919 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13277399
Comment 6 Shinya Kawanaka 2012-07-17 22:46:12 PDT
Created attachment 152930 [details]
Patch
Comment 7 Eric Carlson 2012-07-18 09:39:30 PDT
Comment on attachment 152930 [details]
Patch

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

This is much cleaner, thanks!

> Source/WebCore/html/HTMLMediaElement.cpp:4185
> +    if (ShadowRoot* userAgent = userAgentShadowRoot()) {
> +        Node* node = userAgent->firstChild();
> +        return node && node->isMediaControls();
> +    }

Isn't it true that userAgentShadowRoot()->firstChild() *must* be the media controls? If so, I think "&& node->isMediaControls()" could be replaced with "ASSERT(node->isMediaControls())".
Comment 8 Shinya Kawanaka 2012-07-18 17:58:00 PDT
(In reply to comment #7)
> (From update of attachment 152930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152930&action=review
> 
> This is much cleaner, thanks!
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4185
> > +    if (ShadowRoot* userAgent = userAgentShadowRoot()) {
> > +        Node* node = userAgent->firstChild();
> > +        return node && node->isMediaControls();
> > +    }
> 
> Isn't it true that userAgentShadowRoot()->firstChild() *must* be the media controls? If so, I think "&& node->isMediaControls()" could be replaced with "ASSERT(node->isMediaControls())".

If firstChild() exists, it must be the media control, I believe.
I'll update the patch, and if it passes tests, I'll land it.
Comment 9 Shinya Kawanaka 2012-07-18 18:11:38 PDT
Created attachment 153150 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-07-18 22:09:03 PDT
Comment on attachment 153150 [details]
Patch for landing

Clearing flags on attachment: 153150

Committed r123071: <http://trac.webkit.org/changeset/123071>
Comment 11 WebKit Review Bot 2012-07-18 22:09:09 PDT
All reviewed patches have been landed.  Closing bug.