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

Status

()

Toolkit
Downloads API
--
enhancement
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Aiko, Assigned: Aiko)

Tracking

Trunk
mozilla1.9.2a1
x86
Windows Vista
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Created attachment 375622 [details] [diff] [review]
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.
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?
(Assignee)

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
Status: NEW → ASSIGNED
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 :(
(Assignee)

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:
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
(Assignee)

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.
(Assignee)

Updated

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.
(Assignee)

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.
(Assignee)

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+
(Assignee)

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
http://hg.mozilla.org/mozilla-central/rev/6107ad66e56d
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1

Updated

8 years ago
Flags: in-testsuite+
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
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.