Remove 'replacement' XBL binding (the pluginProblem UI that appears over links on a page)

RESOLVED FIXED in Firefox 62

Status

()

P3
normal
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: bgrins, Assigned: timdream)

Tracking

(Blocks: 1 bug)

unspecified
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

11 months ago
'replacement' extends 'pluginProblem' and applies to the following selectors: 

a[href='https://get.adobe.com/flashplayer/'], a[href='http://get.adobe.com/flashplayer/'], a[href='https://get.adobe.com/flashplayer'], a[href='http://get.adobe.com/flashplayer'], a[href='https://www.adobe.com/go/getflash/'], a[href='http://www.adobe.com/go/getflash/‘]

It's overlaying links on the page with the plugin replacement UI, only after when the "HiddenPlugin" event happens. The code is at: https://dxr.mozilla.org/mozilla-central/rev/28db2c96ac695ce77c532901f5431a18a1d44374/toolkit/pluginproblem/content/pluginProblem.xml#79.
(Reporter)

Comment 1

11 months ago
After talking with Felipe, it sounds like it's not used anymore and can be removed. For context, the reason it exists is:
A) We wanted to offer the click-to-play UI when sites wanted to use Flash
B) We wanted to remove Flash from navigator.plugins if Click-to-Play was activated (even if Flash was installed)

From the website's side:
1) Site wants to use Flash, and checks if Flash is installed by checking navigator.plugins
2) Site sees that Flash doesn't exist, so it doesn't even try to use Flash (by creating the <object> or <embed>), and instead it gives the users the link
3) Since the site doesn't even try to use Flash, we didn't have the chance to offer the click-to-play UI, even though the user had Flash installed

However, on implementing B), we tested this on Nightly (and with telemetry experiments too if I'm not mistaken), and it was found out that it broke the web too much. So we gave up on B) and we do not exclude Flash from navigator.plugins anymore

Note: this is still configurable through the pref plugins.navigator.hidden_ctp_plugin, by setting it to "Shockwave Flash", but it's not on by default

Note 2: I _think_ we still exclude Flash from navigator.plugins for pages blocked through the Flash blocklist, but I don't think we need to worry about the replacement binding in this case
This is not going to be trivial. Aside from removing bug 1289802, everything landed from bug 1186948 is also obsolete.
See Also: → bug 1186948
(Reporter)

Comment 3

11 months ago
Felipe, is there any chance you'd be able to take this code removal bug? As Tim mentions in Comment 2 it looks like there's code involved here that goes more deeply than just the XBL, and you'd probably be best suited to track everything down. Or if you'd prefer we could put up a patch here that removes the frontend components and move further removal out into follow-ups.
Flags: needinfo?(felipc)
I think we can leave other code removals as follow-ups. This 'replacement' XBL binding landed in support of bug 1186948, which is pref'ed off right now, but arguably they are independent things, and I believe bug 1186948 should remain in the tree longer in case we decide we need it later on.
Flags: needinfo?(felipc)

Updated

10 months ago
Priority: -- → P3
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment hidden (obsolete)
Comment hidden (mozreview-request)
Comment on attachment 8979850 [details]
Bug 1456625 - Remove replacement XBL binding (Backout cb51f3bada90)

https://reviewboard.mozilla.org/r/246032/#review252068

::: browser/modules/PluginContent.jsm
(Diff revision 2)
> -    if (eventType == "PluginPlaceholderReplaced") {
> -      plugin.removeAttribute("href");
> -      let overlay = this.getPluginUI(plugin, "main");
> -      this.setVisibility(plugin, overlay, OVERLAY_DISPLAY.FULL);
> -      // Add pseudo class so our styling will take effect
> -      InspectorUtils.addPseudoClassLock(plugin, "-moz-handler-clicktoplay");

Sorry I was confused, please disregard my previous comment. The patch is now a manual backout of bug 1289802 as landed in https://hg.mozilla.org/mozilla-central/rev/cb51f3bada90
Comment hidden (mozreview-request)
Comment on attachment 8979850 [details]
Bug 1456625 - Remove replacement XBL binding (Backout cb51f3bada90)

Need more tweaks.
Attachment #8979850 - Flags: review?(felipc)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

10 months ago
mozreview-review
Comment on attachment 8979850 [details]
Bug 1456625 - Remove replacement XBL binding (Backout cb51f3bada90)

https://reviewboard.mozilla.org/r/246032/#review252706
Attachment #8979850 - Flags: review?(felipc) → review+

Comment 14

10 months ago
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/de604a05f417
Remove replacement XBL binding (Backout cb51f3bada90) r=Felipe

Comment 15

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/de604a05f417
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.