[webkit-reviews] review granted: [Bug 102148] Automatically run small plugins : [Attachment 174030] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Jon Lee <jonlee at apple.com>'s request
for review:
Bug 102148: Automatically run small plugins
https://bugs.webkit.org/show_bug.cgi?id=102148

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=174030&action=review


I’m OK with this change, although Tim’s point is an interesting one.

> Source/WebCore/rendering/RenderEmbeddedObject.h:72
> +    virtual void layout();

As you are moving this, please also add the OVERRIDE keyword.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:41
> +static int autoStartPlugInSizeThresholdWidth = 1;
> +static int autoStartPlugInSizeThresholdHeight = 1;

These should be const. Once they are const they need not be static (although
there is no harm in marking them so, except perhaps to annoy Alexey).

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:153
> +    int width = rect.width(), height = rect.height();

We don’t do multiple variables on one line like this in WebKit.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp:155
> +    if (plugInImageElement()->displayState() < HTMLPlugInElement::Playing
> +	   && (!width || !height || (width <= autoStartPlugInSizeThresholdWidth
&& height <= autoStartPlugInSizeThresholdHeight)))

I suggest computing the content box size only after checking the displayState.

> Source/WebCore/rendering/RenderSnapshottedPlugIn.h:57
> +    virtual void layout();

Please add OVERRIDE.