Bug 208114

Created attachment 391503 [details]
Patch

Wow, tests are failing because the special value of "-1" to mean invalid is apparently web-facing behavior! Who would have thought? I guess I will revert that part of the patch.

Created attachment 391624 [details]
Patch

Comment on attachment 391624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391624&action=review

r=me, assuming this passes EWS.

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:159
> +    return adoptRef(*new TextTrackCueGeneric(downcast<Document>(context), start, end, content));

How do we know this is a Document?

> Source/WebCore/html/track/TextTrackCueGeneric.cpp:236
> +            std::pair<double, double> thisPosition = getPositionCoordinates();
> +            std::pair<double, double> thatPosition = downcast<TextTrackCueGeneric>(*that).getPositionCoordinates();

This looks like a good place for auto, FloatPoint, or structured bindings.

> Source/WebCore/html/track/VTTCue.h:141
>      enum WritingDirection {

It would be nice to make these enum classes, but that kind of thing can explode the patch.

> Source/WebCore/html/track/VTTCue.h:149
>      enum CueAlignment {

ditto.

Comment on attachment 391624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391624&action=review

>> Source/WebCore/html/track/TextTrackCueGeneric.cpp:159
>> +    return adoptRef(*new TextTrackCueGeneric(downcast<Document>(context), start, end, content));
> 
> How do we know this is a Document?

The practical answer to this is that this is derived from TextTrackCue, and the TextTrackCue constructor has this line of code in it:

    ASSERT(m_scriptExecutionContext.isDocument());

The more abstract answer is that these classes can only be used inside documents, not in workers, and ideally our DOM bindings would know that and pass a Document rather than a ScriptExecutionContext to all the create functions and such. Really all this ScriptExecutionContext when we know this is a Document is messy and worth cleaning up. Among other things the type name "Document" is much, much shorter.

>> Source/WebCore/html/track/TextTrackCueGeneric.cpp:236
>> +            std::pair<double, double> thatPosition = downcast<TextTrackCueGeneric>(*that).getPositionCoordinates();
> 
> This looks like a good place for auto, FloatPoint, or structured bindings.

Not the kind of thing I was tempted to touch. Of those three, I think I’d go for auto, though. Related: too bad that the line below is not *quite* std::pair's > or < operator; it’s a mix of both.

>> Source/WebCore/html/track/VTTCue.h:141
>>      enum WritingDirection {
> 
> It would be nice to make these enum classes, but that kind of thing can explode the patch.

Agreed.

Comment on attachment 391624 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=391624&action=review

>>> Source/WebCore/html/track/TextTrackCueGeneric.cpp:159
>>> +    return adoptRef(*new TextTrackCueGeneric(downcast<Document>(context), start, end, content));
>> 
>> How do we know this is a Document?
> 
> The practical answer to this is that this is derived from TextTrackCue, and the TextTrackCue constructor has this line of code in it:
> 
>     ASSERT(m_scriptExecutionContext.isDocument());
> 
> The more abstract answer is that these classes can only be used inside documents, not in workers, and ideally our DOM bindings would know that and pass a Document rather than a ScriptExecutionContext to all the create functions and such. Really all this ScriptExecutionContext when we know this is a Document is messy and worth cleaning up. Among other things the type name "Document" is much, much shorter.

Looks like I can change ConstructorCallWith=ScriptExecutionContext to ConstructorCallWith=Document and fix part of the messiness that way.

Committed r257296: <https://trac.webkit.org/changeset/257296>

<rdar://problem/59756108>