Closed Bug 1373206 Opened 7 years ago Closed 7 years ago

Create a new dialog for notification settings under Firefox Preferences to match the new spec.

Categories

(Firefox :: Settings UI, enhancement, P1)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: prathiksha, Assigned: prathiksha, Mentored)

References

Details

Attachments

(1 file)

Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
Change the existing dialog for notification settings in the Privacy and Security section under Firefox Preferences to look like this (https://mozilla.invisionapp.com/share/4ZB63CHD9#/screens/237173277).
Assignee: nobody → prathikshaprasadsuman
Blocks: 1275599
Summary: Modify the existing dialog for notification settings under Firefox Preferences to match the new spec. → Create a new dialog for notification settings under Firefox Preferences to match the new spec.
Attachment #8883755 - Flags: review?(jhofmann)
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review160100

This looks pretty good already.

I think it's a good idea to start a new .js file for the site permission manager, but if we'd find a good alternative name for permissions.js I would much prefer if we could rename that to avoid confusion.

The only bigger thing I can still see to do is to that user changes should actually be reflected in the UI and stored on "Save Changes". There will be a lot of edge cases to fix and we'll want to implement the searching and sorting, but I'd like to defer that to follow-up bugs. Oh, and this needs tests.

Also, please try to convert the code you're adding to our recent code style:

- Change var to let where it makes sense (should be everywhere)
- Use the early-return style instead of huge if branches
- Don't use hungarian notation (aSite, aData) for argument names

In general, I feel like we can make sitePermissions.js much simpler than permissions.js and avoid unneeded abstractions (I've commented on a couple of things inline).

Again, great work so far.

Thanks

::: browser/components/preferences/permissions.js
(Diff revision 1)
>  
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/AppConstants.jsm");
>  
>  const nsIPermissionManager = Components.interfaces.nsIPermissionManager;
> -const nsICookiePermission = Components.interfaces.nsICookiePermission;

Did you delete this as part of cleaning up permissions.js to not include cookies code? We might want to do that in a different bug (I'm all for doing it, though).

::: browser/components/preferences/sitePermissions.js:6
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/AppConstants.jsm");

Does the code actually use AppConstants?

::: browser/components/preferences/sitePermissions.js:9
(Diff revision 1)
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/AppConstants.jsm");
> +
> +const nsIPermissionManager = Components.interfaces.nsIPermissionManager;
> +const nsICookiePermission = Components.interfaces.nsICookiePermission;

See further below, I don't think you need this. We're not managing cookie settings.

::: browser/components/preferences/sitePermissions.js:16
(Diff revision 1)
> +function Permission(principal, type, capability) {
> +  this.principal = principal;
> +  this.origin = principal.origin;
> +  this.type = type;
> +  this.capability = capability;
> +}

IMO you can get rid of this constructor and just save permission info in simple JS objects or as nsiPermission objects, whatever makes more sense to you.

::: browser/components/preferences/sitePermissions.js:23
(Diff revision 1)
> +var gSitePermissionsManager = {
> +  _type: "",
> +  _permissions: [],
> +  _list: [],
> +  _observerRemoved: false,
> +  _permissionsCount: 0,

I don't see how _permissionsCount can not be derived from _permissions.length, so I'd say let's remove it

::: browser/components/preferences/sitePermissions.js:38
(Diff revision 1)
> +      // reusing an open dialog, clear the old observer
> +      this.uninit();
> +    }
> +
> +    this._type = aParams.permissionType;
> +    this._manageCapability = aParams.manageCapability;

Not sure what this is supposed to do, I think we can remove it.

::: browser/components/preferences/sitePermissions.js:48
(Diff revision 1)
> +    permissionsText.appendChild(document.createTextNode(aParams.introText));
> +
> +    document.title = aParams.windowTitle;
> +
> +    var urlField = document.getElementById("url");
> +    urlField.value = aParams.prefilledHost;

Do we need this prefilledHost stuff?

::: browser/components/preferences/sitePermissions.js:58
(Diff revision 1)
> +
> +    urlField.focus();
> +  },
> +
> +  uninit() {
> +    if (!this._observerRemoved) {

I know this isn't your code but this pattern is a bit weird. Wouldn't it make more sense to set a "this._isObserving" variable and not register the observer if it already exists instead?

::: browser/components/preferences/sitePermissions.js:102
(Diff revision 1)
> +    case nsICookiePermission.ACCESS_ALLOW_FIRST_PARTY_ONLY:
> +      stringKey = "canAccessFirstParty";
> +      break;
> +    case nsICookiePermission.ACCESS_SESSION:
> +      stringKey = "canSession";
> +      break;

Since we're not going to manage cookies with this view, you can remove the last two cases.

::: browser/components/preferences/sitePermissions.js:129
(Diff revision 1)
> +      if (this._permissions[i].principal.equals(aPrincipal)) {
> +        this._permissions.splice(i, 1);
> +        this._permissionsCount--;
> +        break;
> +      }
> +    }

I think this function can be reduced to 

this._permissions = this._permissions.filter((p) => p.principal.equals(aPrincipal));

::: browser/components/preferences/sitePermissions.js:133
(Diff revision 1)
> +      }
> +    }
> +  },
> +
> +  _loadPermissions() {
> +    this._list = document.getElementById("permissionsBox");

Why does this need to be an attribute instead of a local variable?

::: browser/components/preferences/sitePermissions.js:151
(Diff revision 1)
> +
> +    // disable "remove all" button if there are none
> +    document.getElementById("removeAllPermissions").disabled = this._permissions.length == 0;
> +  },
> +
> +  _createPermissionListItem(permission) {

The permission list looks fine for a WIP, but to make it look consistent you should take a look at how the Site Data Settings are structuring the site data list (about:preferences -> Privacy -> Site Data -> Settings). We can probably copy a lot from them.

::: browser/locales/en-US/chrome/browser/preferences/preferences.properties:31
(Diff revision 1)
>  addonspermissionstext=You can specify which websites are allowed to install add-ons. Type the exact address of the site you want to allow and then click Allow.
>  addons_permissions_title=Allowed Sites - Add-ons Installation
>  popuppermissionstext=You can specify which websites are allowed to open pop-up windows. Type the exact address of the site you want to allow and then click Allow.
>  popuppermissionstitle=Allowed Sites - Pop-ups
> -notificationspermissionstext4=Control which websites are always or never allowed to send you notifications. If you remove a site, it will need to request permission again.
> -notificationspermissionstitle=Notification Permissions
> +notificationspermissionstext4=The following websites have requested to send you notifications. You can specify which websites are allowed to send you notifications.
> +notificationspermissionstitle=Settings - Notification Permissions

Note that you need to increment the number behind the localization key if you update it (that's because our localization tooling requires it).
Attachment #8883755 - Flags: review?(jhofmann)
Attachment #8883755 - Flags: feedback+
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review160100

ok, thanks again for the feedback.
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review160100

> Did you delete this as part of cleaning up permissions.js to not include cookies code? We might want to do that in a different bug (I'm all for doing it, though).

Oh no, I deleted this part from permissions.js thinking it was sitePermissions.js. 
And yes, we'll clean permissions.js in a different bug.
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review160100

> Does the code actually use AppConstants?

It does. It used AppConstants.platform. :)

> See further below, I don't think you need this. We're not managing cookie settings.

Yes, I took all the cookie stuff out in the next iteration.
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review160100

> It does. It used AppConstants.platform. :)

uses *
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review160100

> Why does this need to be an attribute instead of a local variable?

Oh, we'll be using it a lot to manage the UI. It will make sense in the next iteration.

> Note that you need to increment the number behind the localization key if you update it (that's because our localization tooling requires it).

I didn't get this. :(
(In reply to Prathiksha from comment #7)
> Comment on attachment 8883755 [details] 
> > Note that you need to increment the number behind the localization key if you update it (that's because our localization tooling requires it).
> 
> I didn't get this. :(

notificationspermissionstext4 should become notificationspermissionstext5 and notificationspermissionstitle should become notificationspermissionstitle2

Our localization tooling needs to know when strings need to be translated again, and for that we increment a magic number after the identifier.

Feel free to ask on IRC if you have more questions about that :)
(In reply to Johann Hofmann [:johannh] from comment #9)
> (In reply to Prathiksha from comment #7)
> > Comment on attachment 8883755 [details] 
> > > Note that you need to increment the number behind the localization key if you update it (that's because our localization tooling requires it).
> > 
> > I didn't get this. :(
> 
> notificationspermissionstext4 should become notificationspermissionstext5
> and notificationspermissionstitle should become
> notificationspermissionstitle2
> 
> Our localization tooling needs to know when strings need to be translated
> again, and for that we increment a magic number after the identifier.
> 
> Feel free to ask on IRC if you have more questions about that :)

Okay. Got it :)
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review161720

::: browser/components/preferences/in-content-new/privacy.js:894
(Diff revision 3)
>     */
>    showNotificationExceptions() {
>      let bundlePreferences = document.getElementById("bundlePreferences");
> -    let params = { permissionType: "desktop-notification" };
> -    params.windowTitle = bundlePreferences.getString("notificationspermissionstitle");
> -    params.introText = bundlePreferences.getString("notificationspermissionstext4");
> +    let params = { permissionType: "desktop-notification"};
> +    params.windowTitle = bundlePreferences.getString("notificationspermissionstitle2");
> +    params.introText = bundlePreferences.getString("notificationspermissionstext5");

You also need to update https://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/browser/components/preferences/in-content-new/privacy.js#346
Attachment #8883755 - Flags: review?(jhofmann)
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review161722

There are a couple of style issues in the file, you can run ./mach eslint browser/components/preferences to find them :)
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review161766

I feel that these are enough comments to be helpful to you. I've only looked at sitePermissions.js so far, I'll look at the rest tomorrow.

I know those are a lot of remarks, but in general this looks really good.

Thank you!

::: browser/components/preferences/sitePermissions.js:21
(Diff revision 4)
> +}
> +
> +var gSitePermissionsManager = {
> +  _type: "",
> +  _permissions: [],
> +  _list: [],

Why is this initialized to an array?

::: browser/components/preferences/sitePermissions.js:34
(Diff revision 4)
> +    this.init(params);
> +  },
> +
> +  init(params) {
> +    if (this._type) {
> +      // reusing an open dialog, clear the old observer

I don't think we have to clear the old observer here, we can do something like

if (!this._isObserving) {
  Services.obs.addObserver(this, "perm-changed");
  this._isObserving = true;
}

in the code below instead.

::: browser/components/preferences/sitePermissions.js:50
(Diff revision 4)
> +    document.title = params.windowTitle;
> +
> +    let image = document.createElement("image");
> +    let urlField = document.getElementById("url");
> +    image.setAttribute("src", "chrome://global/skin/icons/search-textbox.svg");
> +    urlField.appendChild(image);

Sorry for not remembering that earlier when you asked me on IRC, but I think you can just set type="search" on the textbox, that will automatically add a search icon.

::: browser/components/preferences/sitePermissions.js:124
(Diff revision 4)
> +    }
> +  },
> +
> +  _removePermissionFromList(principal) {
> +    for (let i = 0; i < this._permissions.length; ++i) {
> +      if (this._permissions[i].principal.equals(principal)) {

You're doing this kind of loop enough times that I would recommend using a Map of principal.origin to your permission object instead. Makes for more readable code a marginally more efficient lookup.

::: browser/components/preferences/sitePermissions.js:133
(Diff revision 4)
> +      }
> +    }
> +  },
> +
> +  _loadPermissions() {
> +    this._list = document.getElementById("permissionsBox");

Why are you initializing this here? Might be better placed in init

::: browser/components/preferences/sitePermissions.js:156
(Diff revision 4)
> +
> +    let hbox = document.createElement("hbox");
> +    let website = document.createElement("label");
> +    website.setAttribute("value", permission.origin);
> +    website.setAttribute("width", "50");
> +    website.setAttribute("id", "label" + this._permissions.length);

What is this for?

::: browser/components/preferences/sitePermissions.js:157
(Diff revision 4)
> +    let hbox = document.createElement("hbox");
> +    let website = document.createElement("label");
> +    website.setAttribute("value", permission.origin);
> +    website.setAttribute("width", "50");
> +    website.setAttribute("id", "label" + this._permissions.length);
> +    hbox.setAttribute("style", "padding:5px;");

Please don't set inline styles. Feel free to make a new sitePermissions.css file in the same directory.

::: browser/components/preferences/sitePermissions.js:203
(Diff revision 4)
> +      this.onPermissionDelete();
> +      event.preventDefault();
> +    }
> +  },
> +
> +  onPermissionDelete() {

In this function you also need to remove the permission from the permissionsToChange set in case the user had previously changed the capability but then removed the site.

::: browser/components/preferences/sitePermissions.js:205
(Diff revision 4)
> +    }
> +  },
> +
> +  onPermissionDelete() {
> +    if (!this._list.itemCount)
> +      return;

Is it possible to call onPermissionDelete with no items in the list?

::: browser/components/preferences/sitePermissions.js:236
(Diff revision 4)
> +
> +  onPermissionSelect() {
> +    let hasSelection = this._list.selectedCount > 0;
> +    let hasRows = this._list.itemCount > 0;
> +    document.getElementById("removePermission").disabled = !hasRows || !hasSelection;
> +    document.getElementById("removeAllPermissions").disabled = !hasRows;

It looks like it would be possible to factor out setting these disabled states into its own function that can then be called by onPermissionDelete(), onAllPermissionsDelete(), onPermissionSelect() and changePermission().

::: browser/components/preferences/sitePermissions.js:239
(Diff revision 4)
> +    let hasRows = this._list.itemCount > 0;
> +    document.getElementById("removePermission").disabled = !hasRows || !hasSelection;
> +    document.getElementById("removeAllPermissions").disabled = !hasRows;
> +  },
> +
> +  changePermission(permission, capability) {

To be consistent you would have to call this something like onChangePermission, but I wouldn't mind dropping the on from the other function names, too (like deletePermission, deleteAllPermissions, etc.).

Your call, but please make it consistent. :)

::: browser/components/preferences/sitePermissions.js:262
(Diff revision 4)
> +
> +  onApplyChanges() {
> +    // Stop observing permission changes since we are about
> +    // to write out the pending adds/deletes and don't need
> +    // to update the UI
> +    this.uninit();

I don't really see why we would have to do this here. Isn't uninit always called when the window closes? If not, we'd be leaking the observer on cancel. So I would suggest removing this code.

::: browser/components/preferences/sitePermissions.js:266
(Diff revision 4)
> +    // to update the UI
> +    this.uninit();
> +
> +    for (let p of this._permissionsToChange.values()) {
> +      let uri = Services.io.newURI(p.origin);
> +      let c = p.capability == "Allow" ? 1 : 2;

I'd suggest having one attribute "capability" and one attribute "capabilityString" on your permissions to avoid this.

::: browser/components/preferences/sitePermissions.js:272
(Diff revision 4)
> +      SitePermissions.set(uri, p.type, c);
> +    }
> +
> +    for (let p of this._permissionsToDelete.values()) {
> +      let uri = Services.io.newURI(p.origin);
> +      SitePermissions.set(uri, p.type, 0);

There's SitePermissions.remove

https://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/browser/modules/SitePermissions.jsm#415

::: npm-shrinkwrap.json:218
(Diff revision 4)
>      "eslint-plugin-html": {
>        "version": "https://registry.npmjs.org/eslint-plugin-html/-/eslint-plugin-html-2.0.3.tgz",
>        "integrity": "sha1-fImIOrDIX6XSi2ZqFKTpBqqQuJc="
>      },
>      "eslint-plugin-mozilla": {
> -      "version": "file:tools\\lint\\eslint\\eslint-plugin-mozilla"
> +      "version": "file:tools/lint/eslint/eslint-plugin-mozilla"

You should revert these changes.
Attachment #8883755 - Flags: review?(jhofmann)
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review161766

> What is this for?

I had to ID it so I can access the element in the test. But I found a work around now.

> In this function you also need to remove the permission from the permissionsToChange set in case the user had previously changed the capability but then removed the site.

I don't think we have to worry about that. If you look at onApplyChanges(), permission changes are processed first followed by permission delete which means what the user intended(to delete the permission) will happen anyway. What do you think ? :)

> Is it possible to call onPermissionDelete with no items in the list?

Yes, onPermissionDelete is called when the user presses backspace even if no permission is selected or if the list is empty. But I think I can stop that.
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review163400

::: browser/components/preferences/in-content-new/tests/browser.ini:60
(Diff revision 6)
>  [browser_security.js]
>  [browser_siteData.js]
>  [browser_siteData2.js]
>  [browser_site_login_exceptions.js]
>  [browser_subdialogs.js]
> +[browser_permission_dialog_test.js]

Putting the test file here breaks browser_subdialogs.js, because the support-files section below is referring to that test.

::: browser/components/preferences/sitePermissions.js:9
(Diff revision 6)
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://gre/modules/AppConstants.jsm");
> +Components.utils.import("resource:///modules/SitePermissions.jsm");
> +
> +const nsIPermissionManager = Components.interfaces.nsIPermissionManager;

I think you can replace all uses of this with Services.perms.

::: browser/components/preferences/sitePermissions.js:25
(Diff revision 6)
> +  _type: "",
> +  _isObserving: false,
> +  _permissions: new Map(),
> +  _permissionsToChange: new Map(),
> +  _permissionsToDelete: new Map(),
> +  _list: "",

Please initialize to null if this is a DOM node.

::: browser/components/preferences/sitePermissions.js:28
(Diff revision 6)
> +  _permissionsToChange: new Map(),
> +  _permissionsToDelete: new Map(),
> +  _list: "",
> +
> +  onLoad() {
> +    this._bundle = document.getElementById("bundlePreferences");

Nit: since you're in the habit of declaring all member variables at the beginning of gSitePermissionManager (which is good\!), you might want to add a

_bundle: null

too.

Also, you should consider moving this initialization to this.init()

::: browser/components/preferences/sitePermissions.js:84
(Diff revision 6)
> +  },
> +
> +  _handleCapabilityChange(perm) {
> +    let permissionlistitem = document.getElementById(perm.origin);
> +    permissionlistitem.childNodes.item(0).childNodes.item(1).selectedIndex =
> +    perm.capability - 1;

Please don't do arithmetic on permission capabilities. They aren't guaranteed to map to sequential numbers. You should treat them like strings.

::: browser/components/preferences/sitePermissions.js:89
(Diff revision 6)
> +    perm.capability - 1;
> +  },
> +
> +  _getCapabilityString(capability) {
> +    let stringKey = null;
> +    switch (capability) {

We should support "Always Ask" (PROMPT_ACTION).

::: browser/components/preferences/sitePermissions.js:96
(Diff revision 6)
> +      stringKey = "can";
> +      break;
> +    case nsIPermissionManager.DENY_ACTION:
> +      stringKey = "cannot";
> +      break;
> +    }

We need to deal with unknown capability values (it should simply not crash for those). The site identity popup and the page info section don't handle this case really optimal either, unfortunately.

We could either introduce an "Unknown" string or just not show the permission at all. I lean towards the former.

I'm ok with solving that in a follow-up bug, but we shouldn't forget about it :)

::: browser/components/preferences/sitePermissions.js:112
(Diff revision 6)
> +    this._createPermissionListItem(p);
> +  },
> +
> +  _removePermissionFromList(origin) {
> +    this._permissions.delete(origin);
> +    let permissionlistitem = document.getElementById(origin);

Please don't store the the origins in the id, since it's also used for other purposes. You can put them in an "origin" attribute and then use _list.getElementsByAttribute("origin", ...

::: browser/components/preferences/sitePermissions.js:150
(Diff revision 6)
> +    menulist.setAttribute("width", "50");
> +    menulist.appendChild(menupopup);
> +    let m1 = document.createElement("menuitem");
> +    let m2 = document.createElement("menuitem");
> +    m1.setAttribute("label", "Allow");
> +    m1.setAttribute("value", "1");

We can't hardcode strings and/or permission manager constants here. :)

It's probably best to set these labels by iterating over the permission constants you want to show, i.e. [Services.perms.DENY_ACTION, Services.perms.ALLOW_ACTION, Services.perms.PROMPT_ACTION]

::: browser/components/preferences/sitePermissions.js:187
(Diff revision 6)
> +      event.preventDefault();
> +    }
> +  },
> +
> +  _setRemoveButtonState() {
> +    var hasSelection = this._list.selectedCount > 0;

Nit: use let

::: browser/components/preferences/sitePermissions.js:190
(Diff revision 6)
> +
> +  _setRemoveButtonState() {
> +    var hasSelection = this._list.selectedCount > 0;
> +    var hasRows = this._list.itemCount > 1;
> +    document.getElementById("removePermission").disabled =
> +    !hasRows || !hasSelection;

Please indent this kind of row by one level.

::: browser/components/preferences/sitePermissions.js:190
(Diff revision 6)
> +
> +  _setRemoveButtonState() {
> +    var hasSelection = this._list.selectedCount > 0;
> +    var hasRows = this._list.itemCount > 1;
> +    document.getElementById("removePermission").disabled =
> +    !hasRows || !hasSelection;

I wonder if the !hasRows is necessary here, can there be a selection without rows?

::: browser/components/preferences/sitePermissions.js:211
(Diff revision 6)
> +    this._setRemoveButtonState();
> +  },
> +
> +  onAllPermissionsDelete() {
> +    for (var origin of this._permissions.keys()) {
> +      let permission = this._permissions.get(origin);

You can iterate over both key and value of a Map at the same time

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#Iterating_Maps_with_for..of

::: browser/components/preferences/sitePermissions.xul:33
(Diff revision 6)
> +
> +  <vbox class="contentPane largeDialogContainer" flex="1">
> +    <description id="permissionsText" control="url"/>
> +    <separator class="thin"/>
> +    <hbox align="start">
> +      <textbox id="url" flex="1" style="color:gray;" value=" Search Website"

Instead of color:gray value=Search Website you can simply do placeholder="Search Website"

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/placeholder
Attachment #8883755 - Flags: review?(jhofmann)
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review165292

I feel like we're getting quite close. Most of the comments are about the test file, which I hadn't reviewed before.

Please also add a test case for this scenario (comment 16):

> > In this function you also need to remove the permission from the permissionsToChange set in case the user had previously changed the capability but then removed the site.
> 
> I don't think we have to worry about that. If you look at onApplyChanges(),
> permission changes are processed first followed by permission delete which
> means what the user intended(to delete the permission) will happen anyway.
> What do you think ? :)

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:33
(Diff revision 7)
> +  // First item in the richlistbox contains column headers.
> +  Assert.equal(richlistbox.itemCount - 1, 0,
> +               "Number of permission items is 0 initially");
> +
> +  // Add notification permission for a website.
> +  SitePermissions.set(URI, "desktop-notification", 1);

Please use SitePermissions.ALLOW etc. instead of magic numbers.

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:36
(Diff revision 7)
> +
> +  // Add notification permission for a website.
> +  SitePermissions.set(URI, "desktop-notification", 1);
> +
> +  // Observe the added permission changes in the dialog UI.
> +  Assert.equal(richlistbox.itemCount - 1, 1);

It would be great if you could do some additional assertions here, e.g. assert that the origin and the capability are displayed correctly. Ideally you could make a shared function (checkPermissionItem?) that all tests can use to assert that a richlistitem correctly represents a permission.

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:37
(Diff revision 7)
> +  // Add notification permission for a website.
> +  SitePermissions.set(URI, "desktop-notification", 1);
> +
> +  // Observe the added permission changes in the dialog UI.
> +  Assert.equal(richlistbox.itemCount - 1, 1);
> +});

Unless it's absolutely required, it's good practice to have tasks clean up after themselves without spilling state into the next test. 

Calling SitePermissions.set another time in the next test shouldn't be a big problem and your tests become much easier to understand and much more robust this way.

In this case you'd call SitePermissions.remove at the end of the test task.

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:43
(Diff revision 7)
> +
> +add_task(async function observePermissionChange() {
> +  let doc = sitePermissionsDialog.document;
> +  let richlistbox = doc.getElementById("permissionsBox");
> +  let menulist = richlistbox.getItemAtIndex(1).
> +                 childNodes.item(0).childNodes.item(1);

I'd recommend not relying on these positional selectors and rather using e.g. getElementsByTagName or similar.

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:61
(Diff revision 7)
> +
> +  let richlistbox = doc.getElementById("permissionsBox");
> +  Assert.equal(richlistbox.itemCount - 1, 1,
> +               "The box contains one permission item initially");
> +
> +  SitePermissions.set(URI, "desktop-notification", 0);

You can use SitePermissions.remove.

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:83
(Diff revision 7)
> +
> +  Assert.equal(SitePermissions.get(URI, "desktop-notification").state, 1,
> +               "Permission state does not change before saving changes");
> +
> +  let saveChanges = doc.getElementById("btnApplyChanges");
> +  saveChanges.click();

It would be great to have another test that clicks cancel instead of save and checks that nothing changed.

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:91
(Diff revision 7)
> +    SitePermissions.get(URI, "desktop-notification").state == 2);
> +
> +  gBrowser.removeCurrentTab();
> +});
> +
> +add_task(async function openSitePermissionsDialog() {

You shouldn't have to spawn this task several times. You can factor out opening the dialog into a new async function and just await on that in your tests.

I also don't think you need to close the tab and open a new one.

::: browser/components/preferences/sitePermissions.js:82
(Diff revision 7)
> +    }
> +  },
> +
> +  _handleCapabilityChange(perm) {
> +    let permissionlistitem = document.getElementsByAttribute("origin", perm.origin)[0];
> +    let menulist = permissionlistitem.childNodes.item(0).childNodes.item(1);

The way you're accessing the menulist here is prone to break when someone changes the structure of your XUL file.

Can you use getElementsByTagName("menulist")?

::: browser/components/preferences/sitePermissions.js:151
(Diff revision 7)
> +    let menupopup = document.createElement("menupopup");
> +    menulist.setAttribute("flex", "1");
> +    menulist.setAttribute("width", "50");
> +    menulist.appendChild(menupopup);
> +    let states = SitePermissions.getAvailableStates(permission.type);
> +    for (let i = 0; i < states.length; i++) {

Please use

for (let state of states) {

::: browser/components/preferences/sitePermissions.js:153
(Diff revision 7)
> +    menulist.setAttribute("width", "50");
> +    menulist.appendChild(menupopup);
> +    let states = SitePermissions.getAvailableStates(permission.type);
> +    for (let i = 0; i < states.length; i++) {
> +      if (!states[i])
> +        continue;

Is this for ignoring the UNKNOWN state? Please compare directly with SitePermissions.UNKNOWN then.

::: browser/components/preferences/sitePermissions.js:156
(Diff revision 7)
> +    for (let i = 0; i < states.length; i++) {
> +      if (!states[i])
> +        continue;
> +      let m = document.createElement("menuitem");
> +      m.setAttribute("label", this._getCapabilityString(states[i]));
> +      m.setAttribute("value", states[i].toString());

I don't think you need to call .toString() here, it will automatically convert it.

::: browser/components/preferences/sitePermissions.js:162
(Diff revision 7)
> +      menupopup.appendChild(m);
> +    }
> +    menulist.value = permission.capability;
> +
> +    menulist.addEventListener("select", function() {
> +      gSitePermissionsManager.onPermissionChange(permission,

This works for me, I just wanted to note that you could use an arrow function and then call this.onPermissionChange(... instead of gSitePermissionsManager.onPermissionChange.

Whatever you like better. :)

::: browser/components/preferences/sitePermissions.js:197
(Diff revision 7)
> +
> +    let hasSelection = this._list.selectedIndex;
> +    let hasRows = this._list.itemCount > 1;
> +    document.getElementById("removePermission").disabled =
> +      !hasSelection;
> +    document.getElementById("removeAllPermissions").disabled =

I'd cache these two elements in e.g. this._removeButton and this._removeAllButton
Attachment #8883755 - Flags: review?(jhofmann) → review-
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review165292

Thanks for the feedback. I'll fix it :)
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review166642

The code looks good, just a bit of cleaning up left. We're close!

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:1
(Diff revision 9)
> +"use strict";

Please add this header

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:190
(Diff revision 9)
> +});
> +
> +add_task(async function removeTab() {
> +  gBrowser.removeCurrentTab();
> +});
> +function checkPermissionItem(origin, state) {

Nit: we usually put these utility functions at the top of the file

::: browser/components/preferences/in-content-new/tests/browser_permissions_dialog.js:200
(Diff revision 9)
> +
> +  let menulist = doc.getElementsByTagName("menulist")[0];
> +  Assert.equal(menulist.label, state);
> +}
> +
> +async function openPermissionsDialog() {

See above :)

::: browser/components/preferences/sitePermissions.css:1
(Diff revision 9)
> +

Please add the license header.

::: browser/components/preferences/sitePermissions.xul:38
(Diff revision 9)
> +    <hbox align="start">
> +      <textbox id="url" flex="1" placeholder="Search Website"
> +               type="search"/>
> +    </hbox>
> +    <separator class="thin"/>
> +    <richlistbox style="height: 18em;" id="permissionsBox" selected="false"

Please put this inline style in sitePermissions.css. Since we want the richlistbox to resize with the window, it should also be min-height instead.

You will also need to set the flex="1" attribute on the richlistbox element.

::: browser/components/preferences/sitePermissions.xul:42
(Diff revision 9)
> +    <separator class="thin"/>
> +    <richlistbox style="height: 18em;" id="permissionsBox" selected="false"
> +                 hidecolumnpicker="true"
> +                 onkeypress="gSitePermissionsManager.onPermissionKeyPress(event);"
> +                 onselect="gSitePermissionsManager.onPermissionSelect();">
> +    <richlistitem style="height: 35px;">

Nit: indent this item by one level.

::: browser/components/preferences/sitePermissions.xul:42
(Diff revision 9)
> +    <separator class="thin"/>
> +    <richlistbox style="height: 18em;" id="permissionsBox" selected="false"
> +                 hidecolumnpicker="true"
> +                 onkeypress="gSitePermissionsManager.onPermissionKeyPress(event);"
> +                 onselect="gSitePermissionsManager.onPermissionSelect();">
> +    <richlistitem style="height: 35px;">

Please move this inline style to sitePermissions.css as well.

::: browser/locales/en-US/chrome/browser/preferences/permissions.dtd:8
(Diff revision 9)
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <!ENTITY window.title                 "Exceptions">
>  <!ENTITY window.width                 "45em">
>  
> -<!ENTITY treehead.sitename.label      "Site">
> +<!ENTITY treehead.sitename.label      "Website">

Since you touched this string you'd have to update its key like you did in preferences.properties.

Instead, I would suggest not changing this file and leaving it to bug 1375883. :)
Attachment #8883755 - Flags: review?(jhofmann) → review-
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review168786

::: browser/components/preferences/sitePermissions.css:9
(Diff revision 11)
> +
> +.website-name {
> +  padding: 5px;
> +}
> +
> +.richlistbox-size {

You can just use the id selector here (#permissionsBox)

::: browser/components/preferences/sitePermissions.css:13
(Diff revision 11)
> +
> +.richlistbox-size {
> +  min-height: 18em;
> +}
> +
> +.richlistitem-size {

You can use #permissionsBox > richlistitem instead.

::: browser/components/preferences/sitePermissions.js:214
(Diff revision 11)
> +
> +    this._setRemoveButtonState();
> +  },
> +
> +  onAllPermissionsDelete() {
> +    for (var permission of this._permissions.values()) {

Nit: let
Comment on attachment 8883755 [details]
Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec.

https://reviewboard.mozilla.org/r/154688/#review169232

This looks good. Let's get it landed.
Attachment #8883755 - Flags: review?(jhofmann) → review+
Keywords: checkin-needed
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 6fe9bff629a1 -d 7b9d0ff207eb: rebasing 411332:6fe9bff629a1 "Bug 1373206 - Create a new dialog for notification settings under Firefox Preferences to match the new spec. r=johannh" (tip)
merging browser/components/preferences/in-content-new/privacy.js
merging browser/components/preferences/in-content-new/tests/browser.ini
merging browser/locales/en-US/chrome/browser/preferences/preferences.properties
warning: conflicts while merging browser/components/preferences/in-content-new/tests/browser.ini! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/locales/en-US/chrome/browser/preferences/preferences.properties! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
Flags: needinfo?(prathikshaprasadsuman)
Keywords: checkin-needed
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd5645e639c8
Create a new dialog for notification settings under Firefox Preferences to match the new spec. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/fd5645e639c8
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1387971
Depends on: 1387987
Depends on: 1390057
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: