Bug 38097 - Disentangle initializing the main thread from initializing threading
: Disentangle initializing the main thread from initializing threading
Status: RESOLVED FIXED
: WebKit
Web Template Framework
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-04-25 15:32 PST by
Modified: 2010-04-26 16:04 PST (History)


Attachments
Patch (53.22 KB, patch)
2010-04-25 15:58 PST, Sam Weinig
no flags Review Patch | Details | Formatted Diff | Diff
Patch (53.44 KB, patch)
2010-04-25 16:03 PST, Sam Weinig
mjs: review-
Review Patch | Details | Formatted Diff | Diff
Patch 3 (50.89 KB, patch)
2010-04-26 15:09 PST, Sam Weinig
mjs: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2010-04-25 15:32:33 PST
We should disentangle the initialization of WebCores notion of the main thread from the rest of the initialization done in JSC::initializeThreading and WTF::initializeThreading.
------- Comment #1 From 2010-04-25 15:58:33 PST -------
Created an attachment (id=54246) [details]
Patch
------- Comment #2 From 2010-04-25 15:59:32 PST -------
(From update of attachment 54246 [details])
Forgot something.  Obsoleting.
------- Comment #3 From 2010-04-25 16:03:01 PST -------
Created an attachment (id=54247) [details]
Patch
------- Comment #4 From 2010-04-25 16:07:28 PST -------
Attachment 54247 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKit/win/WebKitClassFactory.cpp:61:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebKit/efl/ewk/ewk_main.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #5 From 2010-04-25 19:39:47 PST -------
(From update of attachment 54247 [details])
Some minor comments below. Overall this seems reasonable other than the "deprecated" naming. ALso I think it would be good to log or assert or throw an exception or something in the cases where deprecatedInitializeMainThread is called from a thread other than the main thread (since it implies misuse of the original WebKit API). r- to consider these issues, please upload a fresh version.

JavaScriptCore/ChangeLog:15
 +          (WTF::deprecatedInitializeMainThread):
The difference between initializeMainThread and deprecatedInitializeMainThread is not clear from the names. Also from offline discussion it seems like the "deprecated" version is not going away soon or possibly ever, so that doesn't seem like a very good name.

JavaScriptCore/wtf/MainThread.cpp:101
 +      MutexLocker locker(mainThreadInitializationMutex());
Why a mutex here instead of a "once" type mechanism like pthread_once?

JavaScriptCore/wtf/mac/MainThreadMac.mm:75
 +  void deprecatedInitializeMainThreadPlatform()
it looks to me like mainThreadPthread will never be initialized if WebKit is called off the main thread the first time, even on platforms where it would otherwise be initialized to pthread_self(). Is that ok?
------- Comment #6 From 2010-04-25 21:01:17 PST -------
(In reply to comment #5)
> (From update of attachment 54247 [details] [details])
> Some minor comments below. Overall this seems reasonable other than the
> "deprecated" naming. ALso I think it would be good to log or assert or throw an
> exception or something in the cases where deprecatedInitializeMainThread is
> called from a thread other than the main thread (since it implies misuse of the
> original WebKit API). r- to consider these issues, please upload a fresh
> version.
> 
> JavaScriptCore/ChangeLog:15
>  +          (WTF::deprecatedInitializeMainThread):
> The difference between initializeMainThread and deprecatedInitializeMainThread
> is not clear from the names. Also from offline discussion it seems like the
> "deprecated" version is not going away soon or possibly ever, so that doesn't
> seem like a very good name.

Which name seems better, initializeMainThreadToSystemMainThread() or legacyInitializeMainThread()?

> 
> JavaScriptCore/wtf/MainThread.cpp:101
>  +      MutexLocker locker(mainThreadInitializationMutex());
> Why a mutex here instead of a "once" type mechanism like pthread_once?

Will fix.

> JavaScriptCore/wtf/mac/MainThreadMac.mm:75
>  +  void deprecatedInitializeMainThreadPlatform()
> it looks to me like mainThreadPthread will never be initialized if WebKit is
> called off the main thread the first time, even on platforms where it would
> otherwise be initialized to pthread_self(). Is that ok?

That is fine.  If deprecatedInitializeMainThreadPlatform() is ever called, it is being used from an environment which expects WebCore's main thread to be the system's main thread.
------- Comment #7 From 2010-04-25 21:03:23 PST -------
I didn't note this anywhere, and I feel silly for not having done so, but this is the first step in getting rid of the WEB_THREAD #define and paving the way to allowing mac both WebKit and WebKit2 to use the same WebCore.  I will put this information in the next versions changelog.
------- Comment #8 From 2010-04-26 15:09:49 PST -------
Created an attachment (id=54338) [details]
Patch 3
------- Comment #9 From 2010-04-26 15:14:39 PST -------
Attachment 54338 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/win/WebKitClassFactory.cpp:61:  Alphabetical sorting problem.  [build/include_order] [4]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp"
WebKit/efl/ewk/ewk_main.cpp:29:  wtf includes should be <wtf/file.h> instead of "wtf/file.h".  [build/include] [4]
Total errors found: 2 in 47 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #10 From 2010-04-26 15:20:43 PST -------
(From update of attachment 54338 [details])
r=me
------- Comment #11 From 2010-04-26 15:39:31 PST -------
Landed in r58266.
------- Comment #12 From 2010-04-26 15:45:53 PST -------
Fix tiger build in r58268.
------- Comment #13 From 2010-04-26 16:04:37 PST -------
http://trac.webkit.org/changeset/58266 might have broken Chromium Win Release