| Summary: | Display a TextTrackCue when snap-to-lines flag is set | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Victor Carbune <vcarbune> | ||||||||||||||||||||||||||
| Component: | Media | Assignee: | Victor Carbune <vcarbune> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | cmarcelo, dglazkov, dominicc, eae, eric.carlson, eric, feature-media-reviews, gyuyoung.kim, inferno, jamesr, leviw, macpherson, menard, ojan, rakuco, silviapf, tony, vcarbune, webkit.review.bot | ||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||
| Bug Depends on: | 85657, 94656 | ||||||||||||||||||||||||||||
| Bug Blocks: | 79347 | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
|
Description
Victor Carbune
2012-02-27 23:31:35 PST
Created attachment 147993 [details]
Draft patch
Better design in progress
Comment on attachment 147993 [details] Draft patch Attachment 147993 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12961941 Created attachment 157876 [details]
Updated implementation and added tests
The better design involves an independent renderer for cues, rather than the whole container
Hmm, I meant to only have text dumps of the render tree. Not sure exactly what's the proper way to do this without generating png pixel dumps as well. Comment on attachment 157876 [details] Updated implementation and added tests Attachment 157876 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13475657 New failing tests: http/tests/media/media-source/video-media-source-abort.html http/tests/media/media-source/video-media-source-objects.html http/tests/media/media-source/video-media-source-event-attributes.html http/tests/media/media-source/video-media-source-play.html http/tests/media/media-source/video-media-source-state-changes.html http/tests/media/video-referer.html http/tests/media/video-cancel-load.html http/tests/media/media-source/video-media-source-errors.html http/tests/appcache/video.html http/tests/media/video-play-stall-before-meta-data.html http/tests/media/pdf-served-as-pdf.html http/tests/media/media-source/video-media-source-seek.html http/tests/media/video-useragent.html http/tests/security/contentSecurityPolicy/media-src-allowed.html fast/canvas/webgl/shader-precision-format.html http/tests/media/text-served-as-text.html http/tests/media/video-served-as-text.html http/tests/media/video-load-suspend.html http/tests/media/media-source/video-media-source-add-and-remove-buffers.html http/tests/media/video-query-url.html http/tests/media/video-error-abort.html http/tests/media/remove-while-loading.html http/tests/security/video-cross-origin-readback.html http/tests/media/video-throttled-load-metadata.html http/tests/media/video-load-twice.html http/tests/security/contentSecurityPolicy/media-src-blocked.html http/tests/media/media-document.html Created attachment 157877 [details]
Archive of layout-test-results from gce-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 157876 [details] Updated implementation and added tests Attachment 157876 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13478608 Created attachment 157897 [details]
Updated patch
Fixed minor mistake that triggered some test fails
Comment on attachment 157897 [details] Updated patch Attachment 157897 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13484393 New failing tests: media/track/track-cue-rendering-horizontal.html media/track/track-cue-rendering-vertical.html Created attachment 157901 [details]
Archive of layout-test-results from gce-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 157897 [details] Updated patch Attachment 157897 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13489003 Comment on attachment 157897 [details] Updated patch Attachment 157897 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13475955 Comment on attachment 157897 [details] Updated patch Attachment 157897 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13481697 Comment on attachment 157897 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=157897&action=review I don't know enough about rendering to r+ this so I am only making minor style comments. > Source/WebCore/html/track/TextTrack.cpp:302 > + bool renderedKind = m_kind == captionsKeyword() || m_kind == subtitlesKeyword(); > + bool renderedMode = m_mode == SHOWING || m_showingByDefault; > + > + return renderedKind && renderedMode; This would could also be done with early returns: if (m_kind != captionsKeyword() && m_kind != subtitlesKeyword()) return false; if (m_mode != SHOWING && !m_showingByDefault) return false; return true; > Source/WebCore/html/track/TextTrackCue.cpp:227 > - m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalLr; > - m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalRl; > + m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalRl; > + m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalLr; Good fix! > Source/WebCore/html/track/TextTrackCue.cpp:552 > + // If cue is not associated with a text track, return â1 and abort these > + // steps. "â1" -> "-1" > Source/WebCore/rendering/RenderTextTrackCue.cpp:35 > + LayoutStateMaintainer statePusher( > + view(), > + this, > + locationOffset(), > + hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode()); Nit: the extra line breaks are not necessary. > Source/WebCore/rendering/RenderTextTrackCue.cpp:131 > + if (cue->vertical() == Horizontal > + && ((step < 0 && top < 0) || (step > 0 && bottom > > + parentBlock->absoluteBoundingBoxRect().height()))) > + jumpToSwitchDirection = true; I don't think the second line break helps readablilty. WebKit style is to use braces in a case where the condition takes more than one line. > Source/WebCore/rendering/RenderTextTrackCue.cpp:142 > + if (cue->vertical() != Horizontal > + && ((step < 0 && left < 0) || (step > 0 && right > > + parentBlock->absoluteBoundingBoxRect().width()))) > + jumpToSwitchDirection = true; Ditto. Created attachment 158095 [details]
Minor changes
(In reply to comment #14) > (From update of attachment 157897 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=157897&action=review > > I don't know enough about rendering to r+ this so I am only making minor style comments. CC'ed Ojan and Tony, hopefully they will have some time to look over this bug. > > Source/WebCore/html/track/TextTrack.cpp:302 > > + bool renderedKind = m_kind == captionsKeyword() || m_kind == subtitlesKeyword(); > > + bool renderedMode = m_mode == SHOWING || m_showingByDefault; > > + > > + return renderedKind && renderedMode; > > This would could also be done with early returns: > > if (m_kind != captionsKeyword() && m_kind != subtitlesKeyword()) > return false; > if (m_mode != SHOWING && !m_showingByDefault) > return false; > > return true; I actually did this initially, not sure why I changed. Done. > > Source/WebCore/html/track/TextTrackCue.cpp:227 > > - m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalLr; > > - m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalRl; > > + m_displayWritingModeMap[VerticalGrowingLeft] = CSSValueVerticalRl; > > + m_displayWritingModeMap[VerticalGrowingRight] = CSSValueVerticalLr; > > Good fix! > > > Source/WebCore/html/track/TextTrackCue.cpp:552 > > + // If cue is not associated with a text track, return â1 and abort these > > + // steps. > > "â1" -> "-1" Done. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:35 > > + LayoutStateMaintainer statePusher( > > + view(), > > + this, > > + locationOffset(), > > + hasTransform() || hasReflection() || style()->isFlippedBlocksWritingMode()); > > Nit: the extra line breaks are not necessary. Done. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:131 > > + if (cue->vertical() == Horizontal > > + && ((step < 0 && top < 0) || (step > 0 && bottom > > > + parentBlock->absoluteBoundingBoxRect().height()))) > > + jumpToSwitchDirection = true; > > I don't think the second line break helps readablilty. WebKit style is to use braces in a case where the condition takes more than one line. > > > Source/WebCore/rendering/RenderTextTrackCue.cpp:142 > > + if (cue->vertical() != Horizontal > > + && ((step < 0 && left < 0) || (step > 0 && right > > > + parentBlock->absoluteBoundingBoxRect().width()))) > > + jumpToSwitchDirection = true; > > Ditto. Done. I have temporarily marked the tests as IMAGE fail, they will anyway need rebaseline if they remain like this (the image diffs returned by the bots were 0%). The Mac and Qt bots might still fail the build as I need to add the file properly to XCode, but I will leave this after some more reviews. Created attachment 158099 [details]
Rebased patch
Comment on attachment 158099 [details] Rebased patch Attachment 158099 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13492241 Comment on attachment 158099 [details] Rebased patch Attachment 158099 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13492255 Comment on attachment 158099 [details] Rebased patch Attachment 158099 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13486646 Comment on attachment 158099 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=158099&action=review r- since there are some compile errors on qt and mac. > Source/WebCore/html/track/TextTrackCue.cpp:165 > + String translateX = "-" + String::number(position.first) + "%"; > + String translateY = "-" + String::number(position.second) + "%"; > + String webkitTransformTranslateValue = "translate(" + translateX + "," + translateY + ")"; Can we use a StringBuilder here? It should be a little bit faster. Alternately, maybe you can use String::format. > Source/WebCore/html/track/TextTrackCue.h:61 > + void applyCssProperties(); Nit: applyCSSProperties() > Source/WebCore/html/track/TextTrackCue.h:146 > + std::pair<double, double> getCssPosition() const; > + int getCssSize() const; > + int getCssWritingMode() const; Nit: Normally CSS is always capitalized in WebKit, e.g., getCSSPosition(). The 'get' prefix is also rare, but I guess this file already has some cases with it. > Source/WebCore/html/track/TextTrackCue.h:222 > + std::pair<double, double> m_displayPosition; Nit: Can we use a FloatPoint or does this need to be doubles? > Source/WebCore/rendering/RenderTextTrackCue.cpp:42 > + DEFINE_STATIC_LOCAL(const String, Horizontal, ("")); > + DEFINE_STATIC_LOCAL(const String, VerticalGrowingLeft, ("rl")); Doing String comparisons seems slow. Can we add helper methods on TextTrackCue that return an enum or a bool? > Source/WebCore/rendering/RenderTextTrackCue.cpp:62 > + // 1. Horizontal: Let step be the height of the first line box in boxes. > + // Vertical: Let step be the width of the first line box in boxes. In the long run, I think these comments just add noise. There are old comments like this in places like CSSParser.cpp, but they tend to get out of date and mislead. This code would be easier to read without the comments. You could break this layout() method into some helper methods. That might help to document what's going on. > Source/WebCore/rendering/RenderTextTrackCue.cpp:63 > + double step = cue->vertical() == Horizontal ? firstLineBox->height() : firstLineBox->width(); Why a double? It looks like height() and width() are floats. I suspect you want to convert these to LayoutUnit since we're now switching to layout positions. > Source/WebCore/rendering/RenderTextTrackCue.cpp:77 > + double position = step * linePosition; This should probably be a float too. > Source/WebCore/rendering/RenderTextTrackCue.cpp:122 > + // FIXME(BUG ...): This overlap test is in O(N^2), there might be a much more > + // optimized way too do this using some WebKit rendering methods. This is just the text tracks, right? Is there an upper bound on how many there can be? > Source/WebCore/rendering/RenderTextTrackCue.cpp:123 > + for (RenderObject *box = previousSibling(); box; box = box->previousSibling()) { * in the wrong place > Source/WebCore/rendering/RenderTextTrackCue.cpp:145 > + int bottom = top + firstLineBox->height(); > + int right = left + firstLineBox->width(); Is it OK to use ints here? Do you want to truncate or round? > Source/WebCore/rendering/RenderTextTrackCue.cpp:201 > + setNeedsLayout(false); I think needslayout was already set to false by RenderBlock::layout(). > Source/WebCore/rendering/RenderTextTrackCue.h:37 > + RenderTextTrackCue(Node*); explicit Levi or Emil can probably provide feedback as to the right units to use and when to convert from floats to LayoutUnit. Comment on attachment 158099 [details] Rebased patch View in context: https://bugs.webkit.org/attachment.cgi?id=158099&action=review >> Source/WebCore/rendering/RenderTextTrackCue.cpp:62 >> + // Vertical: Let step be the width of the first line box in boxes. > > In the long run, I think these comments just add noise. There are old comments like this in places like CSSParser.cpp, but they tend to get out of date and mislead. This code would be easier to read without the comments. > > You could break this layout() method into some helper methods. That might help to document what's going on. Functions would make this a lot clearer. Why are the comments numbered? >> Source/WebCore/rendering/RenderTextTrackCue.cpp:63 >> + double step = cue->vertical() == Horizontal ? firstLineBox->height() : firstLineBox->width(); > > Why a double? It looks like height() and width() are floats. I suspect you want to convert these to LayoutUnit since we're now switching to layout positions. LineBox positions are floats. > Source/WebCore/rendering/RenderTextTrackCue.cpp:113 > + // 11. Step loop: > + for (;;) { While not the first boundless for loop in WebCore, they're pretty rare. Is this really necessary? "Step loop" means nothing to me... >> Source/WebCore/rendering/RenderTextTrackCue.cpp:145 >> + int top = absoluteBoundingBoxRect().y(); >> + int left = absoluteBoundingBoxRect().x(); >> + int bottom = top + firstLineBox->height(); >> + int right = left + firstLineBox->width(); > > Is it OK to use ints here? Do you want to truncate or round? You almost assuredly don't want to just assign these to an int but rather enclose the line, no? Why are you using absolute coordinates here instead of local LayoutUnits? (In reply to comment #21) > (From update of attachment 158099 [details]) > > Source/WebCore/rendering/RenderTextTrackCue.cpp:62 > > + // 1. Horizontal: Let step be the height of the first line box in boxes. > > + // Vertical: Let step be the width of the first line box in boxes. > > In the long run, I think these comments just add noise. There are old comments like this in places like CSSParser.cpp, but they tend to get out of date and mislead. This code would be easier to read without the comments. (In reply to comment #23) > Functions would make this a lot clearer. Why are the comments numbered? The comments are text straight from the spec. I think they are extremely useful for two reasons: 1) it makes it much easier to check the logic against what the spec requires, and 2) the comments make it much easier to find parts that need to be changed because of spec updates. The later is a real concern because the spec is not final and will change. We have comments like this in HTMLMediaElement and they have helped in precisely this way. I agree that they need to be kept in sync with both the code and the spec, but that is exactly the point of including them. Re: comments
I would do something like:
RenderTextTrackCue::layout()
{
...
computePositionAndStep(&position, &step);
placesBoxesInDefaultPosition(position);
fixOverlappingTextBoxes(step);
...
}
// This is steps 1-6 from ...
RenderTextTrackCue::computePositionAndStep(&position, &step)
{
...
}
// This is steps 7-9 from ...
RenderTextTrackCue::placesBoxesInDefaultPosition(position)
{
}
// This is steps 11-19 from ...
RenderTextTrackCue::fixOverlappingTextBoxes(step)
{
}
That said, I don't feel super strongly about this, but I doubt these comments will be that useful 5 years from now.
(In reply to comment #25) > Re: comments > > I would do something like: > > RenderTextTrackCue::layout() > { > ... > computePositionAndStep(&position, &step); > placesBoxesInDefaultPosition(position); > fixOverlappingTextBoxes(step); > ... > } > > // This is steps 1-6 from ... > RenderTextTrackCue::computePositionAndStep(&position, &step) > { > ... > } > > // This is steps 7-9 from ... > RenderTextTrackCue::placesBoxesInDefaultPosition(position) > { > } > > // This is steps 11-19 from ... > RenderTextTrackCue::fixOverlappingTextBoxes(step) > { > } I just did a refactoring following pretty much the same idea, I will upload the patch and if the naming / grouping of steps isn't good enough, I will re-iterate. > That said, I don't feel super strongly about this, but I doubt these comments will be that useful 5 years from now. I second Eric's suggestion on keeping them, at least until the WebVTT specification can be anywhere near "stable". However, there are important parts that will certainly change and it's just easier to track for us. Created attachment 158681 [details]
Updated patch
Significant refactoring
Thanks for taking the time and reviewing this! I have addressed most of the comments - the mac and qt still fail, but I'll fix them at the next iteration. (In reply to comment #21) > (From update of attachment 158099 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158099&action=review > > r- since there are some compile errors on qt and mac. > > > Source/WebCore/html/track/TextTrackCue.cpp:165 > > + String translateX = "-" + String::number(position.first) + "%"; > > + String translateY = "-" + String::number(position.second) + "%"; > > + String webkitTransformTranslateValue = "translate(" + translateX + "," + translateY + ")"; > > Can we use a StringBuilder here? It should be a little bit faster. Alternately, maybe you can use String::format. Done, used String::format. > > Source/WebCore/html/track/TextTrackCue.h:61 > > + void applyCssProperties(); > > Nit: applyCSSProperties() Done. > > Source/WebCore/html/track/TextTrackCue.h:146 > > + std::pair<double, double> getCssPosition() const; > > + int getCssSize() const; > > + int getCssWritingMode() const; > > Nit: Normally CSS is always capitalized in WebKit, e.g., getCSSPosition(). The 'get' prefix is also rare, but I guess this file already has some cases with it. Capitalized CSS. > > Source/WebCore/html/track/TextTrackCue.h:222 > > + std::pair<double, double> m_displayPosition; > > Nit: Can we use a FloatPoint or does this need to be doubles? They don't need to be doubles, I changed them to floats. I would leave the the refactoring to FloatPoint for a different patch, if that's fine. > > > Source/WebCore/rendering/RenderTextTrackCue.cpp:42 > > + DEFINE_STATIC_LOCAL(const String, Horizontal, ("")); > > + DEFINE_STATIC_LOCAL(const String, VerticalGrowingLeft, ("rl")); > > Doing String comparisons seems slow. Can we add helper methods on TextTrackCue that return an enum or a bool? I added getWritingDirection and made the enum public and used it in this file as well. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:63 > > + double step = cue->vertical() == Horizontal ? firstLineBox->height() : firstLineBox->width(); > > Why a double? It looks like height() and width() are floats. I suspect you want to convert these to LayoutUnit since we're now switching to layout positions. Switched to LayoutUnit everywhere. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:77 > > + double position = step * linePosition; > > This should probably be a float too. Switched to LayoutUnit. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:122 > > + // FIXME(BUG ...): This overlap test is in O(N^2), there might be a much more > > + // optimized way too do this using some WebKit rendering methods. > > This is just the text tracks, right? Is there an upper bound on how many there can be? It's not an upper bound of how many cues at once you may have, but as the screen fills up this probably gets very inefficient. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:123 > > + for (RenderObject *box = previousSibling(); box; box = box->previousSibling()) { > > * in the wrong place Done. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:145 > > + int bottom = top + firstLineBox->height(); > > + int right = left + firstLineBox->width(); > > Is it OK to use ints here? Do you want to truncate or round? No, mistake. Changed to LayoutUnit. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:201 > > + setNeedsLayout(false); > > I think needslayout was already set to false by RenderBlock::layout(). Indeed, removed. > > Source/WebCore/rendering/RenderTextTrackCue.h:37 > > + RenderTextTrackCue(Node*); > > explicit Done. (In reply to comment #23) > > Why a double? It looks like height() and width() are floats. I suspect you want to convert these to LayoutUnit since we're now switching to layout positions. > > LineBox positions are floats. Changed to LayoutUnit. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:113 > > + // 11. Step loop: > > + for (;;) { > > While not the first boundless for loop in WebCore, they're pretty rare. Is this really necessary? "Step loop" means nothing to me... Refactored. > >> Source/WebCore/rendering/RenderTextTrackCue.cpp:145 > >> + int top = absoluteBoundingBoxRect().y(); > >> + int left = absoluteBoundingBoxRect().x(); > >> + int bottom = top + firstLineBox->height(); > >> + int right = left + firstLineBox->width(); > > > > Is it OK to use ints here? Do you want to truncate or round? > > You almost assuredly don't want to just assign these to an int but rather enclose the line, no? Why are you using absolute coordinates here instead of local LayoutUnits? Oops, these should have been local LayoutUnits indeed, fixed. Comment on attachment 158681 [details] Updated patch Attachment 158681 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13511366 Comment on attachment 158681 [details] Updated patch Attachment 158681 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13511369 Comment on attachment 158681 [details] Updated patch Attachment 158681 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13511372 Comment on attachment 158681 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=158681&action=review I mainly just looked at the code in rendering, but it looks OK to me. Looks like you still have compile errors on Qt and Mac. > Source/WebCore/rendering/RenderTextTrackCue.cpp:45 > + m_cue->snapToLines() ? repositionCueSnapToLinesSet() : repositionCueSnapToLinesNotSet(); Nit: I would just use a regular if/else here. I always feel weird using a ternary without a return/assignment. > Source/WebCore/rendering/RenderTextTrackCue.cpp:112 > +inline bool RenderTextTrackCue::isOutside() const Nit: I normally prefer to let the compiler decide to inline or not unless we know this is going to cause slowness. I guess it's not a big deal here since we call each of these functions once. > Source/WebCore/rendering/RenderTextTrackCue.cpp:168 > + if (m_cue->getWritingDirection() != TextTrackCue::Horizontal) Nit: Can you use an 'else' here? > Source/WebCore/rendering/RenderTextTrackCue.cpp:216 > + // FIXME(BUG 84296): Implement overlapping detection when snap-to-lines is not set. Nit: I've never seen this FIXME() syntax. I would just put the URL after the comment. E.g., // FIXME: Implement overlapping detection when snap-to-lines is not set. http://wkb.ug/84296 > Source/WebCore/rendering/RenderTextTrackCue.h:60 > + TextTrackCue* m_cue; > + > + FloatPoint m_fallbackPosition; > + LayoutUnit m_position; > + LayoutUnit m_step; Having all these member variables is unfortunate. They'll use memory after layout is done even though we don't need them. One way to avoid this would be to make a struct with the params you need and pass it around to each function or just pass a bunch of params around. Created attachment 159189 [details]
Updated patch
Addressed comments and solved Mac build issue
(In reply to comment #32) > (From update of attachment 158681 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=158681&action=review > > I mainly just looked at the code in rendering, but it looks OK to me. Looks like you still have compile errors on Qt and Mac. I should have fixed the compile issues for Mac and added the test cases there as well. I'm not sure about Qt (maybe the IF ENABLE(VIDEO_TRACK) guard I correct fixes it). Other changes: * Changed the TextTrackCueBox to extend HTMLElement, as HTMLDivElement was breaking WebView compiling on Mac. * Changed the RenderTextTrackCue constructor to accept TextTrackCueBox type node. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:45 > > + m_cue->snapToLines() ? repositionCueSnapToLinesSet() : repositionCueSnapToLinesNotSet(); > > Nit: I would just use a regular if/else here. I always feel weird using a ternary without a return/assignment. Done. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:112 > > +inline bool RenderTextTrackCue::isOutside() const > > Nit: I normally prefer to let the compiler decide to inline or not unless we know this is going to cause slowness. I guess it's not a big deal here since we call each of these functions once. Sure, done. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:168 > > + if (m_cue->getWritingDirection() != TextTrackCue::Horizontal) > > Nit: Can you use an 'else' here? Done. > > Source/WebCore/rendering/RenderTextTrackCue.cpp:216 > > + // FIXME(BUG 84296): Implement overlapping detection when snap-to-lines is not set. > > Nit: I've never seen this FIXME() syntax. I would just put the URL after the comment. E.g., > // FIXME: Implement overlapping detection when snap-to-lines is not set. http://wkb.ug/84296 Oops, not sure why I used it like this.. Fixed. > > Source/WebCore/rendering/RenderTextTrackCue.h:60 > > + TextTrackCue* m_cue; > > + > > + FloatPoint m_fallbackPosition; > > + LayoutUnit m_position; > > + LayoutUnit m_step; > > Having all these member variables is unfortunate. They'll use memory after layout is done even though we don't need them. One way to avoid this would be to make a struct with the params you need and pass it around to each function or just pass a bunch of params around. Left the cue and fallback position as local variables. Comment on attachment 159189 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=159189&action=review > Source/WebCore/html/track/TextTrackCue.h:62 > + virtual const AtomicString& shadowPseudoId() const; Nit: OVERRIDE > Source/WebCore/html/track/TextTrackCue.h:67 > + virtual RenderObject* createRenderer(RenderArena*, RenderStyle*); Nit: OVERRIDE > Source/WebCore/rendering/RenderTextTrackCue.h:45 > + virtual void layout(); Nit: OVERRIDE Created attachment 159736 [details]
Landing patch
Fixed nits
Comment on attachment 159736 [details] Landing patch Clearing flags on attachment: 159736 Committed r126233: <http://trac.webkit.org/changeset/126233> All reviewed patches have been landed. Closing bug. This breaks the build on chromium mac and chromium linux w/ clang:
In file included from ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.cpp:36:
../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:216:9: error: private field 'm_displayHeight' is not used [-Werror,-Wunused-private-field]
int m_displayHeight;
^
../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:217:9: error: private field 'm_displayWidth' is not used [-Werror,-Wunused-private-field]
int m_displayWidth;
(In reply to comment #39) > This breaks the build on chromium mac and chromium linux w/ clang: > > In file included from ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.cpp:36: > ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:216:9: error: private field 'm_displayHeight' is not used [-Werror,-Wunused-private-field] > int m_displayHeight; > ^ > ../../third_party/WebKit/Source/WebCore/html/track/TextTrackCue.h:217:9: error: private field 'm_displayWidth' is not used [-Werror,-Wunused-private-field] > int m_displayWidth; Sorry, I'm on it right now Re-opened since this is blocked by 94656 Created attachment 159840 [details]
Build fix
Removed the two unused vars
Comment on attachment 159840 [details] Build fix Rejecting attachment 159840 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: rack-cue-rendering-vertical-expected.txt patching file LayoutTests/platform/chromium/TestExpectations Hunk #1 succeeded at 3448 (offset -4 lines). patching file LayoutTests/platform/mac/media/track/track-cue-rendering-horizontal-expected.txt patching file LayoutTests/platform/mac/media/track/track-cue-rendering-vertical-expected.txt Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Tony Chang']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13570395 Created attachment 160034 [details]
Rebased
Comment on attachment 160034 [details] Rebased Clearing flags on attachment: 160034 Committed r126372: <http://trac.webkit.org/changeset/126372> All reviewed patches have been landed. Closing bug. |