[webkit-reviews] review granted: [Bug 98326] [WK2] Have plugins render initially offscreen to capture snapshot : [Attachment 167782] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jon Lee
<jonlee at apple.com>'s request for review:
Bug 98326: [WK2] Have plugins render initially offscreen to capture snapshot
https://bugs.webkit.org/show_bug.cgi?id=98326

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

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


> Source/WebKit2/WebProcess/Plugins/PluginView.cpp:292
> +    // Null out the plug-in element explicitly so we'll crash earlier if we
try to use
> +    // the plug-in view after it's been destroyed.
> +    m_pluginElement = nullptr;

There doesn't seem to be much point in doing this in the destructor.

> Source/WebKit2/WebProcess/Plugins/PluginView.h:235
> +    // The snapshot is used in two scenarios:
> +    // 1) If plugin snapshotting is enabled, this snapshot represents the
poster image.
> +    // 2) Otherwise, it represents the first image displayed when the plugin
is run, to avoid
> +    // side effects should the plugin perform DOM manipulation on
initialization.
>      RefPtr<ShareableBitmap> m_snapshot;
> +    RefPtr<WebCore::Image> m_snapshotImage;

This comment is ambiguous now; is it referring to m_snapshot, or
m_snapshotImage. Do we still need both of these? Can they be renamed to make
their purposes more obvious?