Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2687943004:
Abstract out ThreadDebugger from V8PerIsolateData (Closed)
|
Created:
3 years, 10 months ago by adithyas Modified:
3 years, 10 months ago CC:
apavlov+blink_chromium.org, blink-reviews, blink-reviews-bindings_chromium.org, caseq+blink_chromium.org, chromium-reviews, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAbstract out ThreadDebugger from V8PerIsolateData
V8PerIsolateData holds an instance of ThreadDebugger, but does not need to know information about its interface. This can be abstracted out with V8PerIsolateData::Data and will make it easier to move V8PerIsolateData to platform/ (as ThreadDebugger is very tightly coupled with core).
BUG=682322
Review-Url: https://codereview.chromium.org/2687943004
Cr-Commit-Position: refs/heads/master@{#450995}
Committed: https://chromium.googlesource.com/chromium/src/+/d9471b0d6e0108c3303e9214daf0564c11eba611
Patch Set 1 #Patch Set 2 : Reparent branch #Patch Set 3 : Fix Windows Build Attempt #1 #Patch Set 4 : Initialize HiddenValue and PrivateProperty in V8Initializer #
Total comments: 30
Patch Set 5 : Only abstract thread debugger #
Total comments: 4
Patch Set 6 : Fix style, add comment #
Total comments: 2
Messages
Total messages: 37 (23 generated)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Abstract out some dependencies from V8PerIsolateData BUG= ========== to ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData should not need to know any information about these classes, so they are abstracted away using V8PerIsolateData::Data. BUG= ==========
Description was changed from ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData should not need to know any information about these classes, so they are abstracted away using V8PerIsolateData::Data. BUG= ========== to ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData should not need to know any information about these classes, so this CL abstracts them away using V8PerIsolateData::Data. BUG= ==========
Description was changed from ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData should not need to know any information about these classes, so this CL abstracts them away using V8PerIsolateData::Data. BUG= ========== to ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData should not need to know any information about these classes, so this CL abstracts them away using V8PerIsolateData::Data. BUG=682322 ==========
Description was changed from ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData should not need to know any information about these classes, so this CL abstracts them away using V8PerIsolateData::Data. BUG=682322 ========== to ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData does not need to know any information about these classes. It just maintains unique_ptrs to instances of these classes, and resets them when the isolate is destroyed. This CL abstracts them away using V8PerIsolateData::Data. BUG=682322 ==========
adithyas@chromium.org changed reviewers: + jbroman@chromium.org
jbroman@chromium.org changed reviewers: + haraken@chromium.org
+haraken, who might want to weigh in on this bikeshed https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:94: explicit Data() {} nit: if you do keep this interface, you don't need the constructor (and if you did declare one, it doesn't need to be explicit if it takes no arguments). https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:139: void setThreadDebugger(Data* threadDebugger) { Please pass ownership by std::unique_ptr, not raw pointer. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:245: std::unique_ptr<Data> m_hiddenValue; V8HiddenValue is going away, and V8PrivateProperty seems straightforward to generalize enough to move to platform/. I'd prefer not to abstract them just for the sake of having more than one thing using Data. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:247: std::unique_ptr<Data> m_threadDebugger; I'm of two minds on this. On the short-term-fix side, we could (ab)use v8_inspector::V8InspectorClient, which has a virtual destructor, which would at least give some type information about what we expect to here. On the other hand, we don't actually call any of its other methods. On the other hand, I think my longer term preference (at least for those things that aren't hot) is a generic map that erases the information about this. In Blink we'd ordinarily use Supplementable for this (but that's now restricted to GC objects), or in Chromium they'd use base::SupportsUserData. Since this isn't hot, that seems okay to me (though it might be a little overkill for now).
https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:245: std::unique_ptr<Data> m_hiddenValue; On 2017/02/14 18:10:48, jbroman wrote: > V8HiddenValue is going away, and V8PrivateProperty seems straightforward to > generalize enough to move to platform/. I'd prefer not to abstract them just for > the sake of having more than one thing using Data. Yeah, agreed. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:247: std::unique_ptr<Data> m_threadDebugger; On 2017/02/14 18:10:48, jbroman wrote: > I'm of two minds on this. > > On the short-term-fix side, we could (ab)use v8_inspector::V8InspectorClient, > which has a virtual destructor, which would at least give some type information > about what we expect to here. On the other hand, we don't actually call any of > its other methods. > > On the other hand, I think my longer term preference (at least for those things > that aren't hot) is a generic map that erases the information about this. In > Blink we'd ordinarily use Supplementable for this (but that's now restricted to > GC objects), or in Chromium they'd use base::SupportsUserData. Since this isn't > hot, that seems okay to me (though it might be a little overkill for now). I don't have any strong opinion. The current Data pattern looks fine to me. We use the Data pattern in some public APIs to let Blink own an thing in Chromium.
Alright. I've left some more comments about the ThreadDebugger part of this CL. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:440: ThreadDebugger::setThreadDebugger(isolate, new MainThreadDebugger(isolate)); This looks like it was more or less fine as-is. I'm not clear on what value the indirection through ThreadDebugger adds. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:247: std::unique_ptr<Data> m_threadDebugger; On 2017/02/15 at 00:06:01, haraken wrote: > On 2017/02/14 18:10:48, jbroman wrote: > > I'm of two minds on this. > > > > On the short-term-fix side, we could (ab)use v8_inspector::V8InspectorClient, > > which has a virtual destructor, which would at least give some type information > > about what we expect to here. On the other hand, we don't actually call any of > > its other methods. > > > > On the other hand, I think my longer term preference (at least for those things > > that aren't hot) is a generic map that erases the information about this. In > > Blink we'd ordinarily use Supplementable for this (but that's now restricted to > > GC objects), or in Chromium they'd use base::SupportsUserData. Since this isn't > > hot, that seems okay to me (though it might be a little overkill for now). > > I don't have any strong opinion. The current Data pattern looks fine to me. We use the Data pattern in some public APIs to let Blink own an thing in Chromium. Alright, okay to leave m_threadDebugger as-is (modulo the comment about using std::unique_ptr<Data> in the setter rather than Data*). https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/ThreadDebugger.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/ThreadDebugger.h:39: static ThreadDebugger* getThreadDebugger(v8::Isolate*); This looks equivalent to ThreadDebugger::from, above. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:59: ThreadDebugger* debugger = ThreadDebugger::getThreadDebugger(isolate); Should be able to use ThreadDebugger::from here. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:62: CHECK(debugger->isWorker()); ASSERT -> DCHECK, not CHECK https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:83: ThreadDebugger::setThreadDebugger(m_isolate, Kinda weird for the setter to go through ThreadDebugger like this. What's wrong with the code as it was before (using makeUnique, passing to V8PerIsolateData::setThreadDebugger)?
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8HiddenValue.h:56: static V8HiddenValue* from(v8::Isolate* isolate) { No need to expose this method as public:, I think. Should be private:. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:29: #include <v8.h> Please use the following instead. #include "v8/include/v8.h" We're going to treat v8.h as a user-defined header. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:30: #include <memory> Chromium's style guide recommends to put an empty line just after system headers. Ditto for all other files. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:91: WTF_MAKE_NONCOPYABLE(Data); Why do we want Data to be non-copyable? I know subclasses are actually non-copyable, but I see no reason to make Data non-copyable from the point of view of V8PerIsolateData. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:133: static V8PrivateProperty* from(v8::Isolate* isolate) { No need to expose this method as public:. Should be private:. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:219: CHECK(isMainThread()); ASSERT corresponds to DCHECK. Is this change intentional? By the way, RELEASE_ASSERT corresponds to CHECK. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:222: CHECK(debugger && !debugger->isWorker()); Ditto.
I reverted the changes to V8HiddenValue and V8PrivateProperty, the Data abstraction is only being used by ThreadDebugger now. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:440: ThreadDebugger::setThreadDebugger(isolate, new MainThreadDebugger(isolate)); On 2017/02/15 at 02:25:23, jbroman wrote: > This looks like it was more or less fine as-is. I'm not clear on what value the indirection through ThreadDebugger adds. I thought it was weird having a getter and setter in a different place, but yeah I think you're right, no additional value. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:30: #include <memory> On 2017/02/15 at 10:00:40, Yuki wrote: > Chromium's style guide recommends to put an empty line just after system headers. Ditto for all other files. Running git cl format reordered the headers like this, so maybe we need to change clang-format to add an empty line. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:91: WTF_MAKE_NONCOPYABLE(Data); On 2017/02/15 at 10:00:40, Yuki wrote: > Why do we want Data to be non-copyable? > I know subclasses are actually non-copyable, but I see no reason to make Data non-copyable from the point of view of V8PerIsolateData. Yeah it doesn't need to be non-copyable, I've changed it now. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:94: explicit Data() {} On 2017/02/14 at 18:10:48, jbroman wrote: > nit: if you do keep this interface, you don't need the constructor (and if you did declare one, it doesn't need to be explicit if it takes no arguments). Removed. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:139: void setThreadDebugger(Data* threadDebugger) { On 2017/02/14 at 18:10:48, jbroman wrote: > Please pass ownership by std::unique_ptr, not raw pointer. Done. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:245: std::unique_ptr<Data> m_hiddenValue; On 2017/02/15 at 00:06:01, haraken wrote: > On 2017/02/14 18:10:48, jbroman wrote: > > V8HiddenValue is going away, and V8PrivateProperty seems straightforward to > > generalize enough to move to platform/. I'd prefer not to abstract them just for > > the sake of having more than one thing using Data. > > Yeah, agreed. Okay, I reverted the changes to V8PrivateProperty and V8HiddenValue. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:247: std::unique_ptr<Data> m_threadDebugger; On 2017/02/15 at 02:25:23, jbroman wrote: > On 2017/02/15 at 00:06:01, haraken wrote: > > On 2017/02/14 18:10:48, jbroman wrote: > > > I'm of two minds on this. > > > > > > On the short-term-fix side, we could (ab)use v8_inspector::V8InspectorClient, > > > which has a virtual destructor, which would at least give some type information > > > about what we expect to here. On the other hand, we don't actually call any of > > > its other methods. > > > > > > On the other hand, I think my longer term preference (at least for those things > > > that aren't hot) is a generic map that erases the information about this. In > > > Blink we'd ordinarily use Supplementable for this (but that's now restricted to > > > GC objects), or in Chromium they'd use base::SupportsUserData. Since this isn't > > > hot, that seems okay to me (though it might be a little overkill for now). > > > > I don't have any strong opinion. The current Data pattern looks fine to me. We use the Data pattern in some public APIs to let Blink own an thing in Chromium. > > Alright, okay to leave m_threadDebugger as-is (modulo the comment about using std::unique_ptr<Data> in the setter rather than Data*). Done! https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/MainThreadDebugger.cpp:219: CHECK(isMainThread()); On 2017/02/15 at 10:00:40, Yuki wrote: > ASSERT corresponds to DCHECK. > Is this change intentional? > > By the way, RELEASE_ASSERT corresponds to CHECK. Ah, my bad. Thanks! https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/ThreadDebugger.h (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/ThreadDebugger.h:39: static ThreadDebugger* getThreadDebugger(v8::Isolate*); On 2017/02/15 at 02:25:23, jbroman wrote: > This looks equivalent to ThreadDebugger::from, above. Yup, don't know why I added this. https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/inspector/WorkerThreadDebugger.cpp:62: CHECK(debugger->isWorker()); On 2017/02/15 at 02:25:23, jbroman wrote: > ASSERT -> DCHECK, not CHECK Fixed, thanks! https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right): https://codereview.chromium.org/2687943004/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:83: ThreadDebugger::setThreadDebugger(m_isolate, On 2017/02/15 at 02:25:24, jbroman wrote: > Kinda weird for the setter to go through ThreadDebugger like this. What's wrong with the code as it was before (using makeUnique, passing to V8PerIsolateData::setThreadDebugger)? I thought it would be better to pair the getter and setter in the same file. But, considering we only set once, setting it directly using V8PerIsolateData isn't a problem, I'll change it back.
The CQ bit was checked by adithyas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Abstract out some dependencies from V8PerIsolateData Currently V8PerIsolateData is responsible for creating and holding a per isolate instance of V8HiddenValue and V8PrivateProperty, and also holds an instance of ThreadDebugger. V8PerIsolateData does not need to know any information about these classes. It just maintains unique_ptrs to instances of these classes, and resets them when the isolate is destroyed. This CL abstracts them away using V8PerIsolateData::Data. BUG=682322 ========== to ========== Abstract out ThreadDebugger from V8PerIsolateData V8PerIsolateData holds an instance of ThreadDebugger, but does not need to know information about its interface. This can be abstracted out with V8PerIsolateData::Data and will make it easier to move V8PerIsolateData to platform/ (as ThreadDebugger is very tightly coupled with core). BUG=682322 ==========
lgtm https://codereview.chromium.org/2687943004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:94: virtual ~Data(){}; super-nit: no semicolon after a function definition, and prefer a space before the {. i.e., either: virtual ~Data() {} or virtual ~Data() = default; (like base/supports_user_data.h)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2687943004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:92: class CORE_EXPORT Data { Add a comment and explain when this class should be used.
LGTM.
https://codereview.chromium.org/2687943004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:92: class CORE_EXPORT Data { On 2017/02/16 at 00:27:10, haraken wrote: > Add a comment and explain when this class should be used. Done. https://codereview.chromium.org/2687943004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:94: virtual ~Data(){}; On 2017/02/15 at 18:39:09, jbroman wrote: > super-nit: no semicolon after a function definition, and prefer a space before the {. i.e., either: > > virtual ~Data() {} > > or > > virtual ~Data() = default; > > (like base/supports_user_data.h) Fixed.
The CQ bit was checked by adithyas@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, jbroman@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2687943004/#ps100001 (title: "Fix style, add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1487258315993410,
"parent_rev": "d35f8c61ab3d89a4fe0f6de3e641d1748dc5ef09", "commit_rev":
"d9471b0d6e0108c3303e9214daf0564c11eba611"}
Message was sent while issue was closed.
Description was changed from ========== Abstract out ThreadDebugger from V8PerIsolateData V8PerIsolateData holds an instance of ThreadDebugger, but does not need to know information about its interface. This can be abstracted out with V8PerIsolateData::Data and will make it easier to move V8PerIsolateData to platform/ (as ThreadDebugger is very tightly coupled with core). BUG=682322 ========== to ========== Abstract out ThreadDebugger from V8PerIsolateData V8PerIsolateData holds an instance of ThreadDebugger, but does not need to know information about its interface. This can be abstracted out with V8PerIsolateData::Data and will make it easier to move V8PerIsolateData to platform/ (as ThreadDebugger is very tightly coupled with core). BUG=682322 Review-Url: https://codereview.chromium.org/2687943004 Cr-Commit-Position: refs/heads/master@{#450995} Committed: https://chromium.googlesource.com/chromium/src/+/d9471b0d6e0108c3303e9214daf0... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/d9471b0d6e0108c3303e9214daf0...
Message was sent while issue was closed.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2687943004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:170: Data* threadDebugger(); Sorry for coming late, but I'm curios what are we trying to hide here? The method is called |threadDebugger|, but returns some abstract data, which sounds like we are tricking ourselves. If we introduce layering, can we have an actual abstraction so that anyone can create an object which lifetime is coupled with isolate? I think current solution is fighting with compiler/DEPS rules, but not the actual semantics.
Message was sent while issue was closed.
https://codereview.chromium.org/2687943004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h (right): https://codereview.chromium.org/2687943004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h:170: Data* threadDebugger(); On 2017/02/18 at 06:11:36, dgozman wrote: > Sorry for coming late, but I'm curios what are we trying to hide here? The method is called |threadDebugger|, but returns some abstract data, which sounds like we are tricking ourselves. > If we introduce layering, can we have an actual abstraction so that anyone can create an object which lifetime is coupled with isolate? I think current solution is fighting with compiler/DEPS rules, but not the actual semantics. You are correct, the current solution simply removes a dependency to core/ rather than actually abstracting away the object. Abstracting away these objects with a map (like what Supplementable does) would be a cleaner solution. But, I don't know if this is necessary considering there is only one object (threadDebugger) that will be abstracted away by this. Members like privateProperty and hiddenValue could be abstracted away too, but they are accessed very frequently, so adding a map access to retrieve them might result in a performance hit. There was some bikeshedding earlier in this CL and the consensus was that the Data abstraction was okay just for this case. |
