Bug 65004

Summary: [V8][Chromium] Run workers in a separate v8::Isolate
Product: WebKit Reporter: Dmitry Lomov <dslomov>
Component: WebCore Misc.Assignee: Dmitry Lomov <dslomov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, antonm, dimich, dslomov, levin, vitalyr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 61016    
Attachments:
Description Flags
Allocates v8::isolate per worker
levin: review+, levin: commit-queue-
CR feedback addressed none

Description Dmitry Lomov 2011-07-21 18:26:21 PDT
Allocate a separate v8::Isolate workers.
Comment 1 Dmitry Lomov 2011-07-21 18:46:12 PDT
Created attachment 101685 [details]
Allocates v8::isolate per worker
Comment 2 Dmitry Lomov 2011-07-21 18:46:39 PDT
Comment on attachment 101685 [details]
Allocates v8::isolate per worker

No cq yet - waiting on chromium trybots.
Comment 3 Dmitry Lomov 2011-07-21 18:47:18 PDT
Comment on attachment 101685 [details]
Allocates v8::isolate per worker

trybots happy.
Comment 4 David Levin 2011-07-26 13:22:24 PDT
Comment on attachment 101685 [details]
Allocates v8::isolate per worker

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

> Source/WebCore/ChangeLog:9
> +        No new tests. (OOPS!)

This should either say what test were added or why test can't be written or what test cover it or that no new functionality is exposed so no new tests are needed.

> Source/WebCore/bindings/v8/V8Binding.h:112
>          DOMDataList& allStores() { return m_DOMDataList; }

Add blank line.

> Source/WebCore/bindings/v8/V8Binding.h:120
> +            m_DOMDataList.remove(m_DOMDataList.find(domDataStore));

What happens if domDataStore isn't in there?

> Source/WebCore/bindings/v8/WorkerScriptController.cpp:59
> +    data->allStores().append(&m_DOMDataStore);

consider swapping these two lines.

(It seems like setDOMDataStore should assert that what it is given is in the allStores.)

> Source/WebCore/bindings/v8/WorkerScriptController.h:82
> +        ScopedDOMDataStore m_DOMDataStore;

m_domDataStore
Comment 5 David Levin 2011-07-26 13:23:42 PDT
Comment on attachment 101685 [details]
Allocates v8::isolate per worker

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

>> Source/WebCore/bindings/v8/V8Binding.h:120
>> +            m_DOMDataList.remove(m_DOMDataList.find(domDataStore));
> 
> What happens if domDataStore isn't in there?

Please add an assert here.
Comment 6 Dmitry Lomov 2011-07-26 17:17:45 PDT
Created attachment 102076 [details]
CR feedback addressed
Comment 7 WebKit Review Bot 2011-07-26 19:41:28 PDT
Comment on attachment 102076 [details]
CR feedback addressed

Clearing flags on attachment: 102076

Committed r91814: <http://trac.webkit.org/changeset/91814>
Comment 8 WebKit Review Bot 2011-07-26 19:41:32 PDT
All reviewed patches have been landed.  Closing bug.