Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
949203002:
Separate the text track container from the media controls (Closed)
|
Created:
5 years, 10 months ago by philipj_slow Modified:
5 years, 9 months ago CC:
blink-reviews, blink-reviews-rendering, feature-media-reviews_chromium.org, gasubic, sof, eae+blinkwatch, eric.carlson_apple.com, leviw+renderwatch, Dominik Röttsches, blink-reviews-dom_chromium.org, dglazkov+blink, nessy, jchaffraix+rendering, pdr+renderingwatchlist_chromium.org, blink-reviews-html_chromium.org, zoltan1, vcarbune.chromium, rwlbuis Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionSeparate the text track container from the media controls
Having the text track container as a part of the media controls makes it
harder to show and hide the controls as a group, which would be useful.
One benefit of the old approach was that the controls automatically
wouldn't overlap the cues. This is now broken, and two tests are
affected. This will be addressed in a follow-up:
https://codereview.chromium.org/1018593004/
TextTrackContainer is never hidden, as that used MediaControlElement's
show() and hide(). If it does not have any cues nothing will be
rendered, it will just sit there. If an optimization of this kind is
worthwhile, it should be to remove the TextTrackContainer entirely from
the DOM.
Some bits of mediaControls.css that should have no effect are removed.
BUG=448795
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192263
Patch Set 1 #Patch Set 2 : only one text track container thank you #Patch Set 3 : same size as media controls #
Total comments: 35
Patch Set 4 : address feedback #Patch Set 5 : update tests #Patch Set 6 : rebase #Patch Set 7 : rebase #Patch Set 8 : remove lies in comment #Patch Set 9 : rebase #
Total comments: 6
Patch Set 10 : address nits #Messages
Total messages: 34 (3 generated)
only one text track container thank you
same size as media controls
philipj@opera.com changed reviewers: + fs@opera.com, rune@opera.com
I was moving things around and tinkering and suddenly everything just worked. This is still a bit rough, but I'd like your feedback, so review with landing in mind. I'll comment on a few things that could need some extra attention.
Braindump. Rune, search for your name for the bits I added you for. https://codereview.chromium.org/949203002/diff/40001/Source/core/css/mediaCon... File Source/core/css/mediaControls.css (left): https://codereview.chromium.org/949203002/diff/40001/Source/core/css/mediaCon... Source/core/css/mediaControls.css:349: padding-bottom: 5px; I don't know why this was here. https://codereview.chromium.org/949203002/diff/40001/Source/core/css/mediaCon... File Source/core/css/mediaControls.css (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/css/mediaCon... Source/core/css/mediaControls.css:347: width: inherit; I think these actually have no effect since the width and height are set LayoutMedia::layout(). The same is true of the media controls, however. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3188: static void assertShadowRootChildren(ShadowRoot& shadowRoot) I don't know if this seems pedantic. I failed to get it right before I added this assert, so here it is. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3231: // FIXME: this probably doesn't hold Um, right. fs, I think we can get here for audio elements as well. Should the code on HTMLMediaElement that is only concerned with rendering be moved to HTMLVideoElement do you think? Could also simply do nothing here if it's an audio element, which would make the check in TextTrackContainer::updateDisplay redundant. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:418: TextTrackContainer* textTrackContainer() const; Oops, will remove this. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... File Source/core/html/track/TextTrackContainer.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... Source/core/html/track/TextTrackContainer.cpp:49: element->setShadowPseudoId(AtomicString("-webkit-media-text-track-container", AtomicString::ConstructFromLiteral)); Maybe this should still be under html/shadow/ because of this, but it seems to me that this shadow id could be removed entirely by moving just a few things from video::-webkit-media-text-track-container to C++. Maybe the color and -webkit-user-select is enough. Not in this CL though. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... Source/core/html/track/TextTrackContainer.cpp:130: videoBox = toLayoutVideo(mediaElement.renderer())->videoBox(); I don't understand why the video size is used here when the whole element is used for rendering. I filed a spec bug for a related issue: https://www.w3.org/Bugs/Public/show_bug.cgi?id=28105 I'm trying to change as little code as possible here, but if you think there should be FIXMEs somewhere just say so. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... File Source/core/html/track/TextTrackContainer.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... Source/core/html/track/TextTrackContainer.h:39: class TextTrackContainer final : public HTMLDivElement { I'm not sure if this actually needs to be an HTMLDivElement. I tried with an Element but you can't create an element without a name, so I had to pick a name, and I picked div... https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutMedia.cpp:60: for (LayoutObject* child = m_children.firstChild(); child; child = child->nextSibling()) { Rune, does this make any sense? I couldn't find any other LayoutObjects iterating its children like this which I have very low confidence that it's correct. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutMedia.cpp:67: layoutBox->setLocation(LayoutPoint(borderLeft(), borderTop()) + LayoutSize(paddingLeft(), paddingTop())); Not sure if this could be done with width/height: inherit instead. Would that be more elegant if it worked? https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutTextTrackContainer.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.cpp:33: #include "core/html/shadow/MediaControlElementTypes.h" OK, this is ugly. Should I replace this with a simple node()->shadowHost() maybe? https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.cpp:46: if (style()->display() == NONE) Rune, is this even possible? I thought that LayoutObjects wouldn't even be created if display is none.
The failing tests are because cues can now be obscured by the controls. I guess I should fix this, but in a follow-up CL. Simply adding a box of the appropriate size seems to do the trick, so I'll work on that now.
https://codereview.chromium.org/949203002/diff/40001/Source/core/css/mediaCon... File Source/core/css/mediaControls.css (left): https://codereview.chromium.org/949203002/diff/40001/Source/core/css/mediaCon... Source/core/css/mediaControls.css:349: padding-bottom: 5px; On 2015/02/26 09:51:52, philipj_UTC7 wrote: > I don't know why this was here. I think this may have been used to get a "margin" between the controls and the cue rendering area. With the approach in the this CL it'd ov course be unnecessary. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3231: // FIXME: this probably doesn't hold On 2015/02/26 09:51:52, philipj_UTC7 wrote: > Um, right. fs, I think we can get here for audio elements as well. Should the > code on HTMLMediaElement that is only concerned with rendering be moved to > HTMLVideoElement do you think? Could also simply do nothing here if it's an > audio element, which would make the check in TextTrackContainer::updateDisplay > redundant. My (loose) plan so far wrt this has been to not even create the text track container for HTMLAudioElements, and then use that to determine if cue display should be updated or not. This is not entirely fleshed out yet though. Current thinking is to have something like a CueDisplayContext which is bound to the TextTrackContainer and passed to the CueTimeline/processing logic, where cues get notified and "attaches" (or detaches) themselves to that. I would support hoisting the check out of TextTrackContainer::updateDisplay as a first step, so maybe that's something that could be done here? https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:418: TextTrackContainer* textTrackContainer() const; On 2015/02/26 09:51:52, philipj_UTC7 wrote: > Oops, will remove this. I'd hope it should be possible to remove the one below too - in time... https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... File Source/core/html/track/TextTrackContainer.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... Source/core/html/track/TextTrackContainer.cpp:130: videoBox = toLayoutVideo(mediaElement.renderer())->videoBox(); On 2015/02/26 09:51:52, philipj_UTC7 wrote: > I don't understand why the video size is used here when the whole element is > used for rendering. I filed a spec bug for a related issue: > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28105 > > I'm trying to change as little code as possible here, but if you think there > should be FIXMEs somewhere just say so. AFAIK though this is only used for the thing below - i.e. resolving a font-size resembling "5vmin" - i.e. it's not actually used for cue positioning. (I half wonder if it wouldn't be better to just push this into the LayoutObject, because it seems like less like "hiding the ugly" - i.e. here it's less obvious that the lifecycle is violated...) https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... File Source/core/html/track/TextTrackContainer.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... Source/core/html/track/TextTrackContainer.h:53: IntRect m_videoDisplaySize; Could you make this an IntSize while we're shuffling it (the position part is never used anyway.) https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutMedia.cpp:60: for (LayoutObject* child = m_children.firstChild(); child; child = child->nextSibling()) { On 2015/02/26 09:51:52, philipj_UTC7 wrote: > Rune, does this make any sense? I couldn't find any other LayoutObjects > iterating its children like this which I have very low confidence that it's > correct. I believe RenderBlockFlow uses a while-loop (see RenderBlockFlow::layoutBlockChildren). (Now, using a RenderBlockFlow in place of LayoutMedia and a shadow "video layer" element might be nice, but feels like that might be making it too difficult at this time...) https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutMedia.cpp:87: if (child->node()->isMediaControls()) Maybe assert child->node() here (in this method) as well. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutTextTrackContainer.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.cpp:33: #include "core/html/shadow/MediaControlElementTypes.h" On 2015/02/26 09:51:52, philipj_UTC7 wrote: > OK, this is ugly. Should I replace this with a simple node()->shadowHost() > maybe? I get the feeling that could just use the layout tree here instead? (Combined with my suggestion in TextTrackContainer::updateDisplay I don't think you'd need to access HTMLMediaElement at all?) https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutTextTrackContainer.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.h:33: #include "core/rendering/RenderFlexibleBox.h" Don't see why this is needed... (or the core/html include.)
https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutMedia.cpp:60: for (LayoutObject* child = m_children.firstChild(); child; child = child->nextSibling()) { On 2015/02/26 09:51:52, philipj_UTC7 wrote: > Rune, does this make any sense? I couldn't find any other LayoutObjects > iterating its children like this which I have very low confidence that it's > correct. I'm not too familiar with layout, but this looks like a custom layout method that makes its children fill the content area of the replaced parent. It would possibly be cleaner to have a single block child of the LayoutMedia in which the media controls and text track container renderers would live, but handling anonymous renderers adds to the complexity as well. I don't feel confident to give you a definite answer about how it should work. I think having Rendering/Layout children of replaced content is messy to begin with. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutTextTrackContainer.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.cpp:33: #include "core/html/shadow/MediaControlElementTypes.h" On 2015/02/26 09:51:52, philipj_UTC7 wrote: > OK, this is ugly. Should I replace this with a simple node()->shadowHost() > maybe? I don't understand this comment about an include file. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.cpp:46: if (style()->display() == NONE) On 2015/02/26 09:51:52, philipj_UTC7 wrote: > Rune, is this even possible? I thought that LayoutObjects wouldn't even be > created if display is none. This shouldn't happen as long as the renderer is created through the RenderTreeBuilder. It will fail the rendererIsNeeded() check when display is 'none' afaict. Have you tried to git-blame this check?
On 2015/02/26 13:17:16, rune wrote: > https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... > File Source/core/layout/LayoutMedia.cpp (right): > > https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... > Source/core/layout/LayoutMedia.cpp:60: for (LayoutObject* child = > m_children.firstChild(); child; child = child->nextSibling()) { > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > Rune, does this make any sense? I couldn't find any other LayoutObjects > > iterating its children like this which I have very low confidence that it's > > correct. > > I'm not too familiar with layout, but this looks like a custom layout method > that makes its children fill the content area of the replaced parent. It would > possibly be cleaner to have a single block child of the LayoutMedia in which the > media controls and text track container renderers would live, but handling > anonymous renderers adds to the complexity as well. I don't feel confident to > give you a definite answer about how it should work. I think having > Rendering/Layout children of replaced content is messy to begin with. Maybe eventually changing things around so that there's a top-level block layout with individual child renderers for poster, controls, text track and the video itself would make sense. Not today. > https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... > File Source/core/layout/LayoutTextTrackContainer.cpp (right): > > https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... > Source/core/layout/LayoutTextTrackContainer.cpp:33: #include > "core/html/shadow/MediaControlElementTypes.h" > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > OK, this is ugly. Should I replace this with a simple node()->shadowHost() > > maybe? > > I don't understand this comment about an include file. Sorry, that was a cryptic note to self. I don't want to use toTextTrackContainer from that header as it's for media controls, it should be trivial to replace with something else. > https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... > Source/core/layout/LayoutTextTrackContainer.cpp:46: if (style()->display() == > NONE) > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > Rune, is this even possible? I thought that LayoutObjects wouldn't even be > > created if display is none. > > This shouldn't happen as long as the renderer is created through the > RenderTreeBuilder. It will fail the rendererIsNeeded() check when display is > 'none' afaict. > > Have you tried to git-blame this check? It's been there since the very start: https://trac.webkit.org/changeset/103242 I'll try removing it after this refactoring is done, in case it needs to be reverted.
address feedback
Thanks for the careful review. With the test update I think this is ready for final review, it wasn't has badly baked as I felt at first. I won't land this until there's also a ready-to-land CL for moving cues out of the way of <video controls>. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3231: // FIXME: this probably doesn't hold On 2015/02/26 12:30:51, fs wrote: > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > Um, right. fs, I think we can get here for audio elements as well. Should the > > code on HTMLMediaElement that is only concerned with rendering be moved to > > HTMLVideoElement do you think? Could also simply do nothing here if it's an > > audio element, which would make the check in TextTrackContainer::updateDisplay > > redundant. > > My (loose) plan so far wrt this has been to not even create the text track > container for HTMLAudioElements, and then use that to determine if cue display > should be updated or not. This is not entirely fleshed out yet though. > > Current thinking is to have something like a CueDisplayContext which is bound to > the TextTrackContainer and passed to the CueTimeline/processing logic, where > cues get notified and "attaches" (or detaches) themselves to that. > > I would support hoisting the check out of TextTrackContainer::updateDisplay as a > first step, so maybe that's something that could be done here? Not creating the container for audio sounds like a good plan. I'm going to opt for a separate CL, and have thrown some ideas of how it might look into https://codereview.chromium.org/962923002. I'll revert the asserts. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... File Source/core/html/track/TextTrackContainer.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... Source/core/html/track/TextTrackContainer.cpp:130: videoBox = toLayoutVideo(mediaElement.renderer())->videoBox(); On 2015/02/26 12:30:51, fs wrote: > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > I don't understand why the video size is used here when the whole element is > > used for rendering. I filed a spec bug for a related issue: > > https://www.w3.org/Bugs/Public/show_bug.cgi?id=28105 > > > > I'm trying to change as little code as possible here, but if you think there > > should be FIXMEs somewhere just say so. > > AFAIK though this is only used for the thing below - i.e. resolving a font-size > resembling "5vmin" - i.e. it's not actually used for cue positioning. (I half > wonder if it wouldn't be better to just push this into the LayoutObject, because > it seems like less like "hiding the ugly" - i.e. here it's less obvious that the > lifecycle is violated...) I've added a FIXME clarifying what I found odd here, also linking to the spec bug. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... File Source/core/html/track/TextTrackContainer.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/track/T... Source/core/html/track/TextTrackContainer.h:53: IntRect m_videoDisplaySize; On 2015/02/26 12:30:51, fs wrote: > Could you make this an IntSize while we're shuffling it (the position part is > never used anyway.) Done. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutMedia.cpp:60: for (LayoutObject* child = m_children.firstChild(); child; child = child->nextSibling()) { On 2015/02/26 12:30:51, fs wrote: > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > Rune, does this make any sense? I couldn't find any other LayoutObjects > > iterating its children like this which I have very low confidence that it's > > correct. > > I believe RenderBlockFlow uses a while-loop (see > RenderBlockFlow::layoutBlockChildren). (Now, using a RenderBlockFlow in place of > LayoutMedia and a shadow "video layer" element might be nice, but feels like > that might be making it too difficult at this time...) Yeah, I'll leave this alone if it's not too terrible. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutMedia.cpp:87: if (child->node()->isMediaControls()) On 2015/02/26 12:30:51, fs wrote: > Maybe assert child->node() here (in this method) as well. Done. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutTextTrackContainer.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.cpp:33: #include "core/html/shadow/MediaControlElementTypes.h" On 2015/02/26 12:30:52, fs wrote: > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > OK, this is ugly. Should I replace this with a simple node()->shadowHost() > > maybe? > > I get the feeling that could just use the layout tree here instead? (Combined > with my suggestion in TextTrackContainer::updateDisplay I don't think you'd need > to access HTMLMediaElement at all?) Do you mean the CueDisplayContext idea? Yeah, whatever the details, it seems like layout could be made unaware of HTMLMediaElement. For now Ill use the layout tree to get to it instead. https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... File Source/core/layout/LayoutTextTrackContainer.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/layout/Layou... Source/core/layout/LayoutTextTrackContainer.h:33: #include "core/rendering/RenderFlexibleBox.h" On 2015/02/26 12:30:52, fs wrote: > Don't see why this is needed... (or the core/html include.) Cleaned up a bit.
update tests
https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:418: TextTrackContainer* textTrackContainer() const; On 2015/02/26 12:30:51, fs wrote: > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > Oops, will remove this. > > I'd hope it should be possible to remove the one below too - in time... Who would be responsible for creating and inserting the TextTrackContainer?
rebase
https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.cpp:3231: // FIXME: this probably doesn't hold On 2015/02/27 07:13:51, philipj_UTC7 wrote: > On 2015/02/26 12:30:51, fs wrote: > > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > > Um, right. fs, I think we can get here for audio elements as well. Should > the > > > code on HTMLMediaElement that is only concerned with rendering be moved to > > > HTMLVideoElement do you think? Could also simply do nothing here if it's an > > > audio element, which would make the check in > TextTrackContainer::updateDisplay > > > redundant. > > > > My (loose) plan so far wrt this has been to not even create the text track > > container for HTMLAudioElements, and then use that to determine if cue display > > should be updated or not. This is not entirely fleshed out yet though. > > > > Current thinking is to have something like a CueDisplayContext which is bound > to > > the TextTrackContainer and passed to the CueTimeline/processing logic, where > > cues get notified and "attaches" (or detaches) themselves to that. > > > > I would support hoisting the check out of TextTrackContainer::updateDisplay as > a > > first step, so maybe that's something that could be done here? > > Not creating the container for audio sounds like a good plan. I'm going to opt > for a separate CL, and have thrown some ideas of how it might look into > https://codereview.chromium.org/962923002. > > I'll revert the asserts. Acknowledged. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:418: TextTrackContainer* textTrackContainer() const; On 2015/02/27 07:32:44, philipj_UTC7 wrote: > On 2015/02/26 12:30:51, fs wrote: > > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > > Oops, will remove this. > > > > I'd hope it should be possible to remove the one below too - in time... > > Who would be responsible for creating and inserting the TextTrackContainer? It would still be the HTMLMediaElement doing that, but it would wrap it up ("CueDisplayContext") and pass it to the cue timeline (this still to be fleshed out, so add pile of salt...)
I'll now park this review until the follow-up is done and then ask for final LGTY of the whole package. https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... File Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/949203002/diff/40001/Source/core/html/HTMLMed... Source/core/html/HTMLMediaElement.h:418: TextTrackContainer* textTrackContainer() const; On 2015/02/27 09:50:31, fs wrote: > On 2015/02/27 07:32:44, philipj_UTC7 wrote: > > On 2015/02/26 12:30:51, fs wrote: > > > On 2015/02/26 09:51:52, philipj_UTC7 wrote: > > > > Oops, will remove this. > > > > > > I'd hope it should be possible to remove the one below too - in time... > > > > Who would be responsible for creating and inserting the TextTrackContainer? > > It would still be the HTMLMediaElement doing that, but it would wrap it up > ("CueDisplayContext") and pass it to the cue timeline (this still to be fleshed > out, so add pile of salt...) OK, that sounds good with some imaginary salt :)
On 2015/02/27 10:00:01, philipj_UTC7 wrote: > I'll now park this review until the follow-up is done and then ask for final > LGTY of the whole package. Noted. FWIW, this lg tm under the current conditions.
On 2015/02/27 10:01:29, fs wrote: > On 2015/02/27 10:00:01, philipj_UTC7 wrote: > > I'll now park this review until the follow-up is done and then ask for final > > LGTY of the whole package. > > Noted. FWIW, this lg tm under the current conditions. Do think we could split this into to (roughly) the two parts: 1. The code movement 2. The structural change to the controls DOM ? I have some pending work that would like to base itself on top of some like (1), but doesn't necessarily depend on (2). I already have CLs similar to (1) locally, but they're not quite the same... I suppose I can even try to do the split myself if that ok.
On 2015/03/06 14:00:10, fs wrote: > On 2015/02/27 10:01:29, fs wrote: > > On 2015/02/27 10:00:01, philipj_UTC7 wrote: > > > I'll now park this review until the follow-up is done and then ask for final > > > LGTY of the whole package. > > > > Noted. FWIW, this lg tm under the current conditions. > > Do think we could split this into to (roughly) the two parts: > > 1. The code movement > 2. The structural change to the controls DOM > > ? > > I have some pending work that would like to base itself on top of some like (1), > but doesn't necessarily depend on (2). I already have CLs similar to (1) > locally, but they're not quite the same... I suppose I can even try to do the > split myself if that ok. I guess you mean the LayoutTextTrackContainerElement -> TextTrackContainer rename only? That's OK, if you want to do that separately I will rebase this CL.
On 2015/03/06 16:32:39, philipj_UTC7 wrote: > On 2015/03/06 14:00:10, fs wrote: > > On 2015/02/27 10:01:29, fs wrote: > > > On 2015/02/27 10:00:01, philipj_UTC7 wrote: > > > > I'll now park this review until the follow-up is done and then ask for > final > > > > LGTY of the whole package. > > > > > > Noted. FWIW, this lg tm under the current conditions. > > > > Do think we could split this into to (roughly) the two parts: > > > > 1. The code movement > > 2. The structural change to the controls DOM > > > > ? > > > > I have some pending work that would like to base itself on top of some like > (1), > > but doesn't necessarily depend on (2). I already have CLs similar to (1) > > locally, but they're not quite the same... I suppose I can even try to do the > > split myself if that ok. > > I guess you mean the LayoutTextTrackContainerElement -> TextTrackContainer > rename only? That's OK, if you want to do that separately I will rebase this CL. Yes, it's mostly that. I'll prepare a CL.
On 2015/03/06 16:34:32, fs wrote: > On 2015/03/06 16:32:39, philipj_UTC7 wrote: > > On 2015/03/06 14:00:10, fs wrote: > > > On 2015/02/27 10:01:29, fs wrote: > > > > On 2015/02/27 10:00:01, philipj_UTC7 wrote: > > > > > I'll now park this review until the follow-up is done and then ask for > > final > > > > > LGTY of the whole package. > > > > > > > > Noted. FWIW, this lg tm under the current conditions. > > > > > > Do think we could split this into to (roughly) the two parts: > > > > > > 1. The code movement > > > 2. The structural change to the controls DOM > > > > > > ? > > > > > > I have some pending work that would like to base itself on top of some like > > (1), > > > but doesn't necessarily depend on (2). I already have CLs similar to (1) > > > locally, but they're not quite the same... I suppose I can even try to do > the > > > split myself if that ok. > > > > I guess you mean the LayoutTextTrackContainerElement -> TextTrackContainer > > rename only? That's OK, if you want to do that separately I will rebase this > CL. > > Yes, it's mostly that. I'll prepare a CL. Done: https://codereview.chromium.org/988763002/ (not quite as straightforward as I'd hoped, but almost...)
rebase
remove lies in comment
rebase
Please also LG TM this when the follow-up is good to go.
drive-by-nit https://codereview.chromium.org/949203002/diff/160001/Source/core/html/track/... File Source/core/html/track/TextTrackContainer.cpp (left): https://codereview.chromium.org/949203002/diff/160001/Source/core/html/track/... Source/core/html/track/TextTrackContainer.cpp:121: // 11. Return output. Unmoored comment.
https://codereview.chromium.org/949203002/diff/160001/Source/core/layout/Layo... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/949203002/diff/160001/Source/core/layout/Layo... Source/core/layout/LayoutMedia.cpp:68: layoutBox->style()->setHeight(Length(newSize.height(), Fixed)); Should probably use "mutableStyleRef" here to avoid tripping the LayoutStyle constification effort. Accompanied by a TODO to try and get rid of it =)
LGTM w/ nits https://codereview.chromium.org/949203002/diff/160001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/949203002/diff/160001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:1713: crbug.com/448795 media/track/track-cue-rendering-horizontal.html [ NeedsRebaseline ] I take it these test are to be "replaced" by the new tests from the other CL?
address nits
Thanks for patient review! https://codereview.chromium.org/949203002/diff/160001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/949203002/diff/160001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:1713: crbug.com/448795 media/track/track-cue-rendering-horizontal.html [ NeedsRebaseline ] On 2015/03/20 10:01:14, fs wrote: > I take it these test are to be "replaced" by the new tests from the other CL? Yes, the overlap avoidance aspect of it is replaced by the new tests. https://codereview.chromium.org/949203002/diff/160001/Source/core/html/track/... File Source/core/html/track/TextTrackContainer.cpp (left): https://codereview.chromium.org/949203002/diff/160001/Source/core/html/track/... Source/core/html/track/TextTrackContainer.cpp:121: // 11. Return output. On 2015/03/20 08:02:14, sof wrote: > Unmoored comment. Thanks, I have added two comments explaining why this all looks a bit odd. https://codereview.chromium.org/949203002/diff/160001/Source/core/layout/Layo... File Source/core/layout/LayoutMedia.cpp (right): https://codereview.chromium.org/949203002/diff/160001/Source/core/layout/Layo... Source/core/layout/LayoutMedia.cpp:68: layoutBox->style()->setHeight(Length(newSize.height(), Fixed)); On 2015/03/20 09:51:01, fs wrote: > Should probably use "mutableStyleRef" here to avoid tripping the LayoutStyle > constification effort. Accompanied by a TODO to try and get rid of it =) Done.
The CQ bit was checked by philipj@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com Link to the patchset: https://codereview.chromium.org/949203002/#ps180001 (title: "address nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/949203002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://src.chromium.org/viewvc/blink?view=rev&revision=192263 |
