747186 - Port |Bug 711618 - implement basic click to play permission model| to Data Manager

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect)

Description

[Jens Hatlak (:InvisibleSmiley)]

Bug 743312 added click-to-play plugins support to SeaMonkey, and bug 746110 will add it to Preferences UI. What's left to do is the Data Manager part (this bug). The FF equivalent was bug 711618.

I don't know much about the Data Manager implementation, but maybe what needs to be done here is similar to what was done in bug 684746.

Comment 1

[Jens Hatlak (:InvisibleSmiley)]

Actually, bug 711618 consisted of two parts. Filed bug 747519 for the non-DM part.

AFAICS, the DM already supports permission management for "plugins". Maybe all we need to do here is some l10n additions? [Maybe also for Page Info as a follow-up?]

Comment 2

[[e-mail]]

(In reply to Jens Hatlak from comment #1)
> AFAICS, the DM already supports permission management for "plugins". Maybe
> all we need to do here is some l10n additions?
You should add it to the list of new permissions to add. You might also want to add code to return the default value of the permission, although it doesn't quite make sense for click-to-play, because when the pref is off, the default is "Allow", but when the pref is on, the default is "Click to play", not "Deny". (Maybe you could return UNKNOWN_ACTION in that case.)

Comment 3

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

I'm really new to DM code so please be gentle. Asking for review from two since I'm not familiar with what's silently agreed (I see bug 684746 was reviewed by Robert, though).

No idea whether what's in the patch is enough or more is needed; I just want to be sure to get CTP done before the uplift (esp. l10n-wise).

There's a slight ambiguity between "Run Plugins" ("object") and "Activate Plugins" ("plugins"). FF only has "Plugins" in about:permissions [1]. If we leave "Run Plugins" as-is, "Plugins" is a bit too general for my taste, though.

[1] I think that relates to CTP but I didn't manage to see an "Allow" there; maybe their implementation is broken.

Comment 4

[[e-mail]]

(In reply to Jens Hatlak from comment #3)
> There's a slight ambiguity between "Run Plugins" ("object") and "Activate
> Plugins" ("plugins").
We could call "object" "Load Plugins" perhaps?

(In reply to [e-mail] from comment #2)
> You should add it to the list of new permissions to add. You might also want
> to add code to return the default value of the permission, although it
> doesn't quite make sense for click-to-play, because when the pref is off,
> the default is "Allow", but when the pref is on, the default is "Click to
> play", not "Deny". (Maybe you could return UNKNOWN_ACTION in that case.)
Actually, when the pref is off, the permission is ignored completely, so it's not so clear what to do in this case...

Comment 5

[[e-mail]]

Comment on attachment 617330 [details] [diff] [review]
patch

I'm happy with this but I think KaiRo should have the final say ;-)

Comment 6

[Robert Kaiser]

Comment on attachment 617330 [details] [diff] [review]
patch

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

If you can land this before the cutover to aurora, that would be great.

::: suite/common/dataman/dataman.js
@@ +1318,5 @@
>        case "password":
>          return Services.perms.ALLOW_ACTION;
> +      case "plugins":
> +        if (Services.prefs.getBoolPref("plugins.click_to_play"))
> +          return Services.perms.UNKNOWN_ACTION;

Have you checked what we are actually doing when we come to this path? I don't think that's a really tested situation, we may need a test for it.
That said, please file a followup bug to actually add tests for this and the other permission types added by Neil recently, we want to have all supported types covered by the test.

Comment 7

[Justin Wood (:Callek)]

Pushed to aurora with a+=KaiRo over IRC.

Comment 8

[Robert Kaiser]

Thanks for pushing.

So, Jens, could you please
1) figure out the .UNKNOWN_ACTION question, either here or in a followup bug (if we need something done here, it should land for 2.11 if possible),
2) file a followup to update tests for this and Neil's recent additions to that list (probably trunk-only but can be done for 2.11 as well),
3) probably file another followup to reduce naming confusion between "object" and "plugins" permissions (this is trunk-only, no L10n changes on aurora or beta)?

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Pushed to central on KaiRo's request on IRC:
http://hg.mozilla.org/comm-central/rev/edf201d1de4b

(In reply to Justin Wood (:Callek) from comment #7)
> Pushed to aurora with a+=KaiRo over IRC.

http://hg.mozilla.org/releases/comm-aurora/rev/95dc7a089da4

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #4)
> (In reply to Jens Hatlak from comment #3)
> > There's a slight ambiguity between "Run Plugins" ("object") and "Activate
> > Plugins" ("plugins").
> We could call "object" "Load Plugins" perhaps?

No idea really. To me it all sounds equally bad by now. Unless someone is willing to dive into the actual implementation, compare actual results of the different content prefs and then improve the strings on top of that, discussing this further might just be a waste of time and/or make things worse.

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

(In reply to [e-mail] from comment #4)
> Actually, when the pref is off, the permission is ignored completely, so
> it's not so clear what to do in this case...

So if it's ignored, for me that equals Allow logically. What's the problem?

[I don't understand the DM. If the pref is off, then I add an Activate Plugins permission, uncheck Use Default and leave Allow, the permission is gone when I switch domains or tabs back and forth. If however I set the permission to Block first and only then to Allow, the permission stays there even after switching domains or tabs. WTF?]

(In reply to Robert Kaiser (:[e-mail]) from comment #6)
> Have you checked what we are actually doing when we come to this path?

What do you mean? It determines the default permission value for a permission that is added using the button and drop-down (in the code, the only caller of getDefault is ~15 line above), i.e. it determines which radio button gets selected. In this case (the code I added), the default value will depend on whether CTP is enabled or not (no radio button selected if yes, Allow selected else).

[BTW: It seems MXR c-c is not up-to-date: I checked in the fix for this bug yesterday but it's still not visible in dataman.js.]

Did I already say I don't really know what I'm doing here?

Comment 12

[Robert Kaiser]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #11)
> [I don't understand the DM. If the pref is off, then I add an Activate
> Plugins permission, uncheck Use Default and leave Allow, the permission is
> gone when I switch domains or tabs back and forth. If however I set the
> permission to Block first and only then to Allow, the permission stays there
> even after switching domains or tabs. WTF?]

I actually think this could be a bug triggered by what I'm explaining below, and exactly why I asked about it.

> (In reply to Robert Kaiser (:[e-mail]) from comment #6)
> > Have you checked what we are actually doing when we come to this path?
> 
> What do you mean? It determines the default permission value for a
> permission that is added using the button and drop-down (in the code, the
> only caller of getDefault is ~15 line above), i.e. it determines which radio
> button gets selected. In this case (the code I added), the default value
> will depend on whether CTP is enabled or not (no radio button selected if
> yes, Allow selected else).

Here's the problem: This function tells us what the radio is set to by default, but you return a value we have no radio for, so you end up with an undefined situation falling back to selecting "something" - and that "something" doesn't reflect reality.
I think we actually need to change this to return the right thing based on the CTP setting so we can select the correct radio item.

Comment 13

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0 SeaMonkey/2.14a1 ID:20120820003006 c-c:8e502ee509af m-c:c676b554c7bb

Trying to find out what has become of this fix.
1. MXR displays both hunks, a little up or down maybe, but they're there.
2. Open the Data Manager for a site with a page currently loaded which has embedded Flash. Add an Activate Plugins perm for the site. Uncheck "Use Default" and select Allow. Switch domains back and forth -> the radiobutton is still there.
3. Switch tabs back and forth, the radiobutton is still there.
4. Check "Use Default", switch domains back and forth, the perm is gone. OK so far.
5. Re-add it, but with "Block" this time. Switch domains back and forth. The perm is still there.
6. Set the radiobutton to "Allow", go to about:config, make sure that plugins.click_to_play is set to true.
7. In about:addons, disable Adblock Plus (bug 784135) which is now restartless. Check that none of NoScript, Greasemonkey, RequestPolicy and redirector is installed (bug 782644).
8. Reload the Flash page mentioned at step 2 by clicking the reload button in the toolbar (I use my skynet.be userpage but I guess YouTube would serve just as well). -> Flash starts.
9. In the Data Manager, set the radiobutton to "Block". Repeat step 8. -> Flash still starts.
10. Toggle CTP to false. Repeat step 8. -> Flash still starts.
11. Restart SeaMonkey. This means I'll have to submit this comment before I'm finished with testing. BRB.

Comment 14

[Tony Mechelynck [:tonymec]]

11 bis. I forgot to check user.js. It still has a line saying:
user_pref("plugins.click_to_play", true);
so the browser restarts with CTP on.
12. -> After restart, the radiobutton is still set to "Block" and Flash doesn't start. No CTP placeholder but a blank rectangle.
13. Toggle the radio to Allow. Repeat step 8. -> Flash starts.
14. Toggle the radio to Block. Repeat step 8. -> Empty rectangle again.
15. Click "Default" in the Data Manager. Repeat step 8. -> CTP placeholder, "Click here to activate plugin".

Conclusion: AFAICT, this fix works according to spec. I'm setting VERIFIED FIXED.
Pushed to: 2.11 Aurora and 2.12 central, see comment #9 (the last 2.10a2 was two days earlier).
Pushed to 2.13 and 2.14 by the release train. Verified today on 2.14.

Before you REOPEN, make sure you retest without any of the extensions mentioned under 7 above, or after the two mentioned bugs are FIXED.

Comment 15

[Tony Mechelynck [:tonymec]]

P.S. Last experiment: After step 15:
16. Click the placeholder -> Flash starts. Phhhwww - it still tests OK.

Comment 16

[Jens Hatlak (:InvisibleSmiley)]

Tony, while I certainly appreciate your QA work, I have to ask you to please not resolve other people's (in this case: my) bugs. I had intentionally left it open because of comment 8 (I should probably assigned it to myself explicitly). Now I'm forced to open a follow-up bug and copy over all the relevant replies to have the discussion in one place again.

Comment 17

[Tony Mechelynck [:tonymec]]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #16)
> Tony, while I certainly appreciate your QA work, I have to ask you to please
> not resolve other people's (in this case: my) bugs. I had intentionally left
> it open because of comment 8 (I should probably assigned it to myself
> explicitly). Now I'm forced to open a follow-up bug and copy over all the
> relevant replies to have the discussion in one place again.

ah, sorry. I had read comment #8 but it said "…or in a followup bug…" on every issue it mentioned, and as far as I could tell everything this bug was about was fixed. It is more obvious when the assignee adds [leave open] to the Whiteboard. Next time I'll try to contact the assignee first.