[webkit-reviews] review denied: [Bug 110511] TextTrackRegion Constructor : [Attachment 189933] Split width and height attributes

[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 189933: Split width and height attributes
https://bugs.webkit.org/attachment.cgi?id=189933&action=review

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