Closed Bug 1161200 Opened 4 years ago Closed 4 years ago

Only show "Edit Site Settings" when actionable

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: antlam, Assigned: liuche)

References

Details

Attachments

(1 file)

We should probably hide the "Edit settings" button if there is nothing for the user to actually do.
Summary: Edit site settings should not be in Site ID doorhanger unless there is something to "edit" → Only show "Edit Site Settings" when actionable
Bug 1161200 - Only show "Edit Site Settings" when actionable. r=margaret
Attachment #8621428 - Flags: review?(margaret.leibovic)
Assignee: nobody → liuche
Comment on attachment 8621428 [details]
MozReview Request: Bug 1161200 - Only show "Edit Site Settings" when actionable. r=margaret

https://reviewboard.mozilla.org/r/10979/#review9679

r+ with comments addressed. This is actually something we've always wanted to do for the menu item, but decided it wasn't worth it, since it would involve update the menu on every page load. Thanks to the laziness of showing this in the site identity popup, we can now afford to do this! Hooray!

::: mobile/android/chrome/content/PermissionsHelper.js:60
(Diff revision 1)
> +        debugger;

Oops, remove this debugger statement.

::: mobile/android/chrome/content/PermissionsHelper.js:58
(Diff revision 1)
> +        check = true;

If you wanted, you could just declare the check variable here, and then it would just be undefined in the "Permissions:Get" case.

::: mobile/android/chrome/content/PermissionsHelper.js:71
(Diff revision 1)
> +              Messaging.sendRequest({

Identation is off here (maybe check your editor settings?)

::: mobile/android/base/toolbar/SiteIdentityPopup.java:162
(Diff revision 1)
> +                        mSiteSettingsLink.setVisibility(settingsEmpty ? View.GONE : View.VISIBLE);

Either this link should always be hidden already, or you should account for needing to hide it in the catch block down below.

::: mobile/android/base/toolbar/SiteIdentityPopup.java:166
(Diff revision 1)
> +                Log.e(LOGTAG, "No 'empty' property in Permissions:CheckResult", e);

To avoid this try/catch, you could just do optBoolean and default to true, since in that case we would just hide the link.

::: mobile/android/chrome/content/PermissionsHelper.js:73
(Diff revision 1)
> +                empty: false

`empty` seems a bit vague, and also confusing because a `false` value is what would result in us showing something in the UI.

Maybe something more explicit like `hasPermissions` would be better?

::: mobile/android/chrome/content/PermissionsHelper.js:98
(Diff revision 1)
> +          return;

It feels like we're already doing basically all the work of getting the permissions, so I'm wondering if we should just make the Permissions:Get call earlier and store the result for potential future use. However, I looked into the code, and that would involve changing around the logic for showing the dialog, so probably out of scope for this bug :/

I feel like we should consider removing the context menu item for site settings, since this site setting dialog is a much clearer entry point, and things always get messy to maintain when we have redundant UI in the browser. Can you a file a bug about that?
Attachment #8621428 - Flags: review?(margaret.leibovic) → review+
https://reviewboard.mozilla.org/r/10979/#review9683

::: mobile/android/base/toolbar/SiteIdentityPopup.java:162
(Diff revision 1)
> +                        mSiteSettingsLink.setVisibility(settingsEmpty ? View.GONE : View.VISIBLE);

Good call. I removed the try/catch here.
https://reviewboard.mozilla.org/r/10979/#review9691

> If you wanted, you could just declare the check variable here, and then it would just be undefined in the "Permissions:Get" case.

Actually, I tried this (declaring and assigning check in "Permissions:Check") and it fails with a reference error.

"ReferenceError: can't access lexical declaration `check' before initialization"

So it looks like to use the falsiness of undefined, you still need to declare the variable.

> It feels like we're already doing basically all the work of getting the permissions, so I'm wondering if we should just make the Permissions:Get call earlier and store the result for potential future use. However, I looked into the code, and that would involve changing around the logic for showing the dialog, so probably out of scope for this bug :/
> 
> I feel like we should consider removing the context menu item for site settings, since this site setting dialog is a much clearer entry point, and things always get messy to maintain when we have redundant UI in the browser. Can you a file a bug about that?

Yeah, I thought about this too. There isn't a way to listen for "all permissions changes", so we would also need to be sure that our cached version isn't stale.

Filed bug 1174366.
https://hg.mozilla.org/mozilla-central/rev/94ff3be9a029
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
You need to log in before you can comment on or make changes to this bug.