Bug 38097 - Disentangle initializing the main thread from initializing threading
: Disentangle initializing the main thread from initializing threading
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Template Framework
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-25 15:32 PDT by Sam Weinig
Modified: 2010-04-26 16:04 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-04-25 15:32:33 PDT
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 Sam Weinig 2010-04-25 15:58:33 PDT
Created attachment 54246 [details]
Patch
Comment 2 Sam Weinig 2010-04-25 15:59:32 PDT
Comment on attachment 54246 [details]
Patch

Forgot something.  Obsoleting.
Comment 3 Sam Weinig 2010-04-25 16:03:01 PDT
Created attachment 54247 [details]
Patch
Comment 4 WebKit Review Bot 2010-04-25 16:07:28 PDT
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 Maciej Stachowiak 2010-04-25 19:39:47 PDT
Comment on attachment 54247 [details]
Patch

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 Sam Weinig 2010-04-25 21:01:17 PDT
(In reply to comment #5)
> (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.

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 Sam Weinig 2010-04-25 21:03:23 PDT
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 Sam Weinig 2010-04-26 15:09:49 PDT
Created attachment 54338 [details]
Patch 3
Comment 9 WebKit Review Bot 2010-04-26 15:14:39 PDT
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 Maciej Stachowiak 2010-04-26 15:20:43 PDT
Comment on attachment 54338 [details]
Patch 3

r=me
Comment 11 Sam Weinig 2010-04-26 15:39:31 PDT
Landed in r58266.
Comment 12 Sam Weinig 2010-04-26 15:45:53 PDT
Fix tiger build in r58268.
Comment 13 WebKit Review Bot 2010-04-26 16:04:37 PDT
http://trac.webkit.org/changeset/58266 might have broken Chromium Win Release