Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
924983002:
Expose v8 context a MessagePort lives in to give content side code a valid context to (de)serialize… (Closed)
|
Created:
5 years, 10 months ago by Marijn Kruisselbrink Modified:
5 years, 10 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, dglazkov+blink, eae+blinkwatch, rwlbuis, sof Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionExpose v8 context a MessagePort lives in to give content side code a valid context to (de)serialize messages in.
This is used in the content side changes in https://codereview.chromium.org/921013002
BUG=426458
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190944
Patch Set 1 #
Total comments: 1
Patch Set 2 : add a warning that this API should not be used #
Total comments: 3
Patch Set 3 : change to use the world of some event listener #
Total comments: 2
Patch Set 4 : Use a separate (per-MessagePort) v8::Context #
Total comments: 2
Patch Set 5 : dispose context in MessagePort destructor #
Total comments: 4
Patch Set 6 : Add FIXME and include v8.h instead of forward declarations #
Total comments: 1
Patch Set 7 : use v8::Local instead of v8::Handle #
Messages
Total messages: 31 (7 generated)
mek@chromium.org changed reviewers: + adamk@chromium.org
https://codereview.chromium.org/924983002/diff/1/Source/core/dom/MessagePort.h File Source/core/dom/MessagePort.h (right): https://codereview.chromium.org/924983002/diff/1/Source/core/dom/MessagePort.... Source/core/dom/MessagePort.h:116: virtual v8::Handle<v8::Context> scriptContext() override; Exposing a Context like this through the API worries me a little bit, as it's easy to mishandle Contexts (memory leaks and security issues). I looked at your Chromium change, but I'm not very familiar with V8ValueConverter or base::Value; perhaps you can outline the high-level need you have here?
On 2015/02/18 02:01:21, adamk wrote: > https://codereview.chromium.org/924983002/diff/1/Source/core/dom/MessagePort.h > File Source/core/dom/MessagePort.h (right): > > https://codereview.chromium.org/924983002/diff/1/Source/core/dom/MessagePort.... > Source/core/dom/MessagePort.h:116: virtual v8::Handle<v8::Context> > scriptContext() override; > Exposing a Context like this through the API worries me a little bit, as it's > easy to mishandle Contexts (memory leaks and security issues). I agree that it is not great. And I don't really understand how blink uses v8 well enough to really understand what I'm doing. At least I think what I'm doing is a lot better than what android webview is currently doing to do a similar WebSerializedScriptValue->base::Value conversion. In the current android webview code it just takes the main world script context from the main frame from some arbitrary top-level webview frame, without even trying to get the right v8 context. (https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/re...) > I looked at your Chromium change, but I'm not very familiar with > V8ValueConverter or base::Value; perhaps you can outline the high-level need you > have here? High level, what I'm trying to implement is using a MessagePort as the javascript-side communication channel to communicate with native code. In my case this native code would be a navigator.connect exposed "system service" or experimental API, in the case of android webview, they are trying to do the same to communicate between java embedder and embedded javascript code. The "cleanest" way to achieve this would probably be to rewrite/refactor WebSerializedScriptValue to make it possible to serialize directly to/from some type that isn't a v8::Value (no real reason to use base::Value here, other than that it is something that exists that supports most of what we care about), but that would be a fairly invasive change. Since we're not actually entirely sure yet that this MessagePort-to-native-code is something that we really want/need, I tried to come up with a less-invasive way of accomplishing the same (and in parallel the android webview team did the same). So that less-invasive approach is taking the WebSerializedScriptValue, converting that to a v8::Value, then converting the v8::Value to a base::Value, and send that to native code (this part actually doesn't need this change, since WebMessagePortChannel::postMessage already is called from within the scope of a valid v8 context). Similarly messages back to the renderer get send as base::Value, where they then are converted to a v8::Value, that v8::Value is serialized using WebSerializedValue, and then passed of to blink (which deserializes it again of course). Instead of exposing the context like in this CL, another way to achieve the same goals would be to pass the context to WebMessagePortChannel::tryGetMessage (or execute that with a context already active). I'm not sure if that would be a better approach though, as in the vast majority of the cases tryGetMessage won't actually need this v8 context. Does that help at least explain a bit better what I'm trying to do and how I got to this "solution"?
adamk@chromium.org changed reviewers: + jochen@chromium.org
Thanks for the response. I've put up my feelings on this patch over at https://codereview.chromium.org/921013002/#msg10 TL;DR: this patch lgtm with a big warning on this API that it's temporary, and only meant to be used until we can teach SerializedScriptValue about base::Value. +jochen for both public/OWNERS and to possibly help answer the "using base from Blink" question.
New patchsets have been uploaded after l-g-t-m from adamk@chromium.org
On 2015/02/18 22:12:48, adamk wrote: > Thanks for the response. I've put up my feelings on this patch over at > https://codereview.chromium.org/921013002/#msg10 > > TL;DR: this patch lgtm with a big warning on this API that it's temporary, and > only meant to be used until we can teach SerializedScriptValue about > base::Value. I added a (hopefully big enough) warning on these API methods.
https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... File Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... Source/core/dom/MessagePort.cpp:278: return toV8Context(executionContext(), DOMWrapperWorld::mainWorld()); what if the message port was created in an isolated world?
jochen: any comments on the question of being able to use base (and base::Value specifically) from blink? Is that something the repository merge will bring, or not planned at all? https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... File Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... Source/core/dom/MessagePort.cpp:278: return toV8Context(executionContext(), DOMWrapperWorld::mainWorld()); On 2015/02/23 12:15:18, jochen (slow) wrote: > what if the message port was created in an isolated world? That's a good question, and unfortunately not something I have an answer for. As far as I know there is currently no way to actually figure out what world a MessagePort was created in, and the best I'd be able to do would be some ugly hack to take the world from some arbitrary message event, if there even is such an event. But maybe I'm missing something obvious in the v8/blink integration?
On 2015/02/23 at 19:20:18, mek wrote: > jochen: any comments on the question of being able to use base (and base::Value specifically) from blink? Is that something the repository merge will bring, or not planned at all? I think kinuko@ wants to unify some parts after the merge, but I'm not sure what the exact outcome will be. Also note that base::Value can't represent all values from a serialized script value. > > https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... > File Source/core/dom/MessagePort.cpp (right): > > https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... > Source/core/dom/MessagePort.cpp:278: return toV8Context(executionContext(), DOMWrapperWorld::mainWorld()); > On 2015/02/23 12:15:18, jochen (slow) wrote: > > what if the message port was created in an isolated world? > > That's a good question, and unfortunately not something I have an answer for. As far as I know there is currently no way to actually figure out what world a MessagePort was created in, and the best I'd be able to do would be some ugly hack to take the world from some arbitrary message event, if there even is such an event. But maybe I'm missing something obvious in the v8/blink integration? I guess we should plumb the world to the creation of the message port (or the actual v8::Context). Using the wrong context for converting the v8 value is a security hazard.
jochen@chromium.org changed reviewers: + haraken@chromium.org
+haraken@
https://codereview.chromium.org/924983002/diff/20001/public/platform/WebMessa... File public/platform/WebMessagePortChannelClient.h (right): https://codereview.chromium.org/924983002/diff/20001/public/platform/WebMessa... public/platform/WebMessagePortChannelClient.h:58: // Returns the V8 context this message port lives in. As jochen pointed out, using a wrong context is a security hazard. Just help me understand: What V8 context do you want to return here? A message port is not a thing associated with a V8 context, so "the V8 context this message port lives in" doesn't make sense to me...
On 2015/02/24 10:17:37, jochen (slow) wrote: > On 2015/02/23 at 19:20:18, mek wrote: > > jochen: any comments on the question of being able to use base (and > base::Value specifically) from blink? Is that something the repository merge > will bring, or not planned at all? > > I think kinuko@ wants to unify some parts after the merge, but I'm not sure what > the exact outcome will be. Also note that base::Value can't represent all values > from a serialized script value. Yeah, I realize base::Value can't represent all values, that's not a problem for my current use cases. > > > > > > https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... > > File Source/core/dom/MessagePort.cpp (right): > > > > > https://codereview.chromium.org/924983002/diff/20001/Source/core/dom/MessageP... > > Source/core/dom/MessagePort.cpp:278: return toV8Context(executionContext(), > DOMWrapperWorld::mainWorld()); > > On 2015/02/23 12:15:18, jochen (slow) wrote: > > > what if the message port was created in an isolated world? > > > > That's a good question, and unfortunately not something I have an answer for. > As far as I know there is currently no way to actually figure out what world a > MessagePort was created in, and the best I'd be able to do would be some ugly > hack to take the world from some arbitrary message event, if there even is such > an event. But maybe I'm missing something obvious in the v8/blink integration? > > I guess we should plumb the world to the creation of the message port (or the > actual v8::Context). Using the wrong context for converting the v8 value is a > security hazard. In the latest patch set I changed this to use the world of some arbitrary onmessage event handler. When there are no event handlers it doesn't really matter if a message can be serialized, and it seems like it should always be safe to do this as well, since that world is going to deserialize the message anyway. Given that this is really just a temporary thing, this seemed easier then trying to plumb through the world a message port was created in.
https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... File Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... Source/core/dom/MessagePort.cpp:281: // port. I don't understand why this is right. Not all event listeners attached to the MessagePort have the same world. Why is it a right thing to use a world of an arbitrary event listener? Remember that serialize/deserialize can execute any arbitrary JavaScript, so it is important to use a correct context.
https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... File Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... Source/core/dom/MessagePort.cpp:281: // port. On 2015/02/25 07:44:34, haraken wrote: > > I don't understand why this is right. Not all event listeners attached to the > MessagePort have the same world. Why is it a right thing to use a world of an > arbitrary event listener? > > Remember that serialize/deserialize can execute any arbitrary JavaScript, so it > is important to use a correct context. The context returned by this is used by WebMessagePortChannelImpl to convert some message from a base::Value to a serialized script value. On one hand you could say that there is no existing context/world that would be suitable for this, since this conversion/serialization is theoretically observable by javascript code living in whatever world/context is used. And I agree with that, which is why all this is only a temporary hack, to get something working, and to be able to experiment (and have others experiment) and find out if this MessagePort to talk with native code thing is really something that is going to be as useful as we hope it is. But no matter the outcome of this experiment, this MessagePort::scriptContext method is not going to stick around. If we decide we want MessagePort<->native code communication, I will work on being able to do this without the extra serialize/deserialize steps. But doing the fairly invasive refactoring required for that now, while it's not entirely clear this is actually useful, would seem like a bad idea, when a "good enough" (to experiment) solution is possible with little change to blink code. And to answer our actual question why I think it is "good enough" to use the world of an arbitrary event listener: whatever world is picked here is going to deserialize the message anyway, that's why it is an event listener. It doesn't seem like there is that much more of a security risk by using the same world/context first to also serialize that same message.
On 2015/02/25 17:35:17, Marijn Kruisselbrink wrote: > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > File Source/core/dom/MessagePort.cpp (right): > > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > Source/core/dom/MessagePort.cpp:281: // port. > On 2015/02/25 07:44:34, haraken wrote: > > > > I don't understand why this is right. Not all event listeners attached to the > > MessagePort have the same world. Why is it a right thing to use a world of an > > arbitrary event listener? > > > > Remember that serialize/deserialize can execute any arbitrary JavaScript, so > it > > is important to use a correct context. > > The context returned by this is used by WebMessagePortChannelImpl to convert > some message from a base::Value to a serialized script value. On one hand you > could say that there is no existing context/world that would be suitable for > this, since this conversion/serialization is theoretically observable by > javascript code living in whatever world/context is used. And I agree with that, > which is why all this is only a temporary hack, to get something working, and to > be able to experiment (and have others experiment) and find out if this > MessagePort to talk with native code thing is really something that is going to > be as useful as we hope it is. But no matter the outcome of this experiment, > this MessagePort::scriptContext method is not going to stick around. If we > decide we want MessagePort<->native code communication, I will work on being > able to do this without the extra serialize/deserialize steps. But doing the > fairly invasive refactoring required for that now, while it's not entirely clear > this is actually useful, would seem like a bad idea, when a "good enough" (to > experiment) solution is possible with little change to blink code. > > And to answer our actual question why I think it is "good enough" to use the > world of an arbitrary event listener: whatever world is picked here is going to > deserialize the message anyway, that's why it is an event listener. It doesn't > seem like there is that much more of a security risk by using the same > world/context first to also serialize that same message. Should we probably create a new Context for the serialize/deseliazation? For example, V8GCController::collectGarbage is creating a new Context because the work shouldn't be associated with any existing context. https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
On 2015/02/25 17:44:01, haraken wrote: > On 2015/02/25 17:35:17, Marijn Kruisselbrink wrote: > > > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > > File Source/core/dom/MessagePort.cpp (right): > > > > > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > > Source/core/dom/MessagePort.cpp:281: // port. > > On 2015/02/25 07:44:34, haraken wrote: > > > > > > I don't understand why this is right. Not all event listeners attached to > the > > > MessagePort have the same world. Why is it a right thing to use a world of > an > > > arbitrary event listener? > > > > > > Remember that serialize/deserialize can execute any arbitrary JavaScript, so > > it > > > is important to use a correct context. > > > > The context returned by this is used by WebMessagePortChannelImpl to convert > > some message from a base::Value to a serialized script value. On one hand you > > could say that there is no existing context/world that would be suitable for > > this, since this conversion/serialization is theoretically observable by > > javascript code living in whatever world/context is used. And I agree with > that, > > which is why all this is only a temporary hack, to get something working, and > to > > be able to experiment (and have others experiment) and find out if this > > MessagePort to talk with native code thing is really something that is going > to > > be as useful as we hope it is. But no matter the outcome of this experiment, > > this MessagePort::scriptContext method is not going to stick around. If we > > decide we want MessagePort<->native code communication, I will work on being > > able to do this without the extra serialize/deserialize steps. But doing the > > fairly invasive refactoring required for that now, while it's not entirely > clear > > this is actually useful, would seem like a bad idea, when a "good enough" (to > > experiment) solution is possible with little change to blink code. > > > > And to answer our actual question why I think it is "good enough" to use the > > world of an arbitrary event listener: whatever world is picked here is going > to > > deserialize the message anyway, that's why it is an event listener. It doesn't > > seem like there is that much more of a security risk by using the same > > world/context first to also serialize that same message. > > Should we probably create a new Context for the serialize/deseliazation? > > For example, V8GCController::collectGarbage is creating a new Context because > the work shouldn't be associated with any existing context. > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Yes, something like that might be an option. Although I'm not sure how heavyweight a ScriptState/v8::Context really is. I assume I definitely don't want to create one for every message. Maybe one per MessagePort (created on demand) would be okay, but I think ideally there would just be one per renderer process. And I don't think it is possible for content code to create a ScriptState, so I'd still need to somewhere add support in blink for this, right?
On 2015/02/25 17:57:43, Marijn Kruisselbrink wrote: > On 2015/02/25 17:44:01, haraken wrote: > > On 2015/02/25 17:35:17, Marijn Kruisselbrink wrote: > > > > > > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > > > File Source/core/dom/MessagePort.cpp (right): > > > > > > > > > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > > > Source/core/dom/MessagePort.cpp:281: // port. > > > On 2015/02/25 07:44:34, haraken wrote: > > > > > > > > I don't understand why this is right. Not all event listeners attached to > > the > > > > MessagePort have the same world. Why is it a right thing to use a world of > > an > > > > arbitrary event listener? > > > > > > > > Remember that serialize/deserialize can execute any arbitrary JavaScript, > so > > > it > > > > is important to use a correct context. > > > > > > The context returned by this is used by WebMessagePortChannelImpl to convert > > > some message from a base::Value to a serialized script value. On one hand > you > > > could say that there is no existing context/world that would be suitable for > > > this, since this conversion/serialization is theoretically observable by > > > javascript code living in whatever world/context is used. And I agree with > > that, > > > which is why all this is only a temporary hack, to get something working, > and > > to > > > be able to experiment (and have others experiment) and find out if this > > > MessagePort to talk with native code thing is really something that is going > > to > > > be as useful as we hope it is. But no matter the outcome of this experiment, > > > this MessagePort::scriptContext method is not going to stick around. If we > > > decide we want MessagePort<->native code communication, I will work on being > > > able to do this without the extra serialize/deserialize steps. But doing the > > > fairly invasive refactoring required for that now, while it's not entirely > > clear > > > this is actually useful, would seem like a bad idea, when a "good enough" > (to > > > experiment) solution is possible with little change to blink code. > > > > > > And to answer our actual question why I think it is "good enough" to use the > > > world of an arbitrary event listener: whatever world is picked here is going > > to > > > deserialize the message anyway, that's why it is an event listener. It > doesn't > > > seem like there is that much more of a security risk by using the same > > > world/context first to also serialize that same message. > > > > Should we probably create a new Context for the serialize/deseliazation? > > > > For example, V8GCController::collectGarbage is creating a new Context because > > the work shouldn't be associated with any existing context. > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > Yes, something like that might be an option. Although I'm not sure how > heavyweight a ScriptState/v8::Context really is. I assume I definitely don't > want to create one for every message. Maybe one per MessagePort (created on > demand) would be okay, but I think ideally there would just be one per renderer > process. And I don't think it is possible for content code to create a > ScriptState, so I'd still need to somewhere add support in blink for this, > right? I've now changed this CL to return a per-MessagePort v8::Context, does that seem reasonable?
On 2015/02/25 18:29:05, Marijn Kruisselbrink wrote: > On 2015/02/25 17:57:43, Marijn Kruisselbrink wrote: > > On 2015/02/25 17:44:01, haraken wrote: > > > On 2015/02/25 17:35:17, Marijn Kruisselbrink wrote: > > > > > > > > > > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > > > > File Source/core/dom/MessagePort.cpp (right): > > > > > > > > > > > > > > https://codereview.chromium.org/924983002/diff/40001/Source/core/dom/MessageP... > > > > Source/core/dom/MessagePort.cpp:281: // port. > > > > On 2015/02/25 07:44:34, haraken wrote: > > > > > > > > > > I don't understand why this is right. Not all event listeners attached > to > > > the > > > > > MessagePort have the same world. Why is it a right thing to use a world > of > > > an > > > > > arbitrary event listener? > > > > > > > > > > Remember that serialize/deserialize can execute any arbitrary > JavaScript, > > so > > > > it > > > > > is important to use a correct context. > > > > > > > > The context returned by this is used by WebMessagePortChannelImpl to > convert > > > > some message from a base::Value to a serialized script value. On one hand > > you > > > > could say that there is no existing context/world that would be suitable > for > > > > this, since this conversion/serialization is theoretically observable by > > > > javascript code living in whatever world/context is used. And I agree with > > > that, > > > > which is why all this is only a temporary hack, to get something working, > > and > > > to > > > > be able to experiment (and have others experiment) and find out if this > > > > MessagePort to talk with native code thing is really something that is > going > > > to > > > > be as useful as we hope it is. But no matter the outcome of this > experiment, > > > > this MessagePort::scriptContext method is not going to stick around. If we > > > > decide we want MessagePort<->native code communication, I will work on > being > > > > able to do this without the extra serialize/deserialize steps. But doing > the > > > > fairly invasive refactoring required for that now, while it's not entirely > > > clear > > > > this is actually useful, would seem like a bad idea, when a "good enough" > > (to > > > > experiment) solution is possible with little change to blink code. > > > > > > > > And to answer our actual question why I think it is "good enough" to use > the > > > > world of an arbitrary event listener: whatever world is picked here is > going > > > to > > > > deserialize the message anyway, that's why it is an event listener. It > > doesn't > > > > seem like there is that much more of a security risk by using the same > > > > world/context first to also serialize that same message. > > > > > > Should we probably create a new Context for the serialize/deseliazation? > > > > > > For example, V8GCController::collectGarbage is creating a new Context > because > > > the work shouldn't be associated with any existing context. > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > > > Yes, something like that might be an option. Although I'm not sure how > > heavyweight a ScriptState/v8::Context really is. I assume I definitely don't > > want to create one for every message. Maybe one per MessagePort (created on > > demand) would be okay, but I think ideally there would just be one per > renderer > > process. And I don't think it is possible for content code to create a > > ScriptState, so I'd still need to somewhere add support in blink for this, > > right? > > I've now changed this CL to return a per-MessagePort v8::Context, does that seem > reasonable? I think it's reasonable if the performance doesn't matter (and if this is just a temporary hack to be removed soon). (If one ScriptState per MessagePort is too heavy, you can cache the ScriptState to V8PerIsolateData. Then one ScriptState per Isolate.) https://codereview.chromium.org/924983002/diff/60001/Source/core/dom/MessageP... File Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/924983002/diff/60001/Source/core/dom/MessageP... Source/core/dom/MessagePort.cpp:280: m_scriptStateForConversion = ScriptState::create(v8::Context::New(isolate), DOMWrapperWorld::create(isolate)); You need to call: m_scriptStateForConversion->disposePerContextData(); in MessagePort's destructor. Otherwise, the context will leak.
https://codereview.chromium.org/924983002/diff/60001/Source/core/dom/MessageP... File Source/core/dom/MessagePort.cpp (right): https://codereview.chromium.org/924983002/diff/60001/Source/core/dom/MessageP... Source/core/dom/MessagePort.cpp:280: m_scriptStateForConversion = ScriptState::create(v8::Context::New(isolate), DOMWrapperWorld::create(isolate)); On 2015/02/25 18:40:33, haraken wrote: > > You need to call: > > m_scriptStateForConversion->disposePerContextData(); > > in MessagePort's destructor. Otherwise, the context will leak. Done.
LGTM https://codereview.chromium.org/924983002/diff/80001/public/platform/WebMessa... File public/platform/WebMessagePortChannelClient.h (right): https://codereview.chromium.org/924983002/diff/80001/public/platform/WebMessa... public/platform/WebMessagePortChannelClient.h:34: namespace v8 { We conventionally just include <v8.h> instead of using forward declarations. https://codereview.chromium.org/924983002/diff/80001/public/platform/WebMessa... public/platform/WebMessagePortChannelClient.h:58: // Returns a V8 context messages sent to this port can be (de)serialized in. Add a FIXME to remove this.
Patchset #6 (id:80002) has been deleted
https://codereview.chromium.org/924983002/diff/80001/public/platform/WebMessa... File public/platform/WebMessagePortChannelClient.h (right): https://codereview.chromium.org/924983002/diff/80001/public/platform/WebMessa... public/platform/WebMessagePortChannelClient.h:34: namespace v8 { On 2015/02/25 18:49:59, haraken wrote: > > We conventionally just include <v8.h> instead of using forward declarations. Done. https://codereview.chromium.org/924983002/diff/80001/public/platform/WebMessa... public/platform/WebMessagePortChannelClient.h:58: // Returns a V8 context messages sent to this port can be (de)serialized in. On 2015/02/25 18:49:59, haraken wrote: > > Add a FIXME to remove this. Done (and filed a bug as well).
Thanks, LGTM.
lgtm https://codereview.chromium.org/924983002/diff/100001/public/platform/WebMess... File public/platform/WebMessagePortChannelClient.h (right): https://codereview.chromium.org/924983002/diff/100001/public/platform/WebMess... public/platform/WebMessagePortChannelClient.h:60: virtual v8::Handle<v8::Context> scriptContextForMessageConversion() = 0; nit. please use v8::Local instead of v8::Handle
The CQ bit was checked by mek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org, haraken@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/924983002/#ps120001 (title: "use v8::Local instead of v8::Handle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/924983002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190944 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
