810082 - click-to-play: improve handling of invisible or hidden plugins (e.g. on music sites like Pandora)

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

With the recent test day for click-to-play (https://wiki.mozilla.org/QA/Plugins/CTP_TopSites ), it has become apparent that the implementation does not deal well enough with invisible or hidden plugins. These are common on music sites such as Pandora.
The issues are a) sites often assume plugins will activate automatically, and thus scripts fail and b) even if a site waits for the plugin to be activated, the UI to do so (click the small plugin icon in the urlbar, click "activate") is not discoverable.
I'm not sure what we can do about problem a, but we should be able to figure out something for b.

Comment 1

[John Schoenick [:johns]]

One thing we may need to be aware of here is the various ways plugins might be invisible. Plugins don't instantiate as display:none, so sites with invisible plugins are using some combination of visibility:none, width/height=0, or |display:absolute;left:-99999px;|, etc..

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Just making sure these comments are included in this discussion.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #0)
> When a plugin is click-to-play and is invisible or otherwise can't be
> clicked on, it seems that many users don't know what to do next. There is an
> icon that shows up in the urlbar, but I certainly wouldn't have noticed it
> unless somebody told me about it.
> 
> I really expected that we would treat CTP like crashed plugins: if the
> plugin instance(s) can't be activated on the page, we should show a
> notification bar which would give the user a more obvious way to play plugin
> content.
> 
> This is really necessary for audio sites that use Flash
> (pandora/grooveshark/jango). See bug 809846 which has some specific examples.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> If the notification bar is too intrusive "always", I still think we should
> show it if/when the site tries to script the plugin element.

Comment 4

[John Schoenick [:johns]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> If the notification bar is too intrusive "always", I still think we should
> show it if/when the site tries to script the plugin element.

This is relatively easy to do, fwiw

Comment 5

[Alex Keybl [:akeybl]]

David - do you have an estimate of when you'll be ready to land this change?

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Alex Keybl [:akeybl] from comment #5)
> David - do you have an estimate of when you'll be ready to land this change?

I talked to John about this, and it looks like the best way to do this would depend on changes made in bug 767635, which he said he'd work on finishing this week. So, I'm thinking late next week, depending on review wait time.

Comment 7

[Alex Keybl [:akeybl]]

I pinged in bug 767635. Are we ready to land this outside of bug 767635 David? Any updates?

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch part 1/?

This patch opens the popup notification when a plugin is scripted on a page and there are no "visible" plugins (the definition of visible is tricky).
This doesn't require bug 767635, but ideally things will be re-arranged a bit when that lands.

I'm also thinking of some other things that would be nice for handling invisible plugins:
1. re-visit the "activate invisible plugins when a visible plugin is clicked on" idea
2. add permissions for plugins to automatically activate for the current session (on the current site) if the popup notification is used (this covers the case where sites aren't as good as, for example, Pandora in handling click-to-play).

Tests to come.

Comment 9

[John Schoenick [:johns]]

Comment on attachment 685883 [details] [diff] [review]
patch part 1/?

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

::: dom/base/nsDOMClassInfo.cpp
@@ +9659,5 @@
> +  if (!xpc::AccessCheck::callerIsChrome()) {
> +    nsContentUtils::DispatchTrustedEvent(content->GetDocument(), content,
> +                                         NS_LITERAL_STRING("PluginScripted"),
> +                                         true, true);
> +  }

This is going to get fired a *lot*, and really should be in nsObjectLoadingContent so we can pseudo-properly track its script-accessed state. It's looking like bug 767635 is not going to be suitable for 18, so I'm going to make a patch for this bug tomorrow to add a better hook separate from 767635

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 685883 [details] [diff] [review]
patch part 1/?

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

::: browser/base/content/browser-plugins.js
@@ +226,5 @@
>          let manageLink = doc.getAnonymousElementByAttribute(plugin, "class", "managePluginsLink");
>          self.addLinkClickCallback(manageLink, "managePlugins");
>          break;
> +
> +      case "PluginScripted":

I don't think the name PluginScripted explains what is going on here. From looking briefly at the nsDOMClassInfo changes, it looks like this event is generated any time content script tries to access a plugin instance. Is that correct?

If we are trying to improve the handling of invisible plugins, I think we should find a different trigger for determining if the notification should get shown. This gets hairy pretty fast though, since we can't just do a check after DOMContentLoaded or Load since plugins can be added to the DOM after either of those events.

@@ +232,5 @@
> +          break;
> +        }
> +        let browser = gBrowser.getBrowserForDocument(doc.defaultView.top.document);
> +        if (!browser._pluginScripted && !browser._handlePluginScriptedTimeout) {
> +          browser._handlePluginScriptedTimeout = setTimeout(function() {

browser._pluginScripted is not set to true until handlePluginScripted is called, so if script on the page tries to get access to a plugin instance twice within 500ms handlePluginScripted will be called twice with separate timeouts. Worst case this means that the function is called at least 500ms apart, meaning that the user will have a half second to react to this doorhanger which can then reappear.

@@ +259,5 @@
> +      if (!overlay)
> +        return false;
> +
> +      // The 240x200 is from pluginProblemContent.css - if an object/embed/etc.
> +      // doesn't have a width or height set, it gets set to 240 or 200,

s/240 or 200/240 by 200/

It would also be nice if we could find a way not to propagate these constants since it will be easy for one set of them to get out of sync with the others.

@@ +263,5 @@
> +      // doesn't have a width or height set, it gets set to 240 or 200,
> +      // respectively. The idea is that if the plugin has set a width/height
> +      // greater than this, it is probably visible.
> +      let pluginRect = plugin.getBoundingClientRect();
> +      return ((pluginRect.width > 240 || pluginRect.height > 200) &&

This should check for width > 240 && height > 200, or else we may consider a 240x0 plugin to be visible.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to John Schoenick [:johns] from comment #9)
> This is going to get fired a *lot*, and really should be in
> nsObjectLoadingContent so we can pseudo-properly track its script-accessed
> state. 

Would it be possible to change this approach to only fire once or only once per N ms? 
I don't know how much work is involved in the whole event-firing/-delivery/-processing path, but this could have noticable performance impacts in some scenarios.

Comment 12

[John Schoenick [:johns]]

Attachment: Part 0.5 - Add script access event for CTP'd plugins

Comment 13

[John Schoenick [:johns]]

Comment on attachment 686368 [details] [diff] [review]
Part 0.5 - Add script access event for CTP'd plugins

Alright, this adds a new scriptRequestPluginInstance call specifically for nsDOMClassInfo that fires a "PluginScripted" event the first time content tries to access any CTP-mode plugins.

I rebased david's patch onto this and it seems to work flawlessly on Pandora.

This has the unfortunate side effect of adding *another* plugin instantiation call to OBJLC, but it makes it easier to remove the SyncStartPluginInstance call in bug 767635, so it ultimately works out.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 686368 [details] [diff] [review]
Part 0.5 - Add script access event for CTP'd plugins

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

This looks great. Just the one question below.

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2193,5 @@
> +nsObjectLoadingContent::ScriptRequestPluginInstance(bool aCallerIsChrome,
> +                                                    nsNPAPIPluginInstance **aResult)
> +{
> +  nsCOMPtr<nsIContent> thisContent =
> +    do_QueryInterface(static_cast<nsIImageLoadingContent*>(this));

Is 'nsIImageLoadingContent' correct here?

Comment 15

[John Schoenick [:johns]]

(In reply to David Keeler from comment #14)
> Comment on attachment 686368 [details] [diff] [review]
> Is 'nsIImageLoadingContent' correct here?

It is actually, ObjectLoadingContent is missing a proper QueryInterface to get back to nsIContent, but directly inherits from ImageLoadingContent which has it, resulting in the hideous QueryInterface(static_cast<>()) construct used all over this file.

Comment 16

[John Schoenick [:johns]]

Attachment: Bug 810082 - Part 0.5 - Add script access event for CTP'd plugins

Cleaned up comments a bit

Comment 17

[Josh Aas]

Comment on attachment 686794 [details] [diff] [review]
Bug 810082 - Part 0.5 - Add script access event for CTP'd plugins

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

::: content/base/public/nsIObjectLoadingContent.idl
@@ +132,5 @@
> +   * Requests the plugin instance for scripting, attempting to spawn it if
> +   * appropriate.
> +   *
> +   * The first time non-chrome code tries to access a pre-empted plugin
> +   * (click-to-play or play preview), an event is dispatched to the front end.

This is a strange way to reference "the front end", since we don't know what that is here. Maybe change it to "a front end".

::: content/base/src/nsObjectLoadingContent.cpp
@@ +2209,5 @@
> +    nsresult rv = NS_DispatchToCurrentThread(ev);
> +    if (NS_FAILED(rv)) {
> +      NS_WARNING("failed to dispatch PluginScripted event");
> +    }
> +    mScriptRequested = true;

After this you're still going to return NS_OK, which seems fine, just want to check with you to be sure that's what you want.

::: content/base/src/nsObjectLoadingContent.h
@@ +435,5 @@
>      // Protects LoadObject from re-entry
>      bool                        mIsLoading : 1;
>  
> +    // For plugin stand-in types (click-to-play, play preview, ...) tracks
> +    // whether the front end has tried to access the plugin script object.

This "the front end" reference seems a bit odd too. Clarify.

Comment 18

[John Schoenick [:johns]]

Attachment: Part 0.5 - Add script access event for CTP'd plugins. r=josh

Updated to address review comments

(In reply to Josh Aas (Mozilla Corporation) from comment #17)
> After this you're still going to return NS_OK, which seems fine, just want
> to check with you to be sure that's what you want.

Yeah, nsDOMClassInfo doesn't expect to always get a plugin, returning OK and
null'ing the pointer is fine.

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: also check XBL, etc.

In addition to not firing the event when we're in chrome code, we don't want to fire it if we're in XBL code or if we're not actually running any JS (this happens when the bindings get instantiated).

Comment 20

[John Schoenick [:johns]]

Attachment: Part 0.5 - Add script access event for CTP'd plugins

So david found we needed a check to ensure that we are actually in content
script, and not in XBL code, for this to be useful. Re-requesting review just
because we're touching scary nsDOMClassInfo things.

Comment 21

[John Schoenick [:johns]]

Attachment: Part 0.5b [for branches only] - Avoid changing nsIObjectLoadingContent interface

This would be folded onto the patch on beta/aurora to avoid changing the OBJLC IID
- it moves the new ScriptRequestPluginInstance to the base class and gets at it
by static_casting, which is ugly, but safe in this instance.

Comment 22

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: part 1 v2

Here's the next take on the patch.
As per https://bugzilla.mozilla.org/show_bug.cgi?id=545514#c11, I'm removing the css that makes unsized plugins 240x200, which simplifies things greatly for this.
I also added some tests.

Comment 23

[John Schoenick [:johns]]

Comment on attachment 687326 [details] [diff] [review]
part 1 v2

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

::: browser/base/content/browser-plugins.js
@@ +233,5 @@
> +
> +      case "PluginScripted":
> +        if (!overlay || !overlay._bindingHandled) {
> +          break;
> +        }

Are we sure we wont potentially race with layout here? (e.g. If someone re-displays a plugin and then accesses it immediately?)

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ -6,5 @@
>  
> -html|object:not([width]), html|object[width=""],
> -html|embed:not([width]), html|embed[width=""],
> -html|applet:not([width]), html|applet[width=""] {
> -  width: 240px;

What did this rule do? Does this mean we no longer assign a default width/height to any embed/object tags?

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 687326 [details] [diff] [review]
part 1 v2

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

::: browser/base/content/browser-plugins.js
@@ +265,5 @@
> +      let overlay = doc.getAnonymousElementByAttribute(plugin, "class", "mainBox");
> +      if (!overlay)
> +        return false;
> +
> +      var tmpDisplay = overlay.style.display;

nit: I don't see a reason to mix & match |let| and |var| here.

@@ +272,5 @@
> +      var computedStyle = contentWindow.getComputedStyle(plugin);
> +      // sometimes 1px by 1px is used to indicate "invisibility"
> +      // (as opposed to 0px by 0px)
> +      var isVisible = (parseInt(computedStyle.width) > 1 &&
> +                       parseInt(computedStyle.height) > 1);

While "visible", this isn't useful for handling if a plugin is going to be clickable by users. The previous default size of 240x200 was much more clickable then a potential 5x5 plugin.

::: toolkit/mozapps/plugins/content/pluginProblemContent.css
@@ -12,5 @@
> -
> -html|object:not([height]), html|object[height=""],
> -html|embed:not([height]), html|embed[height=""],
> -html|applet:not([height]), html|applet[height=""] {
> -  height: 200px;

I'm not sure what will stop working as expected when removing these. What was the major problem with leaving them as-is?

Comment 25

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to John Schoenick [:johns] from comment #23)
> ::: toolkit/mozapps/plugins/content/pluginProblemContent.css
> @@ -6,5 @@
> >  
> > -html|object:not([width]), html|object[width=""],
> > -html|embed:not([width]), html|embed[width=""],
> > -html|applet:not([width]), html|applet[width=""] {
> > -  width: 240px;
> 
> What did this rule do? Does this mean we no longer assign a default
> width/height to any embed/object tags?

I believe so, yes.

(In reply to Jared Wein [:jaws] from comment #24)
> Comment on attachment 687326 [details] [diff] [review]
> part 1 v2
> ::: browser/base/content/browser-plugins.js
> @@ +272,5 @@
> > +      var computedStyle = contentWindow.getComputedStyle(plugin);
> > +      // sometimes 1px by 1px is used to indicate "invisibility"
> > +      // (as opposed to 0px by 0px)
> > +      var isVisible = (parseInt(computedStyle.width) > 1 &&
> > +                       parseInt(computedStyle.height) > 1);
> 
> While "visible", this isn't useful for handling if a plugin is going to be
> clickable by users. The previous default size of 240x200 was much more
> clickable then a potential 5x5 plugin.

It's not really about whether it's clickable or not - it's more about if the website intended for the plugin to be part of visible content or not. My reasoning is that if the site doesn't assign a width/height to the plugin (or it's something like 0x0 or 1x1 and it doesn't inherit a width/height as a result of css/layout other than our overlays), then it's not part of visible content.

The problem with the default 240x200 was that we had no way of determining if the site had intended the plugin to be 240x200 and part of visible content or if we had defaulted it to that.

> ::: toolkit/mozapps/plugins/content/pluginProblemContent.css
> @@ -12,5 @@
> > -
> > -html|object:not([height]), html|object[height=""],
> > -html|embed:not([height]), html|embed[height=""],
> > -html|applet:not([height]), html|applet[height=""] {
> > -  height: 200px;
> 
> I'm not sure what will stop working as expected when removing these. What
> was the major problem with leaving them as-is?

The major problem with leaving this is as I said above: we have no idea if the site meant for the plugin to be 240x200 or if we made it that way. Also, like you said in a previous review, it means having these magic constants with no good way (that I see) to propagate them from that css file to browser-plugins.js.
There's at least one test that expects plugins to be 240x200, but it was added specifically to test this css.
Like I said, bz said in bug 545514 comment 11 that he saw no reason to keep this default-sizing.

Comment 26

[John Schoenick [:johns]]

Would this mean that simple objects like:
> <object data="./movie.swf">

Are now invisible? I'd think that runs a pretty high risk of regressing at least some sites, for something we're landing on beta.

It is also possible to determine if a style rule came from content or chrome, the dev-tools do it, but I don't know how involved it is.

Comment 27

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to John Schoenick [:johns] from comment #26)
> Would this mean that simple objects like:
> > <object data="./movie.swf">
> 
> Are now invisible? I'd think that runs a pretty high risk of regressing at
> least some sites, for something we're landing on beta.

Yes, unless they inherit a size or fill their container or something. I doubt anyone does this, though, because although Chrome/IE also has a default, it's different from Firefox (looks like 300x150), so if anyone cares about what their plugins actually look like, they have to set the size.

> It is also possible to determine if a style rule came from content or
> chrome, the dev-tools do it, but I don't know how involved it is.

I'll look into this.

Comment 28

[Alex Keybl [:akeybl]]

It's highly desirable to get this fix into beta 4 (going to build Tuesday 12/11), so that we can gather feedback from beta users - basically checking if the doorhanger comes down too frequently, since no significant number of users reported this specific issue pre-release (a dooorhanger not coming down automatically at all).

Do we think we can get this resolved on m-c before the end of the week? Getting a try build together for QA to test Google Talk, Pandora, etc. would be great in preparation for landing on beta.

Comment 29

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: part 1 v3

Ok - I couldn't figure out how to see if the size had been set by chrome or content, but it also seems like we're not comfortable with removing the default size right now. I think I've got an okay compromise here - let me know what you think.

Comment 30

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 689018 [details] [diff] [review]
part 1 v3

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

r=me but I'd like John to still give it a look over.

Comment 31

[John Schoenick [:johns]]

Comment on attachment 689018 [details] [diff] [review]
part 1 v3

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

This looks good to me. The 240/200 thing is painful, but we should open a followup to deal with that on trunk.

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

This was the try run:
https://tbpl.mozilla.org/?tree=Try&rev=53c551726c2e

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa058c4350c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c574f7f68309

Comment 33

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/fa058c4350c4
https://hg.mozilla.org/mozilla-central/rev/c574f7f68309

Comment 34

[Paul Silaghi, QA [:pauly]]

Should this bug fix bug 809846 too ? Pandora is N/A outside of the US.

Comment 35

[Jared Wein [:jaws] (please needinfo? me)]

This should be backed out until a better approach is available. The popup is showing up on too many sites and is annoying the people who are helping to test click-to-play functionality.

Comment 36

[alex_mayorga]

Also the "door hanger" it's missing an X on the top right corner to easily dismiss it.

Comment 37

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Jared Wein [:jaws] from comment #35)
> This should be backed out until a better approach is available. The popup is
> showing up on too many sites and is annoying the people who are helping to
> test click-to-play functionality.

I agree Jared comment. The showing popup many times is very annoying.

For example, Chromium/IE/Opera, they do not show a striking information about  hidden plugin elements.
I think Firefox also need not to show its information. The doorhanger icon says that this page has some plugin elements regardless of whether showing or hidden. This provide enough information.

Comment 38

[Benjamin Smedberg]

jaws, could you be more specific? This was implemented as a result of feedback about CTP not being visible enough in the common case. I don't understand what information has changed which would make this undesirable. Note that we are focusing primarily on the automatic CTP for out of date plugins. Perhaps the behavior should be different for people who have specifically opted in to CTP, but that is not the current focus.

Comment 39

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #38)
> jaws, could you be more specific? This was implemented as a result of
> feedback about CTP not being visible enough in the common case. I don't
> understand what information has changed which would make this undesirable.
> Note that we are focusing primarily on the automatic CTP for out of date
> plugins. Perhaps the behavior should be different for people who have
> specifically opted in to CTP, but that is not the current focus.

Sorry. I'm not jaws. But may I comment about your questions?


Not need to backout all patches, only backout the part of showing popup. I think it is not difficult.

Automatic CTP for out of date plugins is emergency purpose. So it need to be outstanding because it urge user to update or disable plugins.

However, the purpose of normal CTP is stop to play plugins automatically. This also relaxes plugins vulnerability. But the main assumption is stopping automatically for usability. It need not to be outstanding extensively.

Comment 40

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #38)
> jaws, could you be more specific? This was implemented as a result of
> feedback about CTP not being visible enough in the common case. I don't
> understand what information has changed which would make this undesirable.
> Note that we are focusing primarily on the automatic CTP for out of date
> plugins. Perhaps the behavior should be different for people who have
> specifically opted in to CTP, but that is not the current focus.

Our blocking is good in that it blocks the loading of the out of date plugins. If a user wishes to ignore this and not update their plugin, they are secure as long as they don't load the plugin.

There is currently no way to say "stop messaging me, I can't/won't update the plugin during this session/day/week".

I agree that opt-in CTP is not the main motivation and so we shouldn't hurt the plugin blocklisting approach for opt-in CTP's benefit, but it would be nice if we backed off of the messaging when users don't take action the first couple of times.

This could be mitigated by only opening this doorhanger once per session, so users can learn where to look on other pages but aren't bothered if it isn't necessary.

Comment 41

[Alex Keybl [:akeybl]]

(In reply to Jared Wein [:jaws] from comment #35)
> This should be backed out until a better approach is available. The popup is
> showing up on too many sites and is annoying the people who are helping to
> test click-to-play functionality.

Where are you seeing this feedback? Are there really that many sites with invisible or hidden Flash plugins? Can we get a list of sites users are reporting so that we can test?

(In reply to Jared Wein [:jaws] from comment #40)
> This could be mitigated by only opening this doorhanger once per session, so
> users can learn where to look on other pages but aren't bothered if it isn't
> necessary.

This may do the trick as a stop gap (if user feedback is truly negative), but should really be combined with a more noticeable animation on the plugin icon on page load for discoverability . Maybe that specific followup won't make Firefox 18.

Comment 42

[Alex Keybl [:akeybl]]

In my own testing, I now understand the annoyance. We're planning to address everybody's concerns in bug 819992.

Comment 43

[Alex Keybl [:akeybl]]

We hope to land this bug along with bug 819992 and bug 820109 for tomorrow's beta (if at all possible).

Comment 44

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: part 1 (branches)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play
User impact if declined: this is needed for click-to-play
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 45

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 687287 [details] [diff] [review]
Part 0.5 - Add script access event for CTP'd plugins

[Approval Request Comment]
Bug caused by (feature/regressing bug #): click-to-play
User impact if declined: needed for click-to-play
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none (well, technically this patch does change an id, but the next patch reverts it)

Comment 46

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 687287 [details] [diff] [review]
Part 0.5 - Add script access event for CTP'd plugins

Meant to include this in the last change. Sorry for the bugspam.

Comment 47

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 687313 [details] [diff] [review]
Part 0.5b [for branches only] - Avoid changing nsIObjectLoadingContent interface

[Approval Request Comment]
(same as above)

Caveat: this only applies to aurora - I'll upload a beta patch next.

Comment 48

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: part 0.5b (for beta)

[Approval Request Comment]
(same as above)

Comment 49

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 690577 [details] [diff] [review]
part 1 (branches)

approving for immediate uplift so this is in tomorrow's beta.

Comment 50

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 687313 [details] [diff] [review]
Part 0.5b [for branches only] - Avoid changing nsIObjectLoadingContent interface

approving these with UUID changes since they are reverted back to original.

Comment 51

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Pushed to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/47681da6d0b2
https://hg.mozilla.org/releases/mozilla-aurora/rev/17a8ba91f080
https://hg.mozilla.org/releases/mozilla-aurora/rev/8cad916d8407

Pushed to beta:

https://hg.mozilla.org/releases/mozilla-beta/rev/ef33b42d4b83
https://hg.mozilla.org/releases/mozilla-beta/rev/a51806efed9a
https://hg.mozilla.org/releases/mozilla-beta/rev/7a5a4297cb48

Comment 52

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

To get this to work on ESR17, it looks like we'll need patch "Part 2" from bug 795275 (modified slightly to change some bools to JSBools). Alex - should I go ahead with that, find another way, or not uplift this to ESR?

Comment 53

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Shoot. I didn't mean to un-set that flag.

Comment 54

[Bobby Holley (:bholley)]

A heads up - I just discovered in bug 820666 that IsCallerXBL has a bug, and returns false for certain XBL scripts. I'm not sure what the deal is with this bug, but I noticed that IsCallerXBL got hoisted to beta on for this, so you should consider uplifting bug 820666 as well. Previously IsCallerXBL wasn't being used for anything significant.

Comment 55

[Jim Mathies [:jimm]]

(In reply to Alex Keybl [:akeybl] from comment #41)
> (In reply to Jared Wein [:jaws] from comment #35)
> > This should be backed out until a better approach is available. The popup is
> > showing up on too many sites and is annoying the people who are helping to
> > test click-to-play functionality.
> 
> Where are you seeing this feedback? Are there really that many sites with
> invisible or hidden Flash plugins? Can we get a list of sites users are
> reporting so that we can test?

I'm seeing this on numerous news sites. Which I don't mind so much except when I made the mistake of choosing "never activate" from the drop down on one, video on the same site stopped working, and the "click to activate" option stopped displaying.

Is there some way to edit the blacklist to undo "never activate" choices?

Comment 56

[John Schoenick [:johns]]

(In reply to Jim Mathies [:jimm] from comment #55)
> I'm seeing this on numerous news sites. Which I don't mind so much except
> when I made the mistake of choosing "never activate" from the drop down on
> one, video on the same site stopped working, and the "click to activate"
> option stopped displaying.

Bug 819992 should improve this behavior

> Is there some way to edit the blacklist to undo "never activate" choices?

Right click -> View Page Info -> Permissions; Alternately: about:permissions
(our UI for per-site permissions could stand to be more discoverable)

Comment 57

[Jim Mathies [:jimm]]

(In reply to John Schoenick [:johns] from comment #56)
> (In reply to Jim Mathies [:jimm] from comment #55)
> > I'm seeing this on numerous news sites. Which I don't mind so much except
> > when I made the mistake of choosing "never activate" from the drop down on
> > one, video on the same site stopped working, and the "click to activate"
> > option stopped displaying.
> 
> Bug 819992 should improve this behavior
> 
> > Is there some way to edit the blacklist to undo "never activate" choices?
> 
> Right click -> View Page Info -> Permissions; Alternately: about:permissions
> (our UI for per-site permissions could stand to be more discoverable)

Hmm, maybe this is a bug:

In Page Info -> Permissions I see under "Activate Plugins" a checked "Always ask" checkbox and greyed out radio buttons for Allow/Block, where Block is selected. 

If I uncheck "Always Ask", I can then change the radios to "Allow". If I then recheck "Always Ask", the radios flip back to blocked. So it seems the only option I have is to set the site to "Allow", which isn't exactly what I want.

Comment 58

[John Schoenick [:johns]]

(In reply to Jim Mathies [:jimm] from comment #57)
> If I uncheck "Always Ask", I can then change the radios to "Allow". If I
> then recheck "Always Ask", the radios flip back to blocked. So it seems the
> only option I have is to set the site to "Allow", which isn't exactly what I
> want.

I'm not sure I follow - if you want it to always prompt you (the default), you leave always ask checked -- what the (now greyed out) boxes show is irrelevant (which is not a great UI, but that's another bug). If you want it to simply always block or always allow, you uncheck always ask and select the relevant option.

Comment 59

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to John Schoenick [:johns] from comment #58)
> (In reply to Jim Mathies [:jimm] from comment #57)
> > If I uncheck "Always Ask", I can then change the radios to "Allow". If I
> > then recheck "Always Ask", the radios flip back to blocked. So it seems the
> > only option I have is to set the site to "Allow", which isn't exactly what I
> > want.
> 
> I'm not sure I follow - if you want it to always prompt you (the default),
> you leave always ask checked -- what the (now greyed out) boxes show is
> irrelevant (which is not a great UI, but that's another bug). If you want it
> to simply always block or always allow, you uncheck always ask and select
> the relevant option.

I think the bug is that this ui wasn't updated after click-to-play plugin permissions were changed in bug 746374. It'll take a fair bit of work to get it to a usable state, so maybe we should disable it for now? Even as it is, I think the only way to really undo the site permissions is to use "clear recent history" and remove site preferences.

Comment 60

[John Schoenick [:johns]]

Bug 775857 exists for this issue

Comment 61

[Bobby Holley (:bholley)]

Attachment: Tag XBL script for <field> elements and child scripts. v1

Attaching a patch. Hopefully Waldo can tell me if there's any other way XBL code
can propagate to new scripts where we miss propagating userBit. Note that I
explictly _don't_ want to do it for Eval. ;-)

Comment 62

[Bobby Holley (:bholley)]

Comment on attachment 691518 [details] [diff] [review]
Tag XBL script for <field> elements and child scripts. v1

Wrong bug, sorry.

Comment 63

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Do we still want to land this (and bug 819992) on esr17?

Comment 64

[Georg Fritzsche [:gfritzsche]]

(In reply to David Keeler from comment #29)
> Created attachment 689018 [details] [diff] [review]
> part 1 v3
> 
> Ok - I couldn't figure out how to see if the size had been set by chrome or
> content, but it also seems like we're not comfortable with removing the
> default size right now. I think I've got an okay compromise here - let me
> know what you think.

So what i am seeing now on trunk is plugins without a size-specification being 0x0 unless they get a plugin-problem overlay/styling, in which they get the previously default size (240x200). 
Was that the intent? A very quick look over the patchs comments suggests not, but i'm not entirely sure.

Comment 65

[Benjamin Smedberg]

both <embed> and <object> without a size specification should default to a visible size; if 240x200 was the previous default we should maintain that.

And we should write a test for that ;-)

Comment 66

[Alex Keybl [:akeybl]]

(In reply to David Keeler from comment #52)
> Alex -
> should I go ahead with that, find another way, or not uplift this to ESR?

If we think it's fairly low risk, let's take patch 2 from bug 795275, and also bug 819992. We'll want to get specific qa testing around this on ESR17.

Comment 67

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I had to fold part 0.5 with part 0.5b to get past the commit hooks, but this should do it:

https://hg.mozilla.org/releases/mozilla-esr17/rev/60e0608c2c2a
https://hg.mozilla.org/releases/mozilla-esr17/rev/8c019db3aa17
https://hg.mozilla.org/releases/mozilla-esr17/rev/5b245c6f46fb

Comment 68

[u279076]

Assigning myself as QA contact for verification. I will coordinate some testing around websites like Pandora with 18RC and 17.0.2esr to make sure nothing regressed.

Comment 69

[Paul Silaghi, QA [:pauly]]

I've tested several music sites on FF 18 RC, 17.0.2 ESR with plugins.click_to_play=TRUE:

http://www.pandora.com - N/A outside of the US
http://www.jango.com/ - bug 809846
http://www.jukeboxalive.com/ - bug 809867
http://grooveshark.com/ - PASS
http://www.listube.com/ - PASS
http://www.audiolizer.com/ - PASS
http://www.live365.com/new/index.live - the CTP doorhanger disappears after 2 seconds (possible dupe of 820448)
http://gaana.com/ - clicking on a song to play and then activate flash from the CTP doorhanger will not play the song
http://www.radiosparx.com/ - clicking on a radio station (e.g. Pop Mix) without enabling flash from the initial popup notification, makes the CTP doorhanger to disappear

Please take a look, I'll file separate bugs if necessary.

Comment 70

[u279076]

I think we can call this verified fixed based on your testing Paul.

David, can you please advise if Paul should file follow-up bugs for any of his issues?

Comment 71

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I've only been filing bugs on sites that should work with how click-to-play works. As an example (sorry it's US-only), Pandora polls for the plugin, so if it doesn't immediately load, it tries to wait for it. This should work with how we've implemented click-to-play, because the user will see the popup, activate flash, and Pandora will continue. Other sites don't wait for the plugin to load and often exit with a Javascript error. These sites will not work with click-to-play. At most, they would be evangelism bugs.

Paul, if you've got time, you might take a look at those sites and see if they're doing something that should work with ctp but doesn't or if they will never work with ctp regardless of what we do. Thanks!

Comment 72

[blogi ram]

Yes I've tested several music sites and all passed
ex: https://bhojpuri-mp3.in
https://djsongi.com

Comment 73

[wiredusa]

(In reply to Paul Silaghi, QA [:pauly] from comment #69)

I've tested several music sites on FF 18 RC, 17.0.2 ESR with
plugins.click_to_play=TRUE:

http://www.pandora.com - N/A outside of the US
http://www.jango.com/ - bug 809846
http://www.jukeboxalive.com/ - bug 809867
http://grooveshark.com/ - PASS
http://www.listube.com/ - PASS
http://www.audiolizer.com/ - PASS
http://www.live365.com/new/index.live - the CTP doorhanger disappears after
2 seconds (possible dupe of 820448)
http://gaana.com/ - clicking on a song to play and then activate flash from
the CTP doorhanger will not play the song
http://www.radiosparx.com/ - clicking on a radio station (e.g. Pop Mix)
without enabling flash from the initial popup notification, makes the CTP
doorhanger to disappear
If the problem still presist then you can also download songs directly from website like. https://downloadming2.com/ and https://musicbadshah.org/
Please take a look, I'll file separate bugs if necessary.

Comment 74

[Mr. Alam]

I've tested several music sites on ESR with plugins.click_to_play=TRUE
https://ganafm.in/ - PASS
https://soundcloud.com/ - PASS
http://odiagit.com/ - PASS
https://gaana.com/ - PASS
http://odiagan.net/ - PASS