Closed Bug 1479335 Opened Last year Closed 10 months ago

Remove permissions close button

Categories

(Firefox :: Site Identity, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: cpearce, Assigned: manishkk)

Details

Attachments

(2 files, 5 obsolete files)

privacy.permissionPrompts.showCloseButton is false, so we never show a close button on permission prompts. We should probably just remove this code, since it's never used.

(Florian noticed this code is unused in bug 1473671 comment 19.)
Priority: -- → P3
Assignee: nobody → 1991manish.kumar
Only need to remove this line?
https://searchfox.org/mozilla-central/source/modules/libpref/init/all.js#1370
Flags: needinfo?(cpearce)
There are a bunch of places we look up the pref value that should also be removed:
https://searchfox.org/mozilla-central/search?q=showCloseButton&path=

Also, you might want to ask in #firefox-dev on irc.mozilla.org, as I'm not an expert in the front-end code. :)
Flags: needinfo?(cpearce)
Attached patch Patch_Bug1479335Splinter Review
Please Review!
Attachment #9020650 - Flags: review?(jhofmann)
Comment on attachment 9020650 [details] [diff] [review]
Patch_Bug1479335

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

Thanks for the patch, though there are two places where you should delete more liberally than just the line highlighted by searchfox.

::: browser/base/content/test/popupNotifications/browser_popupNotification_2.js
@@ -219,2 @@
>    // dismissal callback if privacy.permissionPrompts.showCloseButton is set.
>    { id: "Test#10",

It seems to me like this whole test case can be deleted instead.

::: modules/libpref/init/all.js
@@ +1367,3 @@
>  // If true, close button will be shown on permission prompts
>  // and for all PopupNotifications, the secondary action of
>  // the popup will be called when the popup is dismissed.

Please delete this comment as well.
Attachment #9020650 - Flags: review?(jhofmann) → review-
Attachment #9030003 - Attachment is obsolete: true
I'm a little confused - it seems like the goal of this bug was to remove the privacy.permissionPrompts.showCloseButton pref, and always hide the close button in permission prompt popup notifications. That means forcing hideClose to true instead of getting the value from the pref, not removing the option completely.

Please correct me if I am wrong. If I am right, the patch should be updated to force hideClose to true wherever it's currently being removed.

Additionally, Tanvi: you were asked in bug 1473671 comment 20 whether the reasons for implementing this pref in the first place are still relevant. The patch in this bug to remove the pref is almost ready, so please chime in if you have good reasons not to land it.
Flags: needinfo?(tanvi)
Flags: needinfo?(cpearce)
Flags: needinfo?(1991manish.kumar)
I prefered the behavior with the X - so users have a way to get out of a prompt that is interrupting their behavior without making an un-informed decision.  If they want the doorhanger to get out of their way, they could just click accept and move on.

On the other hand, it could pleasantly be the opposite -  when presented with something like this, users are more likely to hit "deny" when they don't read the message.  Does our telemetry show one way or another?

As far as the pref for the X, we can ask UX if there is any sign of bringing it back.  If not, then lets remove it because at this point it has been shipping without it for so long.
IIRC, the point here was that there's nothing that sets privacy.permissionPrompts.showCloseButton to true, so there's no point in keeping this around as we only ever take the privacy.permissionPrompts.showCloseButton=false branches.
Flags: needinfo?(cpearce)
(In reply to Chris Pearce [:cpearce (GMT+13)] (Away Dec 21  Jan 14) from comment #9)
> IIRC, the point here was that there's nothing that sets
> privacy.permissionPrompts.showCloseButton to true, so there's no point in
> keeping this around as we only ever take the
> privacy.permissionPrompts.showCloseButton=false branches.

True, but the point of that pref, I believe, was to provide a surface for experiments/studies to change this behavior in a controlled manner and get some usage data before we committed to true/false.

(In reply to Tanvi Vyas[:tanvi] from comment #8)
> I prefered the behavior with the X - so users have a way to get out of a
> prompt that is interrupting their behavior without making an un-informed
> decision.  If they want the doorhanger to get out of their way, they could
> just click accept and move on.
> 
> On the other hand, it could pleasantly be the opposite -  when presented
> with something like this, users are more likely to hit "deny" when they
> don't read the message.  Does our telemetry show one way or another?
> 
> As far as the pref for the X, we can ask UX if there is any sign of bringing
> it back.  If not, then lets remove it because at this point it has been
> shipping without it for so long.

Yeah, if nobody is driving such an experiment we can just get rid of the pref. I'll try to find out who to ping from UX about this, or if you know that off the top of your head please needinfo them - not sure when I'll get to it. In the meantime we can land this "by default" and it would be trivial to revert it if there is revived interest in tweaking this.
Flags: needinfo?(tanvi)
Flags: needinfo?(1991manish.kumar)
This pref has been dead and its value decided on for almost two years now. I think it's safe to remove. Personally I would like to avoid reviving the discussion on the pref value.
Agreed. I just realized I left a review comment on the bug without properly leaving a review on Phabricator - sorry. Did that now.
Attachment #9032257 - Attachment is obsolete: true
Attachment #9035782 - Attachment is obsolete: true

(In reply to Manish [:manishkk] from comment #14)

Created attachment 9035798 [details]
Bug 1479335 Remove permissions close button

Patch looks good, but before I r+, please push to try and paste a link here. Not sure if you've pushed to try before; let me know if I can help with that. :)

Flags: needinfo?(1991manish.kumar)

I never pushed on try before.
Please let me know how can I do that?

Flags: needinfo?(1991manish.kumar) → needinfo?(nhnt11)

(In reply to Manish [:manishkk] from comment #16)

I never pushed on try before.
Please let me know how can I do that?

Please check out https://wiki.mozilla.org/ReleaseEngineering/TryServer

This page should contain everything you need to learn to use the try server and some more helpful links. As it mentions, you'll need to get Level 1 Commit Access before you can push - please feel free to flag me to vouch you when the time comes.

You can ask for help with this stuff on #fx-team on IRC, which might be useful for quick feedback. Getting Try access will be useful if you want to keep contributing patches, but if you are not interested in going through the process I (or someone else) will be able to do a Try push on your behalf.

Flags: needinfo?(nhnt11)
Flags: needinfo?(nhnt11)

Looks all good, I'll land it for you.

Flags: needinfo?(nhnt11)

Oh, nevermind, you're still waiting on review from Nihanth.

Flags: needinfo?(nhnt11)

r+. Many apologies for the > 2-week delay, this slipped from my queue.

Flags: needinfo?(nhnt11)
Keywords: checkin-needed
Attachment #9035798 - Attachment description: Bug 1479335 Remove permissions close button → Bug 1479335 - Remove permissions close button

Tried to land this:

Failed to Land
On Tue, February 5, 2019, 6:18 PM GMT+2, by apavel@mozilla.com.
Revisions: D16264 diff 50517
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmp0FSt84\npatching file toolkit/modules/PopupNotifications.jsm\nHunk #1 FAILED at 829\n1 out of 1 hunks FAILED -- saving rejects to file toolkit/modules/PopupNotifications.jsm.rej\nabort: patch failed to apply', '')

Flags: needinfo?(1991manish.kumar)
Keywords: checkin-needed

You'll have to rebase on central yourself (hg pull central --rebase), feel free to submit the patch for review again afterwards if you're unsure about it.

Thanks!

Attachment #9045441 - Attachment is obsolete: true
Attachment #9046156 - Attachment is obsolete: true
Keywords: checkin-needed

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b94501076533
Remove permissions close button r=nhnt11

Keywords: checkin-needed

There were also bc failures on "browser_popupNotification_2.js"
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=230127429&repo=autoland&lineNumber=1453

Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8c7ac177290e
Remove permissions close button r=nhnt11
Flags: needinfo?(ntim.bugs)
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8ca28563b1d0
Remove permissions close button r=nhnt11
Flags: needinfo?(1991manish.kumar)
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
No longer depends on: 1533362
You need to log in before you can comment on or make changes to this bug.