451352 - Further fixes to pwmgr search filtering UI

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[(no longer active)]

Attachment: Patch

From bug 327048 comment 52:

(From update of attachment 280640 [details] [diff] [review])
>+    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.

---

From bug 327048 comment 53:

(From update of attachment 280640 [details] [diff] [review])
>+  // 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).

The attached patch fixes these issues.

Comment 1

[Mike Connor [:mconnor]]

Comment on attachment 334656 [details] [diff] [review]
Patch

dolske would have been faster here.  and slipped a pun in somewhere.

still, seems very obvious and low risk

Comment 2

[[e-mail]]

Comment on attachment 334656 [details] [diff] [review]
Patch

>+  signonsTreeView.rowCount = 0;
>   signonsTree.treeBoxObject.rowCountChanged(0, -signonsTreeView.rowCount);
I don't think this is right...

Comment 3

[(no longer active)]

(In reply to comment #2)
> (From update of attachment 334656 [details] [diff] [review])
> >+  signonsTreeView.rowCount = 0;
> >   signonsTree.treeBoxObject.rowCountChanged(0, -signonsTreeView.rowCount);
> I don't think this is right...

How come?

Comment 4

[(no longer active)]

Attachment: Patch

OK, call me stupid on the previous patch, I deserve it!  ;-)

Comment 5

[[e-mail]]

Comment on attachment 358573 [details] [diff] [review]
Patch

>+  let oldRowCound = signonsTreeView.rowCount;
I call stupid typo ;-)

Comment 6

[(no longer active)]

Attachment: Non-stupid patch

(In reply to comment #5)
> (From update of attachment 358573 [details] [diff] [review])
> >+  let oldRowCound = signonsTreeView.rowCount;
> I call stupid typo ;-)

OK, seriously, what is wrong with me here?! ;-)

/me plays with the idea of WONTFIXing this bug and terminating his bugzilla account...

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 358641 [details] [diff] [review]
Non-stupid patch

I should really r- for lack of tests, but instead I'll just grumble a little and let this fix for year-old nits slide.

Comment 8

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8dd2e82503cd

Comment 9

[Justin Dolske [:Dolske]]

Backed out: http://hg.mozilla.org/mozilla-central/rev/6efc982bb051

This broke the password manger UI. Opening it just gave an empty window with a blank button, and the following errors:

JavaScript error: chrome://passwordmgr/content/passwordManager.js, line 215: missing ; after for-loop initializer
JavaScript error: chrome://passwordmgr/content/passwordManager.xul, line 1: SignonsStartup is not defined

WTF, did you not test this?

Comment 10

[Nochum Sossonko [:Natch]]

Does |let| not work in the passwordManager code?

Comment 11

[[e-mail]]

No, because the XUL script tag fails to specify type="application/javascript".

Comment 12

[(no longer active)]

Attachment: New patch

I'm terribly sorry about the problem with the previous patch -- I blindly assumed that if var works, then let should too. :(

In order to make myself look a bit better, this patch includes a full unit test for the password manager dialog's filtering functionality.  This was in my to-do list long enough, so here it is now!

Comment 13

[Dão Gottwald [:dao]]

How about adding type="application/javascript" to prevent future trouble? Then you can also use let now, which seems appropriate in this case.

Comment 14

[(no longer active)]

Attachment: New patch

(In reply to comment #13)
> How about adding type="application/javascript" to prevent future trouble? Then
> you can also use let now, which seems appropriate in this case.

Sure, why not.

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 359992 [details] [diff] [review]
New patch

>--- a/toolkit/components/passwordmgr/content/passwordManager.js
...

Can you update this patch; it doesn't apply cleanly now.

Sorry for the late review, I was originally waiting until Beta 3 shipped, and then is slipped, and then this slipped my mind.

>+++ b/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js

ZOMG! Tests! Yay! \o/

>+      let win = doc.defaultView;//.window;

Space before comment.

Also, all the other pwmgr tests and code (except for the bits in content/) are using 4-space indent. This test should match, please.

>+      function setFilter(string) {
>+        filter.value = string;
>+        win._filterPasswords();
>+      }

Can you avoid calling _filterPasswords() by having the test simulate pressing enter in the field (or something like that)?

>+      // Prepare a set of tests
>+      let tests = [
>+        {filter: "pass", count: 0, count2: 4},
...

Add a brief comment here explaining the difference between count/count2.

>+      // Show passwords
>+      win.ConfirmShowPasswords = function() true;
>+      win.TogglePasswordVisible();

As with _filterPasswords(), it would be better to have test click the button through the DOM instead of directly calling TogglePasswordVisible().

Also, it's not good to have the test changing ConfirmShowPasswords() [even though that makes life easier. :)]. Have the test set a timer, click the button, and have the timer callback handle clicking "Yes" in the confirmation dialog, and then return to the test. This is similar to what test_prompt.html does (see prompt_common.js).

Comment 16

[(no longer active)]

Attachment: New patch + revised test

(In reply to comment #15)
> Space before comment.

Actually the comment was left there unintentionally.  :-)

> Also, all the other pwmgr tests and code (except for the bits in content/) are
> using 4-space indent. This test should match, please.

Done.

> >+      function setFilter(string) {
> >+        filter.value = string;
> >+        win._filterPasswords();
> >+      }
> 
> Can you avoid calling _filterPasswords() by having the test simulate pressing
> enter in the field (or something like that)?

Done.  The test now simulates sending an enter key on the filter text field as you suggested.

> >+      // Prepare a set of tests
> >+      let tests = [
> >+        {filter: "pass", count: 0, count2: 4},
> ...
> 
> Add a brief comment here explaining the difference between count/count2.

Done.

> >+      // Show passwords
> >+      win.ConfirmShowPasswords = function() true;
> >+      win.TogglePasswordVisible();
> 
> As with _filterPasswords(), it would be better to have test click the button
> through the DOM instead of directly calling TogglePasswordVisible().

Done.

> Also, it's not good to have the test changing ConfirmShowPasswords() [even
> though that makes life easier. :)]. Have the test set a timer, click the
> button, and have the timer callback handle clicking "Yes" in the confirmation
> dialog, and then return to the test. This is similar to what test_prompt.html
> does (see prompt_common.js).

Actually I preferred a direct approach instead of relying on a timer: I catch downwindowopened, hit Enter, and then catch domwindowclosed and proceed with the test.

Comment 17

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/347a0db4df44

Comment 18

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

Comment on attachment 369649 [details] [diff] [review]
New patch + revised test

a191=beltzner

Comment 19

[Shawn Wilsher :sdwilsh]

This bug appears to have introduced a new random test.  See bug 491898 for more details.

Comment 20

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8fffd3cce232

Comment 21

[[e-mail]]

Hey, can we avoid DOS line endings in the tree? Thanks.

Comment 22

[Tony Chung [:tchung]]

With a bunch of litmus tests passing on 191branch and testcase include in the patch, im marking this bug fixed.

Litmus test results: http://tinyurl.com/nvyv24

Comment 23

[(no longer active)]

(In reply to comment #21)
> Hey, can we avoid DOS line endings in the tree? Thanks.

Oops, sorry about that.  Filed bug 496688.