[webkit-reviews] review denied: [Bug 115548] Manage the presentation of the snapshotted plug-in using JavaScript : [Attachment 200548] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has denied Antoine Quint <graouts at apple.com>'s
request for review:
Bug 115548: Manage the presentation of the snapshotted plug-in using JavaScript
https://bugs.webkit.org/show_bug.cgi?id=115548

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=200548&action=review


> Source/WebCore/ChangeLog:36
> +	   * dom/Document.h:
> +	   Expose the new ensurePlugInsInjectedScript method and the
m_injectedPlugInsScript
> +	   property used to ensure injection happens only once per document.

The thing that worries me the most about this change is if it will have any
issues on pages with deeply nested plugins. This is pretty common on news sites
with a lot of advertising (often injected many iframes deep). Our automated
testing for this feature is pretty bad - but I wonder if you've done a lot of
manual testing?

> Source/WebCore/ChangeLog:42
> +	   Make the localized strings static members since they will be shared
by all instances
> +	   and it can be costly to retrieve those strings each time from the
client. Additionally,

This is a great change, but won't quite work. You're only storing the string
once and not taking the mime type into account. That means you'll always get
whatever string was returned for the first plugin in the lifetime of the
process. I suggest you have something like a HashTable indexed by mime type, or
skip this change and do it in a followup.

> Source/WebKit2/ChangeLog:21
> +	   (WebKit):

Remove this line.

> Source/WebKit2/ChangeLog:28
> +	   (WebKit):

And this one.

> Source/WebCore/Resources/plugIns.js:27
> +    var title = this.title =
snapshotLabel.appendChild(document.createElement("div"));

We avoid this style of initialisation in WebKit. The style checker doesn't
complain because it doesn't look at JS (I think).
Use two lines.

> Source/WebCore/Resources/plugIns.js:31
> +    var subtitle = this.subtitle =
snapshotLabel.appendChild(document.createElement("div"));

Ditto.

> Source/WebCore/css/CSSDefaultStyleSheets.cpp:206
>      if (!plugInsStyleSheet && (element->hasTagName(objectTag) ||
element->hasTagName(embedTag))) {
> -	   String plugInsRules = String(plugInsUserAgentStyleSheet,
sizeof(plugInsUserAgentStyleSheet)) +
RenderTheme::themeForPage(element->document()->page())->extraPlugInsStyleSheet(
) + element->document()->page()->chrome()->client()->plugInExtraStyleSheet();
> +	   String plugInsRules =
RenderTheme::themeForPage(element->document()->page())->extraPlugInsStyleSheet(
) + element->document()->page()->chrome()->client()->plugInExtraStyleSheet();
> +	   if (plugInsRules.isEmpty())
> +	       plugInsRules = String(plugInsUserAgentStyleSheet,
sizeof(plugInsUserAgentStyleSheet));

One of the reasons I didn't do this the first time around was because it was
fairly different from the other UA SS approaches. I think it's ok since
snapshotting is the only thing happening with plugins at the moment, but I
wouldn't mind if the SS was always injected in the future.

> Source/WebCore/dom/Document.cpp:6056
> +void Document::ensurePlugInsInjectedScript(DOMWrapperWorld* world)

I don't really understand why the boolean m_injectedPlugInsScript tells me if
this method has been called, but it might have been called on a different
DOMWrapperWorld. Does this make sense?

> Source/WebCore/html/HTMLPlugInImageElement.cpp:60
> +using namespace JSC;

WebKit coding style is to not use "using" (although HTMLNames always seems to
get away with it). Instead use JSC::call when you need it.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:78
> +static const String& titleText(Page* page, String mimeType)
> +{
> +    DEFINE_STATIC_LOCAL(String, titleText, ());
> +    if (titleText.isEmpty())

This is what I was talking about above. If I call titleText(p, "flash/type")
and titleText(p, "silverlight/type") then I would get the same answer.

> Source/WebCore/html/HTMLPlugInImageElement.cpp:366
> +    DEFINE_STATIC_LOCAL(RefPtr<DOMWrapperWorld>, isolatedWorld,
(DOMWrapperWorld::create(JSDOMWindow::commonVM())));
> +    document()->ensurePlugInsInjectedScript(isolatedWorld.get());

And this is what I was confused about above. You're creating a new isolated
world for each plugin, but only testing per document. I guess that's ok, since
it is only injecting the script into the main doc.