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

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied 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 229173: patch
https://bugs.webkit.org/attachment.cgi?id=229173&action=review

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


> Source/WebCore/html/HTMLPlugInImageElement.cpp:604
> +bool HTMLPlugInImageElement::isTopLevelFullPage(const RenderEmbeddedObject&
renderEmbedded, int contentWidth, int contentHeight)

Fewer weird abbreviations please!
isTopLevelFullPagePlugin(const RenderEmbeddedObject& embeddedObject, ...

> Source/WebCore/html/HTMLPlugInImageElement.cpp:606
> +    if (document().frame()->isMainFrame()) {

if (!document().frame()->isMainFrame())
  return false

> Source/WebCore/html/HTMLPlugInImageElement.cpp:613
> +	   if (isFullPage && contentArea > visibleArea *
sizingFullPageAreaRatioThreshold)
> +	       return true;

return isFullPage && (contentArea > visibleArea *
sizingFullPageAreaRatioThreshold);

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

return (contentWidth <= sizingTinyDimensionThreshold || contentHeight <=
sizingTinyDimensionThreshold)

This could be a local static function.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:630
> +	   auto& renderEmbedded = toRenderEmbeddedObject(*this->renderer());

Why not use the renderEmbeddedObject() function (and check its return value)?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:642
> +	   if (displayState() == Playing)
> +	   checkSizeChangeForSnapshotting();

Wrong indentation.

> Source/WebCore/html/HTMLPlugInImageElement.h:144
> +    bool isTopLevelFullPage(const RenderEmbeddedObject&, int contentWidth,
int contentHeight);

Should be a const function.