Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342563003:
[chromecast] Enable Mojo javascript bindings for cast_shell (Closed)
DescriptionAdd Mojo JavaScript support for Chromecast
BUG=647036
Committed: https://crrev.com/effe3e763217348d77b9297cb0155bd810557482
Cr-Commit-Position: refs/heads/master@{#419364}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Add Mojo JavaScript support for Chromecast #
Total comments: 7
Patch Set 3 : Add Mojo JavaScript support for Chromecast #
Total comments: 5
Patch Set 4 : Add Mojo JavaScript support for Chromecast #Patch Set 5 : Add Mojo JavaScript support for Chromecast #Patch Set 6 : Add Mojo JavaScript support for Chromecast #
Messages
Total messages: 34 (14 generated)
Description was changed from ========== Add Mojo JavaScript support for Chromecast BUG= ========== to ========== Add Mojo JavaScript support for Chromecast BUG= ==========
jarhar@google.com changed reviewers: + alokp@chromium.org, derekjchow@chromium.org, rockot@chromium.org
rockot@chromium.org changed reviewers: + jam@chromium.org
lgtm - you'll also need a content/ owner (adding jam@) to approve the addition of the content/grit dependency. This dependency is generally not allowed outside of content, but the only exception to the rule exists for a similar purpose (i.e. exposing Mojo JS resources to the extensions layer). https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... File chromecast/renderer/cast_gin_runner.h (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.h:18: // This class is an exact copy of content::MojoMainRunner used to replicate nit: This documentation is probably unnecessary and is likely to become stale. We're eventually going to move away from using gin (and I suspect cast may want to as well, but that's a separate concern) and these classes will diverge.
https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:177: cast_runner_.reset(new CastGinRunner(frame, context_holder)); I don't think this variable needs to outlive this function call. You can probably remove the member variable and use a local unique_ptr here. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... File chromecast/renderer/cast_content_renderer_client.h (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.h:61: const int resourceId); const pass by value does nothing. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.h:69: std::unique_ptr<CastGinRunner> cast_runner_; I don't think you need this variable. It gets reset everytime RunScriptsAtDocumentState is called. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... File chromecast/renderer/cast_gin_runner.cc (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.cc:16: : frame_(frame), context_holder_(context_holder) { DCHECK(context_holder)? https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... File chromecast/renderer/cast_gin_runner.h (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.h:39: blink::WebFrame* frame_; Can this be blink::WebFrame* const frame_? https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.h:42: gin::ContextHolder* context_holder_; Ditto. Should this variable change over the object lifetime?
rockot@: Are there any content-browsertests or mojo-js integration tests that we can enable on cast_linux bot to ensure that this path is tested? https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:161: void CastContentRendererClient::InjectJavaScript( This does not need to be a member function. Make it a standalone static function. I would also rename it to ExecuteJavascript. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:163: const int resourceId) { resourceId -> resource_id. And no need for const. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:164: const std::string& js_string = ui::ResourceBundle::GetSharedInstance() Use the content-client API to get js-string https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:172: v8::HandleScope handle_scope(blink::mainThreadIsolate()); Add a high-level comment on what exactly it is doing. Better yet, create a function called EnableMojoJsBindings and call it from here. IMPORTANT: Can this function be called multiple times for a frame? If so, we should only be doing this once. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:188: IDR_MOJO_UNICODE_JS, IDR_MOJO_BUFFER_JS, IDR_MOJO_CODEC_JS, Run git-cl format to format this properly. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... File chromecast/renderer/cast_gin_runner.h (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.h:18: // This class is an exact copy of content::MojoMainRunner used to replicate On 2016/09/14 21:13:22, Ken Rockot wrote: > nit: This documentation is probably unnecessary and is likely to become stale. > We're eventually going to move away from using gin (and I suspect cast may want > to as well, but that's a separate concern) and these classes will diverge. Yes we will move away from gin as well. In fact we are planning to bundle mojo-js public API with cast SDK. At that point I wonder what gin dependencies will remain.
Description was changed from ========== Add Mojo JavaScript support for Chromecast BUG= ========== to ========== Add Mojo JavaScript support for Chromecast BUG=647036 ==========
https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:161: void CastContentRendererClient::InjectJavaScript( On 2016/09/14 21:55:36, alokp wrote: > This does not need to be a member function. Make it a standalone static > function. I would also rename it to ExecuteJavascript. Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:163: const int resourceId) { On 2016/09/14 21:55:36, alokp wrote: > resourceId -> resource_id. And no need for const. Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:164: const std::string& js_string = ui::ResourceBundle::GetSharedInstance() On 2016/09/14 21:55:36, alokp wrote: > Use the content-client API to get js-string GetContentClient() cannot be accessed from here because it doesn't have CONTENT_EXPORT on it. https://cs.chromium.org/chromium/src/content/public/common/content_client.h?q... Is there another way that I can access the content client? https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:172: v8::HandleScope handle_scope(blink::mainThreadIsolate()); On 2016/09/14 21:55:36, alokp wrote: > Add a high-level comment on what exactly it is doing. Better yet, create a > function called EnableMojoJsBindings and call it from here. > > IMPORTANT: Can this function be called multiple times for a frame? If so, we > should only be doing this once. MojoBindingsController appears to make the assumption that this is only called once per frame, and there seems to be no issues. I could check to see if a given context already has our mojo bindings to be extra careful. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:177: cast_runner_.reset(new CastGinRunner(frame, context_holder)); On 2016/09/14 21:20:13, derekjchow1 wrote: > I don't think this variable needs to outlive this function call. You can > probably remove the member variable and use a local unique_ptr here. As per our conversation, I have moved it to gin::PerContextData https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:188: IDR_MOJO_UNICODE_JS, IDR_MOJO_BUFFER_JS, IDR_MOJO_CODEC_JS, On 2016/09/14 21:55:36, alokp wrote: > Run git-cl format to format this properly. Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... File chromecast/renderer/cast_content_renderer_client.h (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.h:61: const int resourceId); On 2016/09/14 21:20:13, derekjchow1 wrote: > const pass by value does nothing. Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.h:69: std::unique_ptr<CastGinRunner> cast_runner_; On 2016/09/14 21:20:13, derekjchow1 wrote: > I don't think you need this variable. It gets reset everytime > RunScriptsAtDocumentState is called. Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... File chromecast/renderer/cast_gin_runner.cc (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.cc:16: : frame_(frame), context_holder_(context_holder) { On 2016/09/14 21:20:13, derekjchow1 wrote: > DCHECK(context_holder)? Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... File chromecast/renderer/cast_gin_runner.h (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.h:18: // This class is an exact copy of content::MojoMainRunner used to replicate On 2016/09/14 21:13:22, Ken Rockot wrote: > nit: This documentation is probably unnecessary and is likely to become stale. > We're eventually going to move away from using gin (and I suspect cast may want > to as well, but that's a separate concern) and these classes will diverge. Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.h:39: blink::WebFrame* frame_; On 2016/09/14 21:20:13, derekjchow1 wrote: > Can this be blink::WebFrame* const frame_? Done. https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_gi... chromecast/renderer/cast_gin_runner.h:42: gin::ContextHolder* context_holder_; On 2016/09/14 21:20:14, derekjchow1 wrote: > Ditto. Should this variable change over the object lifetime? Done.
https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:168: content::RenderFrame* render_frame, int resource_id) { DCHECK(render_frame) https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:180: blink::WebLocalFrame* frame = render_frame->GetWebFrame(); You can probably move lines 179 & 181-183 into the constructor of CastGinRunner. We only need |frame|. https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:185: context_data->SetUserData(kCastContextStateKey, runner); Will runner get leaked if you do not explicitly remove it? I think it does. You might want to do something like MojoBindingsController by making CastGinRunner a RenderFrameObserver{Tracker}, and cleaning up when the following functions are called: void WillReleaseScriptContext(v8::Local<v8::Context> context, int world_id) override; void DidClearWindowObject() override; https://cs.chromium.org/chromium/src/content/renderer/mojo_bindings_controller.h
why is this reaching into content to get the resource files, instead of just adding it to its binary as well? are you worried about space? https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:168: content::RenderFrame* render_frame, int resource_id) { On 2016/09/15 00:48:02, derekjchow1 wrote: > DCHECK(render_frame) note this isn't really needed, since if render_frame is null then two lines down will crash which is just as useful of a signal
On 2016/09/15 16:30:12, jam wrote: > why is this reaching into content to get the resource files, instead of just > adding it to its binary as well? are you worried about space? jam@: I do not quite understand your comment (I am not too familiar with grit). Wouldn't that add mojo-js files twice into cast_shell resource? Note that this is not much of a concern because we eventually want to include mojo js files into cast sdk, which gets downloaded with each cast webapp. So is a short-term solution until mojo-js bindings API is stable and backwards compatible. Just wanted to check if this is what you meant.
On 2016/09/15 17:55:59, alokp wrote: > On 2016/09/15 16:30:12, jam wrote: > > why is this reaching into content to get the resource files, instead of just > > adding it to its binary as well? are you worried about space? > > jam@: I do not quite understand your comment (I am not too familiar with grit). > Wouldn't that add mojo-js files twice into cast_shell resource? Note that this > is not much of a concern because we eventually want to include mojo js files > into cast sdk, which gets downloaded with each cast webapp. So is a short-term > solution until mojo-js bindings API is stable and backwards compatible. Just > wanted to check if this is what you meant. yeah I was curious, since in general we don't allow includes other than content/public. i guess we can later move this one resource file to content/public instead of including the JS in each component separately. lgtm
https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:185: context_data->SetUserData(kCastContextStateKey, runner); On 2016/09/15 00:48:02, derekjchow1 wrote: > Will runner get leaked if you do not explicitly remove it? I think it does. You > might want to do something like MojoBindingsController by making CastGinRunner a > RenderFrameObserver{Tracker}, and cleaning up when the following functions are > called: > > void WillReleaseScriptContext(v8::Local<v8::Context> context, > int world_id) override; > void DidClearWindowObject() override; > > https://cs.chromium.org/chromium/src/content/renderer/mojo_bindings_controller.h Done.
lgtm % nits https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:161: void CastContentRendererClient::InjectJavaScript( On 2016/09/15 00:32:41, jarhar wrote: > On 2016/09/14 21:55:36, alokp wrote: > > This does not need to be a member function. Make it a standalone static > > function. I would also rename it to ExecuteJavascript. > > Done. You forgot to make it a non-member function. https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:195: static const int mojoResourceIds[] = { mojoResourceIds -> mojo_resource_ids
lgtm % some nits https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:180: CastGinRunner* runner = Add comment on how CastGinRunner manages it's own lifetime. https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:182: context_data->SetUserData(kCastContextData, runner); Can this be done in the constructor of CastGinRunner? Similarly, can the destructor of CastGinRunner RemoveUserData? https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... File chromecast/renderer/cast_gin_runner.cc (right): https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... chromecast/renderer/cast_gin_runner.cc:69: v8::Local<v8::Context> context = This function should call the destructor of this class, and the contents of this function should be moved to the destructor. See ~MojoBindingsController, which should have a similar lifetime to this class.
https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/1/chromecast/renderer/cast_co... chromecast/renderer/cast_content_renderer_client.cc:161: void CastContentRendererClient::InjectJavaScript( On 2016/09/16 04:27:22, alokp wrote: > On 2016/09/15 00:32:41, jarhar wrote: > > On 2016/09/14 21:55:36, alokp wrote: > > > This does not need to be a member function. Make it a standalone static > > > function. I would also rename it to ExecuteJavascript. > > > > Done. > > You forgot to make it a non-member function. Done. https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/20001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:195: static const int mojoResourceIds[] = { On 2016/09/16 04:27:22, alokp wrote: > mojoResourceIds -> mojo_resource_ids Done. https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... File chromecast/renderer/cast_content_renderer_client.cc (right): https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:180: CastGinRunner* runner = On 2016/09/16 17:02:00, derekjchow1 wrote: > Add comment on how CastGinRunner manages it's own lifetime. Done. https://codereview.chromium.org/2342563003/diff/40001/chromecast/renderer/cas... chromecast/renderer/cast_content_renderer_client.cc:182: context_data->SetUserData(kCastContextData, runner); On 2016/09/16 17:02:00, derekjchow1 wrote: > Can this be done in the constructor of CastGinRunner? Similarly, can the > destructor of CastGinRunner RemoveUserData? Done.
The CQ bit was checked by jarhar@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, jam@chromium.org, alokp@chromium.org, derekjchow@chromium.org Link to the patchset: https://codereview.chromium.org/2342563003/#ps60001 (title: "Add Mojo JavaScript support for Chromecast")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by jarhar@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, alokp@chromium.org, jam@chromium.org, derekjchow@chromium.org Link to the patchset: https://codereview.chromium.org/2342563003/#ps80001 (title: "Add Mojo JavaScript support for Chromecast")
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
Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by jarhar@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, alokp@chromium.org, jam@chromium.org, derekjchow@chromium.org Link to the patchset: https://codereview.chromium.org/2342563003/#ps100001 (title: "Add Mojo JavaScript support for Chromecast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add Mojo JavaScript support for Chromecast BUG=647036 ========== to ========== Add Mojo JavaScript support for Chromecast BUG=647036 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add Mojo JavaScript support for Chromecast BUG=647036 ========== to ========== Add Mojo JavaScript support for Chromecast BUG=647036 Committed: https://crrev.com/effe3e763217348d77b9297cb0155bd810557482 Cr-Commit-Position: refs/heads/master@{#419364} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/effe3e763217348d77b9297cb0155bd810557482 Cr-Commit-Position: refs/heads/master@{#419364} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
