WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
79751
Display a TextTrackCue when snap-to-lines flag is set
https://bugs.webkit.org/show_bug.cgi?id=79751
Summary
Display a TextTrackCue when snap-to-lines flag is set
Victor Carbune
Reported
2012-02-27 23:31:35 PST
Substeps 10.13.1 - 10.13.19 (with snap-to-lines flag set), from section 3.5 of the WebVTT rendering rules:
http://dev.w3.org/html5/webvtt/#webvtt-cue-text-rendering-rules
"10.13 If cue's text track cue snap-to-lines flag is set [...]"
Attachments
Draft patch
(30.92 KB, patch)
2012-06-16 16:52 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Updated implementation and added tests
(149.72 KB, patch)
2012-08-11 13:01 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-04
(1.12 MB, application/zip)
2012-08-11 13:29 PDT
,
WebKit Review Bot
no flags
Details
Updated patch
(150.54 KB, patch)
2012-08-12 07:01 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Archive of layout-test-results from gce-cr-linux-02
(757.69 KB, application/zip)
2012-08-12 08:47 PDT
,
WebKit Review Bot
no flags
Details
Minor changes
(153.90 KB, patch)
2012-08-13 13:50 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Rebased patch
(154.06 KB, patch)
2012-08-13 14:10 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Updated patch
(155.58 KB, patch)
2012-08-15 18:34 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Updated patch
(260.64 KB, patch)
2012-08-17 13:25 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Landing patch
(260.65 KB, patch)
2012-08-21 12:25 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Build fix
(260.69 KB, patch)
2012-08-21 18:44 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Rebased
(260.73 KB, patch)
2012-08-22 16:43 PDT
,
Victor Carbune
no flags
Details
Formatted Diff
Diff
Show Obsolete
(9)
View All
Add attachment
proposed patch, testcase, etc.
Victor Carbune
Comment 1
2012-06-16 16:52:58 PDT
Created
attachment 147993
[details]
Draft patch Better design in progress
Build Bot
Comment 2
2012-06-16 17:20:57 PDT
Comment on
attachment 147993
[details]
Draft patch
Attachment 147993
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12961941
Victor Carbune
Comment 3
2012-08-11 13:01:53 PDT
Created
attachment 157876
[details]
Updated implementation and added tests The better design involves an independent renderer for cues, rather than the whole container
Victor Carbune
Comment 4
2012-08-11 13:10:51 PDT
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.
WebKit Review Bot
Comment 5
2012-08-11 13:29:48 PDT
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
WebKit Review Bot
Comment 6
2012-08-11 13:29:58 PDT
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
Build Bot
Comment 7
2012-08-11 14:19:57 PDT
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
Victor Carbune
Comment 8
2012-08-12 07:01:42 PDT
Created
attachment 157897
[details]
Updated patch Fixed minor mistake that triggered some test fails
WebKit Review Bot
Comment 9
2012-08-12 08:47:13 PDT
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
WebKit Review Bot
Comment 10
2012-08-12 08:47:20 PDT
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
Build Bot
Comment 11
2012-08-12 16:26:54 PDT
Comment on
attachment 157897
[details]
Updated patch
Attachment 157897
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13489003
Early Warning System Bot
Comment 12
2012-08-12 21:07:33 PDT
Comment on
attachment 157897
[details]
Updated patch
Attachment 157897
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/13475955
Early Warning System Bot
Comment 13
2012-08-13 02:14:34 PDT
Comment on
attachment 157897
[details]
Updated patch
Attachment 157897
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/13481697
Eric Carlson
Comment 14
2012-08-13 11:49:10 PDT
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.
Victor Carbune
Comment 15
2012-08-13 13:50:37 PDT
Created
attachment 158095
[details]
Minor changes
Victor Carbune
Comment 16
2012-08-13 13:54:46 PDT
(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.
Victor Carbune
Comment 17
2012-08-13 14:10:53 PDT
Created
attachment 158099
[details]
Rebased patch
Build Bot
Comment 18
2012-08-13 14:41:20 PDT
Comment on
attachment 158099
[details]
Rebased patch
Attachment 158099
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13492241
Early Warning System Bot
Comment 19
2012-08-13 14:56:59 PDT
Comment on
attachment 158099
[details]
Rebased patch
Attachment 158099
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/13492255
Early Warning System Bot
Comment 20
2012-08-13 15:08:41 PDT
Comment on
attachment 158099
[details]
Rebased patch
Attachment 158099
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/13486646
Tony Chang
Comment 21
2012-08-14 00:03:51 PDT
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
Tony Chang
Comment 22
2012-08-14 00:04:43 PDT
Levi or Emil can probably provide feedback as to the right units to use and when to convert from floats to LayoutUnit.
Levi Weintraub
Comment 23
2012-08-14 12:18:35 PDT
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?
Eric Carlson
Comment 24
2012-08-15 08:29:53 PDT
(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.
Tony Chang
Comment 25
2012-08-15 17:14:36 PDT
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.
Victor Carbune
Comment 26
2012-08-15 17:23:01 PDT
(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.
Victor Carbune
Comment 27
2012-08-15 18:34:35 PDT
Created
attachment 158681
[details]
Updated patch Significant refactoring
Victor Carbune
Comment 28
2012-08-15 18:36:11 PDT
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.
Build Bot
Comment 29
2012-08-15 19:03:39 PDT
Comment on
attachment 158681
[details]
Updated patch
Attachment 158681
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/13511366
Early Warning System Bot
Comment 30
2012-08-15 19:16:12 PDT
Comment on
attachment 158681
[details]
Updated patch
Attachment 158681
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/13511369
Early Warning System Bot
Comment 31
2012-08-15 19:26:11 PDT
Comment on
attachment 158681
[details]
Updated patch
Attachment 158681
[details]
did not pass qt-wk2-ews (qt): Output:
http://queues.webkit.org/results/13511372
Tony Chang
Comment 32
2012-08-16 23:31:05 PDT
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.
Victor Carbune
Comment 33
2012-08-17 13:25:00 PDT
Created
attachment 159189
[details]
Updated patch Addressed comments and solved Mac build issue
Victor Carbune
Comment 34
2012-08-17 13:31:49 PDT
(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.
Tony Chang
Comment 35
2012-08-20 13:51:33 PDT
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
Victor Carbune
Comment 36
2012-08-21 12:25:15 PDT
Created
attachment 159736
[details]
Landing patch Fixed nits
WebKit Review Bot
Comment 37
2012-08-21 17:19:32 PDT
Comment on
attachment 159736
[details]
Landing patch Clearing flags on attachment: 159736 Committed
r126233
: <
http://trac.webkit.org/changeset/126233
>
WebKit Review Bot
Comment 38
2012-08-21 17:19:39 PDT
All reviewed patches have been landed. Closing bug.
James Robinson
Comment 39
2012-08-21 17:38:17 PDT
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;
Victor Carbune
Comment 40
2012-08-21 17:42:54 PDT
(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
WebKit Review Bot
Comment 41
2012-08-21 17:48:38 PDT
Re-opened since this is blocked by 94656
Victor Carbune
Comment 42
2012-08-21 18:44:38 PDT
Created
attachment 159840
[details]
Build fix Removed the two unused vars
WebKit Review Bot
Comment 43
2012-08-22 14:57:11 PDT
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
Victor Carbune
Comment 44
2012-08-22 16:43:50 PDT
Created
attachment 160034
[details]
Rebased
WebKit Review Bot
Comment 45
2012-08-22 17:59:53 PDT
Comment on
attachment 160034
[details]
Rebased Clearing flags on attachment: 160034 Committed
r126372
: <
http://trac.webkit.org/changeset/126372
>
WebKit Review Bot
Comment 46
2012-08-22 18:00:01 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug