Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
332693004:
Add a content API for postMessage (Closed)
|
Created:
6 years, 6 months ago by sgurun-gerrit only Modified:
6 years, 5 months ago CC:
android-webview-reviews_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, miu+watch_chromium.org, nasko+codewatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd a content API for postMessage.
BUG=393291
Add a content API for postMessage so webview can utilize this API for posting messages to JS.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284320
Patch Set 1 #Patch Set 2 : Experiment with extending the ViewMsg_PostMessage_Params #Patch Set 3 : get rid of json. plain string is fine. #
Total comments: 6
Patch Set 4 : addressed reviews #Patch Set 5 : Added a test case #Patch Set 6 : extend content api to set source origin #
Total comments: 4
Patch Set 7 : addressed mnaganov's review #
Total comments: 9
Patch Set 8 : address code review from mkosiba & nasko #
Total comments: 3
Patch Set 9 : address jochen's review #
Messages
Total messages: 35 (0 generated)
Hello, I have started working on the postmessage API for WebView. The first goal is to land a content API. For this I need to make sure ViewMsg_PostMessage_Params::data allows passing script data from browser to renderer. I was thinking of converting type of data to base::value, however this hits a snag because not all base::value can be IPCed. Should I follow an approach like here where we use base::binaryvalue? https://code.google.com/p/chromium/codesearch#chromium/src/content/common/and...
On 2014/06/17 23:38:19, sgurun wrote: > Hello, > > I have started working on the postmessage API for WebView. The first goal is to > land a content API. For this I need to make sure > ViewMsg_PostMessage_Params::data allows passing script data from browser to > renderer. > I was thinking of converting type of data to base::value, however this hits a > snag because not all base::value can be IPCed. Should I follow an approach like > here where we use base::binaryvalue? > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/and... Sorry, but what types of base::Value can't be transferred via IPC? I didn't encounter any issues with transferring them. I have introduced GinJavaBridgeValue to pass values that just can't be put into base::Value due its architectural restrictions for maintaining JSON compatibility, like NaN / Infinity values, and "undefined", and also for distinguishing my object ID values from regular integers.
On 2014/06/18 08:45:20, Mikhail Naganov (Cr) wrote: > On 2014/06/17 23:38:19, sgurun wrote: > > Hello, > > > > I have started working on the postmessage API for WebView. The first goal is > to > > land a content API. For this I need to make sure > > ViewMsg_PostMessage_Params::data allows passing script data from browser to > > renderer. > > I was thinking of converting type of data to base::value, however this hits a > > snag because not all base::value can be IPCed. Should I follow an approach > like > > here where we use base::binaryvalue? > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/and... > > Sorry, but what types of base::Value can't be transferred via IPC? I didn't > encounter any issues with transferring them. > > I have introduced GinJavaBridgeValue to pass values that just can't be put into > base::Value due its architectural restrictions for maintaining JSON > compatibility, like NaN / Infinity values, and "undefined", and also for > distinguishing my object ID values from regular integers. The problem should be related to passing MessagePorts which I believe are passed in data member of the struct above. Specifically, RenderFrameHostManagerTest.SupportCrossProcessPostMessageWithMessagePort test fails, and from my limited debugging, I see that the browser does receive this as a null, while the renderer sends it. Will look at it again today.
On 2014/06/18 16:40:35, sgurun wrote: > On 2014/06/18 08:45:20, Mikhail Naganov (Cr) wrote: > > On 2014/06/17 23:38:19, sgurun wrote: > > > Hello, > > > > > > I have started working on the postmessage API for WebView. The first goal is > > to > > > land a content API. For this I need to make sure > > > ViewMsg_PostMessage_Params::data allows passing script data from browser to > > > renderer. > > > I was thinking of converting type of data to base::value, however this hits > a > > > snag because not all base::value can be IPCed. Should I follow an approach > > like > > > here where we use base::binaryvalue? > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/and... > > > > Sorry, but what types of base::Value can't be transferred via IPC? I didn't > > encounter any issues with transferring them. > > > > I have introduced GinJavaBridgeValue to pass values that just can't be put > into > > base::Value due its architectural restrictions for maintaining JSON > > compatibility, like NaN / Infinity values, and "undefined", and also for > > distinguishing my object ID values from regular integers. > > The problem should be related to passing MessagePorts which I believe are passed > in data member of the struct above. Specifically, > RenderFrameHostManagerTest.SupportCrossProcessPostMessageWithMessagePort test > fails, and from my limited debugging, I see that the browser does receive this > as a null, while the renderer sends it. Will look at it again today. I see. I think the problem comes from the fact that WebDOMMessageEvent uses a quite elaborate serialization scheme, and when you put the resulting string through V8ValueConverter, it can be altered in subtle ways. Actually, now I'm not getting your intention here. As WebDOMMessageEvent already provides serialized data as a string, you can simply construct a base::StringValue from it, no need to use V8ValueConverter at all.
On 2014/06/18 17:03:06, Mikhail Naganov (Cr) wrote: > On 2014/06/18 16:40:35, sgurun wrote: > > On 2014/06/18 08:45:20, Mikhail Naganov (Cr) wrote: > > > On 2014/06/17 23:38:19, sgurun wrote: > > > > Hello, > > > > > > > > I have started working on the postmessage API for WebView. The first goal > is > > > to > > > > land a content API. For this I need to make sure > > > > ViewMsg_PostMessage_Params::data allows passing script data from browser > to > > > > renderer. > > > > I was thinking of converting type of data to base::value, however this > hits > > a > > > > snag because not all base::value can be IPCed. Should I follow an approach > > > like > > > > here where we use base::binaryvalue? > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/and... > > > > > > Sorry, but what types of base::Value can't be transferred via IPC? I didn't > > > encounter any issues with transferring them. > > > > > > I have introduced GinJavaBridgeValue to pass values that just can't be put > > into > > > base::Value due its architectural restrictions for maintaining JSON > > > compatibility, like NaN / Infinity values, and "undefined", and also for > > > distinguishing my object ID values from regular integers. > > > > The problem should be related to passing MessagePorts which I believe are > passed > > in data member of the struct above. Specifically, > > RenderFrameHostManagerTest.SupportCrossProcessPostMessageWithMessagePort test > > fails, and from my limited debugging, I see that the browser does receive this > > as a null, while the renderer sends it. Will look at it again today. > > I see. I think the problem comes from the fact that WebDOMMessageEvent uses a > quite elaborate serialization scheme, and when you put the resulting string > through V8ValueConverter, it can be altered in subtle ways. > > Actually, now I'm not getting your intention here. As WebDOMMessageEvent already > provides serialized data as a string, you can simply construct a > base::StringValue from it, no need to use V8ValueConverter at all. Right. Actually I suddenly realized I have not investigated passing MessagePorts between frames that well, but to answer your question: For implementing postMessage(String msg, String framename) I need to be able to convert msg to a WebSerializedScriptValue if I want to use the existing ViewMsg_PostMessageEvent IPC. However, I cannot do this at the Browser side, so I wanted to modify the existing IPC to use base::Value. The alternative is introducing another IPC and keeping the ViewMsg_PostMessageEvent intact. But then how would it work with regards to passing MessagePorts and other cases more complicated than primitive types is not clear in my mind, yet. Will do it today.
Some initial comments. I'm not a fan of the custom code in RenderViewImpl to construct WebSerializedScriptValue. It can easily get out of sync when changes are made and we should look at ways to reuse existing code. https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1460: params.source_routing_id = GetWebContents()->GetRoutingID(); The source routing id is the same routing ID as the target, which doesn't look correct. Since the source is WebView, we probably need a constant to identify this case. We could use MSG_ROUTING_NONE, but not sure yet. https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... content/browser/android/content_view_core_impl.h:164: void PostMessageToMainWindow(JNIEnv* env, jobject obj, jstring message, nit: You are posting message to a frame, not window. https://codereview.chromium.org/332693004/diff/40001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/332693004/diff/40001/content/common/view_mess... content/common/view_messages.h:488: // Whether the data format is supplied as serialized script value, or base::value nit: Lines should be limited to 80 characters. Also an empty line between struct members.
On 2014/06/20 16:56:52, nasko wrote:
> Some initial comments. I'm not a fan of the custom code in RenderViewImpl to
> construct WebSerializedScriptValue. It can easily get out of sync when changes
> are made and we should look at ways to reuse existing code.
Thanks, I will definitely go with your guidance here, since it is an area that I
am just getting familiar with. I just could not find a way to generate a
WebSerializedScriptValue at the browser side, which the renderer expects. The
other option I tried was using
base::Value in IPC (in patch 1). I think this had two disadvantages:
1. Adds one more layer of serialization/deserialization layer to all post
messages, (converting from WebSerializedScriptValue to Base::Value and then back
again)
2. Failed when posting an object, the case that failed was posting a message
{message: "msg-with-port", port: mc.port2}.
If you suggest another route, I can gladly investigate.
>
>
https://codereview.chromium.org/332693004/diff/40001/content/browser/android/...
> File content/browser/android/content_view_core_impl.cc (right):
>
>
https://codereview.chromium.org/332693004/diff/40001/content/browser/android/...
> content/browser/android/content_view_core_impl.cc:1460:
params.source_routing_id
> = GetWebContents()->GetRoutingID();
> The source routing id is the same routing ID as the target, which doesn't look
> correct. Since the source is WebView, we probably need a constant to identify
> this case. We could use MSG_ROUTING_NONE, but not sure yet.
I realized that, but I wasn't sure either.
>
>
https://codereview.chromium.org/332693004/diff/40001/content/browser/android/...
> File content/browser/android/content_view_core_impl.h (right):
>
>
https://codereview.chromium.org/332693004/diff/40001/content/browser/android/...
> content/browser/android/content_view_core_impl.h:164: void
> PostMessageToMainWindow(JNIEnv* env, jobject obj, jstring message,
> nit: You are posting message to a frame, not window.
Will change the name, you are right. Eventually we want to post to any frame,
not only the main frame, so it will take a frame name. maybe we can special case
main frame using a constant?
>
>
https://codereview.chromium.org/332693004/diff/40001/content/common/view_mess...
> File content/common/view_messages.h (right):
>
>
https://codereview.chromium.org/332693004/diff/40001/content/common/view_mess...
> content/common/view_messages.h:488: // Whether the data format is supplied as
> serialized script value, or base::value
> nit: Lines should be limited to 80 characters. Also an empty line between
struct
> members.
will do.
Hello, Hey Jochen, can I also get your guidance (especially wrt to the IPC) on the postMessage API that I am planning to add to content for the use of android webview? https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... content/browser/android/content_view_core_impl.cc:1460: params.source_routing_id = GetWebContents()->GetRoutingID(); On 2014/06/20 16:56:52, nasko (in Munich) wrote: > The source routing id is the same routing ID as the target, which doesn't look > correct. Since the source is WebView, we probably need a constant to identify > this case. We could use MSG_ROUTING_NONE, but not sure yet. Done. https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... File content/browser/android/content_view_core_impl.h (right): https://codereview.chromium.org/332693004/diff/40001/content/browser/android/... content/browser/android/content_view_core_impl.h:164: void PostMessageToMainWindow(JNIEnv* env, jobject obj, jstring message, On 2014/06/20 16:56:52, nasko (in Munich) wrote: > nit: You are posting message to a frame, not window. Done. https://codereview.chromium.org/332693004/diff/40001/content/common/view_mess... File content/common/view_messages.h (right): https://codereview.chromium.org/332693004/diff/40001/content/common/view_mess... content/common/view_messages.h:488: // Whether the data format is supplied as serialized script value, or base::value On 2014/06/20 16:56:52, nasko (in Munich) wrote: > nit: Lines should be limited to 80 characters. Also an empty line between struct > members. Done.
+marja
On 2014/07/11 09:23:00, jochen wrote: > +marja PTAL, thanks.
https://codereview.chromium.org/332693004/diff/100001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/332693004/diff/100001/content/browser/android... content/browser/android/content_view_core_impl.cc:1441: if (host) nit: bail out early if there is no host? https://codereview.chromium.org/332693004/diff/100001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/332693004/diff/100001/content/renderer/render... content/renderer/render_view_impl.cc:2981: base::Value* value = new base::StringValue(params.data); Use scoped_ptr. The converter doesn't acquire ownership of the value.
https://codereview.chromium.org/332693004/diff/100001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/332693004/diff/100001/content/browser/android... content/browser/android/content_view_core_impl.cc:1441: if (host) On 2014/07/14 15:19:29, Mikhail Naganov (Cr) wrote: > nit: bail out early if there is no host? Done. https://codereview.chromium.org/332693004/diff/100001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/332693004/diff/100001/content/renderer/render... content/renderer/render_view_impl.cc:2981: base::Value* value = new base::StringValue(params.data); On 2014/07/14 15:19:29, Mikhail Naganov (Cr) wrote: > Use scoped_ptr. The converter doesn't acquire ownership of the value. Done.
On 2014/07/16 00:43:28, sgurun wrote: > https://codereview.chromium.org/332693004/diff/100001/content/browser/android... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/332693004/diff/100001/content/browser/android... > content/browser/android/content_view_core_impl.cc:1441: if (host) > On 2014/07/14 15:19:29, Mikhail Naganov (Cr) wrote: > > nit: bail out early if there is no host? > > Done. > > https://codereview.chromium.org/332693004/diff/100001/content/renderer/render... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/332693004/diff/100001/content/renderer/render... > content/renderer/render_view_impl.cc:2981: base::Value* value = new > base::StringValue(params.data); > On 2014/07/14 15:19:29, Mikhail Naganov (Cr) wrote: > > Use scoped_ptr. The converter doesn't acquire ownership of the value. > > Done. @nasko need review of content/common/view_messages.h and any ideas in general to make it better @yfriedman need review of content/browser/android and content/public/android @jamesr need review of content/renderer thanks
IPC LGTM https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... content/common/view_messages.h:489: // a simple string. If it is a raw string, we convert from string to a nit: s/we convert/must be converted/ https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... content/common/view_messages.h:491: IPC_STRUCT_MEMBER(bool, is_data_raw_string) nit: Empty line between struct members.
https://codereview.chromium.org/332693004/diff/120001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/PostMessageTest.java (right): https://codereview.chromium.org/332693004/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/PostMessageTest.java:1: // Copyright 2012 The Chromium Authors. All rights reserved. date
https://codereview.chromium.org/332693004/diff/120001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/332693004/diff/120001/content/browser/android... content/browser/android/content_view_core_impl.cc:1429: void ContentViewCoreImpl::PostMessageToFrame(JNIEnv* env, jobject obj, given that this is only for aw and not chrome, can you add this to aw_contents instead?
https://codereview.chromium.org/332693004/diff/120001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/332693004/diff/120001/content/browser/android... content/browser/android/content_view_core_impl.cc:1429: void ContentViewCoreImpl::PostMessageToFrame(JNIEnv* env, jobject obj, Himm, I don't think aw_ can create the ViewMsg_PostMessage message since it can only depend on content/public, or alternatively we can move the message to content/public/common. Would this be acceptable? @nasko can comment maybe. On 2014/07/16 17:18:05, Yaron wrote: > given that this is only for aw and not chrome, can you add this to aw_contents > instead?
https://codereview.chromium.org/332693004/diff/120001/content/browser/android... File content/browser/android/content_view_core_impl.cc (right): https://codereview.chromium.org/332693004/diff/120001/content/browser/android... content/browser/android/content_view_core_impl.cc:1429: void ContentViewCoreImpl::PostMessageToFrame(JNIEnv* env, jobject obj, On 2014/07/16 18:01:27, sgurun wrote: > Himm, I don't think aw_ can create the ViewMsg_PostMessage message since it can > only depend on content/public, or alternatively we can move the message to > content/public/common. Would this be acceptable? @nasko can comment maybe. > > > On 2014/07/16 17:18:05, Yaron wrote: > > given that this is only for aw and not chrome, can you add this to aw_contents > > instead? > I'm not sure about making the message public (can ask an aw owner) but I would prefer aw-specific code in aw
On 2014/07/16 18:09:20, Yaron wrote: > https://codereview.chromium.org/332693004/diff/120001/content/browser/android... > File content/browser/android/content_view_core_impl.cc (right): > > https://codereview.chromium.org/332693004/diff/120001/content/browser/android... > content/browser/android/content_view_core_impl.cc:1429: void > ContentViewCoreImpl::PostMessageToFrame(JNIEnv* env, jobject obj, > On 2014/07/16 18:01:27, sgurun wrote: > > Himm, I don't think aw_ can create the ViewMsg_PostMessage message since it > can > > only depend on content/public, or alternatively we can move the message to > > content/public/common. Would this be acceptable? @nasko can comment maybe. > > > > > > On 2014/07/16 17:18:05, Yaron wrote: > > > given that this is only for aw and not chrome, can you add this to > aw_contents > > > instead? > > > > I'm not sure about making the message public (can ask an aw owner) but I would > prefer aw-specific code in aw I actually discussed it with benm here. We can only use it if the message is public. we either have to make the message public or we have to add some api to content view core. do you still feel like we should investigate making the message public instead?
On 2014/07/16 18:31:23, sgurun wrote: > On 2014/07/16 18:09:20, Yaron wrote: > > > https://codereview.chromium.org/332693004/diff/120001/content/browser/android... > > File content/browser/android/content_view_core_impl.cc (right): > > > > > https://codereview.chromium.org/332693004/diff/120001/content/browser/android... > > content/browser/android/content_view_core_impl.cc:1429: void > > ContentViewCoreImpl::PostMessageToFrame(JNIEnv* env, jobject obj, > > On 2014/07/16 18:01:27, sgurun wrote: > > > Himm, I don't think aw_ can create the ViewMsg_PostMessage message since it > > can > > > only depend on content/public, or alternatively we can move the message to > > > content/public/common. Would this be acceptable? @nasko can comment maybe. > > > > > > > > > On 2014/07/16 17:18:05, Yaron wrote: > > > > given that this is only for aw and not chrome, can you add this to > > aw_contents > > > > instead? > > > > > > > I'm not sure about making the message public (can ask an aw owner) but I would > > prefer aw-specific code in aw > > I actually discussed it with benm here. We can only use it if the message is > public. we either have to make the message public or we have to add some api to > content view core. do you still feel like we should investigate making the > message public instead? I would rather expose an API. IPC messages are implementation detail of content/ and should not be exposed. I'm sad I missed that part in my prior review.
https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... File content/common/view_messages.h (right): https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... content/common/view_messages.h:489: // a simple string. If it is a raw string, we convert from string to a On 2014/07/16 08:06:15, nasko (in Munich) wrote: > nit: s/we convert/must be converted/ Done. https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... content/common/view_messages.h:491: IPC_STRUCT_MEMBER(bool, is_data_raw_string) On 2014/07/16 08:06:15, nasko (in Munich) wrote: > nit: Empty line between struct members. Done. https://codereview.chromium.org/332693004/diff/120001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/PostMessageTest.java (right): https://codereview.chromium.org/332693004/diff/120001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/PostMessageTest.java:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2014/07/16 16:08:44, mkosiba wrote: > date Done.
On 2014/07/16 20:35:16, sgurun wrote: > https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... > File content/common/view_messages.h (right): > > https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... > content/common/view_messages.h:489: // a simple string. If it is a raw string, > we convert from string to a > On 2014/07/16 08:06:15, nasko (in Munich) wrote: > > nit: s/we convert/must be converted/ > > Done. > > https://codereview.chromium.org/332693004/diff/120001/content/common/view_mes... > content/common/view_messages.h:491: IPC_STRUCT_MEMBER(bool, is_data_raw_string) > On 2014/07/16 08:06:15, nasko (in Munich) wrote: > > nit: Empty line between struct members. > > Done. > > https://codereview.chromium.org/332693004/diff/120001/content/public/android/... > File > content/public/android/javatests/src/org/chromium/content/browser/PostMessageTest.java > (right): > > https://codereview.chromium.org/332693004/diff/120001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/PostMessageTest.java:1: > // Copyright 2012 The Chromium Authors. All rights reserved. > On 2014/07/16 16:08:44, mkosiba wrote: > > date > > Done. @yaron ping
lgtm given that CVC is a dumping ground of stuff and this logically fits. However, I think going forward it would be better to spearate aw-specific functionality out of shared code
On 2014/07/17 17:48:54, Yaron wrote: > lgtm given that CVC is a dumping ground of stuff and this logically fits. > However, I think going forward it would be better to spearate aw-specific > functionality out of shared code I can try to refactor webview specific parts out of contentviewrenderer to another file in a follow up CL. This file is becoming way too large anyway. @jamesr need lgtm of content/renderer
On 2014/07/17 18:59:28, sgurun wrote: > On 2014/07/17 17:48:54, Yaron wrote: > > lgtm given that CVC is a dumping ground of stuff and this logically fits. > > However, I think going forward it would be better to spearate aw-specific > > functionality out of shared code > > I can try to refactor webview specific parts out of contentviewrenderer to > another file in a follow up CL. This file is becoming way too large anyway. > > @jamesr need lgtm of content/renderer s/contentviewrenderer/contentviewcore/
I'm not very familiar with WebSerializedScriptValue - you should find a reviewer who is.
On 2014/07/17 20:02:01, jamesr wrote: > I'm not very familiar with WebSerializedScriptValue - you should find a reviewer > who is. jochen can you please review content/renderer
lgtm https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... content/renderer/render_view_impl.cc:2975: v8::HandleScope handle_scope(v8::Isolate::GetCurrent()); should be blink::mainThreadIsolate()
thanks. @jamesr: got jochen's lgtm for content/renderer. now need owner's lgtm. thanks https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... content/renderer/render_view_impl.cc:19: #include "base/json/json_reader.h" removed unintended include. https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... content/renderer/render_view_impl.cc:2975: v8::HandleScope handle_scope(v8::Isolate::GetCurrent()); On 2014/07/18 09:20:27, jochen wrote: > should be blink::mainThreadIsolate() Done.
@jamesr: already got lgtm from jochen. since you are the only owner and since you already asked for somebody else to review, I think you would be ok with his lgtm. On 2014/07/18 16:13:44, sgurun wrote: > thanks. @jamesr: got jochen's lgtm for content/renderer. now need owner's lgtm. > thanks > > https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... > content/renderer/render_view_impl.cc:19: #include "base/json/json_reader.h" > removed unintended include. > > https://codereview.chromium.org/332693004/diff/140001/content/renderer/render... > content/renderer/render_view_impl.cc:2975: v8::HandleScope > handle_scope(v8::Isolate::GetCurrent()); > On 2014/07/18 09:20:27, jochen wrote: > > should be blink::mainThreadIsolate() > > Done.
The CQ bit was checked by sgurun@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sgurun@chromium.org/332693004/160001
On 2014/07/18 21:13:19, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/sgurun@chromium.org/332693004/160001 Himm I missed jochen was content owner. sorry for TBRing.
Message was sent while issue was closed.
Change committed as 284320
Message was sent while issue was closed.
On 2014/07/18 22:31:16, sgurun wrote: > On 2014/07/18 21:13:19, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/sgurun@chromium.org/332693004/160001 > > Himm I missed jochen was content owner. sorry for TBRing. Owners in parents cover child directories, right. I'm useful mostly for renderer-only patches :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
