722163 - Convert JumpListBuilder to use LazyIdleThread

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 1

[malen]

Attachment: Change from nsThread to LazyIdleThread in line 97

Comment 2

[phoang]

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

[phoang]

Attachment: Still need to test more on

We're still need to run more tests

Comment 4

[phoang]

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

[Josh Matthews [:jdm]]

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

Comment 6

[phoang]

Attachment: patch_722163_v2

nothing change much just remove the comment line.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 8

[malen]

Attachment: The correction of alignment and fix some missing on ManualShutdown

Comment 9

[Josh Matthews [:jdm]]

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

[phoang]

Attachment: patch_722163_final_version

This is our final version.

Comment 11

[Josh Matthews [:jdm]]

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

Comment 12

[phoang]

Attachment: Bug 722163 - Convert JumpListBuilder to use LazyIdleThread. r=khuey

Comment 13

[Josh Matthews [:jdm]]

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.

Comment 14

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1a62243dc313

Comment 15

[phoang]

Thanks a lot. Now I got it.

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/1a62243dc313