Closed Bug 1398972 Opened 7 years ago Closed 7 years ago

Update Flash permission doorhanger to use PopupNotifications.jsm without a custom XBL binding

Categories

(Core Graveyard :: Plug-ins, enhancement, P2)

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

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: alexical, Assigned: alexical)

References

Details

Attachments

(5 files)

Bug 1398972 - Plugin doorhanger test cleanup
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1398972 - Update Histograms.json
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1398972 - Replace usage of plugin doorhanger XBL binding
59 bytes, text/x-review-board-request
Felipe
: review+
Details
Bug 1398972 - Remove plugin doorhanger XBL
59 bytes, text/x-review-board-request
Felipe
: review+
Details
flashblink.mov
754.68 KB, video/quicktime
Details 
+++ This bug was initially created as a clone of Bug #1392979 +++

Flash CTP:
> Do you want to allow Flash to run on this site? Only allow Flash on sites you trust.
> 
> [ ] Remember this decision
> 
> [Don’t Allow]   [Allow Flash]


Flash outdated:

> Do you want to allow an outdated version of Flash to run on this site?
> 
> An outdated version can affect browser performance and security.
> 
> [Update Adobe Flash…] <-- this is a link
> 
> 
> [ ] Remember this decision
> 
> [Don’t Allow] [Allow]
Summary: Update Flash permission doorhanger to match the rest of our permission UI → Update Flash permission doorhanger to use PopupNotifications.jsm without a custom XBL binding
Bram, I'm transferring the needinfo from the previous bug[1] to this bug, since the response should probably be in here.

[1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1392979#c21
Flags: needinfo?(bram)
Doug, sorry that I missed your cancelling my NI on the previous bug and re-asking it here.

My reply:
https://bugzilla.mozilla.org/show_bug.cgi?id=1392979#c27

Summary:
When there’s not enough space, I proposed to show only the icon sans text. If this isn’t not possible or a simpler solution exists, go with the approach you’ve proposed (empty, invisible area).
Flags: needinfo?(bram)
Priority: -- → P2
Comment on attachment 8919834 [details]
Bug 1398972 - Plugin doorhanger test cleanup

https://reviewboard.mozilla.org/r/189970/#review196730

I'd highly recommend splitting up this patch to make is easier to digest.

::: browser/components/preferences/sitePermissions.js:120
(Diff revision 1)
> +    case Services.perms.PROMPT_ACTION_QUIET:
> +      stringKey = "prompt_quiet"
> +      break;
> +    case Services.perms.PROMPT_ACTION_QUIET:
> +      stringKey = "prompt_quiet"
> +      break;

What is this for? Why three times? I doesn't seem like you added the string to https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/preferences/preferences.properties#133

::: dom/indexedDB/PermissionRequestBase.h:39
(Diff revision 1)
>  public:
>    enum PermissionValue {
>      kPermissionAllowed = nsIPermissionManager::ALLOW_ACTION,
>      kPermissionDenied = nsIPermissionManager::DENY_ACTION,
> -    kPermissionPrompt = nsIPermissionManager::PROMPT_ACTION
> +    kPermissionPrompt = nsIPermissionManager::PROMPT_ACTION,
> +    kPermissionPromptQuiet = nsIPermissionManager::PROMPT_ACTION_QUIET,

Why are you adding this to indexedDB? Is there any chance that this value will ever be set in indexedDB? You're not exposing it anywhere.

::: netwerk/base/nsIPermissionManager.idl:63
(Diff revision 1)
>     */
>    const uint32_t UNKNOWN_ACTION = 0;
>    const uint32_t ALLOW_ACTION = 1;
>    const uint32_t DENY_ACTION = 2;
>    const uint32_t PROMPT_ACTION = 3;
> +  const uint32_t PROMPT_ACTION_QUIET = 4;

What is PROMPT_ACTION_QUIET? Are you sure that you want to add it to the permission manager? The permission manager is used by a lot of other components, does any of them have a valid use case for PROMPT_ACTION_QUEIT?

Otherwise there is a lot of precedent for storing module-specific integer constants in the permission database.
(In reply to Johann Hofmann [:johannh] from comment #5)
> What is PROMPT_ACTION_QUIET? Are you sure that you want to add it to the
> permission manager? The permission manager is used by a lot of other
> components, does any of them have a valid use case for PROMPT_ACTION_QUEIT?
> 
> Otherwise there is a lot of precedent for storing module-specific integer
> constants in the permission database.

Ah - thanks for the heads up on that. I couldn't find any precedent for that at first. That simplifies things a lot.
Comment on attachment 8920718 [details]
Bug 1398972 - Replace usage of plugin doorhanger XBL binding

https://reviewboard.mozilla.org/r/191722/#review198656

Great work! So nice to see this cleanup. From a code point of view it's great, I just have some questions about the behavior/design that I'd like to see clarified:

- Let's check the flashActivate.message string with Bram again. There's a followup goal from bug 1409148 to remove the string "Plugins may slow down Firefox" from the overlay, counting on the fact that this information is currently shown on the doorhanger message.

- Clicking on Don't allow with the checkbox unchecked results on no action, right? This is just keeping the previous behavior, I know. But now with the state Quiet, I think clicking there should put the plugins on the page in a quiet state too (just not remember it for next time)

- I built the patch and I've been testing it, and choosing `Don't allow` + the remember checkbox, it appears that it takes two clicks for this to work. The first time it just hides the doorhanger, and I have to open it again and re-do these steps for this to take effect

- I believe that the page info UI will need to be updated for this new state.. could be in a follow-up

- I'm still unsure about the state Quiet vs. just using Denied.. Ideally we could just use denied, but I don't know if that would break more sites.. If it doesn't, maybe we could change the design spec to not require the icon in the url bar in that case, although it's certainly nice to have that option..  I'll keep thinking about it and see if there's a hack-ish way around the issue.

::: browser/locales/en-US/chrome/browser/browser.properties:345
(Diff revision 1)
>  pluginBlockNow.accesskey=B
>  pluginContinue.label=Continue Allowing
>  pluginContinue.accesskey=C
>  
> +# Flash activation doorhanger UI
> +flashActivate.message=Do you want to allow Flash to run on this site? Only allow Flash on sites you trust.



::: dom/base/nsIObjectLoadingContent.idl:62
(Diff revision 1)
>    // Blocked by content policy
>    const unsigned long PLUGIN_USER_DISABLED        = 7;
>    /// ** All values >= PLUGIN_CLICK_TO_PLAY are plugin placeholder types that
>    ///    would be replaced by a real plugin if activated (playPlugin())
>    /// ** Furthermore, values >= PLUGIN_CLICK_TO_PLAY and
>    ///    <= PLUGIN_VULNERABLE_NO_UPDATE are click-to-play types.

nit: update this comment

::: dom/base/nsObjectLoadingContent.h:97
(Diff revision 1)
>        // Blocked by content policy
>        eFallbackUserDisabled = nsIObjectLoadingContent::PLUGIN_USER_DISABLED,
>        /// ** All values >= eFallbackClickToPlay are plugin placeholder types
>        ///    that would be replaced by a real plugin if activated (PlayPlugin())
>        /// ** Furthermore, values >= eFallbackClickToPlay and
>        ///    <= eFallbackVulnerableNoUpdate are click-to-play types.

and this too
Attachment #8920718 - Flags: review?(felipc)
Comment on attachment 8919835 [details]
Bug 1398972 - Update Histograms.json

https://reviewboard.mozilla.org/r/189972/#review198702

Note: I'm not totally sure on this, but it's my understanding that you can't change the parameters of a histogram after it has been published already, because that will mess the previous data..
Attachment #8919835 - Flags: review-
Comment on attachment 8920719 [details]
Bug 1398972 - Remove plugin doorhanger XBL

https://reviewboard.mozilla.org/r/191724/#review198704
Attachment #8920719 - Flags: review?(felipc) → review+
(In reply to :Felipe Gomes (needinfo me!) from comment #15)
> - Let's check the flashActivate.message string with Bram again. There's a
> followup goal from bug 1409148 to remove the string "Plugins may slow down
> Firefox" from the overlay, counting on the fact that this information is
> currently shown on the doorhanger message.
> 
> - Clicking on Don't allow with the checkbox unchecked results on no action,
> right? This is just keeping the previous behavior, I know. But now with the
> state Quiet, I think clicking there should put the plugins on the page in a
> quiet state too (just not remember it for next time)

Thoughts on these two points, Bram?

> - I built the patch and I've been testing it, and choosing `Don't allow` +
> the remember checkbox, it appears that it takes two clicks for this to work.
> The first time it just hides the doorhanger, and I have to open it again and
> re-do these steps for this to take effect

On Mac or Windows? I can't reproduce this on Mac. Do you have any more details, or a specific site to try this on?

> - I believe that the page info UI will need to be updated for this new
> state.. could be in a follow-up
> 
> - I'm still unsure about the state Quiet vs. just using Denied.. Ideally we
> could just use denied, but I don't know if that would break more sites.. If
> it doesn't, maybe we could change the design spec to not require the icon in
> the url bar in that case, although it's certainly nice to have that option..
> I'll keep thinking about it and see if there's a hack-ish way around the
> issue.

I think that if we go with denied and thus don't have the icon in the URL bar, it will just be too hard for the user to re-enable Flash, in case they accidentally clicked the wrong button, for instance.
Flags: needinfo?(felipc)
Flags: needinfo?(bram)
Attached video flashblink.mov — 
I'm not sure how, but it might be related to my test page, as I couldn't reproduce it on another one. Take a look at the screencast.

http://felipc.github.io/flashtest/
Flags: needinfo?(felipc)
Comment on attachment 8919835 [details]
Bug 1398972 - Update Histograms.json

https://reviewboard.mozilla.org/r/189972/#review198764

::: commit-message-942c3:5
(Diff revision 3)
> +Bug 1398972 - Update Histograms.json r?liuche
> +
> +Since the user can now block Flash and ask the browser to
> +remember that decision, the histogram that collects user's choices
> +on this has to be updated with a fourth option.

Yeah, Felipe is right that you can't just add onto an existing histogram:

https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html#changing-a-histogram

And from https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html:
"Categorical histograms by default support up to 50 labels, but you can set it higher using the n_values property. If you need to add more labels later, you need to use a new histogram name. The current Telemetry server does not support changing histogram declarations after the histogram has already been released. See Changing a histogram if you need to add more labels later."

Feel free to flag me in a new review.
Attachment #8919835 - Flags: review?(liuche)
(In reply to :Felipe Gomes (needinfo me!) from comment #19)
> Created attachment 8922453 [details]
> flashblink.mov
> 
> I'm not sure how, but it might be related to my test page, as I couldn't
> reproduce it on another one. Take a look at the screencast.
> 
> http://felipc.github.io/flashtest/

There seems to be a race between reloading the plugin and computing the overlay visibility (which consumes the reloaded plugin's fallbackType.) I tagged the plugin object with an override value as a workaround. It's a smidge hacky but it's the best I could come up with. If you have any ideas, though, let me know.
(In reply to Doug Thayer [:dthayer] from comment #18)
> (In reply to :Felipe Gomes (needinfo me!) from comment #15)
> > - Let's check the flashActivate.message string with Bram again. There's a
> > followup goal from bug 1409148 to remove the string "Plugins may slow down
> > Firefox" from the overlay, counting on the fact that this information is
> > currently shown on the doorhanger message.

Yes. Let’s remove the “Plugins may slow down Firefox” from the overlay. This makes our message more concise and responsive to more sizes.

As you have written, the slow down message will still be shown on the doorhanger.


> > - Clicking on Don't allow with the checkbox unchecked results on no action,
> > right? This is just keeping the previous behavior, I know. But now with the
> > state Quiet, I think clicking there should put the plugins on the page in a
> > quiet state too (just not remember it for next time)

Yes. Clicking “Don’t allow” without the checkbox is the equivalent of clicking the “x” on the old doorhanger, or typing “esc”.

However, I didn’t know that there’s a separate quiet state.

What does this state do? The ideal behaviour would be “I am not ready to activate this particular Flash element yet. Ask me when I click on it again.” – but I assume that this quiet state works a bit differently?
Flags: needinfo?(bram)
(In reply to Bram Pitoyo [:bram] from comment #26)
> What does this state do? The ideal behaviour would be “I am not ready to
> activate this particular Flash element yet. Ask me when I click on it
> again.” – but I assume that this quiet state works a bit differently?

The separate quiet state is the one you describe here: https://bugzilla.mozilla.org/show_bug.cgi?id=1392979#c17.

Specifically:

> When the user clicks “Don’t Allow” with “Remember” checked, we should remove
> all the Flash overlays. No more grey boxes.
>
> But we will still show the crossed out plugin icon. Currently, this icon
> has its own doorhanger where you can unblock it. But in the future, it
> should open the Control Center.
Flags: needinfo?(bram)
Comment on attachment 8920718 [details]
Bug 1398972 - Replace usage of plugin doorhanger XBL binding

https://reviewboard.mozilla.org/r/191722/#review199664
Attachment #8920718 - Flags: review?(felipc) → review+
Comment on attachment 8919834 [details]
Bug 1398972 - Plugin doorhanger test cleanup

https://reviewboard.mozilla.org/r/189970/#review199666
Attachment #8919834 - Flags: review?(felipc) → review+
(In reply to Bram Pitoyo [:bram] from comment #26)
> (In reply to Doug Thayer [:dthayer] from comment #18)
> > (In reply to :Felipe Gomes (needinfo me!) from comment #15)
> > > - Let's check the flashActivate.message string with Bram again. There's a
> > > followup goal from bug 1409148 to remove the string "Plugins may slow down
> > > Firefox" from the overlay, counting on the fact that this information is
> > > currently shown on the doorhanger message.
> 
> Yes. Let’s remove the “Plugins may slow down Firefox” from the overlay. This
> makes our message more concise and responsive to more sizes.
> 
> As you have written, the slow down message will still be shown on the
> doorhanger.

Bram, the question here was that this is a new string for the re-styled doorhanger. This new string says:
"Do you want to allow Flash to run on this site? Only allow Flash on sites you trust."

So, this new string is also removing the mention of Flash being slow. If we change both the overlay string (from bug 1409148) and the doorhanger one (from this bug), there will be no more mentions of its slowness.
Comment on attachment 8919835 [details]
Bug 1398972 - Update Histograms.json

https://reviewboard.mozilla.org/r/189972/#review199668

Type 2 data collection - adding a new state to an already existing probe, documentation is in-tree.
Attachment #8919835 - Flags: review?(liuche) → review+
(In reply to Doug Thayer [:dthayer] from comment #27)
> The separate quiet state is the one you describe here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1392979#c17.

Right. In that case, Felipe’s assumption on comment #15 was totally correct.

When you click Don’t Allow and leave the box unchecked, it:
> […] should put the plugins on the page in a
> quiet state too (just not remember it for next time)

In other words, remove all the grey boxes from the page, but show them again the next time the page loads.



(In reply to :Felipe Gomes (needinfo me!) from comment #30)
> This new string says:
> "Do you want to allow Flash to run on this site? Only allow Flash on sites
> you trust."
> 
> So, this new string is also removing the mention of Flash being slow.

I’ll be okay with that, as well.

My original spec called for having no warning at all. It said “Do you want to allow Flash to run on this site?” – a wording that matches the rest of our permission dialogues. The warning was added a little later on.

There was a debate over the exact wording to use. Is it about trusting the site to use non-malicious Flash, or about the fact that Flash causes slowness in most situations when activated?

But at this point in time, as long as there’s a warning – whether it’s about trust or performance – we’re good. The warning itself is what matters.
Flags: needinfo?(bram)
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/91806d4db644
Replace usage of plugin doorhanger XBL binding r=Felipe
https://hg.mozilla.org/integration/autoland/rev/1bbe0eb92a75
Remove plugin doorhanger XBL r=Felipe
https://hg.mozilla.org/integration/autoland/rev/89a32423f471
Plugin doorhanger test cleanup r=Felipe
https://hg.mozilla.org/integration/autoland/rev/ee7541fb487a
Update Histograms.json r=liuche
Depends on: 1413582
Depends on: 1456515
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: