Last Comment Bug 722163 - Convert JumpListBuilder to use LazyIdleThread
: Convert JumpListBuilder to use LazyIdleThread
Status: RESOLVED FIXED
[mentor=khuey][lang=c++]
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: malen
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-29 08:16 PST by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2012-03-07 02:45 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change from nsThread to LazyIdleThread in line 97 (23.41 KB, text/plain)
2012-02-22 23:36 PST, malen
no flags Details
Still need to test more on (1.55 KB, patch)
2012-02-23 14:50 PST, phoang
no flags Details | Diff | Splinter Review
patch_722163_v2 (1.62 KB, patch)
2012-03-03 08:48 PST, phoang
khuey: review+
Details | Diff | Splinter Review
The correction of alignment and fix some missing on ManualShutdown (1.88 KB, text/plain)
2012-03-05 20:36 PST, malen
no flags Details
patch_722163_final_version (1.66 KB, patch)
2012-03-05 23:07 PST, phoang
phoang: review+
Details | Diff | Splinter Review
Bug 722163 - Convert JumpListBuilder to use LazyIdleThread. r=khuey (4.85 KB, patch)
2012-03-06 12:18 PST, phoang
phoang: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-01-29 08:16:54 PST
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 2012-02-22 23:36:21 PST
Created attachment 599900 [details]
Change from nsThread to LazyIdleThread in line 97
Comment 2 phoang 2012-02-23 14:14:13 PST
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 2012-02-23 14:50:33 PST
Created attachment 600187 [details] [diff] [review]
Still need to test more on

We're still need to run more tests
Comment 4 phoang 2012-03-02 22:55:43 PST
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] 2012-03-03 00:02:34 PST
You'll want to delete the commented out line. Let's see what Kyle thinks, otherwise.
Comment 6 phoang 2012-03-03 08:48:04 PST
Created attachment 602626 [details] [diff] [review]
patch_722163_v2

nothing change much just remove the comment line.
Comment 7 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-05 05:28:54 PST
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 2012-03-05 20:36:45 PST
Created attachment 603151 [details]
The correction of alignment and fix some missing on ManualShutdown
Comment 9 Josh Matthews [:jdm] 2012-03-05 21:16:03 PST
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 2012-03-05 23:07:33 PST
Created attachment 603170 [details] [diff] [review]
patch_722163_final_version

This is our final version.
Comment 11 Josh Matthews [:jdm] 2012-03-05 23:17:53 PST
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 2012-03-06 12:18:43 PST
Created attachment 603408 [details] [diff] [review]
Bug 722163 - Convert JumpListBuilder to use LazyIdleThread. r=khuey
Comment 13 Josh Matthews [:jdm] 2012-03-06 12:30:14 PST
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 15 phoang 2012-03-06 13:15:03 PST
Thanks a lot. Now I got it.
Comment 16 Marco Bonardo [::mak] 2012-03-07 02:45:19 PST
https://hg.mozilla.org/mozilla-central/rev/1a62243dc313

Note You need to log in before you can comment on or make changes to this bug.