Closed Bug 566910 Opened 10 years ago Closed 10 years ago

'copy password' context menu item for password manager

Categories

(Toolkit :: Password Manager, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: fryn, Assigned: fryn)

References

Details

Attachments

(1 file, 7 obsolete files)

Provide a context menu item for users to copy a signon's password to the clipboard without needing to show all passwords.
Attachment #446257 - Flags: review?(dolske)
The messy try/catch clause in CopyPassword() will be removed once UpdateCopyPassword() actually does something.

XUL tree seletions *headdesk*
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.
Attachment #446257 - Flags: review?(dolske) → feedback+
(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.
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.
Attachment #446534 - Flags: review?(dolske)
Attachment #446257 - Attachment is obsolete: true
Attached patch patch v3 (the real v2) (obsolete) — Splinter Review
uploaded wrong file for v2 :\
Attachment #446534 - Attachment is obsolete: true
Attachment #446538 - Flags: review?(dolske)
Attachment #446534 - Flags: review?(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.
Attachment #446538 - Flags: review?(dolske) → review+
Attachment #446538 - Attachment is obsolete: true
Attachment #446672 - Flags: review?(dolske)
Attachment #446672 - Attachment is obsolete: true
Attachment #446750 - Flags: review?(dolske)
Attachment #446672 - Flags: review?(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 <ehsan.akhgari@gmail.com> (Original Author)
>+ *   Frank Yan <fyan@mozilla.com>


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.
Attachment #446750 - Flags: review?(dolske) → review+
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
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.
(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 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.
Attachment #446750 - Flags: review-
Attachment #446750 - Attachment is obsolete: true
Attachment #447118 - Flags: review?(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
Attachment #447118 - Flags: review?(dolske) → review-
Attached patch patch v6 (obsolete) — Splinter Review
now preloads the clipboard with manatees to make sure that the actual clipboard check works properly
Attachment #447118 - Attachment is obsolete: true
Attachment #447190 - Flags: review?(dolske)
Attachment #447190 - Attachment is obsolete: true
Attachment #447229 - Flags: review?(dolske)
Attachment #447190 - Flags: review?(dolske)
Comment on attachment 447229 [details] [diff] [review]
patch v7 (refactored)

\o/
Attachment #447229 - Flags: review?(dolske) → review+
Pushed http://hg.mozilla.org/mozilla-central/rev/d57d62799e35
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
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.
(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
(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.
<popup> is obsolete and shouldn't be used anymore, see <https://developer.mozilla.org/en/XUL/popup>.
Blocks: 568181
Blocks: 568287
Depends on: 568351
Ah. We just stole what commonDialog.xul was doing (re: <popup>).
Duplicate of this bug: 242419
Blocks: 571997
No longer blocks: 571997
Depends on: 571997
Depends on: 600881
No longer depends on: 600881
You need to log in before you can comment on or make changes to this bug.