If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in mozilla1.9.2a1



Downloads API
9 years ago
7 years ago


(Reporter: Aiko, Assigned: Aiko)


Windows Vista
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)



(1 attachment, 3 obsolete attachments)



9 years ago
Created attachment 375622 [details] [diff] [review]

(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.
Attachment #375622 - Flags: review?(gavin.sharp)
Looks good, but could you modify test_bug_401430.js to use a non-ASCII filename, and make sure the test passes?

Comment 2

9 years ago
Created attachment 375704 [details] [diff] [review]
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?
Assignee: nobody → Seno.Aiko
The try server does run unit tests - I can push the patch there if you'd like.
You might also try increasing the test's timeout from 1000ms - for some reason SHAddToRecentDocs() doesn't appear to be synchronous :(

Comment 5

9 years ago
(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.
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:

I'll update https://wiki.mozilla.org/Build:TryServer appears to have been updated a while ago:

Comment 7

9 years ago
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.


9 years ago
Attachment #375704 - Attachment is obsolete: true
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

9 years ago
Created attachment 376437 [details] [diff] [review]
unit test patch v2

Ok, this one works for me.
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

9 years ago
Created attachment 376714 [details] [diff] [review]
patch + updated testcase
[Checked in: See comment 15 & 16]
Attachment #375622 - Attachment is obsolete: true
Attachment #376437 - Attachment is obsolete: true
Attachment #376714 - Flags: review?(gavin.sharp)
Attachment #375622 - Flags: review?(gavin.sharp)
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!
Attachment #376714 - Flags: review?(gavin.sharp) → review+

Comment 13

9 years ago
Thanks for the review. Do you prefer an additional sr or can I set checkin-needed?
no sr needed, checkin-needed away!
Keywords: checkin-needed
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1


8 years ago
Flags: in-testsuite+
Comment on attachment 376714 [details] [diff] [review]
patch + updated testcase
[Checked in: See comment 15 & 16]

test part only
Attachment #376714 - Attachment description: patch + updated testcase → patch + updated testcase [Checked in: See comment 15 & 16]
You need to log in before you can comment on or make changes to this bug.