[webkit-reviews] review granted: [Bug 106146] Show label automatically for plugins of significant size : [Attachment 181421] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jan 4 19:38:51 PST 2013


Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jon Lee
<jonlee at apple.com>'s request for review:
Bug 106146: Show label automatically for plugins of significant size
https://bugs.webkit.org/show_bug.cgi?id=106146

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=181421&action=review


> Source/WebCore/html/HTMLPlugInImageElement.cpp:293
> +    LayoutRect plugInClipRect =
node->renderer()->absoluteClippedOverflowRect();

Is node->renderer() always non-null?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:352
> +    if
(shouldPlugInShowLabelAutomatically(document()->page()->mainFrame()->view()->co
ntentsSize(), this))

Is everything in this chain always non-null?

> Source/WebCore/platform/Timer.h:143
> +    void setDelay(double delay) { m_delay = delay; }

Not sure you need this (see later comment).

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:276
> +    m_showLabelDelayTimer.setDelay(reason == UserMousedOver ?
showLabelAfterMouseOverDelay : showLabelAutomaticallyDelay);
> +    m_showLabelDelayTimer.restart();

Can't you just call startOneShot() and avoid touching Timer.h? I don't think
you even need to call stop() beforehand.


More information about the webkit-reviews mailing list