[webkit-reviews] review granted: [Bug 131553] Snapshotted plugins may need to be restarted if style properties are changed after initial load of plugin. : [Attachment 229179] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Roger Fong <roger_fong at apple.com>'s
request for review:
Bug 131553: Snapshotted plugins may need to be restarted if style properties
are changed after initial load of plugin.
https://bugs.webkit.org/show_bug.cgi?id=131553

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

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


> Source/WebCore/html/HTMLPlugInImageElement.cpp:604
> +static inline bool isSmallerThanTinySizingThreshold(int contentWidth, int
contentHeight)

This should take a LayoutSize instead of two ints for the height and width. I
would suggest just calling it size. I’m not sure that “content size” says
anything more than “size” does. What other size would it be?

Maybe a const LayoutSize& or maybe just LayoutSize would be better.

Or maybe we should just have this function take a RenderEmbeddedObject&
instead. That would be more convenient in both call sites.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:606
> +    return (contentWidth <= sizingTinyDimensionThreshold || contentHeight <=
sizingTinyDimensionThreshold);

No need for the parentheses here.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:609
> +bool HTMLPlugInImageElement::isTopLevelFullPagePlugin(const
RenderEmbeddedObject* renderEmbedded, int contentWidth, int contentHeight)
const

This should take a RenderEmbeddedObject&, not a const RenderEmbeddedObject*. Or
const if you insist, but it should not be a pointer since it can never be null.
Also, I suggest naming this just “renderer”. There’s no need to encode the type
in the name.

This function should get the contentBoxRect from the renderer. It’s not really
valuable to pass it in, since both callers compute it the same way.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:612
> +    if (!document().frame()->isMainFrame())
> +	   return false;

Since we use Frame more than once in this function I suggest putting it in a
reference:

    Frame& frame = *document().frame();

> Source/WebCore/html/HTMLPlugInImageElement.cpp:615
> +    bool isFullPage = is100Percent(style.width()) &&
is100Percent(style.height());

Instead of a boolean, we should have an early return here. That’s one of the
advantages of moving this to a separate function. No need to compute the areas
below if the styles aren’t 100%.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:619
> +    IntSize visibleViewSize = document().frame()->view()->visibleSize();
> +    float contentArea = contentWidth * contentHeight;
> +    float visibleArea = visibleViewSize.width() * visibleViewSize.height();
> +    return isFullPage && (contentArea > visibleArea *
sizingFullPageAreaRatioThreshold);

If we made an area helper function (overloaded for IntSize and LayoutSize) then
these four lines could be written in one line like this:

    return area(renderer.contentBoxRect()) > area(frame.view()->visibleSize())
* sizingFullPageAreaRatioThreshold;

I think that’s better.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:627
> +	   RenderEmbeddedObject* renderEmbedded = renderEmbeddedObject();

This should be a reference. The code above will already crash if the renderer
is null so there is no point in using a pointer.

The type of this should be RenderSnapshottedPlugIn, not RenderEmbeddedObject.
We always want to use the most-specific type we can, based on the type check we
did. And here, the type check is isSnapshottedPlugIn, not isEmbeddedObject.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:628
> +	   ASSERT(renderer);

Looks like this won’t compile, because the code above uses the name
renderEmbedded.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:732
> +    RenderEmbeddedObject* renderer = renderEmbeddedObject();

This local variable should remain a reference, not a pointer. The patch changes
it to a pointer for no good reason.