[Contextual Search] Add Mojo-enabled API component.

[Contextual Search] Add mojo-enabled API component.

Add a simple JavaScript v8 wrapper for Contextual Search, that just defines chrome.contextualSearch.setCaption(String, Boolean);

Adds a simple Contextual Search component with mojo communication to handle enabling of the v8 wrapper only on a renderer which is presented in the Contextual Search Overlay view.

BUG=523554
Committed: https://crrev.com/bda9b6f432fb237029ed6f4fac16bbe30da0cde4
Cr-Commit-Position: refs/heads/master@{#364460}

[Donn Denman, 2015-10-02 17:16:28]

donnd@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[Donn Denman, 2015-10-02 17:16:28]

Kausalya, This is ready for a first-pass review now.  PTAL.

Note that I was unable to add a new extension (ala Instant), because extensions
are no longer allowed (issue http://crbug.com/334679 - get rid of all v8
extensions in Chrome), so I made it a gin::wrappable following the guide at
https://code.google.com/p/chromium/codesearch#chromium/src/gin/wrappable.h&q=...
and cribbed off of some existing code.

[Donn Denman, 2015-10-05 19:06:30]

Kausalya, not sure if you already got this, so resending.

This is ready for a first-pass review now.  PTAL.

Note that I was unable to add a new extension (ala Instant), because extensions
are no longer allowed (issue http://crbug.com/334679 - get rid of all v8
extensions in Chrome), so I made it a gin::wrappable following the guide at
https://code.google.com/p/chromium/codesearch#chromium/src/gin/wrappable.h&q=...
and cribbed off of some existing code.

[kmadhusu, 2015-10-05 19:59:01]

On 2015/10/05 19:06:30, Donn Denman wrote:
> Kausalya, not sure if you already got this, so resending.
> 
> This is ready for a first-pass review now.  PTAL.
> 
> Note that I was unable to add a new extension (ala Instant), because
extensions
> are no longer allowed (issue http://crbug.com/334679 - get rid of all v8
> extensions in Chrome), so I made it a gin::wrappable following the guide at
>
https://code.google.com/p/chromium/codesearch#chromium/src/gin/wrappable.h&q=...
> and cribbed off of some existing code.

Donn, I glanced over the changes. 

Few questions:
(1) Is contextual search page rendered in any special privileged process?
(2) In this CL, you are appending kContextualSearchProces switch if it is a
contextual search process and using the same to bind the required JS API. Is my
understanding correct?
(3) When we implemented Embeddedsearch API support in 2011/2012, there was no
other better way to identify Instant processes. That's why we proceeded with the
"AppendSwitch" approach. Did you ask around to see if there is any other better
way to identify these contextual search renderers?

Thanks.

[Donn Denman, 2015-10-06 01:18:37]

On 2015/10/05 19:59:01, kmadhusu wrote:
> On 2015/10/05 19:06:30, Donn Denman wrote:
> > Kausalya, not sure if you already got this, so resending.
> > 
> > This is ready for a first-pass review now.  PTAL.
> > 
> > Note that I was unable to add a new extension (ala Instant), because
> extensions
> > are no longer allowed (issue http://crbug.com/334679 - get rid of all v8
> > extensions in Chrome), so I made it a gin::wrappable following the guide at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/gin/wrappable.h&q=...
> > and cribbed off of some existing code.
> 
> Donn, I glanced over the changes. 
> 
> Few questions:
> (1) Is contextual search page rendered in any special privileged process?
No, it's a regular renderer process.

> (2) In this CL, you are appending kContextualSearchProces switch if it is a
> contextual search process and using the same to bind the required JS API. Is
my
> understanding correct?
That's right.

> (3) When we implemented Embeddedsearch API support in 2011/2012, there was no
> other better way to identify Instant processes. That's why we proceeded with
the
> "AppendSwitch" approach. Did you ask around to see if there is any other
better
> way to identify these contextual search renderers?
No, I didn't.  We'd talked about building this as a much-simpler version of
Instant,
so when I saw that Instant used the command-line-switch, and that it's fairly
widely used,
I just went with that existing mode of communication.  But you raise a good
point
that there may be more modern ways to communicate, so I'll start a thread on
that
and cc you.

[jochen (gone - plz use gerrit), 2015-10-07 13:49:25]

jochen@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-10-07 13:49:26]

https://chromiumcodereview.appspot.com/1385663002/diff/40001/chrome/renderer/...
File chrome/renderer/contextualsearch/contextual_search_wrapper.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/40001/chrome/renderer/...
chrome/renderer/contextualsearch/contextual_search_wrapper.cc:73: void
ContextualSearchWrapper::SetCaption(gin::Arguments* args) {
just use SetCaption(const std::string& caption) - gin will do the conversion for
you then

[Donn Denman, 2015-10-07 19:28:30]

donnd@chromium.org changed reviewers:
+ pedrosimonetti@chromium.org

[Donn Denman, 2015-10-07 19:33:11]

Jochen, thanks for looking at this!  PTAL at the renderer code.  If you have
suggestions for where to refactor boilerplate I'd be happy to try. 

Kausalya, Please review the contextual_search_service files.

Pedro, PTAL at all of this.

Thanks!

https://chromiumcodereview.appspot.com/1385663002/diff/40001/chrome/renderer/...
File chrome/renderer/contextualsearch/contextual_search_wrapper.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/40001/chrome/renderer/...
chrome/renderer/contextualsearch/contextual_search_wrapper.cc:73: void
ContextualSearchWrapper::SetCaption(gin::Arguments* args) {
On 2015/10/07 13:49:25, jochen wrote:
> just use SetCaption(const std::string& caption) - gin will do the conversion
for
> you then

Done.

[kmadhusu, 2015-10-08 22:05:02]

Thanks.

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
File chrome/browser/search/contextual_search_service.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
chrome/browser/search/contextual_search_service.cc:7: #include
"base/memory/singleton.h"
This header file is already included in the .h file.

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
chrome/browser/search/contextual_search_service.cc:54: process_id =
kInvalidProcessId;
Did you mean process_id_?

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
File chrome/browser/search/contextual_search_service.h (right):

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
chrome/browser/search/contextual_search_service.h:39: int process_id_;
I am not sure if I understood the purpose of this class clearly. Do you want to
instantiate this class per profile or per renderer? This class is not doing any
service. It is just a notification observer.

When you used Singleton traits, I thought you are going to store a map/list of
process_ids. To my surprise, you are just storing one id. 

Is it the case that you will have at most one contextual search process at any
point?

[Donn Denman, 2015-10-09 20:46:43]

Thanks for the review Kausalya!  PTAL

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
File chrome/browser/search/contextual_search_service.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
chrome/browser/search/contextual_search_service.cc:7: #include
"base/memory/singleton.h"
On 2015/10/08 22:05:02, kmadhusu wrote:
> This header file is already included in the .h file.

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
chrome/browser/search/contextual_search_service.cc:54: process_id =
kInvalidProcessId;
On 2015/10/08 22:05:01, kmadhusu wrote:
> Did you mean process_id_?

Nice catch!  Done.

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
File chrome/browser/search/contextual_search_service.h (right):

https://chromiumcodereview.appspot.com/1385663002/diff/60001/chrome/browser/s...
chrome/browser/search/contextual_search_service.h:39: int process_id_;
On 2015/10/08 22:05:02, kmadhusu wrote:
> I am not sure if I understood the purpose of this class clearly. Do you want
to
> instantiate this class per profile or per renderer? This class is not doing
any
> service. It is just a notification observer.
> 
> When you used Singleton traits, I thought you are going to store a map/list of
> process_ids. To my surprise, you are just storing one id. 
> 
> Is it the case that you will have at most one contextual search process at any
> point?

Right, this is a singleton class that just tracks which renderer(s) appear in a
Contextual Search Overlay, and therefor should enable our JS API.  Currently CS
only ever has one Overlay active, so this class only needs to track a single ID.
 But it was modeled after Instant, which tracks multiple IDs, and is a
full-fledged service, so that's a point of confusion.  

Maybe I should rename this class to ContextualSearchRendererTracker or something
like that?  I do think the current name does imply a global-singleton facility,
which seems useful.  Also it may need to expand slightly in order to be a
conduit to the Contextual Search Manager once messages from the wrapper are
being sent.

Pedro, do you have an opinion?

[pedro (no code reviews), 2015-10-20 01:24:41]

Hey Donn. Sorry for the delay.

https://codereview.chromium.org/1385663002/diff/60001/chrome/browser/search/c...
File chrome/browser/search/contextual_search_service.h (right):

https://codereview.chromium.org/1385663002/diff/60001/chrome/browser/search/c...
chrome/browser/search/contextual_search_service.h:39: int process_id_;
On 2015/10/09 20:46:43, Donn Denman wrote:
> On 2015/10/08 22:05:02, kmadhusu wrote:
> > I am not sure if I understood the purpose of this class clearly. Do you want
> to
> > instantiate this class per profile or per renderer? This class is not doing
> any
> > service. It is just a notification observer.
> > 
> > When you used Singleton traits, I thought you are going to store a map/list
of
> > process_ids. To my surprise, you are just storing one id. 
> > 
> > Is it the case that you will have at most one contextual search process at
any
> > point?
> 
> Right, this is a singleton class that just tracks which renderer(s) appear in
a
> Contextual Search Overlay, and therefor should enable our JS API.  Currently
CS
> only ever has one Overlay active, so this class only needs to track a single
ID.
>  But it was modeled after Instant, which tracks multiple IDs, and is a
> full-fledged service, so that's a point of confusion.  
> 
> Maybe I should rename this class to ContextualSearchRendererTracker or
something
> like that?  I do think the current name does imply a global-singleton
facility,
> which seems useful.  Also it may need to expand slightly in order to be a
> conduit to the Contextual Search Manager once messages from the wrapper are
> being sent.
> 
> Pedro, do you have an opinion?

I am not familiar with the renderer code, so I don't know what is the best way
to integrate our code in it. If it turns out we only need a Observer, we could
call this class ContextualSearchRendererObserver.

On a side note, what happens in multi window device? In that case, there could
be two ContextualSearch ContentViews displayed at the same time. Would that be a
problem? I don't know if they share the browser process.

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:867:
if (mPolicy.shouldInjectJavaScript()) {
I think we should move this logic to the OverlayPanelContent class since we're
isolating all content related logic there (and that class has a native
component).

I would be okay for addressing this in a followup CL though. In this case,
please put a TODO here.

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:879:
if (mSearchContentViewDelegate != null) {
Should we do anything when we destroy the contentview? If you are doing that
elsewhere, please ignore this comment.

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:313:
boolean shouldInjectJavaScript() {
Maybe calling it shouldProvideJavaScriptAPI would make its purpose clearer?

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:565:
switches::kContextualSearchProcess)) {
Is there a better way to identify a Contextual Search process? Checking for the
command line flag seems brittle.

[jochen (gone - plz use gerrit), 2015-10-20 08:09:56]

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/context...
File chrome/renderer/contextualsearch/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/context...
chrome/renderer/contextualsearch/contextual_search_wrapper.cc:22:
v8::Local<v8::Object> GetOrCreateChromeObject(v8::Isolate* isolate,
I'd move this to content/public/renderer/chrome_object_extensions_utils.h /
content/renderer/chrome_object_extensions_utils.cc

[Donn Denman, 2015-10-20 21:47:54]

Description was changed from

==========
[Contextual Search] Add JS API to set a caption in the Bar.

Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String);

Adds a simple Contextual Search Service to handle enabling of the v8 wrapper
only on a renderer which is presented in the Contextual Search Overlay view.

This functionality is disabled behind a new Finch flag needed to enable this
feature, for now.

Still to be done: sending the caption from the wrapper back to the Browser
process.

BUG=523554
==========

to

==========
[Contextual Search] Add JS API to set a caption in the Bar.

Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String);

Adds a simple Contextual Search API controller to handle enabling of the v8
wrapper only on a renderer which is presented in the Contextual Search Overlay
view.

This functionality is disabled behind a new Finch flag needed to enable this
feature, for now.

Still to be done: sending the caption from the wrapper back to the Browser
process.

BUG=523554
==========

[Donn Denman, 2015-10-20 21:56:46]

jochen and pedro, PTAL, and thanks for the reviews!

https://codereview.chromium.org/1385663002/diff/60001/chrome/browser/search/c...
File chrome/browser/search/contextual_search_service.h (right):

https://codereview.chromium.org/1385663002/diff/60001/chrome/browser/search/c...
chrome/browser/search/contextual_search_service.h:39: int process_id_;
On 2015/10/20 01:24:41, pedrosimonetti wrote:
> On 2015/10/09 20:46:43, Donn Denman wrote:
> > On 2015/10/08 22:05:02, kmadhusu wrote:
> > > I am not sure if I understood the purpose of this class clearly. Do you
want
> > to
> > > instantiate this class per profile or per renderer? This class is not
doing
> > any
> > > service. It is just a notification observer.
> > > 
> > > When you used Singleton traits, I thought you are going to store a
map/list
> of
> > > process_ids. To my surprise, you are just storing one id. 
> > > 
> > > Is it the case that you will have at most one contextual search process at
> any
> > > point?
> > 
> > Right, this is a singleton class that just tracks which renderer(s) appear
in
> a
> > Contextual Search Overlay, and therefor should enable our JS API.  Currently
> CS
> > only ever has one Overlay active, so this class only needs to track a single
> ID.
> >  But it was modeled after Instant, which tracks multiple IDs, and is a
> > full-fledged service, so that's a point of confusion.  
> > 
> > Maybe I should rename this class to ContextualSearchRendererTracker or
> something
> > like that?  I do think the current name does imply a global-singleton
> facility,
> > which seems useful.  Also it may need to expand slightly in order to be a
> > conduit to the Contextual Search Manager once messages from the wrapper are
> > being sent.
> > 
> > Pedro, do you have an opinion?
> 
> I am not familiar with the renderer code, so I don't know what is the best way
> to integrate our code in it. If it turns out we only need a Observer, we could
> call this class ContextualSearchRendererObserver.
Renamed ContextualSearchApiController, which seemed more descriptive of what it
does (rather than describing how it does that).

> 
> On a side note, what happens in multi window device? In that case, there could
> be two ContextualSearch ContentViews displayed at the same time. Would that be
a
> problem? I don't know if they share the browser process.
Although this appears to still be an open question, it should work fine even
with a shared browser process.  There's a very narrow window between when this
ID gets set during overlay creation and when it's checked during renderer
creation for that overlay, so a single value only needs to be valid in that
window.  I think it would be very hard for a user to manipulate both windows on
the device to do a Contextual Search simultaneously.

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java
(right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:867:
if (mPolicy.shouldInjectJavaScript()) {
On 2015/10/20 01:24:41, pedrosimonetti wrote:
> I think we should move this logic to the OverlayPanelContent class since we're
> isolating all content related logic there (and that class has a native
> component).
> 
> I would be okay for addressing this in a followup CL though. In this case,
> please put a TODO here.

Adding a TODO, for now.

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchManager.java:879:
if (mSearchContentViewDelegate != null) {
On 2015/10/20 01:24:41, pedrosimonetti wrote:
> Should we do anything when we destroy the contentview? If you are doing that
> elsewhere, please ignore this comment.

Yep, taken care of elsewhere.

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:313:
boolean shouldInjectJavaScript() {
On 2015/10/20 01:24:41, pedrosimonetti wrote:
> Maybe calling it shouldProvideJavaScriptAPI would make its purpose clearer?

Done.

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/chrome_...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/chrome_...
chrome/renderer/chrome_content_renderer_client.cc:565:
switches::kContextualSearchProcess)) {
On 2015/10/20 01:24:41, pedrosimonetti wrote:
> Is there a better way to identify a Contextual Search process? Checking for
the
> command line flag seems brittle.

No better way that I know of.  I don't think it's brittle since there's a
dedicated flag for this, but it did seem hacky to me when I first saw it in use.

There are advantages to this approch, the main ones being that we're guaranteed
to get the setting established before renderer startup, and that it's fairly
widely used, IMHO.

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/context...
File chrome/renderer/contextualsearch/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/80001/chrome/renderer/context...
chrome/renderer/contextualsearch/contextual_search_wrapper.cc:22:
v8::Local<v8::Object> GetOrCreateChromeObject(v8::Isolate* isolate,
On 2015/10/20 08:09:56, jochen wrote:
> I'd move this to content/public/renderer/chrome_object_extensions_utils.h /
> content/renderer/chrome_object_extensions_utils.cc

I tried this, but I couldn't get it to link, so I'd like to address this in a
follow-on CL.  Possibly I need to add some dependencies in order for Chrome to
access content/.

[Donn Denman, 2015-10-21 20:02:05]

Pedro, PTAL at this latest patch set.  As we discussed f2f, one of your comments
got us investigating what happens in mutli-window environments.  This latest
patch switches back to using a vector of processes to enable the API, since
multiple windows share the same browser process.

[jochen (gone - plz use gerrit), 2015-10-22 12:04:53]

jochen@chromium.org changed reviewers:
+ creis@chromium.org

[jochen (gone - plz use gerrit), 2015-10-22 12:04:53]

+creis for process management

I'd rather not have new notification observers here. I also wonder how you
ensure that nothing else is hosted in your search processes?

[Donn Denman, 2015-10-22 21:00:24]

On 2015/10/22 12:04:53, jochen wrote:
> +creis for process management
> 
> I'd rather not have new notification observers here.
Changed the CS API controller to no longer be a notification observer -- now it
is explicitly notified when the renderer is destroyed.

> I also wonder how you
> ensure that nothing else is hosted in your search processes.
Contextual Search only loads Search requests into the overlay. If navigation
takes place, then the overlay is destroyed and that navigation is done in a
separate Tab.

[Donn Denman, 2015-10-22 21:01:27]

Pedro, PTAL.  Removing the notification observer made the API controller a lot
simpler, but shifted some code into the Overlay.

[pedro (no code reviews), 2015-10-23 05:43:54]

lgtm

LGTM for chrome/android and chrome/browser changes.

I'll leave the renderer changes for the other reviewers more experienced with
this part of the codebase.

https://codereview.chromium.org/1385663002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1385663002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:233:
public void destroyContentView() {
FYI: This method is now private, I think you need to sync and rebase your patch.

https://codereview.chromium.org/1385663002/diff/140001/chrome/browser/android...
File chrome/browser/android/contextualsearch/contextual_search_manager.cc
(right):

https://codereview.chromium.org/1385663002/diff/140001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_manager.cc:151:
ContextualSearchApiController::GetInstance()->AddContextualSearchProcess(
Nit: What is being added is an ID, not a process, so the name should reflect
that. Also, the "ContextualSearch" part of the name is redundant since the
Controller class is specific to ContextualSearch. What about
AddProcessId/RemoveProcessId?

[Donn Denman, 2015-10-23 18:58:07]

Kausalya and Charlie, PTAL.

Pedro, thanks for the review!

https://codereview.chromium.org/1385663002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1385663002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:233:
public void destroyContentView() {
On 2015/10/23 05:43:54, pedrosimonetti wrote:
> FYI: This method is now private, I think you need to sync and rebase your
patch.

Done.

https://codereview.chromium.org/1385663002/diff/140001/chrome/browser/android...
File chrome/browser/android/contextualsearch/contextual_search_manager.cc
(right):

https://codereview.chromium.org/1385663002/diff/140001/chrome/browser/android...
chrome/browser/android/contextualsearch/contextual_search_manager.cc:151:
ContextualSearchApiController::GetInstance()->AddContextualSearchProcess(
On 2015/10/23 05:43:54, pedrosimonetti wrote:
> Nit: What is being added is an ID, not a process, so the name should reflect
> that. Also, the "ContextualSearch" part of the name is redundant since the
> Controller class is specific to ContextualSearch. What about
> AddProcessId/RemoveProcessId?

Done.

[Donn Denman, 2015-10-28 00:26:56]

Jochen and Charlie, here's a quick question for each of you.  

Thanks for the help!

https://chromiumcodereview.appspot.com/1385663002/diff/160001/chrome/renderer...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/160001/chrome/renderer...
chrome/renderer/chrome_content_renderer_client.cc:520: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
Charlie, I'm thinking that we should change this to trigger off of mojo (like
Dom Distiller) instead of a command-line flag, and that should keep us from
adding our JS API based on the render process.  I would think that then the
reusing of the process would not be an issue.  Does that sound right to you?

https://chromiumcodereview.appspot.com/1385663002/diff/160001/chrome/renderer...
chrome/renderer/chrome_content_renderer_client.cc:534: new
dom_distiller::DistillerJsRenderFrameObserver(
Here's how Dom Distiller enables its API.

https://chromiumcodereview.appspot.com/1385663002/diff/160001/chrome/renderer...
File chrome/renderer/contextualsearch/contextual_search_wrapper.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/160001/chrome/renderer...
chrome/renderer/contextualsearch/contextual_search_wrapper.cc:63:
chrome->Set(gin::StringToV8(isolate, kContextualSearchObjectName),
Johen, Can you tell me when this change to the chrome object gets undone or no
longer applies?  Specifically, are we modifying a copy of the chrome object
that's scoped to the given WebFrame, and so our changes will no longer apply
once that frame is destroyed?  Or should we be removing this setter at some
point?  E.g. we could instead pass in a RenderFrame and use a
RenderFrameObserver to unplug our API when the contents are destroyed or
didClearWindowObject is called, etc.

[jochen (gone - plz use gerrit), 2015-10-28 14:23:53]

https://codereview.chromium.org/1385663002/diff/160001/chrome/renderer/contex...
File chrome/renderer/contextualsearch/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/160001/chrome/renderer/contex...
chrome/renderer/contextualsearch/contextual_search_wrapper.cc:63:
chrome->Set(gin::StringToV8(isolate, kContextualSearchObjectName),
On 2015/10/28 at 00:26:56, Donn Denman wrote:
> Johen, Can you tell me when this change to the chrome object gets undone or no
longer applies?  Specifically, are we modifying a copy of the chrome object
that's scoped to the given WebFrame, and so our changes will no longer apply
once that frame is destroyed?  Or should we be removing this setter at some
point?  E.g. we could instead pass in a RenderFrame and use a
RenderFrameObserver to unplug our API when the contents are destroyed or
didClearWindowObject is called, etc.

we never reuse a context - if a frame is navigated, the context is destroyed.
(tl;dr no need to ever remove this)

[Charlie Reis, 2015-10-28 19:51:44]

https://codereview.chromium.org/1385663002/diff/160001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1385663002/diff/160001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:520: if
(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2015/10/28 00:26:56, Donn Denman wrote:
> Charlie, I'm thinking that we should change this to trigger off of mojo (like
> Dom Distiller) instead of a command-line flag, and that should keep us from
> adding our JS API based on the render process.  I would think that then the
> reusing of the process would not be an issue.  Does that sound right to you?

Scoping it to a RenderFrame isn't a bad idea, but it won't help with the concern
about compromised renderer processes.  Once a malicious page takes over a
renderer process, it can pretend to be any RenderFrame in the process.  We
assume attackers can execute arbitrary code in the renderer process.

I think you'll still need to prevent process reuse for the overlay process.

[Donn Denman, 2015-10-30 18:03:10]

donnd@chromium.org changed reviewers:
- kmadhusu@chromium.org

[Donn Denman, 2015-11-16 22:53:47]

Description was changed from

==========
[Contextual Search] Add JS API to set a caption in the Bar.

Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String);

Adds a simple Contextual Search API controller to handle enabling of the v8
wrapper only on a renderer which is presented in the Contextual Search Overlay
view.

This functionality is disabled behind a new Finch flag needed to enable this
feature, for now.

Still to be done: sending the caption from the wrapper back to the Browser
process.

BUG=523554
==========

to

==========
[Contextual Search] Add mojo-enabled API component.
    
Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String);
    
Adds a simple Contextual Search component with mojo communication to handle
enabling of the v8 wrapper only on a renderer which is presented in the
Contextual Search Overlay view.
    
BUG=523554
==========

[Donn Denman, 2015-11-16 23:18:40]

donnd@chromium.org changed reviewers:
+ stuartmorgan@chromium.org

[Donn Denman, 2015-11-16 23:18:40]

Stuart, I'm adding a new component in the latest version of this CL, and I've
never worked on components before so I don't really know how they are usually
structured.  Do you have time to give this a quick review for structure only?

[stuartmorgan, 2015-11-16 23:45:28]

My components experience is pretty light; if you're looking for high-quality
feedback about the structure of a new component, a component/OWNER would be the
best source.

[Donn Denman, 2015-11-17 00:14:37]

donnd@chromium.org changed reviewers:
- stuartmorgan@chromium.org

[Donn Denman, 2015-11-17 00:24:24]

Jochen, Please take another look at this CL.  It has changed significantly, but
the wrapper is essentially unchanged.

I've reworked the code to be all in a new Contextual Search component, use Mojo
for the communication, and removed references to the Contextual Search Manager. 
However, I don't have any experience with Components, so it would be great if
you can take a close look at how this is structured.

In case it's helpful, here's the thread where we were discussing security issues
surrounding this change:
https://groups.google.com/a/google.com/forum/#!searchin/contextual-search-eng...

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_wrapper.cc:24: // TODO(donnd):
merge with the identical code in
I tried your suggestion to "move this to
content/public/renderer/chrome_object_extensions_utils.h /
content/renderer/chrome_object_extensions_utils.cc",
but couldn't get it to link.  Maybe it would be best to do this separately
anyway so we can easily update the other clients.

[jochen (gone - plz use gerrit), 2015-11-17 14:58:12]

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/DEPS (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/DEPS:2: "+content/public",
please don't mix browser and renderer code. Instead, create two sub directories
browser and renderer (or three - common if there's shared code), and move the
respective files there

there should also be separate gn/gyp targets for renderer browser

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/OWNERS (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/OWNERS:2: pedrosimonetti@chromium.org
only chromium committers can be in owners files

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/OWNERS:3: twellington@chromium.org
please add a per-file rule for the *.mojom restricting it to ipc reviewers:

# Changes to Mojo interfaces require a security review to avoid
# introducing new sandbox escapes.
per-file *.mojom=set noparent
per-file *.mojom=palmer@chromium.org
per-file *.mojom=tsepez@chromium.org

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/contextual_search_api_service_impl.h (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_api_service_impl.h:26: // Handle
a call from the JS API to set the caption.
that comment belongs on the base class

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_api_service_impl.h:34: };
disallow copy/assign for all classes

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_wrapper.cc:21: static const char
kContextualSearchObjectName[] = "contextualSearch";
prefer anonymous namespace over file-local static

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_wrapper.cc:24: // TODO(donnd):
merge with the identical code in
On 2015/11/17 at 00:24:24, Donn Denman wrote:
> I tried your suggestion to "move this to
content/public/renderer/chrome_object_extensions_utils.h /
> content/renderer/chrome_object_extensions_utils.cc",
> but couldn't get it to link.  Maybe it would be best to do this separately
anyway so we can easily update the other clients.

what's the link error you got?

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/overlay_js_render_frame_observer.cc (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/overlay_js_render_frame_observer.cc:45: DVLOG(0) <<
"ctxs OverlayJsRenderFrameObserver::DidCreateNewDocument for "
0 is too noisy, please use at least 1. or consider removing those log statements
- this looks more like local debugging code

[Donn Denman, 2015-11-19 01:11:29]

Jochen, PTAL, and thank you very much for the review!

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/DEPS (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/DEPS:2: "+content/public",
On 2015/11/17 14:58:11, jochen wrote:
> please don't mix browser and renderer code. Instead, create two sub
directories
> browser and renderer (or three - common if there's shared code), and move the
> respective files there
> 
> there should also be separate gn/gyp targets for renderer browser

Done.

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/OWNERS (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/OWNERS:2: pedrosimonetti@chromium.org
On 2015/11/17 14:58:11, jochen wrote:
> only chromium committers can be in owners files

Pedro and I are both new Chromium committers.

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/OWNERS:3: twellington@chromium.org
On 2015/11/17 14:58:11, jochen wrote:
> please add a per-file rule for the *.mojom restricting it to ipc reviewers:
> 
> # Changes to Mojo interfaces require a security review to avoid
> # introducing new sandbox escapes.
> per-file *.mojom=set noparent
> per-file mailto:*.mojom=palmer@chromium.org
> per-file mailto:*.mojom=tsepez@chromium.org

Done.

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/contextual_search_api_service_impl.h (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_api_service_impl.h:26: // Handle
a call from the JS API to set the caption.
On 2015/11/17 14:58:12, jochen wrote:
> that comment belongs on the base class

Done.

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_api_service_impl.h:34: };
On 2015/11/17 14:58:11, jochen wrote:
> disallow copy/assign for all classes

Done for this class and all others in this CL.

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_wrapper.cc:21: static const char
kContextualSearchObjectName[] = "contextualSearch";
On 2015/11/17 14:58:12, jochen wrote:
> prefer anonymous namespace over file-local static

Done.

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_wrapper.cc:24: // TODO(donnd):
merge with the identical code in
On 2015/11/17 14:58:12, jochen wrote:
> On 2015/11/17 at 00:24:24, Donn Denman wrote:
> > I tried your suggestion to "move this to
> content/public/renderer/chrome_object_extensions_utils.h /
> > content/renderer/chrome_object_extensions_utils.cc",
> > but couldn't get it to link.  Maybe it would be best to do this separately
> anyway so we can easily update the other clients.
> 
> what's the link error you got?

An undefined reference here to content::GetOrCreateChromeObject.  This is likely
due to the fact that I don't really know how to build proper dependency rules.

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/overlay_js_render_frame_observer.cc (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/overlay_js_render_frame_observer.cc:45: DVLOG(0) <<
"ctxs OverlayJsRenderFrameObserver::DidCreateNewDocument for "
On 2015/11/17 14:58:12, jochen wrote:
> 0 is too noisy, please use at least 1. or consider removing those log
statements
> - this looks more like local debugging code

Done - removed.

[jochen (gone - plz use gerrit), 2015-11-19 09:22:18]

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_wrapper.cc:24: // TODO(donnd):
merge with the identical code in
On 2015/11/19 at 01:11:29, Donn Denman wrote:
> On 2015/11/17 14:58:12, jochen wrote:
> > On 2015/11/17 at 00:24:24, Donn Denman wrote:
> > > I tried your suggestion to "move this to
> > content/public/renderer/chrome_object_extensions_utils.h /
> > > content/renderer/chrome_object_extensions_utils.cc",
> > > but couldn't get it to link.  Maybe it would be best to do this separately
> > anyway so we can easily update the other clients.
> > 
> > what's the link error you got?
> 
> An undefined reference here to content::GetOrCreateChromeObject.  This is
likely due to the fact that I don't really know how to build proper dependency
rules.

Sounds like you forgot to put CONTENT_EXPORT in front of the declaration?

[Donn Denman, 2015-11-19 17:48:03]

donnd@chromium.org changed reviewers:
+ mdjones@chromium.org

[Donn Denman, 2015-11-19 17:48:04]

Matt, would you mind taking a look?  Thanks!

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
File components/contextual_search/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/180001/components/contextual_...
components/contextual_search/contextual_search_wrapper.cc:24: // TODO(donnd):
merge with the identical code in
On 2015/11/19 09:22:17, jochen wrote:
> On 2015/11/19 at 01:11:29, Donn Denman wrote:
> > On 2015/11/17 14:58:12, jochen wrote:
> > > On 2015/11/17 at 00:24:24, Donn Denman wrote:
> > > > I tried your suggestion to "move this to
> > > content/public/renderer/chrome_object_extensions_utils.h /
> > > > content/renderer/chrome_object_extensions_utils.cc",
> > > > but couldn't get it to link.  Maybe it would be best to do this
separately
> > > anyway so we can easily update the other clients.
> > > 
> > > what's the link error you got?
> > 
> > An undefined reference here to content::GetOrCreateChromeObject.  This is
> likely due to the fact that I don't really know how to build proper dependency
> rules.
> 
> Sounds like you forgot to put CONTENT_EXPORT in front of the declaration?

LOL!  I'll give that a try!

[mdjones, 2015-11-19 20:31:24]

https://codereview.chromium.org/1385663002/diff/200001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1385663002/diff/200001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:116: "//components/contextual_search:browser",
Shouldn't this be in the non-iOS section below? Your gypi file specifies
non-iOS.

https://codereview.chromium.org/1385663002/diff/200001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/1385663002/diff/200001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:40: "//components/contextual_search:renderer",
Same as browser; I think this belongs in the non-iOS section.

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
File components/contextual_search/browser/contextual_search_api_service_impl.h
(right):

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
components/contextual_search/browser/contextual_search_api_service_impl.h:11:
#include "content/public/browser/render_frame_host.h"
Is this include needed?

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
File components/contextual_search/common/contextual_search_api_service.mojom
(right):

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
components/contextual_search/common/contextual_search_api_service.mojom:9:
interface ContextualSearchApiService {
I would consider adding "js" or "javascript" somewhere in this name unless it is
to be used for something else; "api" is a little vague.

[Donn Denman, 2015-11-20 00:01:40]

donnd@chromium.org changed reviewers:
+ marq@chromium.org

[Donn Denman, 2015-11-20 00:01:41]

Dr Jones, PTAL, and thanks for the review!

https://codereview.chromium.org/1385663002/diff/200001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1385663002/diff/200001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:116: "//components/contextual_search:browser",
On 2015/11/19 20:31:24, mdjones wrote:
> Shouldn't this be in the non-iOS section below? Your gypi file specifies
> non-iOS.

Yes, I think you're right.  Adding our iOS eng to this review, in case that
helps.

https://codereview.chromium.org/1385663002/diff/200001/chrome/renderer/BUILD.gn
File chrome/renderer/BUILD.gn (right):

https://codereview.chromium.org/1385663002/diff/200001/chrome/renderer/BUILD....
chrome/renderer/BUILD.gn:40: "//components/contextual_search:renderer",
On 2015/11/19 20:31:24, mdjones wrote:
> Same as browser; I think this belongs in the non-iOS section.

Done.

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
File components/contextual_search/browser/contextual_search_api_service_impl.h
(right):

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
components/contextual_search/browser/contextual_search_api_service_impl.h:11:
#include "content/public/browser/render_frame_host.h"
On 2015/11/19 20:31:24, mdjones wrote:
> Is this include needed?

Done.

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
File components/contextual_search/common/contextual_search_api_service.mojom
(right):

https://codereview.chromium.org/1385663002/diff/200001/components/contextual_...
components/contextual_search/common/contextual_search_api_service.mojom:9:
interface ContextualSearchApiService {
On 2015/11/19 20:31:24, mdjones wrote:
> I would consider adding "js" or "javascript" somewhere in this name unless it
is
> to be used for something else; "api" is a little vague.

I wasn't sure, with everything we went through, that the API would actually BE
done in JS.  But now I think we do have some confidence on that, so I'll take
your suggestion.

[mdjones, 2015-11-20 17:48:47]

lgtm

[Donn Denman, 2015-11-20 22:22:21]

Jochen, PTAL.

I added another parameter to the setCaption API call, and updated the code to
use GetOrCreateChromeObject from the new file location in public.

[jochen (gone - plz use gerrit), 2015-11-24 10:43:35]

https://codereview.chromium.org/1385663002/diff/240001/chrome/browser/DEPS
File chrome/browser/DEPS (right):

https://codereview.chromium.org/1385663002/diff/240001/chrome/browser/DEPS#ne...
chrome/browser/DEPS:34: "+components/contextual_search",
restrict to /browser

https://codereview.chromium.org/1385663002/diff/240001/chrome/renderer/DEPS
File chrome/renderer/DEPS (right):

https://codereview.chromium.org/1385663002/diff/240001/chrome/renderer/DEPS#n...
chrome/renderer/DEPS:9: "+components/contextual_search",
restrict to /renderer and

https://codereview.chromium.org/1385663002/diff/240001/components/contextual_...
File components/contextual_search/renderer/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/240001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.cc:62: :
render_frame_(render_frame) {}
the wrapper might outlive the render frame, you need to observe it

https://codereview.chromium.org/1385663002/diff/240001/components/contextual_...
File components/contextual_search/renderer/contextual_search_wrapper.h (right):

https://codereview.chromium.org/1385663002/diff/240001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.h:35:
ContextualSearchWrapper(content::RenderFrame* render_frame);
explicit

https://codereview.chromium.org/1385663002/diff/240001/components/contextual_...
File components/contextual_search/renderer/overlay_js_render_frame_observer.h
(right):

https://codereview.chromium.org/1385663002/diff/240001/components/contextual_...
components/contextual_search/renderer/overlay_js_render_frame_observer.h:24:
OverlayJsRenderFrameObserver(content::RenderFrame* render_frame);
explicit

[Donn Denman, 2015-12-02 03:29:43]

Description was changed from

==========
[Contextual Search] Add mojo-enabled API component.
    
Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String);
    
Adds a simple Contextual Search component with mojo communication to handle
enabling of the v8 wrapper only on a renderer which is presented in the
Contextual Search Overlay view.
    
BUG=523554
==========

to

==========
[Contextual Search] Add mojo-enabled API component.
    
Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String, Boolean);
    
Adds a simple Contextual Search component with mojo communication to handle
enabling of the v8 wrapper only on a renderer which is presented in the
Contextual Search Overlay view.
    
BUG=523554
==========

[Donn Denman, 2015-12-02 03:32:48]

Jochen, thanks for the review!

Does it look like I'm taking the approach you suggested  when you wrote "> the
wrapper might outlive the render frame, you need to observe it"?  I've updated
the existing observer to reset the wrapper, but that render frame observer
doesn't get called as I'd expect (see comment below for details).  I'll keep
working on this aspect, but wanted to check if I understood your previous
comment correctly.  Thanks!

https://chromiumcodereview.appspot.com/1385663002/diff/240001/chrome/browser/...
File chrome/browser/DEPS (right):

https://chromiumcodereview.appspot.com/1385663002/diff/240001/chrome/browser/...
chrome/browser/DEPS:34: "+components/contextual_search",
On 2015/11/24 10:43:35, jochen wrote:
> restrict to /browser

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/240001/components/cont...
File components/contextual_search/renderer/contextual_search_wrapper.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/240001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.cc:62: :
render_frame_(render_frame) {}
On 2015/11/24 10:43:35, jochen wrote:
> the wrapper might outlive the render frame, you need to observe it

Ah, thanks for catching that.

How does this look?

https://chromiumcodereview.appspot.com/1385663002/diff/240001/components/cont...
File components/contextual_search/renderer/contextual_search_wrapper.h (right):

https://chromiumcodereview.appspot.com/1385663002/diff/240001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.h:35:
ContextualSearchWrapper(content::RenderFrame* render_frame);
On 2015/11/24 10:43:35, jochen wrote:
> explicit

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/240001/components/cont...
File components/contextual_search/renderer/overlay_js_render_frame_observer.h
(right):

https://chromiumcodereview.appspot.com/1385663002/diff/240001/components/cont...
components/contextual_search/renderer/overlay_js_render_frame_observer.h:24:
OverlayJsRenderFrameObserver(content::RenderFrame* render_frame);
On 2015/11/24 10:43:35, jochen wrote:
> explicit

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
File components/contextual_search/renderer/overlay_js_render_frame_observer.cc
(right):

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
components/contextual_search/renderer/overlay_js_render_frame_observer.cc:59:
void OverlayJsRenderFrameObserver::FrameWillClose() {
This doesn't get called when I expect it to (when the frame goes away).  I'll
keep working on trying to figure this out, but let me know if you happen to have
a guess why!

[jochen (gone - plz use gerrit), 2015-12-02 12:59:15]

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
File components/contextual_search/renderer/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.cc:53:
gin::CreateHandle(isolate, result.get());
CreateHandle() passes ownership of the object to v8 which will delete it
whenever it sees fit (and an undefined point after the render frame died)

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
File components/contextual_search/renderer/contextual_search_wrapper.h (right):

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.h:25: class
ContextualSearchWrapper : public gin::Wrappable<ContextualSearchWrapper> {
this class should derive from RenderFrameObserver, override OnDestruct() and use
render_frame() and NULL-check it

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.h:28: static
scoped_ptr<ContextualSearchWrapper> Install(
should return bool or boid

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.h:34:
~ContextualSearchWrapper() override;
should be private

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
File components/contextual_search/renderer/overlay_js_render_frame_observer.cc
(right):

https://codereview.chromium.org/1385663002/diff/260001/components/contextual_...
components/contextual_search/renderer/overlay_js_render_frame_observer.cc:47:
contextual_search::ContextualSearchWrapper::Install(render_frame());
this should happen in DidClearWindowObject()

[Donn Denman, 2015-12-03 01:21:09]

Jochen, PTAL, I feel like this is getting close.

There just seems to be one problem, which is our RenderFrameObserver appears to
be getting detached: it is called for all the expected messages as the page
loads, but then doesn't get called later as expected, e.g. for OnDestruct.  I
think this observer notification problem is due to our WebContents being
displayed in an Overlay, and is probably orthogonal to this change: I tested the
gin::Handle.Clear() from an earlier observer override, so I think everything is
set up the way it should be and will work correctly when called. I will pursue
the observer problem in parallel tomorrow (and beyond).  Let me know if you want
to be cc'ed on that.

Thanks so much for your great help with this!

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
File components/contextual_search/renderer/contextual_search_wrapper.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.cc:53:
gin::CreateHandle(isolate, result.get());
On 2015/12/02 12:59:15, jochen wrote:
> CreateHandle() passes ownership of the object to v8 which will delete it
> whenever it sees fit (and an undefined point after the render frame died)

OK, makes sense.

Changed to store the gin Handle, and clearing it OnDestruct.

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
File components/contextual_search/renderer/contextual_search_wrapper.h (right):

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.h:25: class
ContextualSearchWrapper : public gin::Wrappable<ContextualSearchWrapper> {
On 2015/12/02 12:59:15, jochen wrote:
> this class should derive from RenderFrameObserver, override OnDestruct() and
use
> render_frame() and NULL-check it

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.h:28: static
scoped_ptr<ContextualSearchWrapper> Install(
On 2015/12/02 12:59:15, jochen wrote:
> should return bool or boid

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.h:34:
~ContextualSearchWrapper() override;
On 2015/12/02 12:59:15, jochen wrote:
> should be private

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
File components/contextual_search/renderer/overlay_js_render_frame_observer.cc
(right):

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
components/contextual_search/renderer/overlay_js_render_frame_observer.cc:47:
contextual_search::ContextualSearchWrapper::Install(render_frame());
On 2015/12/02 12:59:15, jochen wrote:
> this should happen in DidClearWindowObject()

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/260001/components/cont...
components/contextual_search/renderer/overlay_js_render_frame_observer.cc:59:
void OverlayJsRenderFrameObserver::FrameWillClose() {
On 2015/12/02 03:32:48, Donn Denman wrote:
> This doesn't get called when I expect it to (when the frame goes away).  I'll
> keep working on trying to figure this out, but let me know if you happen to
have
> a guess why!

There seems to be a problem with our RenderFrameObserver messages that's causing
this to not be called when expected.  Also affects OnDestruct.  I will keep
working on that, but I think it's orthogonal to this change.

[Donn Denman, 2015-12-03 23:57:23]

On 2015/12/03 01:21:09, Donn Denman wrote:
> Jochen, PTAL, I feel like this is getting close.
> 
> There just seems to be one problem, which is our RenderFrameObserver appears
to
> be getting detached: it is called for all the expected messages as the page
> loads, but then doesn't get called later as expected, e.g. for OnDestruct.

Now we understand what's happening with RenderFrameObserver#OnDestruct not being
called -- on Android the entire renderer process is killed so destructors don't
get called.  (Details in this thread
https://groups.google.com/a/google.com/forum/#!topic/contextual-search-eng/Ee...).

> I tested the
> gin::Handle.Clear() from an earlier observer override, so I think everything
is
> set up the way it should be and will work correctly when called.
So even though the gin::Handle.Clear() isn't called on Android, it was tested on
Android, and the right code is in place for Desktop (in case the feature is
enabled there some day).  Since the Wrapper lives in the renderer it will get
destroyed along with the RenderFrame.

Sorry I didn't know about this complication.

[jochen (gone - plz use gerrit), 2015-12-04 13:21:36]

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
File components/contextual_search/renderer/contextual_search_wrapper.cc (right):

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.cc:59:
wrapped_object->SetGinHandle(wrapper);
don't hold on to the wrapper

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.cc:78:
render_frame()->GetServiceRegistry()->ConnectToRemoteService(
you have to null-check render_frame() before using it

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.cc:89: if
(!render_frame() && !gin_handle_.IsEmpty()) {
this method should be empty, it's just avoid that the base class does "delete
this;"

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.cc:96:
EnsureServiceConnected();
if render_frame() is null, you can't SetCaption

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
File components/contextual_search/renderer/contextual_search_wrapper.h (right):

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.h:37: void
SetGinHandle(gin::Handle<ContextualSearchWrapper> wrapper);
delete this

https://codereview.chromium.org/1385663002/diff/280001/components/contextual_...
components/contextual_search/renderer/contextual_search_wrapper.h:60:
gin::Handle<ContextualSearchWrapper> gin_handle_;
delete this

[Donn Denman, 2015-12-09 00:57:33]

Jochen, PTAL.

Sorry you needed to point out that needed cleanup in my previous patch.  I sure
appreciate your patience and help with this!

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
File components/contextual_search/renderer/contextual_search_wrapper.cc (right):

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.cc:59:
wrapped_object->SetGinHandle(wrapper);
On 2015/12/04 13:21:36, jochen wrote:
> don't hold on to the wrapper

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.cc:78:
render_frame()->GetServiceRegistry()->ConnectToRemoteService(
On 2015/12/04 13:21:35, jochen wrote:
> you have to null-check render_frame() before using it

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.cc:89: if
(!render_frame() && !gin_handle_.IsEmpty()) {
On 2015/12/04 13:21:36, jochen wrote:
> this method should be empty, it's just avoid that the base class does "delete
> this;"

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.cc:96:
EnsureServiceConnected();
On 2015/12/04 13:21:36, jochen wrote:
> if render_frame() is null, you can't SetCaption

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
File components/contextual_search/renderer/contextual_search_wrapper.h (right):

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.h:37: void
SetGinHandle(gin::Handle<ContextualSearchWrapper> wrapper);
On 2015/12/04 13:21:36, jochen wrote:
> delete this

Done.

https://chromiumcodereview.appspot.com/1385663002/diff/280001/components/cont...
components/contextual_search/renderer/contextual_search_wrapper.h:60:
gin::Handle<ContextualSearchWrapper> gin_handle_;
On 2015/12/04 13:21:36, jochen wrote:
> delete this

Done.

[jochen (gone - plz use gerrit), 2015-12-09 15:40:29]

lgtm

[Donn Denman, 2015-12-09 17:30:07]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2015-12-09 17:30:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
pedrosimonetti@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/1385663002/#ps300001
(title: "Check that render_frame() is not null, and cleaup OnDestruct code.")

[commit-bot: I haz the power, 2015-12-09 17:33:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1385663002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1385663002/300001

[commit-bot: I haz the power, 2015-12-09 17:39:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 17:40:01]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Donn Denman, 2015-12-09 23:24:11]

donnd@chromium.org changed reviewers:
+ yzshen@chromium.org

[Donn Denman, 2015-12-09 23:24:12]

yzshen@, can you give OWNERS approval for including mojo/public in our DEPS in
this new contextual_search component in components/contextual_search/DEPS?

DETAILS: I'm getting this warning on upload:
** Presubmit Messages **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+mojo/public',
Suggested missing target path OWNERS:
    yzshen@chromium.org

[Donn Denman, 2015-12-09 23:24:24]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2015-12-09 23:24:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, jochen@chromium.org, pedrosimonetti@chromium.org
Link to the patchset:
https://chromiumcodereview.appspot.com/1385663002/#ps320001 (title: "Added a
couple of missing dependencies to the new component's BUILD.gn rules for our
renderer secti…")

[commit-bot: I haz the power, 2015-12-09 23:26:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1385663002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1385663002/320001

[commit-bot: I haz the power, 2015-12-09 23:37:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 23:37:35]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Donn Denman, 2015-12-09 23:47:04]

The CQ bit was checked by donnd@chromium.org

[Donn Denman, 2015-12-09 23:47:05]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
pedrosimonetti@chromium.org, mdjones@chromium.org
Link to the patchset: https://codereview.chromium.org/1385663002/#ps340001
(title: "Added a few more missing dependencies to the new component's BUILD.gn
rules for our renderer sectio…")

[commit-bot: I haz the power, 2015-12-09 23:48:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1385663002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1385663002/340001

[commit-bot: I haz the power, 2015-12-10 00:05:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-10 00:05:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[yzshen1, 2015-12-10 20:15:16]

On 2015/12/10 00:05:51, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

LGTM for DEPS changes of adding mojo/public.

[Donn Denman, 2015-12-10 20:22:39]

The CQ bit was checked by donnd@chromium.org

[commit-bot: I haz the power, 2015-12-10 20:24:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1385663002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1385663002/340001

[commit-bot: I haz the power, 2015-12-10 20:39:14]

Description was changed from

==========
[Contextual Search] Add mojo-enabled API component.
    
Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String, Boolean);
    
Adds a simple Contextual Search component with mojo communication to handle
enabling of the v8 wrapper only on a renderer which is presented in the
Contextual Search Overlay view.
    
BUG=523554
==========

to

==========
[Contextual Search] Add mojo-enabled API component.
    
Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String, Boolean);
    
Adds a simple Contextual Search component with mojo communication to handle
enabling of the v8 wrapper only on a renderer which is presented in the
Contextual Search Overlay view.
    
BUG=523554
==========

[commit-bot: I haz the power, 2015-12-10 20:39:16]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-12-10 20:40:08]

Description was changed from

==========
[Contextual Search] Add mojo-enabled API component.
    
Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String, Boolean);
    
Adds a simple Contextual Search component with mojo communication to handle
enabling of the v8 wrapper only on a renderer which is presented in the
Contextual Search Overlay view.
    
BUG=523554
==========

to

==========
[Contextual Search] Add mojo-enabled API component.

Add a simple JavaScript v8 wrapper for Contextual Search, that just defines
chrome.contextualSearch.setCaption(String, Boolean);

Adds a simple Contextual Search component with mojo communication to handle
enabling of the v8 wrapper only on a renderer which is presented in the
Contextual Search Overlay view.

BUG=523554

Committed: https://crrev.com/bda9b6f432fb237029ed6f4fac16bbe30da0cde4
Cr-Commit-Position: refs/heads/master@{#364460}
==========

[commit-bot: I haz the power, 2015-12-10 20:40:10]

Patchset 18 (id:??) landed as
https://crrev.com/bda9b6f432fb237029ed6f4fac16bbe30da0cde4
Cr-Commit-Position: refs/heads/master@{#364460}