[webkit-reviews] review granted: [Bug 108284] Snapshotted plug-in should use shadow root : [Attachment 185828] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Dean Jackson
<dino at apple.com>'s request for review:
Bug 108284: Snapshotted plug-in should use shadow root
https://bugs.webkit.org/show_bug.cgi?id=108284

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

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


> Source/WebCore/css/plugIns.css:42
> +/*
> + * This is the UA StyleSheet for <object> and <embed> elements.
> + *
> + * Such elements, when snapshotted (paused), will contain a ShadowRoot
> + * with the following structure:
> + *
> + * <object>
> + *	#ShadowRoot
> + *	  <div class="snapshot-container">
> + *	    <div class="snapshot-overlay"></div> <!-- e.g. for dimming content
-->
> + *	      <div class="snapshot-label">
> + *		<span class="snapshot-title">Snapshotted Plug-In</span>
> + *		<span class="snapshot-subtitle">Click to restart</span>
> + *	      </div>
> + *	    </div>
> + *	  </div>
> + *
> + */

It's a shame to bog down UA stylesheet parsing with big comments. Or is this
stripped out before we convert to const char[] ?

> Source/WebCore/css/plugIns.css:55
> +object::-webkit-snapshotted-plugin-content * {
> +    cursor: hand;
> +    pointer-events: none;
> +    -webkit-user-select: none;
> +}

Is this necessary on all child nodes or just some container?

> Source/WebCore/css/plugIns.css:91
> +    color: #444;

You have used every one of rgba(), keyword and hex values for colors!

> Source/WebCore/html/HTMLPlugInImageElement.cpp:74
> +    , m_snapshotImage(0)

No need to init a RefPtr.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:311
> +	   String clientTitleText =
document()->page()->chrome()->client()->plugInStartLabelTitle();

Do you need to null-check chrome()->client()?