Bug 65004 - [V8][Chromium] Run workers in a separate v8::Isolate
Summary: [V8][Chromium] Run workers in a separate v8::Isolate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry Lomov
URL:
Keywords:
Depends on:
Blocks: 61016
  Show dependency treegraph
 
Reported: 2011-07-21 18:26 PDT by Dmitry Lomov
Modified: 2011-07-26 19:41 PDT (History)
7 users (show)

See Also:


Attachments
Allocates v8::isolate per worker (8.62 KB, patch)
2011-07-21 18:46 PDT, Dmitry Lomov
levin: review+
levin: commit-queue-
Details | Formatted Diff | Diff
CR feedback addressed (9.83 KB, patch)
2011-07-26 17:17 PDT, Dmitry Lomov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.