Refactor TextTrackCue to use more traditional design patterns
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>