327048 - support filtering in password manager via Search field

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

The password manager should support filtering, just as the cookie manager does, via a "Search" field that restricts the list to matching entries.  Then users will be able to more easily find the one or more (possibly related but discontiguous due to raw alphanumeric URL sort order) entries they are looking for from among the multitude that may be on the list.

Comment 1

[Michael Wu [:mwu]]

This bug is a bit older.

Comment 2

[Michael Wu [:mwu]]

*** Bug 328832 has been marked as a duplicate of this bug. ***

Comment 3

[Michael Wu [:mwu]]

*** Bug 361309 has been marked as a duplicate of this bug. ***

Comment 4

[tech]

It's great that someone is detecting the duplicates...
Now It would be nice if someone tried to resolve it. As it would prevent other duplicates to appear.

Comment 5

[Ria Klaassen (not reading all bugmail)]

*** Bug 362827 has been marked as a duplicate of this bug. ***

Comment 6

[Henrik Gemal]

Perhaps most of the code could be copied from the same feature in the cookie manager?

Comment 7

[(no longer active)]

(In reply to comment #6)
> Perhaps most of the code could be copied from the same feature in the cookie
> manager?

If that's what should be done for this bug, then I'm willing to take over it.

Comment 8

[Allan Beaufour]

Be my guest :)

Comment 9

[(no longer active)]

(In reply to comment #8)
> Be my guest :)

Thanks!  Taking this over...

Comment 10

[(no longer active)]

Attachment: Add filtering support in the View Passwords dialog

The code is mostly based on the filtering code in the View Cookies dialog.  I've tested this patch against the trunk, and it's working.

Comment 13

[Atlanx]

Can you add this patch to seamonkey, too? 

Please. (^_^)/*  Thank you.

Comment 14

[(no longer active)]

(In reply to comment #13)
> Can you add this patch to seamonkey, too? 
> 
> Please. (^_^)/*  Thank you.
> 

Sure.  As soon as I get a review+ on this patch, I'll port it to SeaMonkey as well.  Do you know of any open SeaMonkey bugs regarding this, or should I open my own?

Comment 15

[Steffen Wilberg]

Seamonkey will switch to the toolkit password manager (satchel), see bug 304309, so it will get this for free.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Not a ship-blocker as per the Firefox 3 PRD, but definitely wanted and a P2 item on that list.

Comment 17

[(no longer active)]

(In reply to comment #15)
> Seamonkey will switch to the toolkit password manager (satchel), see bug
> 304309, so it will get this for free.
> 

OK, cool.  Does this need to

Comment 18

[(no longer active)]

Could we get a review on this before alpha6 freeze?  Otherwise we have to
retarget it to the next milestone.

Comment 19

[(no longer active)]

Comment on attachment 266051 [details] [diff] [review]
Add filtering support in the View Passwords dialog

I'm asking bryner for a review on this, since mconnor's review queue seems to be a long one.  If this gets a review soon, we can hope to be able to work on the dependent bugs.

I'm not sure who I should ask for ui-review, since both mconnor and beltzner seem to be busy...

Comment 20

[Myk Melez [:myk] [@mykmelez]]

I don't think bryner is active these days.  I suggest asking either mano or gavin for review.

Comment 21

[(no longer active)]

Comment on attachment 266051 [details] [diff] [review]
Add filtering support in the View Passwords dialog

(In reply to comment #20)
> I don't think bryner is active these days.  I suggest asking either mano or
> gavin for review.
> 

Done.  What about ui-review?

Comment 22

[(no longer active)]

Comment on attachment 266051 [details] [diff] [review]
Add filtering support in the View Passwords dialog

Mike promised to review the patch this week.  Thanks, Mike!  :-)

Comment 23

[Mike Connor [:mconnor]]

Comment on attachment 266051 [details] [diff] [review]
Add filtering support in the View Passwords dialog

 > var signonsTreeView = {
>+  _filtered : false,

is it really necessary to have this as a boolean, vs. just checking _filterSet.length ?  there needs to be a tiny bit of refactoring, but it seems like an extra var to keep synced without a really good reason.

>+  _filterSet : [],
>+  _lastSelectedRanges : [],
>+  _selection: null, 
>+
>   rowCount : 0,
>   setTree : function(tree) {},
>   getImageSrc : function(row,column) {},
>   getProgressMode : function(row,column) {},
>   getCellValue : function(row,column) {},
>   getCellText : function(row,column) {
>     var rv="";
>+    var signon = this._filtered ? this._filterSet[row] : signons[row];
>     if (column.id=="siteCol") {
>-      rv = signons[row].hostname;
>-      if (signons[row].httpRealm) { rv += " (" + signons[row].httpRealm + ")"; }
>+      rv = signon.hostname;
>+      if (signon.httpRealm) { rv += " (" + signon.httpRealm + ")"; }

since you're tweaking this, fix the style:

if (signon.httpRealm)
  rv += " (" + signon.httpRealm + ")";

and might as well make it a switch too, because that should be faster (seemed to help a little with redraw speed when filtering)

    var signon = this._filtered ? this._filterSet[row] : signons[row];
    switch (column.id) {
      case "siteCol":
        var rv = signon.hostname;
        if (signon.httpRealm)
          rv += " (" + signon.httpRealm + ")";

        return rv;
      case "userCol":
        return signon.username;
      case "passwordCol":
        return signon.password;
    }
  },

>+function SignonClearFilter() {
>+  // Restore selection
>+  signonsTreeView.selection.clearSelection();
>+  for (i = 0; i < signonsTreeView._lastSelectedRanges.length; ++i) {
>+    var range = signonsTreeView._lastSelectedRanges[i];
>+    signonsTreeView.selection.rangedSelect(range.min, range.max, true);
>+  }
>+  signonsTreeView._lastSelectedRanges = [];

so, this restores the selection from before one filtered, which actually seems a little wrong.  if I select a range during the filter, then clear the filter, I don't want to select the previous range, i don't think.  Are there apps that treat filters like this?

also, just use for (i in signonsTreeView._lastSelectedRanges)

>+  document.getElementById("signonsIntro").value = kSignonBundle.getString("passwordsAll");
>+  document.getElementById("clearFilter").disabled = true;
>+  document.getElementById("filter").focus();

why focus the textfield explicitly?

>+function SignonMatchesFilter(aSignon, aFilterValue) {
>+  return aSignon.hostname.indexOf( aFilterValue ) != -1 ||
>+         aSignon.username.indexOf( aFilterValue ) != -1 ||)

username is not guaranteed to be present (well, it is _now_ but won't be later).

>+         (aSignon.httpRealm ? aSignon.httpRealm.indexOf( aFilterValue ) != -1 : false) ||
>+         (showingPasswords ? aSignon.password.indexOf( aFilterValue ) != -1 : false);

This is really hard to read, and I'm sure there's a better way to compare the strings, but the real perf bottleneck is rebuilding the view anyway.

>+function FilterPasswords(aFilterValue, view) {
>+  var passwords = [];
>+  for (var i = 0; i < signons.length; ++i) {

for (var i in signons)

>+    var currSignon = signons[i];
>+    if (!currSignon)
>+      continue;

which is this true?  if there's deletions?

why not just use signons[i] directly?  you're not changing the values here

>+    if (SignonMatchesFilter(currSignon, aFilterValue))
>+      passwords.push(currSignon);
>+  }


>+function _filterPasswords()
>+{
>+  var filter = document.getElementById("filter").value;
>+  if (filter == "") {
>+    SignonClearFilter();
>+    return;
>+  }  

nit: trailing whitespace, newline after this block

>+  var view = signonsTreeView;
>+  view._filterSet = FilterPasswords(filter, view);
>+  if (!view._filtered) {
>+    // Save Display Info for the Non-Filtered mode when we first
>+    // enter Filtered mode. 
>+    SignonSaveState();
>+    view._filtered = true;
>+  }

why not just use signonsTreeView directly here?

>+  // Clear the display
>+  var oldCount = view.rowCount;
>+  view.rowCount = 0;
>+  signonsTree.treeBoxObject.rowCountChanged(0, -oldCount);
>+  // Set up the filtered display
>+  view.rowCount = view._filterSet.length;
>+  signonsTree.treeBoxObject.rowCountChanged(0, view.rowCount);

view.rowCount is just a value, you don't need to reset it to 0, just set it directly to the new value.

  // Clear the display
  signonsTree.treeBoxObject.rowCountChanged(0, -view.rowCount);

  // Set up the filtered display
  view.rowCount = view._filterSet.length;
  signonsTree.treeBoxObject.rowCountChanged(0, view.rowCount);

>+function HandleSignonFilterInput() {
>+  if (filterTimeout != -1)
>+    clearTimeout(filterTimeout);
>+
>+  filterTimeout = setTimeout("filterPasswords();", 500);
>+}

don't reinvent the wheel here, just make the textbox timed.

>+function HandleSignonFilterKeyPress(aEvent) {
>+  if (aEvent.keyCode == 27) // ESC key
>+    SignonClearFilter();
>+}

This exits the window unless you trap the event.  I'm not sure that's right at all, why is this necessary?


>-      <description control="signonsTree">&spiel.signonsstored.label;</description>
>+      <!-- filter -->
>+      <hbox align="center">
>+        <label accesskey="&filter.accesskey;" control="filter">&filter.label;</label>
>+        <textbox id="filter" flex="1" oninput="HandleSignonFilterInput();" 
>+                 onkeypress="HandleSignonFilterKeyPress(event);"/>

<textbox id="filter" flex="1" type="timed" timeout="500"
         oncommand="HandleSignonFilterInput();" 
         onkeypress="HandleSignonFilterKeyPress(event);"/>


>+        <button id="clearFilter" icon="clear" label="&clear.label;"
>+                accesskey="&clear.accesskey;" 
>+                oncommand="SignonClearFilter();" disabled="true"/>
>+      </hbox>


>+passwordsAll=Password Manager has saved login information for the following sites:
>+passwordsFiltered=The following passwords match your search:

I'm not sure at all that this switching is necessary/useful, and the label switch feels a little jarring.

this is good, but I'm going to want to see another patch rev.

Comment 24

[Dão Gottwald [:dao]]

(In reply to comment #23)
> >+  for (i = 0; i < signonsTreeView._lastSelectedRanges.length; ++i) {
> [...]
> 
> also, just use for (i in signonsTreeView._lastSelectedRanges)

That's bad practice for arrays, because it breaks if someone modifies Array.prototype (which, for the same reason, is bad practice too). It's also slower.

Comment 25

[Dão Gottwald [:dao]]

... but if you don't care about what I wrote, you can simplify it even more this way: for each (range in signonsTreeView._lastSelectedRanges)

Comment 26

[(no longer active)]

Attachment: Add filtering support in the View Passwords dialog (ver 2)

(In reply to comment #23)
> (From update of attachment 266051 [details] [diff] [review])
>  > var signonsTreeView = {
> >+  _filtered : false,
> 
> is it really necessary to have this as a boolean, vs. just checking
> _filterSet.length ?  there needs to be a tiny bit of refactoring, but it seems
> like an extra var to keep synced without a really good reason.

Right.  The new version of the patch does not include the |_filtered| var.

> >+  _filterSet : [],
> >+  _lastSelectedRanges : [],
> >+  _selection: null, 
> >+
> >   rowCount : 0,
> >   setTree : function(tree) {},
> >   getImageSrc : function(row,column) {},
> >   getProgressMode : function(row,column) {},
> >   getCellValue : function(row,column) {},
> >   getCellText : function(row,column) {
> >     var rv="";
> >+    var signon = this._filtered ? this._filterSet[row] : signons[row];
> >     if (column.id=="siteCol") {
> >-      rv = signons[row].hostname;
> >-      if (signons[row].httpRealm) { rv += " (" + signons[row].httpRealm + ")"; }
> >+      rv = signon.hostname;
> >+      if (signon.httpRealm) { rv += " (" + signon.httpRealm + ")"; }
> 
> since you're tweaking this, fix the style:
> 
> if (signon.httpRealm)
>   rv += " (" + signon.httpRealm + ")";

Done.

> and might as well make it a switch too, because that should be faster (seemed
> to help a little with redraw speed when filtering)
> 
>     var signon = this._filtered ? this._filterSet[row] : signons[row];
>     switch (column.id) {
>       case "siteCol":
>         var rv = signon.hostname;
>         if (signon.httpRealm)
>           rv += " (" + signon.httpRealm + ")";
> 
>         return rv;
>       case "userCol":
>         return signon.username;
>       case "passwordCol":
>         return signon.password;
>     }
>   },

Done.

> >+function SignonClearFilter() {
> >+  // Restore selection
> >+  signonsTreeView.selection.clearSelection();
> >+  for (i = 0; i < signonsTreeView._lastSelectedRanges.length; ++i) {
> >+    var range = signonsTreeView._lastSelectedRanges[i];
> >+    signonsTreeView.selection.rangedSelect(range.min, range.max, true);
> >+  }
> >+  signonsTreeView._lastSelectedRanges = [];
> 
> so, this restores the selection from before one filtered, which actually seems
> a little wrong.  if I select a range during the filter, then clear the filter,
> I don't want to select the previous range, i don't think.  Are there apps that
> treat filters like this?

Hmmm, this keeps the password manager filter behavior in sync with that of the cookies filter in the cookies viewer dialog.  I did this to keep the filtering behavior the same as what already exists in that dialog.  If such a similarity is not desired, I can easily drop this code.

> also, just use for (i in signonsTreeView._lastSelectedRanges)

Based on comment 24, I don't think that's a good idea.  Or maybe I'm mistaken?

> >+  document.getElementById("signonsIntro").value = kSignonBundle.getString("passwordsAll");
> >+  document.getElementById("clearFilter").disabled = true;
> >+  document.getElementById("filter").focus();
> 
> why focus the textfield explicitly?

Because this code can be executed by clicking the Clear button, and if we set focus to the textfield explicitly, the user can immediately type in a new filter string.  Note that the Clear button has become disabled, so taking the focus to another control can't be considered bad, probably.

> >+function SignonMatchesFilter(aSignon, aFilterValue) {
> >+  return aSignon.hostname.indexOf( aFilterValue ) != -1 ||
> >+         aSignon.username.indexOf( aFilterValue ) != -1 ||)
> 
> username is not guaranteed to be present (well, it is _now_ but won't be
> later).

The new patch checks for its existence.

> >+         (aSignon.httpRealm ? aSignon.httpRealm.indexOf( aFilterValue ) != -1 : false) ||
> >+         (showingPasswords ? aSignon.password.indexOf( aFilterValue ) != -1 : false);
> 
> This is really hard to read, and I'm sure there's a better way to compare the
> strings, but the real perf bottleneck is rebuilding the view anyway.

Check out the new implementation.

> >+function FilterPasswords(aFilterValue, view) {
> >+  var passwords = [];
> >+  for (var i = 0; i < signons.length; ++i) {
> 
> for (var i in signons)

Again I think that the issues in comment 24 apply here.

> >+    var currSignon = signons[i];
> >+    if (!currSignon)
> >+      continue;
> 
> which is this true?  if there's deletions?

On a second look, deletions remove the items in the |signons| array, so the above check would be redundant.

> why not just use signons[i] directly?  you're not changing the values here

Done.

> >+    if (SignonMatchesFilter(currSignon, aFilterValue))
> >+      passwords.push(currSignon);
> >+  }
> 
> 
> >+function _filterPasswords()
> >+{
> >+  var filter = document.getElementById("filter").value;
> >+  if (filter == "") {
> >+    SignonClearFilter();
> >+    return;
> >+  }  
> 
> nit: trailing whitespace, newline after this block

Done.

> >+  var view = signonsTreeView;
> >+  view._filterSet = FilterPasswords(filter, view);
> >+  if (!view._filtered) {
> >+    // Save Display Info for the Non-Filtered mode when we first
> >+    // enter Filtered mode. 
> >+    SignonSaveState();
> >+    view._filtered = true;
> >+  }
> 
> why not just use signonsTreeView directly here?

Done.

> >+  // Clear the display
> >+  var oldCount = view.rowCount;
> >+  view.rowCount = 0;
> >+  signonsTree.treeBoxObject.rowCountChanged(0, -oldCount);
> >+  // Set up the filtered display
> >+  view.rowCount = view._filterSet.length;
> >+  signonsTree.treeBoxObject.rowCountChanged(0, view.rowCount);
> 
> view.rowCount is just a value, you don't need to reset it to 0, just set it
> directly to the new value.
> 
>   // Clear the display
>   signonsTree.treeBoxObject.rowCountChanged(0, -view.rowCount);
> 
>   // Set up the filtered display
>   view.rowCount = view._filterSet.length;
>   signonsTree.treeBoxObject.rowCountChanged(0, view.rowCount);

Done.

> >+function HandleSignonFilterInput() {
> >+  if (filterTimeout != -1)
> >+    clearTimeout(filterTimeout);
> >+
> >+  filterTimeout = setTimeout("filterPasswords();", 500);
> >+}
> 
> don't reinvent the wheel here, just make the textbox timed.

Done.

> >+function HandleSignonFilterKeyPress(aEvent) {
> >+  if (aEvent.keyCode == 27) // ESC key
> >+    SignonClearFilter();
> >+}
> 
> This exits the window unless you trap the event.  I'm not sure that's right at
> all, why is this necessary?

This is also the same behavior in the cookies filter code.  I can remove it if such a compatibility is not required.

In the new patch, I have modified this to exit the window only if the filter textbox is empty.

> >-      <description control="signonsTree">&spiel.signonsstored.label;</description>
> >+      <!-- filter -->
> >+      <hbox align="center">
> >+        <label accesskey="&filter.accesskey;" control="filter">&filter.label;</label>
> >+        <textbox id="filter" flex="1" oninput="HandleSignonFilterInput();" 
> >+                 onkeypress="HandleSignonFilterKeyPress(event);"/>
> 
> <textbox id="filter" flex="1" type="timed" timeout="500"
>          oncommand="HandleSignonFilterInput();" 
>          onkeypress="HandleSignonFilterKeyPress(event);"/>

Done.  Of course the oncommand handler should invoke |window.filterPasswords()|.

> >+        <button id="clearFilter" icon="clear" label="&clear.label;"
> >+                accesskey="&clear.accesskey;" 
> >+                oncommand="SignonClearFilter();" disabled="true"/>
> >+      </hbox>
> 
> 
> >+passwordsAll=Password Manager has saved login information for the following sites:
> >+passwordsFiltered=The following passwords match your search:
> 
> I'm not sure at all that this switching is necessary/useful, and the label
> switch feels a little jarring.

Again the same behavior in the cookies filtering.

> this is good, but I'm going to want to see another patch rev.

Thanks!  Hope to get a review on the new patch soon.  :-)

Comment 27

[Mike Connor [:mconnor]]

(In reply to comment #26)

> > >+function SignonClearFilter() {
> > >+  // Restore selection
> > >+  signonsTreeView.selection.clearSelection();
> > >+  for (i = 0; i < signonsTreeView._lastSelectedRanges.length; ++i) {
> > >+    var range = signonsTreeView._lastSelectedRanges[i];
> > >+    signonsTreeView.selection.rangedSelect(range.min, range.max, true);
> > >+  }
> > >+  signonsTreeView._lastSelectedRanges = [];
> > 
> > so, this restores the selection from before one filtered, which actually seems
> > a little wrong.  if I select a range during the filter, then clear the filter,
> > I don't want to select the previous range, i don't think.  Are there apps that
> > treat filters like this?
> 
> Hmmm, this keeps the password manager filter behavior in sync with that of the
> cookies filter in the cookies viewer dialog.  I did this to keep the filtering
> behavior the same as what already exists in that dialog.  If such a similarity
> is not desired, I can easily drop this code.

Well, the cookie manager is seltype="single" so its really different.  Need to think about it a little more.

> > also, just use for (i in signonsTreeView._lastSelectedRanges)
> 
> Based on comment 24, I don't think that's a good idea.  Or maybe I'm mistaken?
> 
> > >+  document.getElementById("signonsIntro").value = kSignonBundle.getString("passwordsAll");
> > >+  document.getElementById("clearFilter").disabled = true;
> > >+  document.getElementById("filter").focus();
> > 
> > why focus the textfield explicitly?
> 
> Because this code can be executed by clicking the Clear button, and if we set
> focus to the textfield explicitly, the user can immediately type in a new
> filter string.  Note that the Clear button has become disabled, so taking the
> focus to another control can't be considered bad, probably.

focusing something that's already focused _sometimes_ gets wacky with focus, iirc.

> > >+         (aSignon.httpRealm ? aSignon.httpRealm.indexOf( aFilterValue ) != -1 : false) ||
> > >+         (showingPasswords ? aSignon.password.indexOf( aFilterValue ) != -1 : false);
> > 
> > This is really hard to read, and I'm sure there's a better way to compare the
> > strings, but the real perf bottleneck is rebuilding the view anyway.
> 
> Check out the new implementation.

ok, better, wish there was a cleaner way to do this, but strings suck.  I can't help wondering if array.filter() is at all faster, but I think the tree display stuff is the real bottleneck anyway.
 
> > >+function HandleSignonFilterKeyPress(aEvent) {
> > >+  if (aEvent.keyCode == 27) // ESC key
> > >+    SignonClearFilter();
> > >+}
> > 
> > This exits the window unless you trap the event.  I'm not sure that's right at
> > all, why is this necessary?
> 
> This is also the same behavior in the cookies filter code.  I can remove it if
> such a compatibility is not required.
> 
> In the new patch, I have modified this to exit the window only if the filter
> textbox is empty.

Well, in the cookies window, Esc doesn't seem to exit, nor should it based on the window bits (window controls, not dialog controls)

> > >+passwordsAll=Password Manager has saved login information for the following sites:
> > >+passwordsFiltered=The following passwords match your search:
> > 
> > I'm not sure at all that this switching is necessary/useful, and the label
> > switch feels a little jarring.
> 
> Again the same behavior in the cookies filtering.

Might be a little jarring because the cookie version shares the same beginning string ("The following cookies") so it isn't a full switch.

Ok, so, because the pwmgr is a <prefwindow> and the cookie manager is a <window> the behaviours differ.  I think we want to make the pwmgr be a <window> so we don't get Esc etc behaving wrongly.

Will think about the seltype thing, and then do another pass once I've figured this out in my head a bit more.

Comment 28

[(no longer active)]

(In reply to comment #27)
> > Hmmm, this keeps the password manager filter behavior in sync with that of the
> > cookies filter in the cookies viewer dialog.  I did this to keep the filtering
> > behavior the same as what already exists in that dialog.  If such a similarity
> > is not desired, I can easily drop this code.
> 
> Well, the cookie manager is seltype="single" so its really different.  Need to
> think about it a little more.

OK.

> focusing something that's already focused _sometimes_ gets wacky with focus,
> iirc.

How come?

> > Check out the new implementation.
> 
> ok, better, wish there was a cleaner way to do this, but strings suck.  I can't
> help wondering if array.filter() is at all faster, but I think the tree display
> stuff is the real bottleneck anyway.

Yeah, I think so too, but if you believe array.filter() is the better choice here, I can use it.

> > This is also the same behavior in the cookies filter code.  I can remove it if
> > such a compatibility is not required.
> > 
> > In the new patch, I have modified this to exit the window only if the filter
> > textbox is empty.
> 
> Well, in the cookies window, Esc doesn't seem to exit, nor should it based on
> the window bits (window controls, not dialog controls)

So, do you want me to disable closing the dialog via the Esc key altogether?

> > > >+passwordsAll=Password Manager has saved login information for the following sites:
> > > >+passwordsFiltered=The following passwords match your search:
> > > 
> > > I'm not sure at all that this switching is necessary/useful, and the label
> > > switch feels a little jarring.
> > 
> > Again the same behavior in the cookies filtering.
> 
> Might be a little jarring because the cookie version shares the same beginning
> string ("The following cookies") so it isn't a full switch.

I could change passwordAll to something like this: "The following passwords have been saved by the Password Manager", but I didn't want to touch the text as it appears today (without the filter option).

> Ok, so, because the pwmgr is a <prefwindow> and the cookie manager is a
> <window> the behaviours differ.  I think we want to make the pwmgr be a
> <window> so we don't get Esc etc behaving wrongly.

Hmmm, a quick look on the passwordManager.xul file shows that not much of the facilities provided by a <prefwindow> is used there, so I guess the job of converting it to a <window> wouldn't be too difficult.  If you want, I can edit the patch to make the pwmgr dialog a <window>.

> Will think about the seltype thing, and then do another pass once I've figured
> this out in my head a bit more.

OK, waiting for your comments.

Comment 29

[Mike Connor [:mconnor]]

Sorry for the delay, was away for Black Hat and the Canuckian holiday weekend.

(In reply to comment #28)
> (In reply to comment #27)
> > > Hmmm, this keeps the password manager filter behavior in sync with that of the
> > > cookies filter in the cookies viewer dialog.  I did this to keep the filtering
> > > behavior the same as what already exists in that dialog.  If such a similarity
> > > is not desired, I can easily drop this code.
> > 
> > Well, the cookie manager is seltype="single" so its really different.  Need to
> > think about it a little more.
> 
> OK.

Yeah, so let's do this in the single-selection case only for now (i.e. selection.length == 1)  Otherwise just select the top of the list.

> > focusing something that's already focused _sometimes_ gets wacky with focus,
> > iirc.
> 
> How come?

Because focus is a house of cards?
 

> > > This is also the same behavior in the cookies filter code.  I can remove it if
> > > such a compatibility is not required.
> > > 
> > > In the new patch, I have modified this to exit the window only if the filter
> > > textbox is empty.
> > 
> > Well, in the cookies window, Esc doesn't seem to exit, nor should it based on
> > the window bits (window controls, not dialog controls)
> 
> So, do you want me to disable closing the dialog via the Esc key altogether?

Yes, via changing it to be a window instead of a prefwindow, to answer your other question.

> > > > >+passwordsAll=Password Manager has saved login information for the following sites:
> > > > >+passwordsFiltered=The following passwords match your search:
> > > > 
> > > > I'm not sure at all that this switching is necessary/useful, and the label
> > > > switch feels a little jarring.
> > > 
> > > Again the same behavior in the cookies filtering.
> > 
> > Might be a little jarring because the cookie version shares the same beginning
> > string ("The following cookies") so it isn't a full switch.
> 
> I could change passwordAll to something like this: "The following passwords
> have been saved by the Password Manager", but I didn't want to touch the text
> as it appears today (without the filter option).

Yeah, I think I like that better.  Let's do that.

Can you make the changes outlined here and I'll review?

Comment 30

[Mike Connor [:mconnor]]

Comment on attachment 274260 [details] [diff] [review]
Add filtering support in the View Passwords dialog (ver 2)

clearing review until we get the patch with the requested changes

Comment 31

[Justin Dolske [:Dolske]]

Is there an updated patch forthcoming? The M8 freeze is approaching [Sept. 5], and it would probably be best to get this in by then.

Comment 32

[(no longer active)]

(In reply to comment #31)
> Is there an updated patch forthcoming? The M8 freeze is approaching [Sept. 5],
> and it would probably be best to get this in by then.
> 

Yeah, sorry for the delay.  I'll try to update the patch according to 29 and post it in the next few days.

Comment 33

[(no longer active)]

Attachment: Add filtering support in the View Passwords dialog (ver 3)

(In reply to comment #29)
> Sorry for the delay, was away for Black Hat and the Canuckian holiday weekend.

Sorry for my long delay, too.

> Yeah, so let's do this in the single-selection case only for now (i.e.
> selection.length == 1)  Otherwise just select the top of the list.

Done.

> > > focusing something that's already focused _sometimes_ gets wacky with focus,
> > > iirc.
> > 
> > How come?
> 
> Because focus is a house of cards?

OK, the new code checks to see whether the filter text box is already focused before focusing it.

> Yes, via changing it to be a window instead of a prefwindow, to answer your
> other question.

Done.

> > > > > >+passwordsAll=Password Manager has saved login information for the following sites:
> > > > > >+passwordsFiltered=The following passwords match your search:
> > > Might be a little jarring because the cookie version shares the same beginning
> > > string ("The following cookies") so it isn't a full switch.
> > 
> > I could change passwordAll to something like this: "The following passwords
> > have been saved by the Password Manager", but I didn't want to touch the text
> > as it appears today (without the filter option).
> 
> Yeah, I think I like that better.  Let's do that.

Done.

> Can you make the changes outlined here and I'll review?

Waiting for your review.  Hope it's not too late to land it for M8...

Comment 34

[Justin Dolske [:Dolske]]

Comment on attachment 279273 [details] [diff] [review]
Add filtering support in the View Passwords dialog (ver 3)

I thought I'd take a pass at a review, since I'm not sure what mconnor's queue/schedule is looking like... I only caught a few small things, some of which could be fixed post-M8.

>Index: toolkit/components/passwordmgr/content/passwordManager.js
>===================================================================
> function SignonsStartup() {
>   kSignonBundle = document.getElementById("signonBundle");
>   document.getElementById("togglePasswords").label = kSignonBundle.getString("showPasswords");
>+  window.filterPasswords = _filterPasswords;

I'm not sure I understand why you're setting window.filterPasswords() here. Any place that's calling window.filterPassword() should be able to call _filterPasswords() directly, no?

>-  getCellProperties : function(row,column,prop) {}
>+  getCellProperties : function(row,column,prop) {},
>+  get selection () { return this._selection; },
>+  set selection (val) { this._selection = val; return val; }

Why use a getter/setter to mirror _selection?

>+function SignonClearFilter() {
>+  var singleSelection = (signonsTreeView.selection.count == 1);
...

I didn't look closely at this chunk, since it seems to be be unmodified from the last patch.

>Index: toolkit/components/passwordmgr/content/passwordManager.xul
>===================================================================

>+  <keyset>
>+    <key key="&windowClose.key;" modifiers="accel" oncommand="window.close();"/>
>+    <key key="&focusSearch1.key;" modifiers="accel" oncommand="FocusFilterBox();"/>
>+    <key key="&focusSearch2.key;" modifiers="accel" oncommand="FocusFilterBox();"/>
>+  </keyset>

Mconnor, should these be platform-specific, and/or they predefined somewhere?

[Didn't look closely at the rest of this file, as the guts seem to basically be the same as the last patch.]

>+  <hbox align="end">
>+    <hbox class="actionButtons" flex="1">
>+      <spacer flex="1"/>
>+#ifndef XP_MACOSX
>+      <button oncommand="close();" icon="close"
>+              label="&closebutton.label;" accesskey="&closebutton.accesskey;"/>
>+#endif
>+    </hbox>
>+    <resizer dir="bottomright"/>
>+  </hbox>

Hmm, isn't there something like <dialog> instead of <window>, or some type/class that can be set to automagically give the window a close button and such when needed? Maybe I'm just imagining that. :)


>Index: toolkit/themes/winstripe/global/passwordmgr.css
>===================================================================
>RCS file: toolkit/themes/winstripe/global/passwordmgr.css
>diff -N toolkit/themes/winstripe/global/passwordmgr.css


>+ The Original Code is Mozilla Communicator client code, released
>+ March 31, 1998.

This, in both files, can just be "The Original Code is Login Manager code." I don't think this code snipped actually came from the 10 year old code, and in any case it's such a purely functional CSS fragment that it shouldn't where it came from.

>Index: toolkit/themes/pinstripe/global/jar.mn
>===================================================================

>++  skin/classic/global/passwordmgr.css

Huh, the Diff view in bugzilla seems to think the pinstripe jar.mn wasn't changed. I bet it's probably just confused by all the lines in the manifest starting with "+".

>Index: toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd
>===================================================================
>+<!ENTITY      windowClose.key                 "w">
>+<!ENTITY      focusSearch1.key                "f">
>+<!ENTITY      focusSearch2.key                "k">

Same concern as the <keyset> inthe window, which is using these.

Comment 35

[Justin Dolske [:Dolske]]

Belated note: can a caller open thepwmgr window with a filter string already set? Page Info could use this to allow opening a list of passwords for the current site only.

[This is probably followup bug fodder?]

Comment 36

[(no longer active)]

(In reply to comment #35)
> Belated note: can a caller open thepwmgr window with a filter string already
> set? Page Info could use this to allow opening a list of passwords for the
> current site only.
> 
> [This is probably followup bug fodder?]
> 

See bug 381692, which I reported in May.  That bug is dependent on this one.  As soon as we land this, the fix to that bug would be pretty trivial.  I'm going to take over that bug as well.

Comment 37

[(no longer active)]

(In reply to comment #34)
> (From update of attachment 279273 [details] [diff] [review])
> I thought I'd take a pass at a review, since I'm not sure what mconnor's
> queue/schedule is looking like... I only caught a few small things, some of
> which could be fixed post-M8.

I'm going to look into your comments shortly, and post a new patch if required.  Would it be OK to "steal" this from mconnor and request a review from you on the next patch, so that you can r+ it in case there are no more concerns?  I'd really like to get this landed for M8...

Comment 38

[Justin Dolske [:Dolske]]

Gavin's been reviewing all the password manager stuff recently, so I volunteer him. :-) [[e-mail]]

Comment 39

[(no longer active)]

Attachment: Add filtering support in the View Passwords dialog (ver 4)

(In reply to comment #34)
> >Index: toolkit/components/passwordmgr/content/passwordManager.js
> >===================================================================
> > function SignonsStartup() {
> >   kSignonBundle = document.getElementById("signonBundle");
> >   document.getElementById("togglePasswords").label = kSignonBundle.getString("showPasswords");
> >+  window.filterPasswords = _filterPasswords;
> 
> I'm not sure I understand why you're setting window.filterPasswords() here. Any
> place that's calling window.filterPassword() should be able to call
> _filterPasswords() directly, no?

Yeah, my mistake.  Changed the patch accordingly.

> >-  getCellProperties : function(row,column,prop) {}
> >+  getCellProperties : function(row,column,prop) {},
> >+  get selection () { return this._selection; },
> >+  set selection (val) { this._selection = val; return val; }
> 
> Why use a getter/setter to mirror _selection?

Hmmm, I had seen such code in other places of the code base, and I thought that may be considered good practice.  No particular reason other than that.  Edited the patch accordingly.

> >+function SignonClearFilter() {
> >+  var singleSelection = (signonsTreeView.selection.count == 1);
> ...
> 
> I didn't look closely at this chunk, since it seems to be be unmodified from
> the last patch.


Yes.

> >Index: toolkit/components/passwordmgr/content/passwordManager.xul
> >===================================================================
> 
> >+  <keyset>
> >+    <key key="&windowClose.key;" modifiers="accel" oncommand="window.close();"/>
> >+    <key key="&focusSearch1.key;" modifiers="accel" oncommand="FocusFilterBox();"/>
> >+    <key key="&focusSearch2.key;" modifiers="accel" oncommand="FocusFilterBox();"/>
> >+  </keyset>
> 
> Mconnor, should these be platform-specific, and/or they predefined somewhere?

This is exactly what as done in the cookies manager code.

> [Didn't look closely at the rest of this file, as the guts seem to basically be
> the same as the last patch.]

Right.

> >+  <hbox align="end">
> >+    <hbox class="actionButtons" flex="1">
> >+      <spacer flex="1"/>
> >+#ifndef XP_MACOSX
> >+      <button oncommand="close();" icon="close"
> >+              label="&closebutton.label;" accesskey="&closebutton.accesskey;"/>
> >+#endif
> >+    </hbox>
> >+    <resizer dir="bottomright"/>
> >+  </hbox>
> 
> Hmm, isn't there something like <dialog> instead of <window>, or some
> type/class that can be set to automagically give the window a close button and
> such when needed? Maybe I'm just imagining that. :)


Again, stole from the cookies manager code.

> >Index: toolkit/themes/winstripe/global/passwordmgr.css
> >===================================================================
> >RCS file: toolkit/themes/winstripe/global/passwordmgr.css
> >diff -N toolkit/themes/winstripe/global/passwordmgr.css
> 
> 
> >+ The Original Code is Mozilla Communicator client code, released
> >+ March 31, 1998.
> 
> This, in both files, can just be "The Original Code is Login Manager code." I
> don't think this code snipped actually came from the 10 year old code, and in
> any case it's such a purely functional CSS fragment that it shouldn't where it
> came from.

Done.

> >Index: toolkit/themes/pinstripe/global/jar.mn
> >===================================================================
> 
> >++  skin/classic/global/passwordmgr.css
> 
> Huh, the Diff view in bugzilla seems to think the pinstripe jar.mn wasn't
> changed. I bet it's probably just confused by all the lines in the manifest
> starting with "+".

Did you file a bug on that?  ;-)

> >Index: toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd
> >===================================================================
> >+<!ENTITY      windowClose.key                 "w">
> >+<!ENTITY      focusSearch1.key                "f">
> >+<!ENTITY      focusSearch2.key                "k">
> 
> Same concern as the <keyset> inthe window, which is using these.
>

Comment 40

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 280155 [details] [diff] [review]
Add filtering support in the View Passwords dialog (ver 4)

>Index: toolkit/components/passwordmgr/content/passwordManager.js

>+      case "userCol":
>+        return signon.username ? signon.username : "";
>+      case "passwordCol":
>+        return signon.password ? signon.password : "";

nit: prefer 
a || b;
rather than:
a ? a : b;

>-  getCellProperties : function(row,column,prop) {}
>+  getCellProperties : function(row,column,prop) {},
>  };

Why are you adding a trailing comma? That will cause a strict warning.

>+function FocusFilterBox()
>+{
>+  if (document.getElementById("filter").getAttribute("focused") != "true")
>+    document.getElementById("filter").focus();

Why bother checking whether it's already focused? There's no way for this function to be called while the user isn't interacting with the app, that I can see, so it shouldn't be necessary.

>+function SignonMatchesFilter(aSignon, aFilterValue) {
>+  if (aSignon.hostname.indexOf(aFilterValue) != -1)
>+    return true;
>+  else if (aSignon.username && aSignon.username.indexOf(aFilterValue) != -1)
>+    return true;
>+  else if (aSignon.httpRealm && aSignon.httpRealm.indexOf(aFilterValue) != -1)
>+    return true;
>+  else if (showingPasswords && aSignon.password &&
>+           aSignon.password.indexOf(aFilterValue) != -1)
>+    return true;
>+  else
>+    return false;

You could combine this into a single return statement... at least get rid of all the "else"s.

>+function FilterPasswords(aFilterValue, view) {
>+  var passwords = [];
>+  for (var i = 0; i < signons.length; ++i)
>+    if (SignonMatchesFilter(signons[i], aFilterValue))
>+      passwords.push(signons[i]);
>+  return passwords;

This can be simplified to:
return signons.filter(function (s) SignonMatchesFilter(s, aFilterValue));
or just inline it, since there seems to be only one consumer.

(see http://wiki.ecmascript.org/doku.php?id=proposals:expression_closures , which is currently implemented in SpiderMonkey)

>+function HandleSignonFilterKeyPress(aEvent) {
>+  if (aEvent.keyCode == 27) // ESC key
>+    SignonClearFilter();
>+}

I guess this is the same code that bug 377784 copied from cookies.js to applications.js. Since we're copying it again, perhaps we should consider an XBL binding, or some other way of factoring out the common code? Something for a followup bug. In the meantime, use KeyEvent.DOM_VK_ESCAPE here.

>Index: toolkit/components/passwordmgr/content/passwordManager.xul

>-<prefwindow id="SignonViewerDialog"

>+<window id="SignonViewerDialog"

Why the change to a <window>? Seems gratuitous... Keeping patches small and focused on one thing makes them much easier to review, for what it's worth.

>Index: toolkit/components/passwordmgr/content/passwordManagerCommon.js

>   observe: function(subject, topic, state) {

>     if (topic == "signonChanged") {
>       if (state == "signons") {

>+        // apply the filter if needed
>+        if (document.getElementById("filter").value != "") {

I was going to point out that this file is shared, and there won't be a "filter" element in passwordManagerExceptions.xul, so this would throw, but turns out this code is never actually called. Filed bug 395681.

>Index: toolkit/themes/pinstripe/global/passwordmgr.css
>Index: toolkit/themes/winstripe/global/passwordmgr.css

>+@import url("chrome://global/skin/");

This extra indirection doesn't seem worth it, just leave the reference in the XUL file?

>Index: toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd

>-<!ENTITY      spiel.signonsstored.label       "Password Manager has saved login information for the following sites:">
>+<!ENTITY      spiel.signonsstored.label       "The following passwords have been saved by the Password Manager:">

Is there a reason for this change? If it's all the same, I'd prefer leaving nonfunctional changes like this to a separate patch, unless you want to wait on UI review.

Comment 41

[(no longer active)]

Attachment: Add filtering support in the View Passwords dialog (ver 5)

(In reply to comment #40)
> (From update of attachment 280155 [details] [diff] [review])
> >Index: toolkit/components/passwordmgr/content/passwordManager.js
> 
> >+      case "userCol":
> >+        return signon.username ? signon.username : "";
> >+      case "passwordCol":
> >+        return signon.password ? signon.password : "";
> 
> nit: prefer 
> a || b;
> rather than:
> a ? a : b;

Done.

> >-  getCellProperties : function(row,column,prop) {}
> >+  getCellProperties : function(row,column,prop) {},
> >  };
> 
> Why are you adding a trailing comma? That will cause a strict warning.

My mistake.  Fixed.

> >+function FocusFilterBox()
> >+{
> >+  if (document.getElementById("filter").getAttribute("focused") != "true")
> >+    document.getElementById("filter").focus();
> 
> Why bother checking whether it's already focused? There's no way for this
> function to be called while the user isn't interacting with the app, that I can
> see, so it shouldn't be necessary.

This is an issue that mconnor rose in comment 27...

> >+function SignonMatchesFilter(aSignon, aFilterValue) {
> >+  if (aSignon.hostname.indexOf(aFilterValue) != -1)
> >+    return true;
> >+  else if (aSignon.username && aSignon.username.indexOf(aFilterValue) != -1)
> >+    return true;
> >+  else if (aSignon.httpRealm && aSignon.httpRealm.indexOf(aFilterValue) != -1)
> >+    return true;
> >+  else if (showingPasswords && aSignon.password &&
> >+           aSignon.password.indexOf(aFilterValue) != -1)
> >+    return true;
> >+  else
> >+    return false;
> 
> You could combine this into a single return statement... at least get rid of
> all the "else"s.

Originally in the patch, there was a single return statement (see attachment 266051 [details] [diff] [review]), and mconnor asked me to make the code more readable (see comment 23).  Anyway, I got rid of the else's.

> >+function FilterPasswords(aFilterValue, view) {
> >+  var passwords = [];
> >+  for (var i = 0; i < signons.length; ++i)
> >+    if (SignonMatchesFilter(signons[i], aFilterValue))
> >+      passwords.push(signons[i]);
> >+  return passwords;
> 
> This can be simplified to:
> return signons.filter(function (s) SignonMatchesFilter(s, aFilterValue));
> or just inline it, since there seems to be only one consumer.
> 
> (see http://wiki.ecmascript.org/doku.php?id=proposals:expression_closures ,
> which is currently implemented in SpiderMonkey)

I think inlining it hurts readability, so I just used your Array.filter() suggestion.

> >+function HandleSignonFilterKeyPress(aEvent) {
> >+  if (aEvent.keyCode == 27) // ESC key
> >+    SignonClearFilter();
> >+}
> 
> I guess this is the same code that bug 377784 copied from cookies.js to
> applications.js. Since we're copying it again, perhaps we should consider an
> XBL binding, or some other way of factoring out the common code? Something for
> a followup bug. In the meantime, use KeyEvent.DOM_VK_ESCAPE here.

Done.

> >Index: toolkit/components/passwordmgr/content/passwordManager.xul
> 
> >-<prefwindow id="SignonViewerDialog"
> 
> >+<window id="SignonViewerDialog"
> 
> Why the change to a <window>? Seems gratuitous... Keeping patches small and
> focused on one thing makes them much easier to review, for what it's worth.

This is what mconnor had requested in his review in comment 29.

> >Index: toolkit/components/passwordmgr/content/passwordManagerCommon.js
> 
> >   observe: function(subject, topic, state) {
> 
> >     if (topic == "signonChanged") {
> >       if (state == "signons") {
> 
> >+        // apply the filter if needed
> >+        if (document.getElementById("filter").value != "") {
> 
> I was going to point out that this file is shared, and there won't be a
> "filter" element in passwordManagerExceptions.xul, so this would throw, but
> turns out this code is never actually called. Filed bug 395681.

I added the check for the filter element's existence just in case that this code survives in the tree after bug 395681 is resolved.

> >Index: toolkit/themes/pinstripe/global/passwordmgr.css
> >Index: toolkit/themes/winstripe/global/passwordmgr.css
> 
> >+@import url("chrome://global/skin/");
> 
> This extra indirection doesn't seem worth it, just leave the reference in the
> XUL file?

Done.

> >Index: toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd
> 
> >-<!ENTITY      spiel.signonsstored.label       "Password Manager has saved login information for the following sites:">
> >+<!ENTITY      spiel.signonsstored.label       "The following passwords have been saved by the Password Manager:">
> 
> Is there a reason for this change? If it's all the same, I'd prefer leaving
> nonfunctional changes like this to a separate patch, unless you want to wait on
> UI review.

This is also a request from mconnor, but since I don't want a ui-review to delay this work, I'm dropping it.  We can work on it in a followup bug if necessary.

Comment 42

[(no longer active)]

Hopefully we'll land this in M9.

Comment 43

[(no longer active)]

Comment on attachment 280640 [details] [diff] [review]
Add filtering support in the View Passwords dialog (ver 5)

Waited for a review from gavin for about one month...

Re-requesting review from mconnor.

Comment 44

[Mike Connor [:mconnor]]

Comment on attachment 280640 [details] [diff] [review]
Add filtering support in the View Passwords dialog (ver 5)

this looks good now, we should get it in ASAP.

Comment 45

[(no longer active)]

Marking this for checkin...

Comment 46

[Reed Loden [:reed]]

Checking in toolkit/components/passwordmgr/content/passwordManager.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/content/passwordManager.js,v  <--  passwordManager.js
new revision: 1.18; previous revision: 1.17
done
Checking in toolkit/components/passwordmgr/content/passwordManager.xul;
/cvsroot/mozilla/toolkit/components/passwordmgr/content/passwordManager.xul,v  <--  passwordManager.xul
new revision: 1.15; previous revision: 1.14
done
Checking in toolkit/components/passwordmgr/content/passwordManagerCommon.js;
/cvsroot/mozilla/toolkit/components/passwordmgr/content/passwordManagerCommon.js,v  <--  passwordManagerCommon.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordManager.dtd,v  <--  passwordManager.dtd
new revision: 1.6; previous revision: 1.5
done
Checking in toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties;
/cvsroot/mozilla/toolkit/locales/en-US/chrome/passwordmgr/passwordmgr.properties,v  <--  passwordmgr.properties
new revision: 1.12; previous revision: 1.11
done
Checking in toolkit/themes/pinstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.35; previous revision: 1.34
done
Checking in toolkit/themes/winstripe/global/jar.mn;
/cvsroot/mozilla/toolkit/themes/winstripe/global/jar.mn,v  <--  jar.mn
new revision: 1.37; previous revision: 1.36
done
RCS file: /cvsroot/mozilla/toolkit/themes/pinstripe/global/passwordmgr.css,v
done
Checking in toolkit/themes/pinstripe/global/passwordmgr.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/global/passwordmgr.css,v  <--  passwordmgr.css
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/themes/winstripe/global/passwordmgr.css,v
done
Checking in toolkit/themes/winstripe/global/passwordmgr.css;
/cvsroot/mozilla/toolkit/themes/winstripe/global/passwordmgr.css,v  <--  passwordmgr.css
initial revision: 1.1
done

Comment 47

[Marcia Knous [:marcia]]

Litmus triage team: tomcat will handle this test case in Litmus.

Comment 48

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=5055 has been added to cover this scenario.

Comment 49

[(no longer active)]

(In reply to comment #48)
> https://litmus.mozilla.org/show_test.cgi?id=5055 has been added to cover this
> scenario.

This looks good, but it's not complete.  It needs to cover the following cases as well:

a) when something is entered in the filter box, and the Show Passwords button is pressed, the entries which have the entered search string only in the password field should be added to the search results.

b) after case (a), when the Hide Passwords button is pressed, the items added in case (a) should be removed, and the previous search results list should be displayed again, as if the Show Passwords button had never been pressed.

Comment 50

[Marcia Knous [:marcia]]

Ehsan: If the functionality of the Search Passwords is the same in Page Info as it is in regular Password preferences, then I think scenarios (a) and (b) should be in those test cases. I though the new one that I added was more to verify that the functionality works in Page Info when you click the button. If it is OK with you I will verify that (a) and (b) are part of the regular Password Manager test cases that involve searching.

(In reply to comment #49)
> (In reply to comment #48)
> > https://litmus.mozilla.org/show_test.cgi?id=5055 has been added to cover this
> > scenario.
> 
> This looks good, but it's not complete.  It needs to cover the following cases
> as well:
> 
> a) when something is entered in the filter box, and the Show Passwords button
> is pressed, the entries which have the entered search string only in the
> password field should be added to the search results.
> 
> b) after case (a), when the Hide Passwords button is pressed, the items added
> in case (a) should be removed, and the previous search results list should be
> displayed again, as if the Show Passwords button had never been pressed.
> 

Comment 51

[(no longer active)]

(In reply to comment #50)
> Ehsan: If the functionality of the Search Passwords is the same in Page Info as
> it is in regular Password preferences, then I think scenarios (a) and (b)
> should be in those test cases. I though the new one that I added was more to
> verify that the functionality works in Page Info when you click the button. If
> it is OK with you I will verify that (a) and (b) are part of the regular
> Password Manager test cases that involve searching.

The functionality of the Search field is the same in Page Info as it is in the Password preferences.  I think the test case for this bug should only limit itself to Password Manager test cases, and include the (a) and (b) above.  The test case for bug 381692 however should include the Page Info related test cases, which is just limited to verifying that the Passwords dialog opens with the search string preset to the domain name of the site being visited, and the passwords lists is filtered accordingly.

Note that I've set the in-litmus? flag on that bug as well...

Comment 52

[[e-mail]]

Comment on attachment 280640 [details] [diff] [review]
Add filtering support in the View Passwords dialog (ver 5)

>+    for (i = 0; i < signonsTreeView._lastSelectedRanges.length; ++i) {
JavaScript strict warning: assignment to undeclared variable i

There's also an unexpected row count changed somewhere, but I don't have layout symbols here.

Comment 53

[[e-mail]]

Comment on attachment 280640 [details] [diff] [review]
Add filtering support in the View Passwords dialog (ver 5)

>+  // Clear the Filter and the Tree Display
>+  document.getElementById("filter").value = "";
>+  signonsTreeView.rowCount = 0;
>+  signonsTree.treeBoxObject.rowCountChanged(0, -signonsTreeView._filterSet.length);

>+  // Clear the display
>+  signonsTree.treeBoxObject.rowCountChanged(0, -signonsTreeView.rowCount);
You forgot to clear the row count this time (this asserts in debug builds, even though you and I know that you're going to adjust the row count again).

Comment 54

[(no longer active)]

I'll be posting a patch to fix the issues in comment 52 and 53 soon.  Thanks Neil for tracking these down.

Comment 55

[(no longer active)]

Attachment: Further fixes

Simple fixes to correct the problems described in comment 52 and comment 53.

Comment 56

[(no longer active)]

Comment on attachment 301555 [details] [diff] [review]
Further fixes

Mike, maybe you can take a look at this patch?  It's relatively simple to review....

Comment 57

[Justin Dolske [:Dolske]]

It would be better to do any "further fixes" in new bugs -- this bug was closed back in October 2007.

Comment 58

[Mike Connor [:mconnor]]

Ehsan, please file a new bug for this patch, thanks!  I'll be happy to review when you do that.

Comment 59

[tech]

(In reply to comment #15)
> Seamonkey will switch to the toolkit password manager (satchel), see bug
> 304309, so it will get this for free.
> 

When will this be done ?

Comment 60

[Steffen Wilberg]

(In reply to comment #59)
> (In reply to comment #15)
> > Seamonkey will switch to the toolkit password manager (satchel), see bug
> > 304309, so it will get this for free.
> > 
> When will this be done ?
Bug 304309 was just about the back-end. The front-end work is bug 390025.

Comment 61

[(no longer active)]

Comment on attachment 301555 [details] [diff] [review]
Further fixes

Filed bug 451352, and attached an hg version of this patch there (attachment 334656 [details] [diff] [review]).

Comment 62

[(no longer active)]

Now that I'm more familiar with automated tests, I think we can get a browser chrome test for this bug, right?  The only thing I'm not sure about is whether it's OK for the test of this bug to be a browser chrome test, since it's a toolkit bug.  We could get the test in browser/, but in that case other apps using the toolkit won't benefit from the automated test...

Comment 63

[(no longer active)]

Tests landed on mozilla-central as part of <http://hg.mozilla.org/mozilla-central/rev/347a0db4df44>.

Comment 64

[mcdavis941 (sporadically reading bugmail)]

https://hg.mozilla.org/mozilla-central/rev/172599f782f7 (for the historical record)