Bug 110511

Summary: TextTrackRegion Constructor
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Victor Carbune <vcarbune>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, buildbot, cmarcelo, eric.carlson, esprehn+autocc, feature-media-reviews, gyuyoung.kim, jer.noble, ojan.autocc, philn, rakuco, rniwa, silviapf, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 109570, 116546    
Attachments:
Description Flags
Initial Patch
none
Addressed first comments
none
Split width and height attributes
none
Simplified patch
none
Refined patch
none
Changed to FloatSize
none
Patch for landing none

Description Victor Carbune 2013-02-21 14:04:13 PST
Implement TextTrackRegion with appropriate build guards.
Comment 1 Victor Carbune 2013-02-21 14:09:51 PST
Created attachment 189598 [details]
Initial Patch

Work in progress
Comment 2 Build Bot 2013-02-21 14:42:31 PST
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 3 Adam Barth 2013-02-21 14:49:15 PST
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.
Comment 4 Victor Carbune 2013-02-22 10:06:09 PST
Created attachment 189789 [details]
Addressed first comments
Comment 5 Victor Carbune 2013-02-22 10:13:06 PST
(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.
Comment 6 Adam Barth 2013-02-22 10:52:49 PST
(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 7 Adam Barth 2013-02-22 10:53:58 PST
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?
Comment 8 Victor Carbune 2013-02-22 12:25:50 PST
(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.
Comment 9 Adam Barth 2013-02-22 12:48:55 PST
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.
Comment 10 Victor Carbune 2013-02-23 10:19:58 PST
Created attachment 189933 [details]
Split width and height attributes
Comment 11 Victor Carbune 2013-02-23 10:24:42 PST
(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 12 Adam Barth 2013-02-23 10:32:59 PST
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.
Comment 13 Adam Barth 2013-02-23 10:35:45 PST
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.
Comment 14 Victor Carbune 2013-02-23 15:28:30 PST
Created attachment 189942 [details]
Simplified patch
Comment 15 Victor Carbune 2013-02-23 15:42:20 PST
(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 16 Build Bot 2013-02-23 17:05:31 PST
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 17 Build Bot 2013-02-23 19:29:42 PST
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
Comment 18 Victor Carbune 2013-03-03 10:21:35 PST
(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?
Comment 19 Adam Barth 2013-03-03 11:50:19 PST
> 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.
Comment 20 Victor Carbune 2013-03-06 14:15:59 PST
Created attachment 191831 [details]
Refined patch
Comment 21 Victor Carbune 2013-03-06 14:39:33 PST
(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 22 Adam Barth 2013-03-06 15:03:04 PST
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.
Comment 23 Victor Carbune 2013-03-06 15:20:29 PST
Created attachment 191848 [details]
Changed to FloatSize
Comment 24 Victor Carbune 2013-03-06 15:24:58 PST
(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 25 Adam Barth 2013-03-06 15:45:51 PST
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.
Comment 26 Victor Carbune 2013-03-07 01:08:27 PST
Created attachment 191942 [details]
Patch for landing
Comment 27 Victor Carbune 2013-03-07 01:13:36 PST
(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 28 WebKit Review Bot 2013-03-07 01:31:13 PST
Comment on attachment 191942 [details]
Patch for landing

Clearing flags on attachment: 191942

Committed r145053: <http://trac.webkit.org/changeset/145053>
Comment 29 WebKit Review Bot 2013-03-07 01:31:20 PST
All reviewed patches have been landed.  Closing bug.