Bug 213674

Created attachment 402939 [details]
Patch

Comment on attachment 402939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=402939&action=review

r=me

> Source/WTF/ChangeLog:16
> +        initializeThreading() is a superset. (The opposite is true). I think

I think the parenthesis around "The opposite is true" are not necessary.

> Source/WTF/ChangeLog:17
> +        this is probably because âthreadingâ can be read as "all threadingâ.

Please fix the non-ascii characters.

> Source/WebCore/bindings/js/ScriptController.cpp:85
> +// FIXME: Remove this function because it's a no-op.

Can you elaborate more on why this comment is true?  I see that this is only called from commonVMSlow() and DatabaseManager::openDatabase().  How do we guarantee that JSC::initialize() and WTF::initializeMainThread() will always be called already before anyone calls commonVM()?

> r=me

Thanks!

> > +        initializeThreading() is a superset. (The opposite is true). I think
> 
> I think the parenthesis around "The opposite is true" are not necessary.

Will fix.

> > +        this is probably because âthreadingâ can be read as "all threadingâ.
> 
> Please fix the non-ascii characters.

Will fix. (Do we require ASCII in ChangeLogs? I wonder sometimes when reviewing. Maybe we should fix our diff tool.)

> > Source/WebCore/bindings/js/ScriptController.cpp:85
> > +// FIXME: Remove this function because it's a no-op.
> 
> Can you elaborate more on why this comment is true?  I see that this is only
> called from commonVMSlow() and DatabaseManager::openDatabase().  How do we
> guarantee that JSC::initialize() and WTF::initializeMainThread() will always
> be called already before anyone calls commonVM()?

I moved the comment to the call sites:

    // FIXME: Remove this call to ScriptController::initializeMainThread(). The
    // main thread should have been initialized by a WebKit entrypoint already.
    // Also, initializeMainThread() does nothing on iOS.
    ScriptController::initializeMainThread();

Created attachment 402979 [details]
Patch for landing

ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.

Created attachment 403006 [details]
Patch for landing

Committed r263635: <https://trac.webkit.org/changeset/263635>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403006 [details].

<rdar://problem/64866938>

<rdar://problem/64866939>