[webkit-reviews] review denied: [Bug 110511] TextTrackRegion Constructor : [Attachment 189598] Initial Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Victor Carbune
<vcarbune at chromium.org>'s request for review:
Bug 110511: TextTrackRegion Constructor
https://bugs.webkit.org/show_bug.cgi?id=110511

Attachment 189598: Initial Patch
https://bugs.webkit.org/attachment.cgi?id=189598&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.