Closed Bug 1192927 Opened 9 years ago Closed 6 years ago

Remove plugins specific code and initialize the flash plugin in Page Info like other permissions

Categories

(Firefox :: Page Info Window, defect, P5)

Product:
Firefox
Component:
Page Info Window
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: alfredkayser, Assigned: prathiksha)

References

Details

Attachments

(3 files)

Screenshot of the issue
64.54 KB, image/png
Details
Mockup of how it could look: much simpler, easier to oversee
53.90 KB, image/png
Details
Bug 1192927 - Remove plugins specific code and initialize the flash plugin row in Page Info and support flash in Site Identity.
59 bytes, text/x-review-board-request
Felipe
: review+
johannh
: review+
Details
Attached image Screenshot of the issue — 
See attached screenshot.
For Activate Plugins the "Use Default" option is one of the radiobox options,
but for other permissions the "Use Default" option is a checkbox, enabling/disabling the radiobox.

I propose, in terms of consistency and simplicity to make all permissions the same, with "Use default" (lowercase d) as one of the radiobox options.
Priority: -- → P5
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Well ok then.  Good luck and thanks!
I think we should use this bug to track removing plugins specific code and initializing Flash like the other permissions in Page Info since we only support Flash right now and not other plugins.
Summary: Consistent "Use Default" in Permissions tab of Page/Frame Info → Remove plugins specific code and initialize the flash plugin in Page Info like other permissions
What this patch includes:

- Support for Flash permission in the Site Identity panel. All custom states for Flash are indicated in the panel except the “Hide” state. (read more about “Hide”: https://bugzilla.mozilla.org/show_bug.cgi?id=1427779#c0)

- A plugin-blocked icon is used to indicate “Blocked” state in the Site Identity panel and in the identity-box. When Flash is not “Blocked” (i.e “Allowed”, “Always Ask” or “Hide”), we show a blue/gray icon in the URL bar that indicates whether Flash is active/inactive on that page. This patch does not include any changes related to the blue/gray icon.

- Plugin specific code was removed from pageInfo.xul and /browser/base/content/pageinfo/permissions.js.

- Changed the number used to identify PLUGIN_PERMISSION_PROMPT_ACTION_QUIET(the “Hide” state) from 8 to 12 because SitePermissions.ALLOW_COOKIES_FOR_SESSION (or https://searchfox.org/mozilla-central/source/netwerk/cookie/nsICookiePermission.idl#33) is already given the value 8. Since “Allow for Session” and “Hide” are two different states and we need both states in Page Info, PLUGIN_PERMISSION_PROMPT_ACTION_QUIET (or SitePermissions.PROMT_HIDE) was newly set to 12.


What this patch does not include/ what I think still needs to be fixed:

- Flash in Page Info is not in sync with about:addons#plugins. Is  about:addons#plugins going to be removed or replaced anytime soon?

- The blue/gray plugin icon that indicates whether Flash is active/inactive on a page requires a refresh to show the correct state of the icon on Flash permission state change. For example, if we set “Block” through Page Info, the blue/gray icon will only disappear on refresh.
Comment on attachment 8952206 [details]
Bug 1192927 - Remove plugins specific code and initialize the flash plugin row in Page Info and support flash in Site Identity.

https://reviewboard.mozilla.org/r/221454/#review227798

The frontend parts look mostly good to me (see comments below). This is mostly not an r+ because I'd like to be able to review the next iterations addressing the PROMPT_ACTION_QUIET problem. I have to defer to Felipe for some of the more in-depth plugin questions, though :/

> - Flash in Page Info is not in sync with about:addons#plugins. Is  about:addons#plugins going to be removed or replaced anytime soon?

Sounds like a follow-up bug to me.

> - The blue/gray plugin icon that indicates whether Flash is active/inactive on a page requires a refresh to show the correct state of the icon on Flash permission state change. For example, if we set “Block” through Page Info, the blue/gray icon will only disappear on refresh.

That too, in fact.

Thank you!

::: browser/base/content/browser.js:8002
(Diff revision 1)
>      container.setAttribute("align", "center");
>  
>      let img = document.createElement("image");
> -    let classes = "identity-popup-permission-icon " + aPermission.id + "-icon";
> +    let classes;
> +    if (aPermission.id == "plugin:flash") {
> +      classes = "plugin-icon";

I think it would be better to add the identity-popup-permission-icon class here, too, so that this icon gets all the changes we make to the other icons.

At this point it might be sensible to make this all use img.classList, too.

::: browser/base/content/pageinfo/permissions.js:25
(Diff revision 1)
>  
>  var permissionObserver = {
>    observe(aSubject, aTopic, aData) {
>      if (aTopic == "perm-changed") {
>        var permission = aSubject.QueryInterface(Components.interfaces.nsIPermission);
> -      if (permission.matchesURI(gPermURI, true)) {
> +      if (permission.matchesURI(gPermURI, true) && gPermissions.indexOf(permission.type) > -1) {

Nit: While touching this code you could modernize it to gPermissions.includes(permission.type)

::: browser/locales/en-US/chrome/browser/browser.dtd:236
(Diff revision 1)
>  <!ENTITY urlbar.geolocationBlocked.tooltip       "You have blocked location information for this website.">
>  <!ENTITY urlbar.webNotificationsBlocked.tooltip  "You have blocked notifications for this website.">
>  <!ENTITY urlbar.persistentStorageBlocked.tooltip "You have blocked persistent storage for this website.">
>  <!ENTITY urlbar.popupBlocked.tooltip             "You have blocked pop-ups for this website.">
>  <!ENTITY urlbar.canvasBlocked.tooltip            "You have blocked canvas data extraction for this website.">
> +<!ENTITY urlbar.flashPluginBlocked.tooltip       "You have blocked this website from using the flash plugin.">

Capitalize Flash and it might be Adobe Flash...

::: browser/locales/en-US/chrome/browser/browser.properties:339
(Diff revision 1)
>  pluginActivateDisabled.message=“%S” is disabled.
>  pluginActivateDisabled.label=Disabled
>  pluginActivateDisabled.manage=Manage plugins…
>  pluginEnabled.message=“%S” is enabled on %S.
>  pluginEnabledOutdated.message=Outdated plugin “%S” is enabled on %S.
>  pluginEnabledVulnerable.message=Insecure plugin “%S” is enabled on %S.

Huh, a quick searchfox search tells me that none of the strings from line 331 to 340 are still in use. Maybe Felipe can verify that we can remove them?

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:28
(Diff revision 1)
>  # Used to label permission state checkboxes in the page info dialog.
>  state.multichoice.alwaysAsk = Always Ask
>  state.multichoice.allow = Allow
>  state.multichoice.allowForSession = Allow for Session
>  state.multichoice.block = Block
> +state.multichoice.hide = Hide

Please add this to the localization note above.

::: browser/locales/en-US/chrome/browser/sitePermissions.properties:43
(Diff revision 1)
>  permission.geo.label = Access Your Location
>  permission.shortcuts.label = Override Keyboard Shortcuts
>  permission.focus-tab-by-prompt.label = Switch to this Tab
>  permission.persistent-storage.label = Store Data in Persistent Storage
>  permission.canvas.label = Extract Canvas Data
> +permission.flash-plugin.label = Activate Flash plugin

I believe for legal reasons we say "Adobe Flash", but Felipe will probably be able to verify.

::: browser/modules/SitePermissions.jsm:146
(Diff revision 1)
>    UNKNOWN: Services.perms.UNKNOWN_ACTION,
>    ALLOW: Services.perms.ALLOW_ACTION,
>    BLOCK: Services.perms.DENY_ACTION,
>    PROMPT: Services.perms.PROMPT_ACTION,
>    ALLOW_COOKIES_FOR_SESSION: Components.interfaces.nsICookiePermission.ACCESS_SESSION,
> +  PROMPT_HIDE: Components.interfaces.nsIObjectLoadingContent.PLUGIN_PERMISSION_PROMPT_ACTION_QUIET,

Maybe call this PLUGIN_PROMPT_HIDE and throw a similar error to https://searchfox.org/mozilla-central/rev/9a8d3f8191b15cdca89a7ce044c7bea2dd0462dc/browser/modules/SitePermissions.jsm#403

::: browser/modules/SitePermissions.jsm:542
(Diff revision 1)
>     *        The scope to get the label for.
>     *
>     * @return {String|null} the localized label or null if an
>     *         unknown state was passed.
>     */
>    getCurrentStateLabel(state, scope = null) {

Don't you need to handle PROMPT_HIDE here as well?

::: dom/base/nsIObjectLoadingContent.idl:77
(Diff revision 1)
>    // Plugins-specific permission indicating that we want to prompt the user
>    // to decide whether they want to allow a plugin, but to do so in a less
>    // intrusive way than PROMPT_ACTION would entail. At the time of writing,
>    // this means hiding all in-content plugin overlays, but still showing the
>    // plugin badge in the URL bar.
> -  const unsigned long PLUGIN_PERMISSION_PROMPT_ACTION_QUIET = 8;
> +  const unsigned long PLUGIN_PERMISSION_PROMPT_ACTION_QUIET = 12;

I'll defer to Felipe for finding a good solution to this problem :/
Attachment #8952206 - Flags: review?(jhofmann)
Comment on attachment 8952206 [details]
Bug 1192927 - Remove plugins specific code and initialize the flash plugin row in Page Info and support flash in Site Identity.

clearing up review as I'll wait for the updated patch. We talked on IRC about not changing the const value, and just not having the "Hide" option in Page Info, which was what caused the problem.
"Hide" is more similar in behavior to "Always Ask", so the goal is that it will show as "Always Ask" in Page Info, and correctly show as Hide in the Permission Panel.

As prathiksha mentioned, it's hard to understand the meaning for "Hide".. Thinking more about it, how about we instead use "Don't Ask" for the string?

Oh and yeah, just to confirm johannh's point, we should call the plugin "Adobe Flash" everywhere.
Attachment #8952206 - Flags: review?(felipc)
Comment on attachment 8952206 [details]
Bug 1192927 - Remove plugins specific code and initialize the flash plugin row in Page Info and support flash in Site Identity.

https://reviewboard.mozilla.org/r/221454/#review229546

This looks good to me as long as Felipe is fine with the behavior of the plugin-specific permission.

Are you sure that there are no other tests that need to be modified?

Thanks!

::: browser/modules/SitePermissions.jsm:542
(Diff revision 2)
> +   *        The permission to get the state label for.
>     *
>     * @return {String|null} the localized label or null if an
>     *         unknown state was passed.
>     */
> -  getCurrentStateLabel(state, scope = null) {
> +  getCurrentStateLabel(state, scope = null, id) {

if you're not going to make id optional please move it in front of scope. Optional arguments should always be last.

::: browser/modules/SitePermissions.jsm:661
(Diff revision 2)
>    },
>  
>    "canvas": {
>    },
> +
> +  "plugin:flash": {

You will have to add this permission to the tests in https://searchfox.org/mozilla-central/source/browser/modules/test/unit/test_SitePermissions.js
Attachment #8952206 - Flags: review?(jhofmann) → review+
Comment on attachment 8952206 [details]
Bug 1192927 - Remove plugins specific code and initialize the flash plugin row in Page Info and support flash in Site Identity.

https://reviewboard.mozilla.org/r/221454/#review228760

Thanks a lot for this!

::: browser/modules/SitePermissions.jsm:663
(Diff revision 2)
>    "canvas": {
>    },
> +
> +  "plugin:flash": {
> +    labelID: "flash-plugin",
> +    states: [ SitePermissions.UNKNOWN, SitePermissions.ALLOW, SitePermissions.BLOCK ],

add a comment here explaining why the HIDE state is not here
Attachment #8952206 - Flags: review?(felipc) → review+
Keywords: checkin-needed
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8e5b1525be69
Remove plugins specific code and initialize the flash plugin row in Page Info and support flash in Site Identity. r=Felipe,johannh
Keywords: checkin-needed
Backout by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f7fa97d431b
Backed out changeset 8e5b1525be69 or failing testing\firefox-ui\tests\functional\security\test_no_certificate.py TestNoCertificate.test_no_certificate on a CLOSED TREE
Backed out for failing testing\firefox-ui\tests\functional\security\test_no_certificate.py TestNoCertificate.test_no_certificate 

Push that started the failures:  https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8e5b1525be692c82dec8ed6e1573c4afe6cf6af4

Recent failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=165492141&repo=autoland&lineNumber=40127

Backout: https://hg.mozilla.org/integration/autoland/rev/5f7fa97d431be663aedbceca2e83f4839f3f5f05
Flags: needinfo?(prathikshaprasadsuman)
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
(In reply to Johann Hofmann [:johannh] from comment #21)
> I don't think you're fixing the right test here. This file is failing:
> https://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/
> functional/security/test_no_certificate.py

Ok, I'm stupid, my bad, it looks like you were fixing the right thing. I'll land this. Apologies.
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2d3065417d04
Remove plugins specific code and initialize the flash plugin row in Page Info and support flash in Site Identity. r=Felipe,johannh
https://hg.mozilla.org/mozilla-central/rev/2d3065417d04
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1450808
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: