Bug 109820

I think JS API support could be submitted before parsing, as it will provide a method of properly testing the functionality of the parser.

please propose to the W3C for standardization, then you may reopen this bug

(In reply to comment #2) > please propose to the W3C for standardization, then you may reopen this bug i mean, that you may reopen after the W3C has proposed a solution (if they choose to do so) in at least a draft specification

(In reply to comment #3) > (In reply to comment #2) > > please propose to the W3C for standardization, then you may reopen this bug > As noted in the master bug, there is a proposed solution [1] [2] which has had a great deal of discussion in the text community group mailing list (eg. [3]). > i mean, that you may reopen after the W3C has proposed a solution (if they choose to do so) in at least a draft specification Please don't close bugs without discussion. [1] https://dvcs.w3.org/hg/text-tracks/raw-file/default/608toVTT/region.html [2] http://www.w3.org/community/texttracks/wiki/MultiCueBox [3] http://lists.w3.org/Archives/Public/public-texttracks/2012Dec/0000.html

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > please propose to the W3C for standardization, then you may reopen this bug > > > As noted in the master bug, there is a proposed solution [1] [2] which has had a great deal of discussion in the text community group mailing list (eg. [3]). Thanks for pointing this out. If this information had been referenced in the bug, I would have been less likely to close it. In any case, he bug should make clear the specification status and the relative maturity of the specification. Since it appears to me that the specification for any JS API extensions is very early in the process, and not yet in any W3C spec, care should be taken to qualify this early implementation work, e.g., by noting that prefixes will be used for any experimental interface extensions. > > > i mean, that you may reopen after the W3C has proposed a solution (if they choose to do so) in at least a draft specification > > Please don't close bugs without discussion. It's easy enough to reopen, so I don't view this as a problem.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > please propose to the W3C for standardization, then you may reopen this bug > > > > > As noted in the master bug, there is a proposed solution [1] [2] which has had a great deal of discussion in the text community group mailing list (eg. [3]). > > Thanks for pointing this out. If this information had been referenced in the bug, I would have been less likely to close it. In any case, he bug should make clear the specification status and the relative maturity of the specification. The specs are noted in https://bugs.webkit.org/show_bug.cgi?id=109570, which is marked as blocked by this. That is what I meant by "master bug" in my comment. > Since it appears to me that the specification for any JS API extensions is very early in the process, and not yet in any W3C spec, care should be taken to qualify this early implementation work, e.g., by noting that prefixes will be used for any experimental interface extensions. > Agreed!

Created attachment 192461 [details] Patch Extension of TextTrack to support list of regions

Nit: You might want to test all fields for updatedRegion, not just viewportAnchorX . Other than that, FWIW: LGTM.

Created attachment 194826 [details] Updated test

Comment on attachment 194826 [details] Updated test View in context: https://bugs.webkit.org/attachment.cgi?id=194826&action=review > Source/WebCore/html/track/TextTrack.cpp:303 > +void TextTrack::addRegion(PassRefPtr<TextTrackRegion> region) > +{ Nit: it is probably worth adding ASSERT(region) here. The coding guidelines say the following about using a PassRefPtr<> as a function argument [1] "Unless the use of the argument is very simple, the argument should be transferred to a RefPtr at the start of the function; the argument can be named with a “prp” prefix in such cases."' [1] http://www.webkit.org/coding/RefPtr.html > Source/WebCore/html/track/TextTrack.cpp:309 > + // 1. If the given region is in a text track list of regions, then remove > + // region from that text track list of regions. > + if (regionList->remove(region.get())) > + return; Isn't the spec saying that the region can only be assigned to one track at a time, and therefore this should test "region->track() != this"? > Source/WebCore/html/track/TextTrack.cpp:315 > + TextTrackRegion* existingRegion = ensureTextTrackRegionList()->getRegionById(region->id()); Nit: you call ensureTextTrackRegionList above. > Source/WebCore/html/track/TextTrackRegion.h:108 > + TextTrack* m_track; A comment about why it is safe for this to be a raw pointer would be useful. > LayoutTests/media/track/regions-webvtt/text-track-region-list.html:20 > + consoleWrite("<br>** Implicit mode disabled and the regions attribute is null **"); > + testExpected("testTrack.track.mode", "disabled"); > + testExpected("testTrack.track.regions", null); It might be worth starting the test some time after the body's 'load' event fires because the automatic track selection logic may not run before 'load' fires, so this test may miss the track begin automatically enabled.

Created attachment 194916 [details] Refined patch

Comment on attachment 194916 [details] Refined patch Attachment 194916 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17132908

Comment on attachment 194916 [details] Refined patch Attachment 194916 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17290667

Comment on attachment 194916 [details] Refined patch Attachment 194916 [details] did not pass efl-ews (efl): Output: http://webkit-commit-queue.appspot.com/results/17237480

Comment on attachment 194916 [details] Refined patch Attachment 194916 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17144810

Created attachment 194925 [details] Proper guard

Comment on attachment 194925 [details] Proper guard View in context: https://bugs.webkit.org/attachment.cgi?id=194925&action=review > Source/WebCore/html/track/TextTrackRegion.cpp:195 > + ExceptionCode ec = 0; > + setScroll(region->scroll(), ec); > + > + ASSERT(!ec); Why not use ASSERT_NO_EXCEPTION? > LayoutTests/media/track/regions-webvtt/text-track-region-list.html:88 > + setTimeout(startTest, 500); Nit: I think 500ms is much longer than it needs to be,and it guarantees that the test takes .5 seconds. I would guess that 100ms would be sufficient.

Created attachment 194934 [details] Patch for landing

(In reply to comment #17) > (From update of attachment 194925 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194925&action=review > > > Source/WebCore/html/track/TextTrackRegion.cpp:195 > > + ExceptionCode ec = 0; > > + setScroll(region->scroll(), ec); > > + > > + ASSERT(!ec); > > Why not use ASSERT_NO_EXCEPTION? Done. > > LayoutTests/media/track/regions-webvtt/text-track-region-list.html:88 > > + setTimeout(startTest, 500); > > Nit: I think 500ms is much longer than it needs to be,and it guarantees that the test takes .5 seconds. I would guess that 100ms would be sufficient. Done. Thanks a lot for the review, Eric!

Comment on attachment 194934 [details] Patch for landing Clearing flags on attachment: 194934 Committed r146825: <http://trac.webkit.org/changeset/146825>

All reviewed patches have been landed. Closing bug.