Bug 77936

We would like to eliminate Element::ensureShadowRoot(). HTMLMediaElement should not use it, too.

Created attachment 151823 [details] Patch

Comment on attachment 151823 [details] Patch Attachment 151823 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13207474 New failing tests: fast/dom/shadow/shadow-disable.html

Created attachment 151841 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Created attachment 151856 [details] Patch

Created attachment 151898 [details] Patch

Comment on attachment 151898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151898&action=review > Source/WebCore/ChangeLog:14 > + <video> and <audio> add UserAgentShadowRoot dynamically, and assume that it's the oldest ShadowRoot. > + However, before <video> and <audio> add UserAgentShadowRoot, an AuthorShadowRoot can be added by a user. > + It breaks the assumption that the UserAgentShadowRoot is the oldest. > + > + When AuthorShadowRoot is being added, this patch adds UserAgentShadowRoot before adding AuthorShadowRoot. > + So the assumption holds after a user added an AuthorShadowRoot. This allows user to add a Shadow DOM to > + <video> and <audio>. I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list? > Source/WebCore/ChangeLog:26 > + * dom/Element.h: > + (WebCore::Element::willAddAuthorShadowRoot): > + * dom/ElementShadow.cpp: > + (WebCore::ElementShadow::addShadowRoot): > + * dom/ElementShadow.h: > + (ElementShadow): > + * dom/ShadowRoot.cpp: > + (WebCore::allowsAuthorShadowRoot): > + (WebCore::ShadowRoot::create): This is a bug about HTMLMediaElement, so it seems like these generic changes should be made in separate fix/patch. > LayoutTests/fast/dom/shadow/shadowdom-for-media.html:22 > +var shadowRootForVideoWithControls = addShadowDOM(videoWithControls); > +var oldestShadowRootForVideoWithControls = internals.oldestShadowRoot(videoWithControls); > +var youngerShadowRootForVideoWithControls = internals.youngerShadowRoot(oldestShadowRootForVideoWithControls); Ditto my question from above about assuming the purpose/identity/structure of an element based on its position in the list - why?

(In reply to comment #7) > (From update of attachment 151898 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151898&action=review > > > Source/WebCore/ChangeLog:14 > > + <video> and <audio> add UserAgentShadowRoot dynamically, and assume that it's the oldest ShadowRoot. > > + However, before <video> and <audio> add UserAgentShadowRoot, an AuthorShadowRoot can be added by a user. > > + It breaks the assumption that the UserAgentShadowRoot is the oldest. > > + > > + When AuthorShadowRoot is being added, this patch adds UserAgentShadowRoot before adding AuthorShadowRoot. > > + So the assumption holds after a user added an AuthorShadowRoot. This allows user to add a Shadow DOM to > > + <video> and <audio>. > > I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list? The reason why we assume UserAgentShadowRoot is the oldest is: if it is not so, we will miss the content of AuthorShadowRoot. ShadowDOM for <video> or <audio> does not contain any <content> or <shadow>, as the spec describes so. More practical reason is: we would like to keep consistent behavior. Page author should not know about the Shadow DOM for <video> or <audio>. If UserAgentShadowDOM is added after AuthorShadowDOM, and if yet another AuthorShadowDOM is added after that UserAgentShadowDOM, what happens? An author won't assume that the older ShadowDOM of the second AuthorShadowDOM is UserAgentShadowDOM. > > > Source/WebCore/ChangeLog:26 > > + * dom/Element.h: > > + (WebCore::Element::willAddAuthorShadowRoot): > > + * dom/ElementShadow.cpp: > > + (WebCore::ElementShadow::addShadowRoot): > > + * dom/ElementShadow.h: > > + (ElementShadow): > > + * dom/ShadowRoot.cpp: > > + (WebCore::allowsAuthorShadowRoot): > > + (WebCore::ShadowRoot::create): > > This is a bug about HTMLMediaElement, so it seems like these generic changes should be made in separate fix/patch. Right. > > > LayoutTests/fast/dom/shadow/shadowdom-for-media.html:22 > > +var shadowRootForVideoWithControls = addShadowDOM(videoWithControls); > > +var oldestShadowRootForVideoWithControls = internals.oldestShadowRoot(videoWithControls); > > +var youngerShadowRootForVideoWithControls = internals.youngerShadowRoot(oldestShadowRootForVideoWithControls); > > Ditto my question from above about assuming the purpose/identity/structure of an element based on its position in the list - why?

Created attachment 152105 [details] Patch

@shinya I guess some diagram would definitely help your explanation to be understood.

(In reply to comment #10) > @shinya I guess some diagram would definitely help your explanation > to be understood. https://docs.google.com/a/chromium.org/drawings/d/1rq6JixSqgUpXNNMkGFs8BR2LEg5U0IIyFsooloKjXAU/edit Added a diagram. I hope it would be helpful.

Comment on attachment 152105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152105&action=review > Source/WebCore/ChangeLog:38 > +2012-07-11 Shinya Kawanaka <shinyak@chromium.org> > + > + > + When AuthorShadowRoot is being added, this patch adds UserAgentShadowRoot before adding AuthorShadowRoot. > + So the assumption holds after a user added an AuthorShadowRoot. This allows user to add a Shadow DOM to > + <video> and <audio>. > + > + Test: fast/dom/shadow/shadowdom-for-media.html > + You forgot to remove this from the ChangeLog.

(In reply to comment #8) > (In reply to comment #7) > > > > I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list? > > The reason why we assume UserAgentShadowRoot is the oldest is: if it is not so, we will miss the content of AuthorShadowRoot. ShadowDOM for <video> or <audio> does not contain any <content> or <shadow>, as the spec describes so. > > More practical reason is: we would like to keep consistent behavior. Page author should not know about the Shadow DOM for <video> or <audio>. If UserAgentShadowDOM is added after AuthorShadowDOM, and if yet another AuthorShadowDOM is added after that UserAgentShadowDOM, what happens? An author won't assume that the older ShadowDOM of the second AuthorShadowDOM is UserAgentShadowDOM. I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there?

(In reply to comment #13) > (In reply to comment #8) > > (In reply to comment #7) > > > > > > I know absolutely nothing about author-added shadow DOM so I might be missing something obvious, but why would we assume anything about the type of a shadow root based on its position in the list? > > > > The reason why we assume UserAgentShadowRoot is the oldest is: if it is not so, we will miss the content of AuthorShadowRoot. ShadowDOM for <video> or <audio> does not contain any <content> or <shadow>, as the spec describes so. > > > > More practical reason is: we would like to keep consistent behavior. Page author should not know about the Shadow DOM for <video> or <audio>. If UserAgentShadowDOM is added after AuthorShadowDOM, and if yet another AuthorShadowDOM is added after that UserAgentShadowDOM, what happens? An author won't assume that the older ShadowDOM of the second AuthorShadowDOM is UserAgentShadowDOM. > > I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. > > My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there? Sorry for late response. Actually there is a lot of reasons why I chose this design. (1) The ShadowDOM spec says that <video> or <audio> should behave like having UserAgentShadowDOM as the oldest ShadowDOM. This is the most important reason. (2) We don't want to make UserAgentShadowRoot special. It's our (= team implementing Shadow DOM) design choice. This is why "type" is there only in the debug build. (3) Our code assumes that Shadow DOM is added into the top of the stack. Shadow DOM is a kind of stack, and we (intentionally) don't assume some Shadow DOM is added into not the top of Shadow DOM stack.

Created attachment 152670 [details] Fixed ChangeLog

Comment on attachment 152670 [details] Fixed ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=152670&action=review > Source/WebCore/html/HTMLMediaElement.cpp:4212 > + if (ElementShadow* elementShadow = shadow()) Is it possible to have shadow() but not to have oldestShadowRoot()? > Source/WebCore/html/HTMLMediaElement.h:88 > + void willAddAuthorShadowRoot() OVERRIDE; Let's say virtual to align the code around.

Comment on attachment 152670 [details] Fixed ChangeLog View in context: https://bugs.webkit.org/attachment.cgi?id=152670&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:4212 >> + if (ElementShadow* elementShadow = shadow()) > > Is it possible to have shadow() but not to have oldestShadowRoot()? Yes in some timing, but it won't happen in this case... >> Source/WebCore/html/HTMLMediaElement.h:88 >> + void willAddAuthorShadowRoot() OVERRIDE; > > Let's say virtual to align the code around. Right.

Created attachment 152696 [details] Patch

Comment on attachment 152696 [details] Patch Looks like reaching a good shape. I hope "create shadow if not there yet" pattern to be factored out somehow. But it could be done by a separate patch.

Comment on attachment 152696 [details] Patch Clearing flags on attachment: 152696 Committed r122816: <http://trac.webkit.org/changeset/122816>

All reviewed patches have been landed. Closing bug.

(In reply to comment #17) > (From update of attachment 152670 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=152670&action=review > > >> Source/WebCore/html/HTMLMediaElement.cpp:4212 > >> + if (ElementShadow* elementShadow = shadow()) > > > > Is it possible to have shadow() but not to have oldestShadowRoot()? > > Yes in some timing, but it won't happen in this case... > Why? It may be clear to you, but it obviously isn't to others, please clarify.

(In reply to comment #14) > (In reply to comment #13) > > I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. > > > > My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there? > > Sorry for late response. Actually there is a lot of reasons why I chose this design. > (1) The ShadowDOM spec says that <video> or <audio> should behave like having UserAgentShadowDOM as the oldest ShadowDOM. This is the most important reason. Where does the spec say that? The only thing I see is say specifically about <video> and <audio> is that the subtree "Contains no insertion points"[1]. It also says that all HTML elements "must have an equivalent of a shadow DOM subtree that is created and populated at the time of element instantiation", which you don't do here. Why? > (2) We don't want to make UserAgentShadowRoot special. It's our (= team implementing Shadow DOM) design choice. This is why "type" is there only in the debug build. Wait, isn't this patch necessary *because* UserAgentShadowRoot is special (different)? > (3) Our code assumes that Shadow DOM is added into the top of the stack. Shadow DOM is a kind of stack, and we (intentionally) don't assume some Shadow DOM is added into not the top of Shadow DOM stack. Sigh, we are talking past one another. I asked "is wise to depend on the position of the node to determine its purpose" and your answer is "Our code assumes that Shadow DOM is added into the top of the stack"? I *know* that you assume that the shadow DOM for the controls is added first, and i *know* that it needs to be kept separate from user added shadow DOM - that is what the code does. My question is if it is a *good idea* for the code to do what it does. So again I ask - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. [1] http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#html-elements-and-their-shadow-dom-subtrees

Comment on attachment 152696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152696&action=review Please let's make sure that we don't make assumptions about the position of UA shadow root. I would even suggest that we always create an empty shadow root in HTMLMediaElement upon instantiation to conform with the spec. > Source/WebCore/html/HTMLMediaElement.cpp:4212 > + createShadowSubtree(); This seems wrong. This: var video = document.createElement("video"); var r00t = new WebKitShadowRoot(video); video.controls = true; will result in r00t being the oldest shadow root and tripping the ASSERT. Right?

Comment on attachment 152696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152696&action=review >> Source/WebCore/html/HTMLMediaElement.cpp:4212 >> + createShadowSubtree(); > > This seems wrong. This: > > var video = document.createElement("video"); > var r00t = new WebKitShadowRoot(video); > video.controls = true; > > will result in r00t being the oldest shadow root and tripping the ASSERT. Right? No. willAddShadowRoot() guarantees the host to have UA shadow root in the oldest position. This ensure-ua-shadow-before-any-author-shadows pattern is the foundation for making replaced elements shadow-ready.

Hi Eric, I'm sorry for r+ing this before your feedback. Let me explain the rationale that forces ua-shadows being the first(oldest) shadow: At first, any UA shadow should be the oldest if there is such a shadow on the host element. This is because author shadow should be able to "decorate" it like adding extra container element or additional buttons around the original control. And only younger shadows can decorate the older. This patch makes sure this contraint by ensuring UA shadow just before the first author shadow arrives, which is done in willAddShadowRoot(). Possible alternative is to insert the UA shadow before the existing author shadows. But it's tricky because ShadowRoot takes append-only design and there are assumptions that ShadowRoot is never removed once it is added to the host element. For this reason, UA shadow need to be the oldest. So what is the best way to find the UA shadow in the shadow list? In my understanding, your point is that we could have some explicit state to indicate the fact. Actually the patch contains a debug-only state for that purpose. We could use it not only for debug but also finding an ua shadow. On the other hand, I want to ensure the position invariant of UA shadow. If we search an UA shadow by inspecting its type, this invariant needs to be checked separately. This isn't very robust. After all, we need either A. to find ua shadow by type and assert the position invariant, or B. to find ua shadow by position and assert the type invariant. And this patch picks B. I don't think there is significant difference between these two approach. Well, in a second thought, I noticed that both A and Be can be felt fragile as long as such assertion is done by each caller site. I think we can introduce ElementShadow::userAgentShadowRoot() and having such an assert in that method. It will be more robust and intention revealing. I filed Bug 91564 for that. Does this make sense?

Hi Eric, though morrita has already answered some of your questions, but I would like to answer to you, too. (Just I want to explain what I did). I'm sorry my explanation was poor before. (In reply to comment #23) > (In reply to comment #14) > > (In reply to comment #13) > > > I understand that we must have the same behavior whether or not the shadow DOM for controls has been allocated, and that the page author should have to account for it. > > > > > > My question is - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. Why not have something in a shadow root object that identifies its purpose instead of inferring so much from the position? I see that ShadowRoot has a "type" in a debug build. Why only there? > > > > Sorry for late response. Actually there is a lot of reasons why I chose this design. > > (1) The ShadowDOM spec says that <video> or <audio> should behave like having UserAgentShadowDOM as the oldest ShadowDOM. This is the most important reason. > > Where does the spec say that? The only thing I see is say specifically about <video> and <audio> is that the subtree "Contains no insertion points"[1]. It also says that all HTML elements "must have an equivalent of a shadow DOM subtree that is created and populated at the time of element instantiation", which you don't do here. Why? > > > > (2) We don't want to make UserAgentShadowRoot special. It's our (= team implementing Shadow DOM) design choice. This is why "type" is there only in the debug build. > > Wait, isn't this patch necessary *because* UserAgentShadowRoot is special (different)? For (1), (2): > "must have an equivalent of a shadow DOM subtree that is created and populated at the time of element instantiation", which you don't do here Actually we would like to do this. However, if we don't use UserAgentShadowDOM, it uses unnecessary memory. So we introduce lazy instantiation pattern for UserAgentShadowRoot. This is what I did. > isn't this patch necessary *because* UserAgentShadowRoot is special (different)? And... in some meaning, UserAgentShadowRoot is different from AuthorShadowRoot. This is absolutely right. But we would like to treat with as the same as possible. So we use its type only for the ShadowRoot instantiation. After ShadowRoot is instantiated, we don't use its type. And it is right this patch is necessary because UserAgentShadoweRoot is special. Right. But the reason why UserAgentShadowRoot is special is that it should be instantiated when the host is instantiated. > > > > (3) Our code assumes that Shadow DOM is added into the top of the stack. Shadow DOM is a kind of stack, and we (intentionally) don't assume some Shadow DOM is added into not the top of Shadow DOM stack. > > Sigh, we are talking past one another. I asked "is wise to depend on the position of the node to determine its purpose" and your answer is "Our code assumes that Shadow DOM is added into the top of the stack"? > > I *know* that you assume that the shadow DOM for the controls is added first, and i *know* that it needs to be kept separate from user added shadow DOM - that is what the code does. My question is if it is a *good idea* for the code to do what it does. > > So again I ask - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. I understand that it is not so wise. However, since we decided not to use the type of ShadowRoot in release, I don't have any option to determine the type by position. I think you said we should have ShadowRoot type and we should use it. But as morrita said, I don't think there is not significant difference... Also... as morrita said, we woud like introduce userAgentShadowRoot() to make our code intention clearer. I hope this will solve your anticipation... > > > [1] http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#html-elements-and-their-shadow-dom-subtrees

Morrita - (In reply to comment #26) > > I'm sorry for r+ing this before your feedback. > Let me explain the rationale that forces ua-shadows being the first(oldest) shadow: > > At first, any UA shadow should be the oldest if there is such a shadow on the host element. > This is because author shadow should be able to "decorate" it like adding extra container > element or additional buttons around the original control. > And only younger shadows can decorate the older. > > This patch makes sure this contraint by ensuring UA shadow just before the first author > shadow arrives, which is done in willAddShadowRoot(). > Possible alternative is to insert the UA shadow before the existing author shadows. > But it's tricky because ShadowRoot takes append-only design and there are assumptions > that ShadowRoot is never removed once it is added to the host element. > > For this reason, UA shadow need to be the oldest. So what is the best way to find the UA shadow > in the shadow list? > > In my understanding, your point is that we could have some explicit state to indicate the fact. > Actually the patch contains a debug-only state for that purpose. We could use it not only for debug > but also finding an ua shadow. > > On the other hand, I want to ensure the position invariant of UA shadow. > If we search an UA shadow by inspecting its type, this invariant needs to be checked separately. > This isn't very robust. After all, we need either > A. to find ua shadow by type and assert the position invariant, or > B. to find ua shadow by position and assert the type invariant. > > And this patch picks B. I don't think there is significant difference between these two approach. > > Well, in a second thought, I noticed that both A and Be can be felt fragile > as long as such assertion is done by each caller site. > I think we can introduce ElementShadow::userAgentShadowRoot() > and having such an assert in that method. It will be more robust and intention revealing. > I filed Bug 91564 for that. > > Does this make sense? My concern has been with the pervasive use of "oldest" and "younger" - implementation details - throughout the code and tests, so my thought to use the debug-only 'type' field was just a suggestion. Encapsulating the use of position with a method like ElementShadow::userAgentShadowRoot (with appropriate assertions) sounds like a great solution. Thank you for taking the time to think about my concerns and for your reply!

(In reply to comment #27) > (In reply to comment #23) > Actually we would like to do this. However, if we don't use UserAgentShadowDOM, it uses unnecessary memory. > So we introduce lazy instantiation pattern for UserAgentShadowRoot. This is what I did. > I understand why it is done lazily, but that is contrary to the spec text. If the spec says something that doesn't make sense, we might decide to do something else but we should *also* try to get the spec changed (fixed). Have you done this? I think you know where to find the spec editor :-). > > Sigh, we are talking past one another. I asked "is wise to depend on the position of the node to determine its purpose" and your answer is "Our code assumes that Shadow DOM is added into the top of the stack"? > > > > I *know* that you assume that the shadow DOM for the controls is added first, and i *know* that it needs to be kept separate from user added shadow DOM - that is what the code does. My question is if it is a *good idea* for the code to do what it does. > > > > So again I ask - is wise to depend on the position of the node to determine its purpose? This seems extremely fragile. > > I understand that it is not so wise. However, since we decided not to use the type of ShadowRoot in release, I don't have any option to determine the type by position. > First, let me say that I am happy with Morrita's suggested solution. I have to point out, however, that your argument here is nonsense. Saying that you "don't have any option" because of a decision that *you* made is circular reasoning, it is a logical fallacy. You decided not to include the ShadowRoot type in a release build, so *you* could change that decision.

(In reply to comment #29) > (In reply to comment #27) > > (In reply to comment #23) > > Actually we would like to do this. However, if we don't use UserAgentShadowDOM, it uses unnecessary memory. > > So we introduce lazy instantiation pattern for UserAgentShadowRoot. This is what I did. > > > I understand why it is done lazily, but that is contrary to the spec text. If the spec says something that doesn't make sense, we might decide to do something else but we should *also* try to get the spec changed (fixed). Have you done this? I think you know where to find the spec editor :-). To be fair, the way I wrote this is using the "equivalence" language. In other words, as long as the lazy instantiation is not detectable from script, we're satisfying the spec text.