Convert JumpListBuilder to use LazyIdleThread

RESOLVED FIXED in mozilla13

Status

()

Core
Widget: Win32
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: malen)

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=khuey][lang=c++])

Attachments

(1 attachment, 5 obsolete attachments)

The JumpListBuilder uses an nsThread for I/O.  (http://mxr.mozilla.org/mozilla-central/search?string=mIOThread&find=JumpList)  We should use LazyIdleThread instead so that the thread will go away when not needed (http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/LazyIdleThread.h).

This is probably a good second bug.

Updated

5 years ago
Assignee: nobody → malenesok007
(Assignee)

Comment 1

5 years ago
Created attachment 599900 [details]
Change from nsThread to LazyIdleThread in line 97

Updated

5 years ago
Attachment #599900 - Attachment is obsolete: true

Comment 2

5 years ago
We're still testing if I works fine in windows. This is the error that we had when trying to run pymake. I also create a windows path variable to all the mozilla-build. If anyone had this problem before please let us know. Thanks.

$ python -OO build/pymake/make.py -f client.mk
c:\mozilla-build\python\python.exe: can't open file 'build/pymake/make.py': [Err
no 2] No such file or directory

Comment 3

5 years ago
Created attachment 600187 [details] [diff] [review]
Still need to test more on

We're still need to run more tests

Comment 4

5 years ago
We have been run lot of tests on a windows 32bit for the patch and It seems working fine. Does any one have a chance to test the patch please give us a feedback?. Thanks

Comment 5

5 years ago
You'll want to delete the commented out line. Let's see what Kyle thinks, otherwise.

Updated

5 years ago
Attachment #600187 - Flags: review?(khuey)

Comment 6

5 years ago
Created attachment 602626 [details] [diff] [review]
patch_722163_v2

nothing change much just remove the comment line.
Attachment #600187 - Attachment is obsolete: true
Attachment #600187 - Flags: review?(khuey)
Comment on attachment 602626 [details] [diff] [review]
patch_722163_v2

>@@ -82,17 +87,20 @@ JumpListBuilder::JumpListBuilder() :
>   mMaxItems(0),
>   mHasCommit(false)
> {
>   ::CoInitialize(NULL);
>
>   CoCreateInstance(CLSID_DestinationList, NULL, CLSCTX_INPROC_SE
>                    IID_ICustomDestinationList, getter_AddRefs(mJ
>
>-  NS_NewThread(getter_AddRefs(mIOThread));
>+  // Make a lazy thread for any IO we need (like clearing or enu
>+    // contents of indexedDB database directories).
>+    mIOThread = new LazyIdleThread(DEFAULT_THREAD_TIMEOUT_MS,
>+                                             LazyIdleThread::Man

Please fix the indentation so that these are all lined up.  It also looks like you lost some characters here at the end (this should be 'LazyIdleThread::ManualShutdown);').

Other than that, it looks great.  Thanks for the patch.
Attachment #602626 - Flags: review+
(Assignee)

Comment 8

5 years ago
Created attachment 603151 [details]
The correction of alignment and fix some missing on ManualShutdown
(Assignee)

Updated

5 years ago
Attachment #603151 - Attachment is obsolete: true
Attachment #603151 - Attachment is patch: false

Updated

5 years ago
Attachment #602626 - Attachment is obsolete: true

Updated

5 years ago
Attachment #603151 - Attachment is obsolete: false
Attachment #603151 - Attachment is patch: true

Comment 9

5 years ago
Malen, could you upload a final version of the patch that follows the instructions at https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F? That way it will be easy for someone to commit it.

Comment 10

5 years ago
Created attachment 603170 [details] [diff] [review]
patch_722163_final_version

This is our final version.
Attachment #603170 - Flags: review+
That didn't address the request in comment 9 - the patch doesn't include an author or commit message (something like "Bug 722163 - Convert JumpListBuilder to use LazyIdleThread. r=khuey" would be great).
(Assignee)

Updated

5 years ago
Attachment #603151 - Attachment is obsolete: true
Attachment #603151 - Attachment is patch: false

Comment 12

5 years ago
Created attachment 603408 [details] [diff] [review]
Bug 722163 - Convert JumpListBuilder to use LazyIdleThread. r=khuey
Attachment #603170 - Attachment is obsolete: true
Attachment #603408 - Flags: review+
To avoid further back and forth, I'm going to check in this patch and add the missing information. However, please read the link in comment 9 again - it gives specific steps for how to generate a patch that requires no modifications. Compare your patch file with this one: https://bug725430.bugzilla.mozilla.org/attachment.cgi?id=603283 - yours is missing the metadata at the top beginning that makes it easier for other people to commit it.
http://hg.mozilla.org/integration/mozilla-inbound/rev/1a62243dc313

Comment 15

5 years ago
Thanks a lot. Now I got it.
https://hg.mozilla.org/mozilla-central/rev/1a62243dc313
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.