Closed Bug 1456625 Opened 6 years ago Closed 6 years ago

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

Categories

(Core Graveyard :: Plug-ins, task, P3)

Product:
Core Graveyard
Component:
Plug-ins
Type:
task

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: bgrins, Assigned: timdream)

References

Details

Attachments

(1 file)

Bug 1456625 - Remove replacement XBL binding (Backout cb51f3bada90)
59 bytes, text/x-review-board-request
Felipe
: review+
Details 
'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.
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: → 1186948
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)
Priority: -- → P3
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Attachment #8979850 - Flags: review?(felipc)
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 on attachment 8979850 [details]
Bug 1456625 - Remove replacement XBL binding (Backout cb51f3bada90)

Need more tweaks.
Attachment #8979850 - Flags: review?(felipc)
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+
Pushed by timdream@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/de604a05f417
Remove replacement XBL binding (Backout cb51f3bada90) r=Felipe
https://hg.mozilla.org/mozilla-central/rev/de604a05f417
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Type: enhancement → task
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: