Closed Bug 1385222 Opened 3 years ago Closed 3 years ago

Add the search functionality in the sitePermissions.xul dialog.

Categories

(Firefox :: Preferences, enhancement)

enhancement
Not set

Tracking

()

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: prathiksha, Assigned: prathiksha, Mentored)

References

Details

Attachments

(1 file)

Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Comment on attachment 8895442 [details]
Bug 1385222 - Add the search functionality in the sitePermissions.xul dialog.

https://reviewboard.mozilla.org/r/166632/#review172342

This seems mostly good, just a couple of fixes necessary. And this needs a test!

There's also an edge case that we're not covering correctly right now: What if you search for google.com but then add a permission for facebook.com while the dialog is open? It will show the facebook.com item along with the google.com one.

We could check the search box value in _createPermissionItem, but that sounds inefficient. I'd be okay with deferring the problem to a follow-up bug.

::: browser/components/preferences/sitePermissions.js:138
(Diff revision 1)
>      }
>  
>      // disable "remove all" button if there are none
>      this._setRemoveButtonState();
> +
> +    if (this._searchBox.value !== "") {

_loadPermissions() is only called in _init(), is it even possible for the searchBox to have a value here?

::: browser/components/preferences/sitePermissions.js:264
(Diff revision 1)
>        SitePermissions.remove(uri, p.type);
>      }
>      window.close();
>    },
> +
> +  buildPermissionsList() {

Since this is used exclusively for filtering the list, I'd call it filterPermissionList or searchPermissionList or something like that.

::: browser/components/preferences/sitePermissions.js:274
(Diff revision 1)
> +    }
> +
> +    let keyword = this._searchBox.value.toLowerCase().trim();
> +    let permissions = this._permissions;
> +    for (let [origin, permission] of permissions) {
> +      console.log(origin);

Remove the console.log

::: browser/components/preferences/sitePermissions.xul:35
(Diff revision 1)
>  
>    <vbox class="contentPane largeDialogContainer" flex="1">
>      <description id="permissionsText" control="url"/>
>      <separator class="thin"/>
>      <hbox align="start">
> -      <textbox id="url" flex="1" placeholder="Search Website"
> +      <textbox id="searchBox" flex="1" placeholder="Search Website" aria-controls="permissionsBox"

Wait, did we check in the placeholder string without putting it in a translation file? Please put this in permissions.dtd.
Attachment #8895442 - Flags: review?(jhofmann) → review-
Comment on attachment 8895442 [details]
Bug 1385222 - Add the search functionality in the sitePermissions.xul dialog.

https://reviewboard.mozilla.org/r/166632/#review172636

This would get an r+ if it had a test :)

::: browser/locales/en-US/chrome/browser/preferences/permissions.dtd:29
(Diff revision 2)
>  <!ENTITY button.cancel.label          "Cancel">
>  <!ENTITY button.cancel.accesskey      "C">
>  <!ENTITY button.ok.label              "Save Changes">
>  <!ENTITY button.ok.accesskey          "S">
>  
> +<!ENTITY searchboxplaceholder.label   "Search Website">

This is not a label, so you should call it searchbox.placeholder
Attachment #8895442 - Flags: review?(jhofmann)
Comment on attachment 8895442 [details]
Bug 1385222 - Add the search functionality in the sitePermissions.xul dialog.

https://reviewboard.mozilla.org/r/166632/#review172684

Great, thank you!
Attachment #8895442 - Flags: review?(jhofmann) → review+
Comment on attachment 8895442 [details]
Bug 1385222 - Add the search functionality in the sitePermissions.xul dialog.

https://reviewboard.mozilla.org/r/166632/#review172686

::: browser/components/preferences/sitePermissions.js:78
(Diff revision 3)
>      if (permission.type !== this._type)
>        return;
>  
>      if (data == "added") {
>        this._addPermissionToList(permission);
> +      if (this._searchBox != "") {

If (this._searchBox.value != "")
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/7fbddf36c4cd
Add the search functionality in the sitePermissions.xul dialog. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7fbddf36c4cd
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
"Add the search functionality in the sitePermissions.xul dialog" has been implemented with Latest Nightly 57.0a1 on Windows 8.1, 64 bit.

Build ID : 20170817100132
User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:57.0) Gecko/20100101 Firefox/57.0
QA Whiteboard: [bugday-20170816]
"Add the search functionality in the sitePermissions.xul dialog" has been implemented with Latest Nightly 57.0a1 on Ubuntu 16.04 , 64 bit.

Build ID :   20170817100132
User Agent :  Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0

[bugday-20170816]
As per Comment 11 and Comment 12, I am marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.