Bug 115548 – Manage the presentation of the snapshotted plug-in using JavaScript

• Component: Plug-ins
• Status: RESOLVED FIXED

[Antoine Quint]

Browsers embedding WebKit will likely want to customize the appearance of the snapshotted plug-in. As such, it's not practical to want to generate markup for the plug-in's shadow root within WebCore. We should instead allow the chrome client to customize this by providing some JavaScript that will be injected into the document and called with the shadow root as well as whatever is necessary to localize any text shown for the snapshotted plug-in.

[Antoine Quint]

<rdar://problem/13722263>

[Antoine Quint]

Created attachment 200417 [details]
Patch for build testing, no need to review yet.

[Build Bot]

Comment on attachment 200417 [details]
Patch for build testing, no need to review yet.

Attachment 200417 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/391481

[kov's GTK+ EWS bot]

Comment on attachment 200417 [details]
Patch for build testing, no need to review yet.

Attachment 200417 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/401102

[Dean Jackson]

Comment on attachment 200417 [details]
Patch for build testing, no need to review yet.

View in context: https://bugs.webkit.org/attachment.cgi?id=200417&action=review

Looks good. I think you can split it into a couple of separate patches though.

> Source/WebCore/CMakeLists.txt:3097
> +# Generate plug-in resources
> +add_custom_command(
> +    OUTPUT ${DERIVED_SOURCES_WEBCORE_DIR}/PlugInsResourcesData.cpp ${DERIVED_SOURCES_WEBCORE_DIR}/PlugInsResources.h
> +    MAIN_DEPENDENCY ${WEBCORE_DIR}/css/make-css-file-arrays.pl
> +    DEPENDS ${WebCore_PLUG_INS_RESOURCES} ${WEBCORE_DIR}/bindings/scripts/preprocessor.pm
> +    COMMAND ${PERL_EXECUTABLE} -I${WEBCORE_DIR}/bindings/scripts ${WEBCORE_DIR}/css/make-css-file-arrays.pl --defines "${FEATURE_DEFINES_WITH_SPACE_SEPARATOR}" --preprocessor "${CODE_GENERATOR_PREPROCESSOR}" ${DERIVED_SOURCES_WEBCORE_DIR}/PlugInsResources.h ${DERIVED_SOURCES_WEBCORE_DIR}/PlugInsResourcesData.cpp ${WebCore_PLUG_INS_RESOURCES}
> +    VERBATIM)
> +list(APPEND WebCore_SOURCES ${DERIVED_SOURCES_WEBCORE_DIR}/PlugInsResourcesData.cpp)
> +ADD_SOURCE_WEBCORE_DERIVED_DEPENDENCIES(${WEBCORE_DIR}/css/StyleResolver.cpp PlugInsResourcesData.cpp PlugInsResources.h)

All cool stuff. Didn't even know how to do this.

> Source/WebCore/DerivedSources.make:838
> +PlugInsResources.h : css/make-css-file-arrays.pl bindings/scripts/preprocessor.pm $(PLUG_INS_RESOURCES)

make-css-file-arrays?

> Source/WebCore/css/plugIns.css:34
> - *       <div class="snapshot-overlay"></div> <!-- e.g. for dimming content -->
> - *         <div class="snapshot-label">
> - *           <div class="snapshot-title">Snapshotted Plug-In</div>
> - *           <div class="snapshot-subtitle">Click to restart</div>
> + *     #ShadowRoot
> + *         <div pseudo="-webkit-snapshotted-plugin-content">
> + *             <div class="snapshot-overlay" aria-label="[Title]: [Subtitle]" role="button">

We use the snapshot-overlay in our client/Safari code for something special. I think it needs to be a separate element (it's not clear from crappy formatting that that should be the case)

> Source/WebCore/css/plugIns.css:51
> -object::-webkit-snapshotted-plugin-content .snapshot-container,
> -embed::-webkit-snapshotted-plugin-content .snapshot-container
> +*::-webkit-snapshotted-plugin-content > .snapshot-overlay

I stuck with specific selectors because I didn't want this to impact performance of other content. Remember, this file is evaluated for every page that has a plugin.

[Antoine Quint]

(In reply to comment #5)
> (From update of attachment 200417 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200417&action=review
> 
> Looks good. I think you can split it into a couple of separate patches though.

Yes, will do.
 
> > Source/WebCore/DerivedSources.make:838
> > +PlugInsResources.h : css/make-css-file-arrays.pl bindings/scripts/preprocessor.pm $(PLUG_INS_RESOURCES)
> 
> make-css-file-arrays?

Yeah, that confused me as well, but that script is more generic and handles both CSS and JS.
 
> > Source/WebCore/css/plugIns.css:34
> > - *       <div class="snapshot-overlay"></div> <!-- e.g. for dimming content -->
> > - *         <div class="snapshot-label">
> > - *           <div class="snapshot-title">Snapshotted Plug-In</div>
> > - *           <div class="snapshot-subtitle">Click to restart</div>
> > + *     #ShadowRoot
> > + *         <div pseudo="-webkit-snapshotted-plugin-content">
> > + *             <div class="snapshot-overlay" aria-label="[Title]: [Subtitle]" role="button">
> 
> We use the snapshot-overlay in our client/Safari code for something special. I think it needs to be a separate element (it's not clear from crappy formatting that that should be the case)

I think you mean the -webkit-appearance "snapshotted-plugin-overlay" value. We should rename it to be "snapshotted-plugin-overlay-blur" since this is what it is. We don't have to worry about CSS class names as much since the WebCore plugIns.css file is not injected if the client chrome provides a CSS file of its own. It makes no sense to have a generic CSS since there is no generic shadow tree generated.

> > Source/WebCore/css/plugIns.css:51
> > -object::-webkit-snapshotted-plugin-content .snapshot-container,
> > -embed::-webkit-snapshotted-plugin-content .snapshot-container
> > +*::-webkit-snapshotted-plugin-content > .snapshot-overlay
> 
> I stuck with specific selectors because I didn't want this to impact performance of other content. Remember, this file is evaluated for every page that has a plugin.

OK, will do.

[Antoine Quint]

Created attachment 200541 [details]
Updated patch for build, hopefully fixes GTK and Windows failures.

[WebKit Commit Bot]

Attachment 200541 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/WebCore/DerivedSources.cpp', u'Source/WebCore/DerivedSources.make', u'Source/WebCore/DerivedSources.pri', u'Source/WebCore/GNUmakefile.am', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/Resources/plugIns.js', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/css/CSSDefaultStyleSheets.cpp', u'Source/WebCore/css/plugIns.css', u'Source/WebCore/dom/Document.cpp', u'Source/WebCore/dom/Document.h', u'Source/WebCore/html/HTMLPlugInImageElement.cpp', u'Source/WebCore/html/HTMLPlugInImageElement.h', u'Source/WebCore/page/ChromeClient.h', u'Source/WebCore/rendering/RenderSnapshottedPlugIn.cpp', u'Source/WebKit2/UIProcess/DrawingAreaProxyImpl.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundlePage.h', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageUIClient.cpp', u'Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageUIClient.h', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.h', u'Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp']" exit_code: 1
Source/WebCore/dom/Document.h:1207:  The parameter name "world" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antoine Quint]

The first step towards this is being done in https://bugs.webkit.org/show_bug.cgi?id=115596.

[Antoine Quint]

Created attachment 200548 [details]
Patch

[Dean Jackson]

Comment on attachment 200548 [details]
Patch

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.

[Antoine Quint]

(In reply to comment #11)
> (From update of attachment 200548 [details])
> 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?

Do you have suggestions for sites to browse?

> > 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.

Duh. I'll try to make the change in this patch, should be pretty minor to introduce a HashTable.

> > Source/WebKit2/ChangeLog:21
> > +        (WebKit):
> 
> Remove this line.
> 
> > Source/WebKit2/ChangeLog:28
> > +        (WebKit):
> 
> And this one.

Will do.

> > 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.


Will do.
 
> > 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.

I just don't see the point in injecting CSS that would basically never be used and could cause problems when authoring custom CSS for snapshotted plug-ins.

> > 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?

What's important is that it injects scripts only once per document. Since there should be a single shared world for the life-time of the browser session, it really shouldn't matter what the passed world is.

> > 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.

OK.

> > 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.

My intent was to create a single shared DOMWrapperWorld for the entire browser session. Maybe I misunderstand how DEFINE_STATIC_LOCAL works. Maybe this is what it should be?

    DEFINE_STATIC_LOCAL(RefPtr<DOMWrapperWorld>, isolatedWorld, ());
    if (!isolatedWorld)
        isolatedWorld = DOMWrapperWorld::create(JSDOMWindow::commonVM());

Do you think this world should be created by the Document to have this done in ensurePlugInsInjectedScript()? We'd still have a single DOMWrapperWorld of course.

[Antoine Quint]

Created attachment 200595 [details]
Patch

[Dean Jackson]

Comment on attachment 200595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200595&action=review

I hope it's ok to potentially have a lot of isolated worlds in a page.

> Source/WebCore/css/plugIns.css:57
> +    
> +    top: 5px;

blank line?

> Source/WebCore/dom/Document.cpp:6068
> +    page()->mainFrame()->script()->evaluateInWorld(ScriptSourceCode(jsString), world);
> +
> +    m_hasInjectedPlugInsScript = true;

Maybe here we should also check that there is now a createWorld function. This could be in a followup.

[Jon Lee]

Comment on attachment 200595 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200595&action=review

> Source/WebCore/ChangeLog:40
> +        (WebCore::subtitleText):

Are these functions still necessary, if the overlay content is determined by the JS? Couldn’t we include a separate localized string JS file, provided by the client, and have the injected JS determine the contents using that?

Could be done as a follow-up patch, if it makes sense.

[Antoine Quint]

Created attachment 200621 [details]
Patch for landing

[Antoine Quint]

Created attachment 200638 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 200638 [details]
Patch for landing

Clearing flags on attachment: 200638

Committed r149586: <http://trac.webkit.org/changeset/149586>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.