566910 - 'copy password' context menu item for password manager

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Frank Yan (:fryn)]

Attachment: patch v1 (needs help with detecting single selection)

Provide a context menu item for users to copy a signon's password to the clipboard without needing to show all passwords.

Comment 1

[Frank Yan (:fryn)]

The messy try/catch clause in CopyPassword() will be removed once UpdateCopyPassword() actually does something.

XUL tree seletions *headdesk*

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 446257 [details] [diff] [review]
patch v1 (needs help with detecting single selection)

>+function UpdateCopyPassword() {
>+  // Enable menu item if and only if exactly one item is selected
>+  var singleSelection = (signonsTreeView.selection.getRangeCount() == 1);

I think all this function needs to know is if signonsTreeView.selection.count == 1?

>+<!ENTITY      copyPasswordCmd.label           "Copy Password">
>+<!ENTITY      copyPasswordCmd.accesskey       "c">

Should be "C" instead of "c", although I'm not sure if that actually matters.

Otherwise looks on the right track, other than a few style nits I'll point out.

Comment 3

[Frank Yan (:fryn)]

(In reply to comment #2)
> (From update of attachment 446257 [details] [diff] [review])
> >+function UpdateCopyPassword() {
> >+  // Enable menu item if and only if exactly one item is selected
> >+  var singleSelection = (signonsTreeView.selection.getRangeCount() == 1);
> 
> I think all this function needs to know is if signonsTreeView.selection.count
> == 1?

I tried that. Didn't seem to work. I could try it again. Perhaps I skipped a step somewhere so I didn't actually test a build with that line.

Comment 4

[Frank Yan (:fryn)]

Attachment: patch v2 (fixed the single selection detection)

for those who are curious: it turned out to be an issue with the command attribute linking the menuitem's disabled state to the command.

Comment 5

[Frank Yan (:fryn)]

Attachment: patch v3 (the real v2)

uploaded wrong file for v2 :\

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 446538 [details] [diff] [review]
patch v3 (the real v2)

r+, but let's try writing a simple browser test for this too.

Comment 7

[Frank Yan (:fryn)]

Attachment: patch v4 (now with NaN% more tests)

Comment 8

[Frank Yan (:fryn)]

Attachment: patch v4.1 (now with NaN% more tests and 100% less typos)

Comment 9

[Justin Dolske [:Dolske]]

Comment on attachment 446750 [details] [diff] [review]
patch v4.1 (now with NaN% more tests and 100% less typos)

>--- a/toolkit/components/passwordmgr/test/browser/browser_passwordmgr_sort.js
...
>  * the License. You may obtain a copy of the License at
>  * http://www.mozilla.org/MPL/
>- * 
>+ *
>  * Software distributed under the License is distributed on an "AS IS" basis,
>  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License

As a general rule patches shouldn't included unrelated whitespace changes (and this file wasn't otherwise modified). Check your qdiff output before attaching.

>+++ b/toolkit/components/passwordmgr/test/browser/browser_passwordmgrcopypwd.js
...
>+ * The Initial Developer of the Original Code is
>+ * Ehsan Akhgari.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Ehsan Akhgari <[e-mail]> (Original Author)
>+ *   Frank Yan <[e-mail]>


The initial developer should be "the Mozilla Foundation". You can remove ehsan, since this is a new file that basically gutted everything.

>+	let str = new Object();
>+	let strLength = new Object();

var foo = {} is preferred over "new Object()".

>+	trans.getTransferData("text/unicode", str, strLength);
>+	if (str)
>+	    str = str.value.QueryInterface(Ci.nsISupportsString);

str will always be true here.

>+	if (str)
>+	    str = str.data.substring(0, strLength.value / 2);

Err, what? This shouldn't be needed, but maybe some of the clipboard code is dumb?

>+	is(str, view.getCellText(2, pwdCol), "Copy password to clipboard");

The reference here should be logins[2].password, to ensure that the expected value isn't going to change should the <tree> have a weird bug in the future or something.

So, I'd just make this chunk something like:

trans.getTransferData("text/unicode", str, {});
ok(str.value instanceof Ci.nsISupportsString, "should be a nsISupportsString);
is(str.value.data, logins[2].password, "check expected pass");


>--- a/toolkit/components/passwordmgr/test/browser/browser_passwordmgrdlg.js
...
>  * the License. You may obtain a copy of the License at
>  * http://www.mozilla.org/MPL/
>- * 
>+ *
>  * Software distributed under the License is distributed on an "AS IS" basis,
>  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License

Ditto regarding whitespace here.

r+ with these nits fixed, pending tryserver results.

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 446750 [details] [diff] [review]
patch v4.1 (now with NaN% more tests and 100% less typos)

>+++ b/toolkit/components/passwordmgr/test/browser/browser_passwordmgrcopypwd.js
>+ * Portions created by the Initial Developer are Copyright (C) 2009

With Dolske's other change, this can be 2010 now.

>+    function doTest() {
>+        let doc = pwmgrdlg.document;
>+        let tree = doc.getElementById("signonsTree");
>+	let box = tree.treeBoxObject;
>+        let view = box.view;

Looks like you're mixing tabs & spaces - please use only spaces.

>+	trans.addDataFlavor("text/unicode");
>+	clip.getData(trans, clip.kGlobalClipboard);
>+	trans.getTransferData("text/unicode", str, strLength);

Pretty sure this can throw, at least in the case where this test would fail (no text/unicode on the clipboard)

>+	if (str)
>+	    str = str.value.QueryInterface(Ci.nsISupportsString);
>+	if (str)
>+	    str = str.data.substring(0, strLength.value / 2);

I think Dolske's right that you don't need to do this, at least not when you use copyString. If you were putting the unicode on there yourself, you'd need to multiply by 2. At least that's how I've done it in the past... I could be wrong. I see that you took that snippet from https://developer.mozilla.org/en/Using_the_Clipboard...

I've just done
> str = str.value.QueryInterface(Ci.nsISupportsString).data

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Oh one more thing... If gavin were reviewing this it would be r- for the test

Apparently the clipboard can be async on some platforms, so tests like this have led to failures. I just had to rewrite my tests in bug 556061 to use a polling pattern. So this test really should do something similar.

Take a look at browser_bug556061.js, browser_410196_paste_into_tags.js, and browser_bug321000.js for some examples.

Comment 12

[Justin Dolske [:Dolske]]

(In reply to comment #10)

> Pretty sure this can throw, at least in the case where this test would fail (no
> text/unicode on the clipboard)

Sure, but then the test would fail as expected, no? [Assuming the async issue is fixed.]

> >+	if (str)
> >+	    str = str.value.QueryInterface(Ci.nsISupportsString);
> >+	if (str)
> >+	    str = str.data.substring(0, strLength.value / 2);
> 
> I think Dolske's right that you don't need to do this, at least not when you
> use copyString.

Interestingly it's in the DevMo article (end of https://developer.mozilla.org/en/Using_the_Clipboard), Frank's checking to see why the test is passing as-is.


(In reply to comment #11)
> Oh one more thing... If gavin were reviewing this it would be r- for the test

Feel free to mark it, you know you want to. :) Do eet!

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Comment on attachment 446750 [details] [diff] [review]
patch v4.1 (now with NaN% more tests and 100% less typos)

(In reply to comment #12)
> Feel free to mark it, you know you want to. :) Do eet!

Bam! r- for the sync stuff.

Comment 14

[Frank Yan (:fryn)]

Attachment: patch v5 (now with NaN% more asynchronicity)

Comment 15

[Justin Dolske [:Dolske]]

Comment on attachment 447118 [details] [diff] [review]
patch v5 (now with NaN% more asynchronicity)

>+    // the meat of the test
>+    function doTest() {
>+        let doc = pwmgrdlg.document;
>+        let clip = Cc["@mozilla.org/widget/clipboard;1"].getService(Ci.nsIClipboard);
>+    let prevData = "";

Nit: indentation is off.


>+            if (data != prevData) {
>+                is(data, "coded", "Password should match clipboard contents");
>+                cleanUp();
>+            }

So, this won't work if the clipboard had anything else on it previously. [prevData is never assigned to, so this could just be |data != ""|.]

You'll either need to call nsIClipboard.emptyClipboard() -- and, presumably, poll it to ensure it's empty -- or perhaps just initialize prevData from the clipboard (before triggering Copy Password) and avoid the need for an extra poll.

It's a good thing zpao brought this up, because I think all his tests are broken like this too. :-P

Comment 16

[Frank Yan (:fryn)]

Attachment: patch v6

now preloads the clipboard with manatees to make sure that the actual clipboard check works properly

Comment 17

[Frank Yan (:fryn)]

Attachment: patch v7 (refactored)

Comment 18

[Justin Dolske [:Dolske]]

Comment on attachment 447229 [details] [diff] [review]
patch v7 (refactored)

\o/

Comment 19

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/d57d62799e35

Comment 20

[(no longer active)]

Thanks a lot, Frank, for this patch.  This will be one of my favorite patches of all times.  :)

Two comments on the patch, though.  Why didn't we use a menupopup element instead of a popup element?  Also, the test should probably test the case where Copy Password might be disabled, for example, with no item or multiple items selected.

Comment 21

[Frank Yan (:fryn)]

(In reply to comment #20)
> Thanks a lot, Frank, for this patch.  This will be one of my favorite patches
> of all times.  :)

The test also features your name. :)

Why not menupopup? No reason. What are the advantages of menupopup compared to popup?
If it's really better to use a menupopup, I can put that together in a 2-line patch.
Justin had first suggested fixing this bug as practice, so we kept the test simple.
It has already taken 4 days to get through when what was initially a patch hacked together in 5 minutes. XUL <commands /> and Clipboard stuff OTL

Comment 22

[(no longer active)]

(In reply to comment #21)
> (In reply to comment #20)
> > Thanks a lot, Frank, for this patch.  This will be one of my favorite patches
> > of all times.  :)
> 
> The test also features your name. :)

Heh, yes!

> Why not menupopup? No reason. What are the advantages of menupopup compared to
> popup?

I _think_ you'd get native menu styling if you use menupopup, but I'm not 100% sure.  Maybe Dao knows?

> If it's really better to use a menupopup, I can put that together in a 2-line
> patch.

That can also be done in a follow-up bug.

> Justin had first suggested fixing this bug as practice, so we kept the test
> simple.

To be fair, the test isn't that simple! ;-)

> It has already taken 4 days to get through when what was initially a patch
> hacked together in 5 minutes. XUL <commands /> and Clipboard stuff OTL

Welcome to the world of Mozilla hacking!  But you'll get much quicker in no time.  Testing the menu item update status doesn't take a lot of effort, you basically should play with the selected items <http://hg.mozilla.org/mozilla-central/rev/d57d62799e35#l4.91>, and then call UpdateCopyPassword and check the disabled attribute on the menu item.  Of course, you can get even fancier by actually triggering a right-click event so that the menu actually shows up, etc.  (I'll leave the choice to you and dolske.)

Again, this can also be done in a follow-up bug, but without that test, the changes landed as part of this bug are not fully tested yet.

Comment 23

[Dão Gottwald [:dao]]

<popup> is obsolete and shouldn't be used anymore, see <https://developer.mozilla.org/en/XUL/popup>.

Comment 24

[Justin Dolske [:Dolske]]

Ah. We just stole what commonDialog.xul was doing (re: <popup>).