1161200 - Only show "Edit Site Settings" when actionable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

We should probably hide the "Edit settings" button if there is nothing for the user to actually do.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1161200 - Only show "Edit Site Settings" when actionable. r=margaret

Bug 1161200 - Only show "Edit Site Settings" when actionable. r=margaret

Comment 2

[:Margaret Leibovic]

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?

Comment 3

[Chenxia Liu [:liuche]]

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.

Comment 4

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/94ff3be9a029

Comment 5

[Chenxia Liu [:liuche]]

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.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/94ff3be9a029