Bug 110511

Created attachment 189598 [details] Initial Patch Work in progress

Comment on attachment 189598 [details] Initial Patch Attachment 189598 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16693313

Comment on attachment 189598 [details] Initial Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189598&action=review > Source/WebCore/html/track/TextTrackRegion.h:106 > + unsigned m_width; > + unsigned m_height; Should this be an IntSize? > Source/WebCore/html/track/TextTrackRegion.h:109 > + std::pair<unsigned, unsigned> m_regionAnchor; > + std::pair<unsigned, unsigned> m_viewportAnchor; Should these be IntSizes? > Source/WebCore/html/track/TextTrackRegion.h:113 > + ScriptExecutionContext* m_scriptExecutionContext; How is the lifetime of this object related to m_scriptExecutionContext? Should this object be a ContextDestructionObserver? > Source/WebCore/html/track/TextTrackRegion.h:114 > + TextTrack* m_track; How is the lifetime of this object related to m_track? > Source/WebCore/html/track/TextTrackRegion.idl:33 > + JSCustomMarkFunction, > + JSCustomIsReachable, Why do you need custom mark and reachability functions for JSC and not for V8? > Source/WebCore/html/track/TextTrackRegion.idl:34 > + ImplementationLacksVTable This isn't true. The implementation has a vtable.

Created attachment 189789 [details] Addressed first comments

(In reply to comment #3) > (From update of attachment 189598 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189598&action=review > > > Source/WebCore/html/track/TextTrackRegion.h:106 > > + unsigned m_width; > > + unsigned m_height; > > Should this be an IntSize? I changed the code to use IntSize. I hesitated to make them a pair at first since they are different units (width is a percentage value, while height is the number of lines). > > Source/WebCore/html/track/TextTrackRegion.h:109 > > + std::pair<unsigned, unsigned> m_regionAnchor; > > + std::pair<unsigned, unsigned> m_viewportAnchor; > > Should these be IntSizes? Done. > > Source/WebCore/html/track/TextTrackRegion.h:113 > > + ScriptExecutionContext* m_scriptExecutionContext; > > How is the lifetime of this object related to m_scriptExecutionContext? Should this object be a ContextDestructionObserver? Yes, I think it's appropriate for it to be a ContextDestructionObserver. I wasn't aware of it, so thanks! > > Source/WebCore/html/track/TextTrackRegion.h:114 > > + TextTrack* m_track; > > How is the lifetime of this object related to m_track? It should be fairly independent. I 'borrowed' this variable from TextTrackCue (since it's should be the same lifetime). > > Source/WebCore/html/track/TextTrackRegion.idl:33 > > + JSCustomMarkFunction, > > + JSCustomIsReachable, > > Why do you need custom mark and reachability functions for JSC and not for V8? Looking closer to the TextTrackCue implementation, I now think these aren't actually needed. > > Source/WebCore/html/track/TextTrackRegion.idl:34 > > + ImplementationLacksVTable > > This isn't true. The implementation has a vtable. Indeed, sorry.

(In reply to comment #5) > (In reply to comment #3) > > (From update of attachment 189598 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=189598&action=review > > > > > Source/WebCore/html/track/TextTrackRegion.h:106 > > > + unsigned m_width; > > > + unsigned m_height; > > > > Should this be an IntSize? > I changed the code to use IntSize. > > I hesitated to make them a pair at first since they are different units > (width is a percentage value, while height is the number of lines). That's very unclear from the code. Perhaps we should use different names? > > > Source/WebCore/html/track/TextTrackRegion.h:114 > > > + TextTrack* m_track; > > > > How is the lifetime of this object related to m_track? > > It should be fairly independent. > I 'borrowed' this variable from TextTrackCue (since it's should be the same lifetime). Doesn't that mean there's a risk of use-after-free if the TextTrack is deleted before the TextTrackRegion?

Comment on attachment 189789 [details] Addressed first comments I'm sorry that I don't have time to review your patch in detail, but it looks like there are a number of issues that need to be addressed before we can land this patch. Who is a better person to review this change?

(In reply to comment #6) > (In reply to comment #5) > > I hesitated to make them a pair at first since they are different units > > (width is a percentage value, while height is the number of lines). > > That's very unclear from the code. Perhaps we should use different names? I will split them back and perhaps use the name m_heightInLines? > > > > Source/WebCore/html/track/TextTrackRegion.h:114 > > > > + TextTrack* m_track; > > > > > > How is the lifetime of this object related to m_track? > > > > It should be fairly independent. > > I 'borrowed' this variable from TextTrackCue (since it's should be the same lifetime). > > Doesn't that mean there's a risk of use-after-free if the TextTrack is deleted before the TextTrackRegion? The current patch doesn't handle the TextTrackRegionList, but in the patch that will handle this, the destructor of TextTrack will clear the track pointers for all the TextTrackRegion instances that are in its TextTrackRegionList.

It's better not to introduce the member variable until it doesn't point to freed memory. It also sounds like we need to teach the JavaScript engine about the relationship between these objects so that it doesn't prematurely collect the wrappers for one or more of these objects.

Created attachment 189933 [details] Split width and height attributes

(In reply to comment #9) > It's better not to introduce the member variable until it doesn't point to freed memory. I removed the setter for m_track, but I would keep the getter to keep the complete TextTrackRegion IDL in this patch. Would this work? > It also sounds like we need to teach the JavaScript engine about the relationship between these objects so that it doesn't prematurely collect the wrappers for one or more of these objects. I'm don't feel familiar enough, but I would expect that TextTrackCue alread went through such a review and following it would be a safe path? Either way, if there are extra concerns I would be interested in discussing more about this (maybe live on IRC?) about this.

Comment on attachment 189933 [details] Split width and height attributes View in context: https://bugs.webkit.org/attachment.cgi?id=189933&action=review I'm worried that you've authored this patch via copy and paste without really understanding what each part of it does. It would be better to only include things that are needed... > Source/WebCore/html/track/TextTrackRegion.cpp:57 > + : ContextDestructionObserver(context) Why do you need the context? You don't seem to be using it anywhere... > Source/WebCore/html/track/TextTrackRegion.cpp:79 > + if (std::isinf(value) || std::isnan(value)) { How can either of these things return true for a variable of type |long|? They only make sense for floating point values. Do you have a test case that triggers this condition? > Source/WebCore/html/track/TextTrackRegion.h:87 > + using RefCounted<TextTrackRegion>::ref; > + using RefCounted<TextTrackRegion>::deref; This should not be necessary. > Source/WebCore/html/track/TextTrackRegion.h:110 > + TextTrack* m_track; I see. You're saying that this is always zero. I think its better to have the getter just return 0 directly.

Is there someone you can discuss this patch with locally? I'm worried that going back and forth on this bug will take a while.

Created attachment 189942 [details] Simplified patch

(In reply to comment #12) > (From update of attachment 189933 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=189933&action=review > > I'm worried that you've authored this patch via copy and paste without really understanding what each part of it does. It would be better to only include things that are needed... Indeed I followed the TextTrackCue skelet, as I've previously mentioned.. I went again through all the elements and hopefully refined properly the patch. > > Source/WebCore/html/track/TextTrackRegion.cpp:57 > > + : ContextDestructionObserver(context) > > Why do you need the context? You don't seem to be using it anywhere... It's not, so I changed the constructor to not take any arguments. From my understanding it still needs to be passed through the static create method. > > Source/WebCore/html/track/TextTrackRegion.cpp:79 > > + if (std::isinf(value) || std::isnan(value)) { > > How can either of these things return true for a variable of type |long|? They only make sense for floating point values. Do you have a test case that triggers this condition? Argh, this remained from the first versoin of the patch - the IDL was declared with double, but they should actually be long (I removed them now). > > Source/WebCore/html/track/TextTrackRegion.h:87 > > + using RefCounted<TextTrackRegion>::ref; > > + using RefCounted<TextTrackRegion>::deref; > > This should not be necessary. Yes, sorry. > > Source/WebCore/html/track/TextTrackRegion.h:110 > > + TextTrack* m_track; > > I see. You're saying that this is always zero. I think its better to have the getter just return 0 directly. Done. (In reply to comment #13) > Is there someone you can discuss this patch with locally? I'm worried that going back and forth on this bug will take a while. No, but I'm very often online on IRC so there shouldn't be communication issues. I have removed for now from the patch the changes in files that enabled WEBVTT_REGIONS in various build systems.

Comment on attachment 189942 [details] Simplified patch Attachment 189942 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16726100 New failing tests: media/track/track-region-constructor.html

Comment on attachment 189942 [details] Simplified patch Attachment 189942 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16722992 New failing tests: media/track/track-region-constructor.html

(In reply to comment #17) > (From update of attachment 189942 [details]) > Attachment 189942 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://queues.webkit.org/results/16722992 > > New failing tests: > media/track/track-region-constructor.html Since this is an experimental feature, is the appropriate method to commit patches while keeping it disabled in all the ports and skip tests in TestExpectations?

> Since this is an experimental feature, is the appropriate method to commit > patches while keeping it disabled in all the ports and skip tests in TestExpectations? Yep.

Created attachment 191831 [details] Refined patch

(In reply to comment #15) > (In reply to comment #12) > > > Source/WebCore/html/track/TextTrackRegion.cpp:79 > > > + if (std::isinf(value) || std::isnan(value)) { > > > > How can either of these things return true for a variable of type |long|? They only make sense for floating point values. Do you have a test case that triggers this condition? > > Argh, this remained from the first versoin of the patch - the IDL was declared > with double, but they should actually be long (I removed them now). After a bit of discussion, these will be kept as double in the specification for better positioning precision of the cue, so I re-added the tests for Inf and Nan and added the cases in the layout test as well. I also removed completely the ScriptExecutionContext, skipped tests and disabled the feature implicitly

Comment on attachment 191831 [details] Refined patch View in context: https://bugs.webkit.org/attachment.cgi?id=191831&action=review > Source/WebCore/html/track/TextTrackRegion.cpp:43 > +// The region occupies by default 100% of the width of the video viewport. > +static const int defaultWidth = 100; > + > +// The region has, by default, 3 lines of text. > +static const int defaultHeight = 3; These names read very symmetrically, but their semantics aren't symmetric. Perhaps defaultHeightInLines to match m_heightInLines? > Source/WebCore/html/track/TextTrackRegion.h:64 > + double regionAnchorX() const { return m_regionAnchor.width(); } > + void setRegionAnchorX(double, ExceptionCode&); Using LayoutSize for m_regionAnchor means that this DOM API behaves different when we're using subpixel layout. Is that intentional? That doesn't seem like something we should expose via this sort of API.

Created attachment 191848 [details] Changed to FloatSize

(In reply to comment #22) > (From update of attachment 191831 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191831&action=review > > > Source/WebCore/html/track/TextTrackRegion.cpp:43 > > +// The region occupies by default 100% of the width of the video viewport. > > +static const int defaultWidth = 100; > > + > > +// The region has, by default, 3 lines of text. > > +static const int defaultHeight = 3; > > These names read very symmetrically, but their semantics aren't symmetric. Perhaps defaultHeightInLines to match m_heightInLines? Good idea, done. > > Source/WebCore/html/track/TextTrackRegion.h:64 > > + double regionAnchorX() const { return m_regionAnchor.width(); } > > + void setRegionAnchorX(double, ExceptionCode&); > > Using LayoutSize for m_regionAnchor means that this DOM API behaves different when we're using subpixel layout. Is that intentional? That doesn't seem like something we should expose via this sort of API. I was tempted to use LayoutSize because of explicit handling of doubles, but the precision for cues can be handled with FloatSize. Thanks for the patience of going back and forth through this patch!

Comment on attachment 191848 [details] Changed to FloatSize View in context: https://bugs.webkit.org/attachment.cgi?id=191848&action=review Ok, this is looking good now. My comments below are now just tiny nits. > Source/WebCore/html/track/TextTrackRegion.cpp:43 > +// The region occupies by default 100% of the width of the video viewport. > +static const int defaultWidth = 100; > + > +// The region has, by default, 3 lines of text. > +static const int defaultHeightInLines = 3; These types don't match the member variables, but maybe that doesn't matter. > Source/WebCore/html/track/TextTrackRegion.h:82 > +protected: > + TextTrackRegion(); Why protected? Normally we just mark the constructors as private. > Source/WebCore/html/track/TextTrackRegion.h:85 > + enum RegionSetting { None, Id, Width, Height, RegionAnchor, ViewportAnchor, Scroll }; Normally we put each enum value on its own line. > Source/WebCore/html/track/TextTrackRegion.h:89 > + RegionSetting getSettingFromString(const String&); > + > + void parseSettingValue(RegionSetting, const String&); > + void parseSetting(const String&, unsigned*); I would skip these for now. You can always add them once you actually need to call them.

Created attachment 191942 [details] Patch for landing

(In reply to comment #25) > > Source/WebCore/html/track/TextTrackRegion.cpp:43 > > +// The region occupies by default 100% of the width of the video viewport. > > +static const int defaultWidth = 100; > > + > > +// The region has, by default, 3 lines of text. > > +static const int defaultHeightInLines = 3; > > These types don't match the member variables, but maybe that doesn't matter. Fixed. > > Source/WebCore/html/track/TextTrackRegion.h:82 > > +protected: > > + TextTrackRegion(); > > Why protected? Normally we just mark the constructors as private. It should private should be, thanks for catching it. > > Source/WebCore/html/track/TextTrackRegion.h:85 > > + enum RegionSetting { None, Id, Width, Height, RegionAnchor, ViewportAnchor, Scroll }; > > Normally we put each enum value on its own line. Fixed. > > Source/WebCore/html/track/TextTrackRegion.h:89 > > + RegionSetting getSettingFromString(const String&); > > + > > + void parseSettingValue(RegionSetting, const String&); > > + void parseSetting(const String&, unsigned*); > > I would skip these for now. You can always add them once you actually need to call them. They're purpose was more informaly so you can see the whole intended layout of the class (since I'm splitting code in multiple patches). I removed them for now from the class and the ChangeLog. Thanks!

Comment on attachment 191942 [details] Patch for landing Clearing flags on attachment: 191942 Committed r145053: <http://trac.webkit.org/changeset/145053>

All reviewed patches have been landed. Closing bug.