Bug 110511 – TextTrackRegion Constructor

• Component: Media
• Status: RESOLVED FIXED

[Victor Carbune]

Implement TextTrackRegion with appropriate build guards.

[Victor Carbune]

Created attachment 189598 [details]
Initial Patch

Work in progress

[Build Bot]

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

[Adam Barth]

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.

[Victor Carbune]

Created attachment 189789 [details]
Addressed first comments

[Victor Carbune]

(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.

[Adam Barth]

(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?

[Adam Barth]

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?

[Victor Carbune]

(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.

[Adam Barth]

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.

[Victor Carbune]

Created attachment 189933 [details]
Split width and height attributes

[Victor Carbune]

(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.

[Adam Barth]

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.

[Adam Barth]

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.

[Victor Carbune]

Created attachment 189942 [details]
Simplified patch

[Victor Carbune]

(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.

[Build Bot]

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

[Build Bot]

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

[Victor Carbune]

(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?

[Adam Barth]

> 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.

[Victor Carbune]

Created attachment 191831 [details]
Refined patch

[Victor Carbune]

(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

[Adam Barth]

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.

[Victor Carbune]

Created attachment 191848 [details]
Changed to FloatSize

[Victor Carbune]

(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!

[Adam Barth]

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.

[Victor Carbune]

Created attachment 191942 [details]
Patch for landing

[Victor Carbune]

(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!

[WebKit Review Bot]

Comment on attachment 191942 [details]
Patch for landing

Clearing flags on attachment: 191942

Committed r145053: <http://trac.webkit.org/changeset/145053>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.