814168 - Implement click-to-play for plugins in thunderbird

Status: RESOLVED INVALID

(Thunderbird :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Firefox recently gained the ability to make plugins click-to-play by default (and, more importantly, to force click-to-play on outdated or vulnerable plugins).

We should port the work that was done over to our fork of plugins.js.

Comment 1

[prasoon92.iitr]

(In reply to Mike Conley (:mconley) from comment #0)
> Firefox recently gained the ability to make plugins click-to-play by default
> (and, more importantly, to force click-to-play on outdated or vulnerable
> plugins).
> 
> We should port the work that was done over to our fork of plugins.js.

Hi. Can you please elaborate the problem more? Like, links to some code, exact location of problem etc.
I'd like to know more about the problem so that I can decide if I can work on this or not  { that is if I'll even be capable enough or not. }

Comment 2

[Magnus Melin [:mkmelin]]

Firefox implemented click-to-play in bug 711552 + a lot of followups. 

Mostly this code lives in 
http://mxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser-plugins.js

The mail code, at least http://mxr.mozilla.org/comm-central/source/mail/base/content/plugins.js needs to be updated for thunderbird to have it too.

[I didn't actually look at the code much, but that should be the essence of it, Mike may want to chip in]

Comment 3

[Mike Conley (:mconley) (:⚙️)]

What Magnus said is correct - though it's not immediately clear to me if there were other front-end changes in browser/ that we should port beyond what's in browser-plugins.js. That would likely be the first step in tackling this bug - noting what changes were made in platform, and which were made in browser/, and then figuring out how to port what was changed in browser/ to TB.

Comment 4

[sakshi]

Hello,

I would like to ficx this bug.

Comment 5

[Magnus Melin [:mkmelin]]

Feel free to take it. If you don't have enough privileges, see this: https://wiki.mozilla.org/Thunderbird:Bug_Triage#Canconfirm_and_Editbugs_Privileges

Comment 6

[sakshi]

Can someone just give me some more idea on how to start fixing this bug. I went through some of the code but its not very clear as how to start.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

(In reply to sakshi from comment #6)
> Can someone just give me some more idea on how to start fixing this bug. I
> went through some of the code but its not very clear as how to start.

Hey Sakshi,

Sure - so, here's what I'd do: my strategy would be to see how Firefox fixed this issue, and then porting their approach to Thunderbird.

It's quite possible that a lot of the code will already be in toolkit/ (and therefore, shared between Firefox and Thunderbird) so Thunderbird gets it for free. We need to find the stuff that Thunderbird *doesn't* get for free, and port it for ourselves.

The stuff we don't get for free is usually contained under the browser/ subdirectory.

So, here's what I suggest:

1) Read bug 711552 and its dependencies. Get a sense of what was changed, and take note of what changed in browser/, and what didn't.

2) Once you have a sense of what changed in browser/, give a brief summary of those changes here in this bug.

Once #2 is done, I can guide you on how to port it to Thunderbird.

Sound OK? Let me know if you have any questions,

-Mike

Comment 8

[sakshi]

This is a brief idea of what I have understood: 

browser/base/content/browser.js

1) There is a function to show the notification by getting the bool value of plugins.click_to_play. 

2) Initially the click-to-play state is set by making 
     aBrowser._clickToPlayDoorhangerShown = false;
     aBrowser._clickToPlayPluginsActivated = false;

3) There is a switch case for PluginClickToPlay:
   a) PluginClickToPlay: this case handles event if the PluginClickToPlay is enabled. Takes a plugin as parameter checks if it is activated and then loads.

4) There is an event which handles, if the plugin gets crashed while it is not selected.

5)A function to handle the activation of the plugin.

6)A function to show the notifications of the plugin.

   b)PluginDisabled : this case handles if the PluginClickToPlay is disabled.

browser/locales/en-US/chrome/browser/browser.properties

1) Includes activate keys and message labels.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Hey sakshi,

Alright, so in order to port these things to Thunderbird, you'll need to edit mail/base/content/plugins.js. This is almost a verbatim copy of browser-plugins.js, but the treatment of windows is just a tad different.

What I would do is simply to start through your list, bringing over the changes to plugins.js. For an added bonus, it'd be excellent if you wrote some tests for this as well.

You can find our current plugin tests under mail/test/mozmill/content-tabs. They are test-plugin-[blocked|crashing|outdated|unknown].js.

You can run all of these by running the content-tabs Mozmill tests like this:

(in your objdir):

make SOLO_TEST=content-tabs mozmill-one

or you can run an individual test like this:

make SOLO_TEST=content-tabs/test-plugin-blocked.js mozmill-one

Those tests are great for example code on how to write your own set of tests.

Do you need further guidance on how to proceed?

-Mike

Comment 10

[sakshi]

Attachment: WIP

I have edited mail/base/content/plugins.js to port almost all the changes. However I don't know how to port the following:

1) function pageShowEventHandlers(event)
2) onLocationChange- which checks Browser.contentWindow and aWebProgress.DOMWindow

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 742780 [details] [diff] [review]
WIP

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

Looking good, Sakshi! Some style nits. I think I'm also going to what to see some tests for this. Check out the plugin tests under mail/test/mozmill/content-tabs for some ideas on how to set that up.

::: mail/base/content/plugins.js
@@ +323,5 @@
> +      let objLoadingContent = aPlugin.QueryInterface(Ci.nsIObjectLoadingContent);
> +      objLoadingContent.playPlugin();
> +      return;
> +    }
> +    

Nit - all of the trailing whitespace in here needs to go.

@@ +326,5 @@
> +    }
> +    
> +    let overlay = doc.getAnonymousElementByAttribute(aPlugin, "class", "mainBox");
> +    overlay.addEventListener("click", function(aEvent) {
> +    if (aEvent.button == 0 && aEvent.isTrusted)

Indent these two lines to make it more clear that they're in the function.

@@ +335,5 @@
> +      gPluginHandler._showClickToPlayNotification(browser);
> +    },
> +  
> +    reshowClickToPlayNotification: function PH_reshowClickToPlayNotification() {
> +    if (!Services.prefs.getBoolPref("plugins.click_to_play"))

Indent all of the lines in this function.

Comment 12

[Mark Banner (:standard8)]

(In reply to sakshi from comment #10)
> Created attachment 742780 [details] [diff] [review]
> However I don't know how to port the following:
> 
> 1) function pageShowEventHandlers(event)

I think you can just add that as a listener, and set it up in one of the functions that is initialised when the 3-pane window is opened. This is how FF registers it:

http://hg.mozilla.org/mozilla-central/annotate/ab5f29823236/browser/base/content/browser.js#l973

From what I've seen the pageshow event seems to be global to gecko, so we should just be able to re-use it.

> 2) onLocationChange- which checks Browser.contentWindow and
> aWebProgress.DOMWindow

For content tabs & web search tabs, look in webSearchTab.js and specialTabs.js.

We're probably going to need to add an explicit progress listener for the messagepane browser element though.

Comment 13

[Kushagra Singh [:kushagra]]

hi mark,i would like to pick up this bug and continue the work it.if the current assignee isnt active anymore can you please assign me to it?

Comment 14

[Magnus Melin [:mkmelin]]

Here you go!

Comment 15

[Kushagra Singh [:kushagra]]

sorry for the late reply,was stuck in tests.
i followed mark's pointers in comment 12:-

1) gBrowser.addEventListener("pageshow", function(event) {
      if (content && event.target == content.document)
        setTimeout(pageShowEventHandlers, 0, event.persisted);
    }, true);
  function pageShowEventHandlers(persisted) {
   charsetLoadListener();
   XULBrowserWindow.asyncUpdateUI();
   if (persisted)
     gPluginHandler.reshowClickToPlayNotification();
  }

will this successfully port pageShowEventHandlers(event) ?
plus i cant find the function that is initialized when a 3 pane window is opened.

2)onLocationChange- which checks Browser.contentWindow and aWebProgress.DOMWindow

this piece of code no longer exists in the mozilla central codebase. how should i proceed now?

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Hey kushagra - thanks for hacking on this. I'll have some answers for you tonight!

Comment 17

[Mike Conley (:mconley) (:⚙️)]

(In reply to Kushagra Singh [:kushagra] from comment #15)
> sorry for the late reply,was stuck in tests.
> i followed mark's pointers in comment 12:-
> 
> 1) gBrowser.addEventListener("pageshow", function(event) {
>       if (content && event.target == content.document)
>         setTimeout(pageShowEventHandlers, 0, event.persisted);
>     }, true);
>   function pageShowEventHandlers(persisted) {
>    charsetLoadListener();
>    XULBrowserWindow.asyncUpdateUI();
>    if (persisted)
>      gPluginHandler.reshowClickToPlayNotification();
>   }
> 
> will this successfully port pageShowEventHandlers(event) ?

No, I don't think so - both charsetLoadListener and XULBrowserWindow are not defined for Thunderbird - those can come out.

> plus i cant find the function that is initialized when a 3 pane window is
> opened.

Perhaps try the gPluginHandler.addEventListeners function? This function is called with each browser tab that might have a plugin in it.

> 
> 2)onLocationChange- which checks Browser.contentWindow and
> aWebProgress.DOMWindow
> 
> this piece of code no longer exists in the mozilla central codebase. how
> should i proceed now?

If I'm not mistaken, onLocationChange has moved to here: http://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#3563

I'm not sure I've made things more clear here - it's been a while since somebody's hacked on this, and there's been considerable churn in mozilla-central since the last attempt on this bug, so I'm not sure if the previous patch can still apply and be effective...

Comment 18

[Kushagra Singh [:kushagra]]

let me first port these two remaining functions then we can check if the patch works or not. if not then we can get down to making the required changes.

Comment 19

[Akshendra Pratap Singh]

What's the status of this bug?

Comment 20

[Georg Fritzsche [:gfritzsche]]

It wasn't worked on for a few months, so presumable Kushagra isn't working on it anymore.
I don't think this is a "good first bug" though, it is a bit more involved than that.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

I agree - especially now that browser-plugins.js has changed so much from bug 899347, this is no longer a GFB.

Comment 22

[:aceman]

Plugin support was removed from Thunderbird in bug 1508942.
Of course, there is still support for extensions (addons).