Bug 213674 - Rename initializeThreading to initialize
Summary: Rename initializeThreading to initialize
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Geoffrey Garen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-26 20:11 PDT by Geoffrey Garen
Modified: 2020-06-28 13:56 PDT (History)
12 users (show)

See Also:


Attachments
Patch (51.53 KB, patch)
2020-06-26 20:16 PDT, Geoffrey Garen
mark.lam: review+
Details | Formatted Diff | Diff
Patch for landing (51.91 KB, patch)
2020-06-27 20:13 PDT, Geoffrey Garen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch for landing (51.89 KB, patch)
2020-06-28 12:16 PDT, Geoffrey Garen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Geoffrey Garen 2020-06-26 20:11:08 PDT
Rename initializeThreading to initialize
Comment 1 Geoffrey Garen 2020-06-26 20:16:37 PDT
Created attachment 402939 [details]
Patch
Comment 2 Mark Lam 2020-06-26 23:15:05 PDT
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()?
Comment 3 Geoffrey Garen 2020-06-27 08:19:06 PDT
> 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();
Comment 4 Geoffrey Garen 2020-06-27 20:13:37 PDT
Created attachment 402979 [details]
Patch for landing
Comment 5 EWS 2020-06-27 20:15:39 PDT
ChangeLog entry in Source/JavaScriptCore/ChangeLog contains OOPS!.
Comment 6 Geoffrey Garen 2020-06-28 12:16:01 PDT
Created attachment 403006 [details]
Patch for landing
Comment 7 EWS 2020-06-28 13:55:14 PDT
Committed r263635: <https://trac.webkit.org/changeset/263635>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403006 [details].
Comment 8 Radar WebKit Bug Importer 2020-06-28 13:56:16 PDT
<rdar://problem/64866938>
Comment 9 Radar WebKit Bug Importer 2020-06-28 13:56:16 PDT
<rdar://problem/64866939>