491311 - Simplify "Add to recent documents" code in nsDownload::SetState

Status: RESOLVED FIXED

(Toolkit :: Downloads API, enhancement)

Description

[Aiko]

Attachment: use SHARD_PATHW

(follow up to bug 310071)

The code to add a download to the recent documents list got more complicated then it needs to be because MSDN's documentation of SHAddToRecentDocs sucks a bit. This patch should achieve exactly the same.

Comment 1

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

Looks good, but could you modify test_bug_401430.js to use a non-ASCII filename, and make sure the test passes?

Comment 2

[Aiko]

Attachment: unit test patch

Hm, I've run into a problem here: this test fails for me even without both of my patches and I've got no clue why. However, after I ran the testcase I find a shortcut to testãЛざ124146xxxxxx.doc in my recently used list both with and without the SetState patch, so that looks good. 

The Windows try server still doesn't run unit tests, right?

Comment 3

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

The try server does run unit tests - I can push the patch there if you'd like.

Comment 4

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

You might also try increasing the test's timeout from 1000ms - for some reason SHAddToRecentDocs() doesn't appear to be synchronous :(

Comment 5

[Aiko]

(In reply to comment #3)
> The try server does run unit tests - I can push the patch there if you'd like.

Yes, please. (The page at https://wiki.mozilla.org/Build:TryServer said unit tests are not run.)

Also tried it with a 10s timeout -- the new entry was definitely there but the test still failed.

Comment 6

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

Ah, it might be a problem with how we convert the binary value from the registry into a string, then.

I pushed it to the try server, but the test failed:
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1241480651.1241489121.16467.gz&fulltext=1#err7

I'll update https://wiki.mozilla.org/Build:TryServer appears to have been updated a while ago:
https://wiki.mozilla.org/index.php?title=Build:TryServer&diff=134173&oldid=130790

Comment 7

[Aiko]

Yes, that's exactly the same error I got. The value in the registry however is 100% ok. 

So, after reading http://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIWindowsRegKey.idl#269 I see no way to get the raw value of a binary registry value. That leaves only a C++ test and honestly that's too much effort for such little gain.

Comment 8

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

You can just test for the expanded string, "\xE3\x1b\x04\x56\x30", I guess. It'll get truncated to "\xE3\x1B\x04V0" since it's not valid on it's own, but that matches the value we get from nsIWindowsRegKey, which I think is good enough for now.

Comment 9

[Aiko]

Attachment: unit test patch v2

Ok, this one works for me.

Comment 10

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

Could you combine the two patches, and also move the remove() call for the test file to before the do_check_eq test? This caused trouble in bug 491930. Looks good otherwise, though.

Comment 11

[Aiko]

Attachment: patch + updated testcase [Checked in: See comment 15 & 16]

Comment 12

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

Comment on attachment 376714 [details] [diff] [review]
patch + updated testcase
[Checked in: See comment 15 & 16]

I didn't know charCodeAt returned NaN for nonexistent characters! That's kind of handy.

r=me, thanks!

Comment 13

[Aiko]

Thanks for the review. Do you prefer an additional sr or can I set checkin-needed?

Comment 14

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

no sr needed, checkin-needed away!

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/6107ad66e56d

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 376714 [details] [diff] [review]
patch + updated testcase
[Checked in: See comment 15 & 16]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ed4e2b396665
test part only