[Webkit-unassigned] [Bug 131553] Snapshotted plugins may need to be restarted if style properties are changed after initial load of plugin.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=131553


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #229179|review?                     |review+
               Flag|                            |




--- Comment #9 from Darin Adler <darin at apple.com>  2014-04-12 13:04:07 PST ---
(From update of attachment 229179)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.