743312 - Implement click-to-play plugins (port FF bug 711552 and bug 744964)

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Firefox bug 711552 implemented click-to-play plugins. We should port this in time. Unfortunately our implementation is a bit different (partly XBL: notification.xml), to it's not a simple porting action. Probably needs some help from Neil.

Original changeset was:
http://hg.mozilla.org/mozilla-central/rev/fed1018b5842

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Before I forget: In that changeset, toolkit/mozapps/plugins/content/pluginProblemContent.css was changed. Our Modern theme currently doesn't include that file. No idea whether we need it.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

Attachment: WIP patch

Warning: The attached doesn't work yet *at all* so don't try. This is for discussion only. Also not taking yet since I may very well be the wrong one to go ahead.

I tried to move the methods from FF's browser.js to our notification.xml but faced several issues where I wasn't sure how to solve them. Neil, can you take a look at the TODOs (compare with the original changeset if you like) and direct me at how it's supposed to work in our XBL implementation? No need for a full review yet.

Comment 3

[[e-mail]]

Comment on attachment 613002 [details] [diff] [review]
WIP patch

Ok, so the basic idea is that notification.xml is a tab-specific version of all the gXXXHandlers all rolled into one, so it only has to handle events for its own tab, which makes for some convenient code simplification. It also gets to use <handler> to handle events, rather than mucking about with adding and removing event listeners in script. It's also a web progress listener.

There are actually two bindings. The first binding only implements notification bars. (Because it inherits from the main notification binding, this results in yet more code simplification.) The second binding implements overrides for doorhangers. I don't expect you to code both implementations, but they are roughly similar so you might be able to work it out. There's a pref to turn off doorhangers (which only takes effect in new windows).

> function pageShowEventHandlers(event)
> {
>   // Filter out events that are not about the document load we are interested in
>   if (event.originalTarget == content.document) {
>     checkForDirectoryListing();
>+    // The PluginClickToPlay events are not fired when navigating using the
>+    // BF cache. |event.persisted| is true when the page is loaded from the
>+    // BF cache, so this code reshows the notification if necessary.
>+    if (event.persisted)
>+      // TODO: replace gPluginHandler by...?
>+      gPluginHandler.reshowClickToPlayNotification();
notification.xml needs to have its own pageshow <handler> which calls this.reshowClickToPlayNotification(); (naturally this handler only ever sees events that are about the document load we are interested in!)

>+      // initialize the click-to-play state
>+      browser._clickToPlayDoorhangerShown = false;
>+      browser._clickToPlayPluginsActivated = false;
This probably belongs in notification.xml's onLocationChange method. (Bug in firefox that background tabs don't update their doorhangers correctly?)

>+            // TODO: the FF changeset included an event.type PluginCrashed/PluginClickToPlay check
[Maybe they're reusing the same method for multiple events? I need to look.]

>+            // TODO: gBrowser -> this.activeBrowser ?
>+            var browser = gBrowser.getBrowserForDocument(aContentWindow.document);
No, browser -> this.activeBrowser (so no need to pass it around everywhere).

>+            var notification = PopupNotifications.getNotification("click-to-play-plugins", browser);
>+            if (notification)
>+              notification.remove();
Note that the doorhangers part of the code belongs in the popup-notification binding, with at least a placeholder method in the main binding too.

>+                gPluginHandler.activatePlugins(aEvent.target.ownerDocument.defaultView.top);
this.activatePlugins();
[since we already know what our content document is...]

>+              gPluginHandler._showPluginClickToPlayDoorhanger(browser);
this.showPluginClickToPlayPrompt();
[with at least a placeholder method in the main binding again.]

>+            // TODO: gPluginHandler -> this ?
Indeed!

Comment 4

[Philip Chee]

> +            if (!browser._clickToPlayDoorhangerShown)
> +              gPluginHandler._showPluginClickToPlayDoorhanger(browser);
I think this is _showClickToPlayNotification()

Comment 5

[[e-mail]]

Comment on attachment 613002 [details] [diff] [review]
WIP patch

>             // Hide the in-content UI if it's too big. The crashed plugin handler already does this.
>+            // TODO: the FF changeset included an event.type PluginCrashed/PluginClickToPlay check
So, it turns out that the existing comment explains the situation - the crashed plugin handler has its own code that doesn't use the pluginUnavailable method and does its own UI hiding.

Comment 6

[[e-mail]]

(Firefox could have avoided that check by returning directly from those two cases of its switch statement. We of course don't have a switch statement...)

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

This almost works. I get the below error, which seems to be a timing issue: With Venkman, when the code part was still in its own method, I could set a breakpoint and re-execute the line above it (without the "var " in front) and from there it worked (either clicking the plugin placeholder or the doorhanger button).

Error: overlay is null
Source File: chrome://communicator/content/bindings/notification.xml
Line: 1781

Otherwise I've done some refactoring and used bind(this) to fix scoping issues. No support for notification bars yet.

Comment 8

[[e-mail]]

Comment on attachment 614284 [details] [diff] [review]
patch

>+      <property name="cwu" readonly="true">
Would it hurt to rename this contentWindowUtils?

>+              this.activeBrowser._clickToPlayDoorhangerShown = false;
>+              this.activeBrowser._clickToPlayPluginsActivated = false;
As far as I can tell, they only use the browser because they haven't got anywhere more convenient; we could just use a couple of <field>s.

Also, you have a mixture of Doorhanger, Notification and Prompt. Please choose one of the latter two and stick to it.

>+      <method name="reshowClickToPlayNotification">
Nit: this is small enough to be inlined into the handler, either as one long if statement or multiple early returns.
(No, I don't know why they did it the way they did.)

>+          overlay.addEventListener("click",
Please use addLinkClickCallback.

>+              notification.remove();
The four lines of code that do the actual removal should be in their own overridden method removeClickToPlayThingamajig but the actual plugin activation should be in the main binding. r=me with that fixed.
(I did once consider adding a generic notification removal method.)

>+            let options = { dismissed: true };
Ah, that explains why I didn't notice it immediately ;-)

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

Well great, r+! But I cannot land this as-is. The reason is that this:

Error: overlay is null
Source File: chrome://communicator/content/bindings/notification.xml
Line: 1781

is now:

Error: linkNode is null
Source File: chrome://communicator/content/bindings/notification.xml
Line: 540

(same problem source: "overlay" is null) and I don't know how to fix it. :-/


(In reply to [e-mail] from comment #8)
> >+      <method name="reshowClickToPlayNotification">
> Nit: this is small enough to be inlined into the handler, either as one long
> if statement or multiple early returns.

I hope you accept a single early return, too. ;-)

> >+          overlay.addEventListener("click",
> Please use addLinkClickCallback.

Oh, nice! But that didn't solve my problem (granted, how could it?).

> >+            let options = { dismissed: true };
> Ah, that explains why I didn't notice it immediately ;-)

No idea what you're trying to say, but I take it there's no hidden action item included.

Comment 10

[[e-mail]]

(In reply to Jens Hatlak from comment #9)
> (same problem source: "overlay" is null) and I don't know how to fix it. :-/
Try adding plugin.getBoundingClientRect(); before getting its document etc.

> > >+            let options = { dismissed: true };
> > Ah, that explains why I didn't notice it immediately ;-)
> No idea what you're trying to say, but I take it there's no hidden action
> item included.
I assumed this prevents the notification from popping up immediately.

Comment 11

[[e-mail]]

Comment on attachment 614581 [details] [diff] [review]
patch v2

Well, the worst possible way would have been something like this:

if (event.persisted) {
  if (!this._prefs.getBoolPref("plugins.click_to_play"))
     return;
  if (this.contentWindowUtils.plugins.length)
    this.showPluginClickToPlayPrompt();
}

So you did at least managed to avoid a -

Comment 12

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2a

(In reply to [e-mail] from comment #10)
> (In reply to Jens Hatlak from comment #9)
> > (same problem source: "overlay" is null) and I don't know how to fix it. :-/
> Try adding plugin.getBoundingClientRect(); before getting its document etc.

That worked! :-) I just wonder whether we should somehow document this magic.

> I assumed this prevents the notification from popping up immediately.

Yes, that's the case.

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3

As Philip noticed in parallel to me, bug 744964 changed the icon from the generic add-on one to the one for plugins. Since it's a simple change, I'd like to roll that change into this one.

PR0PHET/Philip: Please don't "randomly" set dependencies on other people's bugs. Neither of the two bugs you added are strictly dependencies (i.e. things that need to be fixed before this bug can be fixed, or things the scope of this bug includes - and extending the scope is primarily up to the assignee and reviewer(s), or through discussion). Please add an explanatory comment instead and let the assignee decide in such cases what to do.

[Will obsolete the current patch if the new one gets r+.]

Comment 14

[Jens Hatlak (:InvisibleSmiley)]

Neil, when you review this, please note my question from comment 12.

Comment 15

[Philip Chee]

Sorry, am picking up bad habits from Serge :P

Comment 16

[[e-mail]]

(In reply to Jens Hatlak from comment #12)
> (In reply to comment #10)
> > (In reply to Jens Hatlak from comment #9)
> > > (same problem source: "overlay" is null) and I don't know how to fix it. :-/
> > Try adding plugin.getBoundingClientRect(); before getting its document etc.
> That worked! :-) I just wonder whether we should somehow document this magic.
Toolkit does it all over the place too, although sometimes they use other properties such as offsetTop. If you wanted you could add a comment along the lines of /* flush layout to force XBL binding attachment */ or some such.

Comment 17

[[e-mail]]

Comment on attachment 615044 [details] [diff] [review]
patch v3

>             <image id="password-notification-icon" class="notification-anchor-icon" role="button"/>
>+            <image id="plugins-notification-icon" class="notification-anchor-icon" role="button"/>
You put the plugin notification icon after the password notification icon, and you put most of the CSS in the same order, except for
>+.popup-notification-icon[popupid="click-to-play-plugins"] {
which you put before indexedDB for some reason. r=me with that fixed.

Comment 18

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3a [Checkin: Comment 19]

(In reply to [e-mail] from comment #16)
> Toolkit does it all over the place too, although sometimes they use other
> properties such as offsetTop. If you wanted you could add a comment along
> the lines of /* flush layout to force XBL binding attachment */ or some such.

Actually I chose to go with what's already used in method PluginDisabled (plugin.clientTop) in the end, including the comment. It's even visible in the context of the patch! :-)

(In reply to [e-mail] from comment #17)
> >             <image id="password-notification-icon" class="notification-anchor-icon" role="button"/>
> >+            <image id="plugins-notification-icon" class="notification-anchor-icon" role="button"/>
> You put the plugin notification icon after the password notification icon,
> and you put most of the CSS in the same order, except for
> >+.popup-notification-icon[popupid="click-to-play-plugins"] {
> which you put before indexedDB for some reason.

Well one could argue that "pl" (as in "plugins") comes after "pa" (as in "password"), but "cl" (as in "click") comes before "in" (as in "indexedDB"). But then I'd have to re-sort the entries in suite/browser/navigator.css, and you would like that even less I think. ;-)

Comment 19

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 615086 [details] [diff] [review]
patch v3a [Checkin: Comment 19]

http://hg.mozilla.org/comm-central/rev/0944835c672c

Comment 20

[Jens Hatlak (:InvisibleSmiley)]

Removing bug 743429 from the dependencies list (that one depends on this one, but I cannot move it to the Blocks list since it's already a dependency of base bug 711552 and Bugzilla forbids the dependency loop that the adding here would create). [Seems it's going to be fixed in the back-end anyway.]

Comment 21

[[e-mail]]

Attachment: notificationbox methods for disabled doorhangers

I moved the methods to match the order that they are in doorhanger code.

There's a fair amount of cut and paste from the doorhanger methods.

Comment 22

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 616156 [details] [diff] [review]
notificationbox methods for disabled doorhangers

Review of attachment 616156 [details] [diff] [review]:
-----------------------------------------------------------------

Works as expected, code looks good, r=me. :-)

Comment 23

[[e-mail]]

Comment on attachment 616156 [details] [diff] [review]
notificationbox methods for disabled doorhangers

Pushed changeset 78c57aba3724 to comm-central.

Comment 24

[[e-mail]]

Comment on attachment 615086 [details] [diff] [review]
patch v3a [Checkin: Comment 19]

> crashedpluginsMessage.title=The %S plugin has crashed.
> crashedpluginsMessage.reloadButton.label=Reload page
> crashedpluginsMessage.reloadButton.accesskey=R
> crashedpluginsMessage.submitButton.label=Submit a crash report
> crashedpluginsMessage.submitButton.accesskey=S
> crashedpluginsMessage.learnMore=Learn Moreâ¦
> 
>+activatePluginsMessage.message=Would you like to activate the plugins on this page?
>+activatePluginsMessage.label=Activate plugins
>+activatePluginsMessage.accesskey=A
Bah, I just noticed that this doesn't follow our naming convension (compare crashedpluginsMessage above). Will try to fix in a separate bug.

Comment 25

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120423 Firefox/14.0a1 SeaMonkey/2.11a1 ID:20120423003002

I'm using plugins.click_to_play set to true since some days ago, and now AFAICT embedded plugin objects appear on page load as grey rectangles with a dashed Lego piece on them, and the text:

Tap here to activate plugin.
Click here to activate plugins.

I believe that the misleading use of the singular on one line and of the plural on the other deserves a followup bug of "minor" Severity. Otherwise I VERIFY this bug. Feel free to REOPEN (with details) if there is something else not properly FIXED. In particular if "tapping" and "clicking" behaviours are supposed to be different I can't check those differences on this desktop computer, equipped with neither touchscreen nor trackpad.

I suppose that, like all fixes introducing new (not yet translated) texts, this fix will not be ported to aurora, beta, etc.

Comment 26

[Tony Mechelynck [:tonymec]]

P.S. I did check that Flash objects activate on clicking, and I noticed (on the occasion of a software update to the adobe-flash package) that (unlike on Firefox 3.6 which of course hasn't got the fix) those "unclicked" (stalled) plugin objects did not "use" the plugin *.so file.

Comment 27

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Tony Mechelynck [:tonymec] from comment #25)
> I'm using plugins.click_to_play set to true since some days ago, and now
> AFAICT embedded plugin objects appear on page load as grey rectangles with a
> dashed Lego piece on them, and the text:
> 
> Tap here to activate plugin.
> Click here to activate plugins.

Any strings that appear in content live in Core/Toolkit. If you find any issues there you need to find/open bugs in that area, not for SeaMonkey. If you check the patch(es) attached to this bug you'll see that the l10n involved only concerns the chrome UI part (doorhangers/notification bars). Same goes for any follow-ups like bug 747519.

> I suppose that, like all fixes introducing new (not yet translated) texts,
> this fix will not be ported to aurora, beta, etc.

Correct.