Add "Open with <ARC-app-name>" items to the context menu

Add "Open with <ARC-app-name>" items to the context menu

so that the user can browse a URL with an ARC app.

BUG=591780
TEST=ran unittest, manually checked with a Chromebook too

Committed: https://crrev.com/1168edac81b324d505851ca7d4982ccf305d0226
Cr-Commit-Position: refs/heads/master@{#390015}

[Yusuke Sato, 2016-03-05 00:01:09]

yusukes@chromium.org changed reviewers:
+ avi@chromium.org, dcheng@chromium.org, hidehiko@chromium.org

[Yusuke Sato, 2016-03-05 00:01:10]

OWNERs, could you have a look at this? This is for ARC (App Runtime for Chrome),
and is based on a mock from the UX team (png at b/25865815#comment6). The new
context menu items are implemented in the same way as
chrome/browser/renderer_context_menu/spelling_menu_observer.h.

avi: chrome/browser/renderer_context_menu/
dcheng: components/arc/common/

Hidehiko, please take a look at the ARC specific part of the new class,
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc.

Thanks!

[Avi (use Gerrit), 2016-03-05 00:24:11]

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:136:
return NULL;
nullptr here and below

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:145:
LOG(WARNING) << "ARC intent helper instance is too old.";
Here and above, why are we warning? DLOG I could understand, but this is this
something that could be useful?

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:913: #if
!defined(OS_CHROMEOS)
Reverse this #if, and move the comment above it into the #else block.

[Yusuke Sato, 2016-03-05 01:38:24]

Thanks for the review, Avi. Please take another look.

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:136:
return NULL;
On 2016/03/05 00:24:11, Avi wrote:
> nullptr here and below

Done.

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:145:
LOG(WARNING) << "ARC intent helper instance is too old.";
On 2016/03/05 00:24:11, Avi wrote:
> Here and above, why are we warning? DLOG I could understand, but this is this
> something that could be useful?

Changed to DLOG as they are mostly for debugging. Done.

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1760773004/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:913: #if
!defined(OS_CHROMEOS)
On 2016/03/05 00:24:11, Avi wrote:
> Reverse this #if, and move the comment above it into the #else block.

Done.

[Avi (use Gerrit), 2016-03-05 02:32:33]

lgtm

[dcheng, 2016-03-05 05:06:17]

ipc lgtm

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:64:
size_t sub_items = std::min<size_t>(num_apps - (kMaxArcAppsInMainMenu - 1),
Just make these constants size_t so you don't have to explicitly specify the
template parameter.

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:997: if
(!arc_app_menu_observer_.get())
Nit: no .get()

[Yusuke Sato, 2016-03-05 20:49:51]

Thanks Daniel. Hidehiko, PTAL.

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:64:
size_t sub_items = std::min<size_t>(num_apps - (kMaxArcAppsInMainMenu - 1),
On 2016/03/05 05:06:17, dcheng wrote:
> Just make these constants size_t so you don't have to explicitly specify the
> template parameter.

Done.

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/1760773004/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:997: if
(!arc_app_menu_observer_.get())
On 2016/03/05 05:06:17, dcheng wrote:
> Nit: no .get()

Done.

[hidehiko, 2016-03-07 14:40:07]

lgtm

https://codereview.chromium.org/1760773004/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1760773004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:729: +          <message
name="IDS_CONTENT_CONTEXT_LOADING_ARC_APP_NAMES" desc="he place-holder message
shown while Chrome is waiting for a reply from ARC.">
nit s/he/The/

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:181: //
Check if ARC apps can handle the |link_url_|. Sicne the information is
nit: s/Sicne/Since/

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:241:
const size_t index = GetIndexForCommandId(command_id);
nit: GetIndexForCommandId can return a negative value on error. So, accepting by
size_t somehow looks a bit confusing. Maybe comment?

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:253:
handlers_ = std::move(handlers);
nit: I'm ok with the current code, but how about having a map from CommandId to
UrlHandlerInfoPtr?
Then, the map would release you from index management? It's totally up to you.

[Yusuke Sato, 2016-03-07 22:25:16]

https://codereview.chromium.org/1760773004/diff/40001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1760773004/diff/40001/chrome/app/generated_re...
chrome/app/generated_resources.grd:729: +          <message
name="IDS_CONTENT_CONTEXT_LOADING_ARC_APP_NAMES" desc="he place-holder message
shown while Chrome is waiting for a reply from ARC.">
On 2016/03/07 14:40:07, hidehiko wrote:
> nit s/he/The/

Done.

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:181: //
Check if ARC apps can handle the |link_url_|. Sicne the information is
On 2016/03/07 14:40:07, hidehiko wrote:
> nit: s/Sicne/Since/

Done.

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:241:
const size_t index = GetIndexForCommandId(command_id);
On 2016/03/07 14:40:07, hidehiko wrote:
> nit: GetIndexForCommandId can return a negative value on error. So, accepting
by
> size_t somehow looks a bit confusing. Maybe comment?

Done.

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:253:
handlers_ = std::move(handlers);
On 2016/03/07 14:40:07, hidehiko wrote:
> nit: I'm ok with the current code, but how about having a map from CommandId
to
> UrlHandlerInfoPtr?
> Then, the map would release you from index management? It's totally up to you.

That allows us to remove GetIndexForCommandId() as you said, but then I think we
need to provide a way to map i (from 0 to handlers.size()) to command id to
convert the mojo array to the map. We could use |enabled| to convert i to
command id assuming that IDC_CONTENT_CONTEXT_ARC_* are properly sorted in
chrome_command_ids.h and |enabled| is an ordered set, but the assumptions
(especially the former) seem to make the code a bit harder to read IMO. So let
me continue to use the current way.

[Yusuke Sato, 2016-03-07 23:02:03]

yusukes@chromium.org changed reviewers:
+ grt@chromium.org, isherman@chromium.org

[Yusuke Sato, 2016-03-07 23:02:03]

+more OWNERs

Greg, Ilya, could you have a look at chrome_command_ids.h and histograms.xml?
This is for ARC (App Runtime for Chrome), and is based on a mock from the UX
team (png at b/25865815#comment6).

Looks like trybots are mostly offline right now, but patch set #3 passed all
tests. #4 only has some comment changes.

[Ilya Sherman, 2016-03-08 00:26:53]

histograms.xml rs lgtm

[grt (UTC plus 2), 2016-03-08 16:13:06]

https://codereview.chromium.org/1760773004/diff/60001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1760773004/diff/60001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:328: #define IDC_CONTENT_CONTEXT_ARC_APP1 50191
could you start with 50190 for consistency with the blocks above?

[Ben Goodger (Google), 2016-03-08 16:27:53]

The src/chrome layer should not know about ARC, and it'd be good to avoid adding
more #ifdef OS_CHROMEOS in src/chrome too.

Instead I think you can have a submenu item in the browser context menu for
"open-with apps", make sure it doesn't show up when there are no items in it.
The submenu can be populated by some API exposed by ash which uses the arc
bridge.

[Yusuke Sato, 2016-03-11 23:39:46]

Ben, thanks for the review. I modified the CL following your suggestion. Please
take a look.

I didn't put all "Open with <app>" menu items in a sub menu so I can continue to
follow a UX mock (based on UX study) from the UX team, but other than that, I
think patch set #5 addresses the issues you mentioned.

hidehiko@, can you also take another look at chrome/browser/chromeos/arc/? 


* ash/renderer_context_menu/open_with_menu_controller*

New files.

* chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc

Set ARC-specific delegate which uses ARC bridge to the new Ash controller class.

* chrome/browser/chromeos/arc/open_with_menu_controller_delegate*

The delegate. Moved from
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos* with very
minimal modifications, then renamed.

https://codereview.chromium.org/1760773004/diff/60001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1760773004/diff/60001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:328: #define IDC_CONTENT_CONTEXT_ARC_APP1 50191
On 2016/03/08 16:13:06, grt wrote:
> could you start with 50190 for consistency with the blocks above?

Done.

[hidehiko, 2016-03-14 12:07:05]

My main comment is just
chrome/browser/renderer_context_menu/open_with_menu_observer.cc:50

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:253:
handlers_ = std::move(handlers);
On 2016/03/07 22:25:16, Yusuke Sato wrote:
> On 2016/03/07 14:40:07, hidehiko wrote:
> > nit: I'm ok with the current code, but how about having a map from CommandId
> to
> > UrlHandlerInfoPtr?
> > Then, the map would release you from index management? It's totally up to
you.
> 
> That allows us to remove GetIndexForCommandId() as you said, but then I think
we
> need to provide a way to map i (from 0 to handlers.size()) to command id to
> convert the mojo array to the map. We could use |enabled| to convert i to
> command id assuming that IDC_CONTENT_CONTEXT_ARC_* are properly sorted in
> chrome_command_ids.h and |enabled| is an ordered set, but the assumptions
> (especially the former) seem to make the code a bit harder to read IMO. So let
> me continue to use the current way.

FYI: what I imagined was;

unordered_map<int /* command id */, UrlHandlerInfoPtr> command_map;

max_main_commands = min(num_main_commands - 1, handlers.size());
for (i = 0; i < max_main_commands; ++i) {
  command_map[main_start_command + i] = std::move(handlers[i]);
}
for (i = max_main_commands; i < handlers.size(); ++i) {
  command_map[IDS_..._ARC_APP1 + i - max_main_commands] =
std::move(handlers[i]);
}

// Update main menu items.
for (i = main_start_command; i < main_start_command + num_main_commands - 1;
++i) {
  if (command_map.find(i) != command_map.end()) {
    proxy_->UpdateMenuItem(i, true, false, GetLabelForCommandId(command_id)); 
// enable
  } else {
    proxy_->UpdateMenuItem(i, false, true, EmptyString16());  // disable
  }
}

// Update "more"
if (num_main_commands <= handlers.size()) {
  proxy_->UpdateMenuItem(...) // enable.
} else {
  proxy_->UpdateMenuItem(...) // disable.
}

// Update sub menu items.
for (i = IDC_..._ARC_APP1; i < IDC_..._ARC_APP_LAST; ++i) {
  if (command_map.find(i) != command_map.end()) {
    proxy_->UpdateMenuItem(i, true, false, GetLabelForCommandId(command_id)); 
// enable
  } else {
    proxy_->UpdateMenuItem(i, false, true, EmptyString16());  // disable
  }
}

or something. So that, we can encapsulate indexing work only in this method.
Probably, I'd extract "building a map" part into a function, for testing
convenience.

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc (right):

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:86: if
(command_id >= menu_id_start &&
nit: how about short-circuit return?

if (...) {
  return command_id - menu_id_start;
}
if (...) {
  return ...;
}
return -1;

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:223:
std::set<int> OpenWithMenuControllerDelegate::GetCommandIdsToEnableForTesting(
Could you make those testing utility just as proxies, and move all test-only
logic to the _test.cc file?
These are out of this file's scope.

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_observer.cc (right):

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_observer.cc:38: #if
defined(USE_ASH)
No error handling is needed?

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_observer.cc:50: (command_id
<= IDC_CONTENT_CONTEXT_OPEN_WITH_LAST);
Handled value range was changed from, at least, PS4.
What is the expected behavior, here?

[Yusuke Sato, 2016-03-14 21:36:15]

Patchset #9 (id:160001) has been deleted

[Yusuke Sato, 2016-03-15 00:00:13]

Please take another look.

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc
(right):

https://codereview.chromium.org/1760773004/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/arc_app_menu_observer_chromeos.cc:253:
handlers_ = std::move(handlers);
On 2016/03/14 12:07:05, hidehiko wrote:
> On 2016/03/07 22:25:16, Yusuke Sato wrote:
> > On 2016/03/07 14:40:07, hidehiko wrote:
> > > nit: I'm ok with the current code, but how about having a map from
CommandId
> > to
> > > UrlHandlerInfoPtr?
> > > Then, the map would release you from index management? It's totally up to
> you.
> > 
> > That allows us to remove GetIndexForCommandId() as you said, but then I
think
> we
> > need to provide a way to map i (from 0 to handlers.size()) to command id to
> > convert the mojo array to the map. We could use |enabled| to convert i to
> > command id assuming that IDC_CONTENT_CONTEXT_ARC_* are properly sorted in
> > chrome_command_ids.h and |enabled| is an ordered set, but the assumptions
> > (especially the former) seem to make the code a bit harder to read IMO. So
let
> > me continue to use the current way.
> 
> FYI: what I imagined was;
> 
> unordered_map<int /* command id */, UrlHandlerInfoPtr> command_map;
> 
> max_main_commands = min(num_main_commands - 1, handlers.size());
> for (i = 0; i < max_main_commands; ++i) {
>   command_map[main_start_command + i] = std::move(handlers[i]);
> }
> for (i = max_main_commands; i < handlers.size(); ++i) {
>   command_map[IDS_..._ARC_APP1 + i - max_main_commands] =
> std::move(handlers[i]);
> }
> 
> // Update main menu items.
> for (i = main_start_command; i < main_start_command + num_main_commands - 1;
> ++i) {
>   if (command_map.find(i) != command_map.end()) {
>     proxy_->UpdateMenuItem(i, true, false, GetLabelForCommandId(command_id)); 
> // enable
>   } else {
>     proxy_->UpdateMenuItem(i, false, true, EmptyString16());  // disable
>   }
> }
> 
> // Update "more"
> if (num_main_commands <= handlers.size()) {
>   proxy_->UpdateMenuItem(...) // enable.
> } else {
>   proxy_->UpdateMenuItem(...) // disable.
> }
> 
> // Update sub menu items.
> for (i = IDC_..._ARC_APP1; i < IDC_..._ARC_APP_LAST; ++i) {
>   if (command_map.find(i) != command_map.end()) {
>     proxy_->UpdateMenuItem(i, true, false, GetLabelForCommandId(command_id)); 
> // enable
>   } else {
>     proxy_->UpdateMenuItem(i, false, true, EmptyString16());  // disable
>   }
> }
> 
> or something. So that, we can encapsulate indexing work only in this method.
> Probably, I'd extract "building a map" part into a function, for testing
> convenience.

Ah okay, I guess I misunderstood your comment. Thanks for the clarification,
this works. Done.

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc (right):

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:86: if
(command_id >= menu_id_start &&
On 2016/03/14 12:07:05, hidehiko wrote:
> nit: how about short-circuit return?
> 
> if (...) {
>   return command_id - menu_id_start;
> }
> if (...) {
>   return ...;
> }
> return -1;

Removed the function. Done.

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:223:
std::set<int> OpenWithMenuControllerDelegate::GetCommandIdsToEnableForTesting(
On 2016/03/14 12:07:05, hidehiko wrote:
> Could you make those testing utility just as proxies, and move all test-only
> logic to the _test.cc file?
> These are out of this file's scope.

Yeah that's better of course. Done, thanks.

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_observer.cc (right):

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_observer.cc:38: #if
defined(USE_ASH)
On 2016/03/14 12:07:05, hidehiko wrote:
> No error handling is needed?

For non-ASH configurations, InitMenu does nothing and that's just fine.
IsCommandIdEnabled() still returns an appropriate result.

https://codereview.chromium.org/1760773004/diff/140001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_observer.cc:50: (command_id
<= IDC_CONTENT_CONTEXT_OPEN_WITH_LAST);
On 2016/03/14 12:07:05, hidehiko wrote:
> Handled value range was changed from, at least, PS4.
> What is the expected behavior, here?

PS4's also works fine, but was overkill. The function is called only with
command_ids for the main menu.

[hidehiko, 2016-03-15 07:34:29]

LGTM for */arc/*.

https://codereview.chromium.org/1760773004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc (right):

https://codereview.chromium.org/1760773004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:34: void
BuildHandlersMap(int menu_id_start,
nit: now we have std::move. How about return a pair of values?

[Yusuke Sato, 2016-03-15 19:11:56]

https://codereview.chromium.org/1760773004/diff/180001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc (right):

https://codereview.chromium.org/1760773004/diff/180001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:34: void
BuildHandlersMap(int menu_id_start,
On 2016/03/15 07:34:29, hidehiko wrote:
> nit: now we have std::move. How about return a pair of values?

Done.

[Yusuke Sato, 2016-03-15 21:03:41]

Ben, could you do an owner review?

[hidehiko, 2016-03-16 02:23:53]

still lgtm

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc (right):

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:219: auto
result = BuildHandlersMap(main_menu_id_start_, num_main_menu_items_,
nit: FYI. You can remove |result|.

int sub_menu_parent;
std::tie(handlers_, sub_menu_parent) = BuildHandlersMap(...);

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.h (right):

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.h:9: 
nit: #include <utility> here for std::pair

[Ben Goodger (Google), 2016-03-16 16:32:50]

Why is there still code referencing ARC in src/chrome? Also, can you come up
with a way of doing this that doesn't involve so many ifdefs?

My thinking was, you'd provide a subsection of the context menu that could be
populated/controlled by some external entity (implemented entirely in src/ash &
arc). A small amount of code in Chrome would register that instance with the
context menu factory. On other platforms the context menu builder would have a
null populator and not construct these items.

[Yusuke Sato, 2016-03-16 17:43:12]

IDs in chrome/app/chrome_command_ids.h and chrome/app/generated_resources.grd as
well as a comment in
chrome/browser/renderer_context_menu/open_with_menu_observer.h still used ARC by
mistake. I'm fixing them. I'll also be using the factory to reduce the number of
(or eliminate) #ifdefs in my chrome/browser/renderer_context_menu/ code.

Let me ask one question. My understanding is that it is okay to continue to use
the existing arc directory, chrome/browser/chromeos/arc/, where some mojom APIs
that are supposed to interact with Chrome are implemented. Is that correct?
Please let me know if it is not.

[Ben Goodger (Google), 2016-03-16 17:56:04]

On 2016/03/16 17:43:12, Yusuke Sato wrote:
> IDs in chrome/app/chrome_command_ids.h and chrome/app/generated_resources.grd
as
> well as a comment in
> chrome/browser/renderer_context_menu/open_with_menu_observer.h still used ARC
by
> mistake. I'm fixing them. I'll also be using the factory to reduce the number
of
> (or eliminate) #ifdefs in my chrome/browser/renderer_context_menu/ code.
> 
> Let me ask one question. My understanding is that it is okay to continue to
use
> the existing arc directory, chrome/browser/chromeos/arc/, where some mojom
APIs
> that are supposed to interact with Chrome are implemented. Is that correct?
> Please let me know if it is not.

I haven't taken a close look at chrome/browser/arc yet, but my assumption would
be that it exists to wrap functionality that only exists within the chrome
browser process, e.g. profile.

Taking a brief look at ArcIntentHelperBridge, I wish it were not in src/chrome,
and that src/chrome just exposed an API (e.g. via ash::ShellDelegate) to open a
URL in a new tab etc. The only reason it uses Profile, Browser etc is to
implement such an API in an ad-hoc way.

The reason I'm pushing back on changes like these is that they increase the
complexity of code in src/chrome & make it harder for us to disentangle the
ash/wm layer from the browser process, which hurts ARC in the long run. The
boundary between ash, components/arc etc and src/chrome should not be considered
completely porous :-)

[Yusuke Sato, 2016-03-31 02:24:45]

Patchset #12 (id:240001) has been deleted

[Yusuke Sato, 2016-03-31 14:46:29]

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc (right):

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.cc:219: auto
result = BuildHandlersMap(main_menu_id_start_, num_main_menu_items_,
On 2016/03/16 02:23:53, hidehiko wrote:
> nit: FYI. You can remove |result|.
> 
> int sub_menu_parent;
> std::tie(handlers_, sub_menu_parent) = BuildHandlersMap(...);

I know std::tie is already used in some directories in Chromium, but
http://chromium-cpp.appspot.com/ says it's still banned. So let me avoid it for
now. FYI it says "General use of std::tuple, and std::tie for unpacking or
multiple assignments is still not allowed."

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/open_with_menu_controller_delegate.h (right):

https://codereview.chromium.org/1760773004/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.h:9: 
On 2016/03/16 02:23:53, hidehiko wrote:
> nit: #include <utility> here for std::pair

Done.

[Yusuke Sato, 2016-03-31 14:50:01]

Ben, I've rebased this CL on top of the ArcIntentHelper refactoring change
(https://codereview.chromium.org/1843563003/). Could you take another look?

Patch Set #11 moved
chrome/browser/chromeos/arc/open_with_menu_controller_delegate.* to
components/arc/ to reduce code in c/b/chromeos/arc/. Now the CL doesn't touch
c/b/chromeos/arc/ at all.

Details:
* Moved strings from chrome/app/generated_resources.grd to
components/arc_strings.grdp.
* Removed all direct use of IDC_XXX from open_with_menu_controller_delegate.cc
so that it can be moved to components/.
* Instead, slightly changed PopulateOpenWithMenu() Ash interface so Chrome can
pass IDC IDs for the sub menu to ARC.

* No other changes. Most of the files in the CL are untouched.
* chrome/ directory no longer knows anything about ARC. Confirmed with:
   $ git diff HEAD~N chrome/ | grep -i arc

Thanks!

[Yusuke Sato, 2016-04-02 02:10:59]

I forgot to address your suggestion on the context factory in patchset #11.
Sorry about that. Patchset #13 has addressed that, and now only
components/arc/intent_helper/open_with_menu_observer.cc has actual code for
populating the menu.

Please take a look.

[Luis Héctor Chávez, 2016-04-04 23:12:42]

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

[Luis Héctor Chávez, 2016-04-04 23:12:44]

https://codereview.chromium.org/1760773004/diff/300001/components/arc/intent_...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/1760773004/diff/300001/components/arc/intent_...
components/arc/intent_helper/arc_intent_helper_bridge.cc:30:
ash::Shell::GetInstance()->open_with_menu_controller()->SetDelegate(NULL);
nit: s/NULL/nullptr/

[Ben Goodger (Google), 2016-04-06 21:09:38]

It would seem to me that the code in arc shouldn't be manipulating the menu in
Chrome directly. Rather, it should be communicating structured data with Chrome
(via the code in ash), in the form of an OpenWith model. This exposes the set of
items that can be shown & provides an interface for actually performing the
action. Is there a reason you need to expose the menu (or a proxy to the menu)
directly to components/arc?

A common pattern in UI development is MVC, where the code that constructs a UI
is separate from the data model that populates it.

Is there some detail of this feature that I'm missing?

[Yusuke Sato, 2016-04-08 18:36:09]

On 2016/04/06 21:09:38, Ben Goodger (Google) wrote:
> It would seem to me that the code in arc shouldn't be manipulating the menu in
> Chrome directly. Rather, it should be communicating structured data with
Chrome
> (via the code in ash), in the form of an OpenWith model. This exposes the set
of
> items that can be shown & provides an interface for actually performing the
> action. Is there a reason you need to expose the menu (or a proxy to the menu)
> directly to components/arc?
> 
> A common pattern in UI development is MVC, where the code that constructs a UI
> is separate from the data model that populates it.
> 
> Is there some detail of this feature that I'm missing?

TL;DR I'll change the Ash API so it returns ui::MenuModel.


Returning a model from the Ash API required more code in
c/b/renderer_context_menu/, probably ~200 lines or so, and I wasn't really sure
if having the additional code was a good idea. The reasons I needed the
additional code were:

* The "open with" menu items has to be added not only as a submenu of the main
context menu, but also to the main menu (UX requirement). Because of this, a
ui::MenuModel returned from the Ash API cannot be simply passed to
RenderViewContextMenu (to let RenderViewContextMenu own it.) Instead, some
additional code in c/b/renderer_context_menu/ has to scan the returned
ui::MenuModel and insert main menu items to RenderViewContextMenu. This is
relatively a small issue.
* The "open with" menu is populated asynchronously. One mojo IPC is needed to
get a list of (properly ordered) app names from ARC side. Although the IPC is
fast and only takes a few milliseconds, it's still asynchronous. The problem
here is that RenderViewContextMenu reads its ui::MenuModels only once, and to
dynamically update the menu, the RenderViewContextMenuProxy interface has to be
used rather than directly updating the model.

https://code.google.com/p/chromium/codesearch#chromium/src/components/rendere...

Having that said, both issues can be fixed for sure by having the ~200 lines of
additional code in c/b/renderer_context_menu/ which converts ui::MenuModel
returned from Ash into RenderViewContextMenuProxy calls. The code does the same
when the model is updated too.

I'm going to change the Ash API to something like the following. Does this look
better to you? (and can I use ui::MenuModel there?)

 [ash/renderer_context_menu/open_with_menu_controller.h]
 class ASH_EXPORT OpenWithMenuController {
 ...
   // Returns a menu model of the "Open with <app>" context menu. The returned
   // menu model may have up to one submenu. Both main and submenus only have
   // command items. When such a menu for the |url| cannot be created, returns
   // nullptr.
   virtual scoped_ptr<ui::MenuModel> CreateMenu(
       const GURL& url,
       int reserved_command_id_first,
       int reserved_command_id_last) = 0;

-Yusuke

[Ben Goodger (Google), 2016-04-08 19:55:58]

I would actually not suggest using ui::MenuModel, but defining your own
interface in the ash layer, e.g. OpenWithItems, implementing it in
components/arc and calling it from Chrome. If components/arc can't depend
on ash (which might be reasonable), then just put it somewhere else, like
ui/base for the time being.

-Ben

On Fri, Apr 8, 2016 at 11:36 AM, <yusukes@chromium.org> wrote:

> On 2016/04/06 21:09:38, Ben Goodger (Google) wrote:
> > It would seem to me that the code in arc shouldn't be manipulating the
> menu in
> > Chrome directly. Rather, it should be communicating structured data with
> Chrome
> > (via the code in ash), in the form of an OpenWith model. This exposes
> the set
> of
> > items that can be shown & provides an interface for actually performing
> the
> > action. Is there a reason you need to expose the menu (or a proxy to the
> menu)
> > directly to components/arc?
> >
> > A common pattern in UI development is MVC, where the code that
> constructs a UI
> > is separate from the data model that populates it.
> >
> > Is there some detail of this feature that I'm missing?
>
> TL;DR I'll change the Ash API so it returns ui::MenuModel.
>
>
> Returning a model from the Ash API required more code in
> c/b/renderer_context_menu/, probably ~200 lines or so, and I wasn't really
> sure
> if having the additional code was a good idea. The reasons I needed the
> additional code were:
>
> * The "open with" menu items has to be added not only as a submenu of the
> main
> context menu, but also to the main menu (UX requirement). Because of this,
> a
> ui::MenuModel returned from the Ash API cannot be simply passed to
> RenderViewContextMenu (to let RenderViewContextMenu own it.) Instead, some
> additional code in c/b/renderer_context_menu/ has to scan the returned
> ui::MenuModel and insert main menu items to RenderViewContextMenu. This is
> relatively a small issue.
> * The "open with" menu is populated asynchronously. One mojo IPC is needed
> to
> get a list of (properly ordered) app names from ARC side. Although the IPC
> is
> fast and only takes a few milliseconds, it's still asynchronous. The
> problem
> here is that RenderViewContextMenu reads its ui::MenuModels only once, and
> to
> dynamically update the menu, the RenderViewContextMenuProxy interface has
> to be
> used rather than directly updating the model.
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/rendere...
>
> Having that said, both issues can be fixed for sure by having the ~200
> lines of
> additional code in c/b/renderer_context_menu/ which converts ui::MenuModel
> returned from Ash into RenderViewContextMenuProxy calls. The code does the
> same
> when the model is updated too.
>
> I'm going to change the Ash API to something like the following. Does this
> look
> better to you? (and can I use ui::MenuModel there?)
>
> [ash/renderer_context_menu/open_with_menu_controller.h]
> class ASH_EXPORT OpenWithMenuController {
> ...
> // Returns a menu model of the "Open with <app>" context menu. The returned
> // menu model may have up to one submenu. Both main and submenus only have
> // command items. When such a menu for the |url| cannot be created, returns
> // nullptr.
> virtual scoped_ptr<ui::MenuModel> CreateMenu(
> const GURL& url,
> int reserved_command_id_first,
> int reserved_command_id_last) = 0;
>
> -Yusuke
>
>
> https://codereview.chromium.org/1760773004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Yusuke Sato, 2016-04-12 07:04:38]

Patchset #16 (id:340001) has been deleted

[Yusuke Sato, 2016-04-12 16:14:01]

Done. The ash API now looks like the following:

 class ASH_EXPORT OpenWithMenuController {
 ...
    std::unique_ptr<OpenWithItems> CreateMenu(const GURL& url,
                                              int reserved_command_id_first,
                                              int reserved_command_id_last);

Please take another look.


On 2016/04/08 19:55:58, Ben Goodger (Google) wrote:
> I would actually not suggest using ui::MenuModel, but defining your own
> interface in the ash layer, e.g. OpenWithItems, implementing it in
> components/arc and calling it from Chrome. If components/arc can't depend
> on ash (which might be reasonable), then just put it somewhere else, like
> ui/base for the time being.
> 
> -Ben
> 
> On Fri, Apr 8, 2016 at 11:36 AM, <mailto:yusukes@chromium.org> wrote:
> 
> > On 2016/04/06 21:09:38, Ben Goodger (Google) wrote:
> > > It would seem to me that the code in arc shouldn't be manipulating the
> > menu in
> > > Chrome directly. Rather, it should be communicating structured data with
> > Chrome
> > > (via the code in ash), in the form of an OpenWith model. This exposes
> > the set
> > of
> > > items that can be shown & provides an interface for actually performing
> > the
> > > action. Is there a reason you need to expose the menu (or a proxy to the
> > menu)
> > > directly to components/arc?
> > >
> > > A common pattern in UI development is MVC, where the code that
> > constructs a UI
> > > is separate from the data model that populates it.
> > >
> > > Is there some detail of this feature that I'm missing?
> >
> > TL;DR I'll change the Ash API so it returns ui::MenuModel.
> >
> >
> > Returning a model from the Ash API required more code in
> > c/b/renderer_context_menu/, probably ~200 lines or so, and I wasn't really
> > sure
> > if having the additional code was a good idea. The reasons I needed the
> > additional code were:
> >
> > * The "open with" menu items has to be added not only as a submenu of the
> > main
> > context menu, but also to the main menu (UX requirement). Because of this,
> > a
> > ui::MenuModel returned from the Ash API cannot be simply passed to
> > RenderViewContextMenu (to let RenderViewContextMenu own it.) Instead, some
> > additional code in c/b/renderer_context_menu/ has to scan the returned
> > ui::MenuModel and insert main menu items to RenderViewContextMenu. This is
> > relatively a small issue.
> > * The "open with" menu is populated asynchronously. One mojo IPC is needed
> > to
> > get a list of (properly ordered) app names from ARC side. Although the IPC
> > is
> > fast and only takes a few milliseconds, it's still asynchronous. The
> > problem
> > here is that RenderViewContextMenu reads its ui::MenuModels only once, and
> > to
> > dynamically update the menu, the RenderViewContextMenuProxy interface has
> > to be
> > used rather than directly updating the model.
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/rendere...
> >
> > Having that said, both issues can be fixed for sure by having the ~200
> > lines of
> > additional code in c/b/renderer_context_menu/ which converts ui::MenuModel
> > returned from Ash into RenderViewContextMenuProxy calls. The code does the
> > same
> > when the model is updated too.
> >
> > I'm going to change the Ash API to something like the following. Does this
> > look
> > better to you? (and can I use ui::MenuModel there?)
> >
> > [ash/renderer_context_menu/open_with_menu_controller.h]
> > class ASH_EXPORT OpenWithMenuController {
> > ...
> > // Returns a menu model of the "Open with <app>" context menu. The returned
> > // menu model may have up to one submenu. Both main and submenus only have
> > // command items. When such a menu for the |url| cannot be created, returns
> > // nullptr.
> > virtual scoped_ptr<ui::MenuModel> CreateMenu(
> > const GURL& url,
> > int reserved_command_id_first,
> > int reserved_command_id_last) = 0;
> >
> > -Yusuke
> >
> >
> > https://codereview.chromium.org/1760773004/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Ben Goodger (Google), 2016-04-12 19:48:25]

So, my preference would be that the model described in ash & implemented in
components/arc would not know about the submenu structure as specified in the
mocks. Instead the model would just provide the metadata for all apps, and the
code in chrome that constructs the menu from the model would make decisions
about what to show in the top menu & what to push down into a submenu.

As it stands, the current model doesn't quite live up to the intent of MVC
because it encodes UI-specific details.

What Chrome needs from the shell is two things:

- a vector of metadata for each open with app
- a function to call when an app menu item is selected.

I'm not sure why enable state etc needs to be conveyed through the model, since
these items should always be enabled?

-Ben

[Yusuke Sato, 2016-04-12 21:09:35]

On 2016/04/12 19:48:25, Ben Goodger (Google) wrote:
> So, my preference would be that the model described in ash & implemented in
> components/arc would not know about the submenu structure as specified in the
> mocks. Instead the model would just provide the metadata for all apps, and the
> code in chrome that constructs the menu from the model would make decisions
> about what to show in the top menu & what to push down into a submenu.
> 
> As it stands, the current model doesn't quite live up to the intent of MVC
> because it encodes UI-specific details.
> 
> What Chrome needs from the shell is two things:
> 
> - a vector of metadata for each open with app
> - a function to call when an app menu item is selected.
> 
> I'm not sure why enable state etc needs to be conveyed through the model,
since
> these items should always be enabled?
> 
> -Ben

Sure, I modeled it after ui::MenuModel, but yours is much better. I'm rewriting
the interface to the following. Please let me know if this looks better to you:

 class ASH_EXPORT OpenWithMenuController {
 ...
   std::unique_ptr<OpenWithItems> CreateMenu(const GURL& url);


 class ASH_EXPORT OpenWithItems {
 ...
  class Delegate {
    virtual void OnMenuItemChanged() = 0;
  };
  virtual int GetItemCount() = 0;
  virtual base::string16 GetLabelAt(int index) = 0;
  virtual bool GetIconAt(int index, gfx::Image* icon) = 0;
  // (note: Removed IsEnabled())
  virtual void ActivatedAt(int index) = 0;
 };

The CreateMenu implementation in components/arc/ returns an empty OpenWithItems
first, then asynchronously gets a list of app names from ARC side (in a few
milliseconds), and then calls OnMenuItemChanged() to ask Chrome to update the
UI.

[Ben Goodger (Google), 2016-04-12 21:51:12]

Here's how I think about it.

Let's say you have some abstract concept of a LinkHandler. We could define a
struct describing one:

struct LinkHandlerInfo {
  std::string name;
  gfx::Image* icon;
  uint32_t id;
  // .. etc
};

Then some system interface exists @ the ash layer:

class LinkHandlerModel {
 public:
  class Observer {
   public:
    // Called once when the observer is added, and then subsequently whenever
the underlying model changes.
    virtual void ModelChanged(const std::vector<LinkHandlerInfo>& handlers) = 0;
  };

  virtual void AddObserver(Observer* observer) = 0;
  virtual void OpenLinkWithHandler(const GURL& url, uint32_t handler_id) = 0;
};

This is the simplest interface I can come up with. If you don't want to blow
away and reconstruct the model every time it shifts, you could add
HandlerAdded/Removed methods to the observer interface, though I doubt it
matters that much for this specific feature since the menus are transient.

[Yusuke Sato, 2016-04-13 20:31:17]

Patchset #17 (id:380001) has been deleted

[Yusuke Sato, 2016-04-13 21:44:21]

Please take another look.

[hidehiko, 2016-04-20 04:40:18]

I have quick walk-through. I'll take another look in more details, tomorrow.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_factory.h (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory.h:19:
DISALLOW_COPY_AND_ASSIGN(OpenWithMenuFactory);
DISALLOW_IMPLICIT_CONSTRUCTORS.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:77:
ui::SimpleMenuModel* submenu = new ui::SimpleMenuModel(&submenu_delegate_);
I'd prefer
std::unique_ptr here, too. Instead, std::move can be used at L83.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:87: if (i ==
kNumMainMenuCommands - 1) {
How about iterating between [i, kNumMainMenuCommands - 1), and doing this just
after the loop?

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:101:
command_id <= IDC_CONTENT_CONTEXT_OPEN_WITH_LAST;
indent?

[Ben Goodger (Google), 2016-04-20 17:56:50]

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
File ash/renderer_context_menu/link_handler_model.h (right):

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
ash/renderer_context_menu/link_handler_model.h:5: #ifndef
ASH_RENDERER_CONTEXT_MENU_LINK_HANDLER_MODEL_
ash shouldn't know about the concept of a renderer. From its perspective, it's
providing an interface that apps can use to populate UI & dispatch links. I'd
just put this under src/ash.

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
File ash/renderer_context_menu/open_with_menu_controller.h (right):

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
ash/renderer_context_menu/open_with_menu_controller.h:20: // renderer context
menu.
your class names and comments should not refer to:
- renderers
- menus

in the ash layer.

-> LinkHandlerModelFactory.

Rather than having a delegate, can you just have the code in components/arc
subclass this & override a do-nothing impl of CreateModel? Then you can have the
code in components/arc just set the property on Shell.

[hidehiko, 2016-04-20 18:10:18]

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:119: void
ModelChanged(
As we discussed offline, if this is called twice or more, it could be
problematic. Would you mind to fix?

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
components/arc/intent_helper/arc_intent_helper_bridge.cc:30:
ash::Shell::GetInstance()->open_with_menu_controller()->SetDelegate(nullptr);
Could you verify this is always called before this instance's destruction?

Could you delete the instance returned by CreateModel?
It has the reference to ArcBridgeService via raw ptr, but there looks no path to
destruct it on ABS's destruction?

[dcheng, 2016-04-21 04:20:45]

https://codereview.chromium.org/1760773004/diff/400001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/app/generated_r...
chrome/app/generated_resources.grd:737: +          </message>
Nit: Hangouts

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.h:198:
scoped_ptr<RenderViewContextMenuObserver> open_with_menu_observer_;
Nit: std::unique_ptr

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
File components/arc/intent_helper/link_handler_model_impl.cc (right):

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
components/arc/intent_helper/link_handler_model_impl.cc:47: NotifyObserver();
Btw, it seems weird that we have to call NotifyObserver() here. Is there no
synchronization guarantee that we'll call AddObserver() before calling into
ARC++ to update the URL handler list?

[Yusuke Sato, 2016-04-21 23:37:20]

Patchset #19 (id:420002) has been deleted

[Yusuke Sato, 2016-04-22 02:40:39]

Patchset #20 (id:470001) has been deleted

[Yusuke Sato, 2016-04-22 05:27:48]

Please take another look

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
File ash/renderer_context_menu/link_handler_model.h (right):

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
ash/renderer_context_menu/link_handler_model.h:5: #ifndef
ASH_RENDERER_CONTEXT_MENU_LINK_HANDLER_MODEL_
On 2016/04/20 17:56:49, Ben Goodger (Google) wrote:
> ash shouldn't know about the concept of a renderer. From its perspective, it's
> providing an interface that apps can use to populate UI & dispatch links. I'd
> just put this under src/ash.

Done.

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
File ash/renderer_context_menu/open_with_menu_controller.h (right):

https://codereview.chromium.org/1760773004/diff/400001/ash/renderer_context_m...
ash/renderer_context_menu/open_with_menu_controller.h:20: // renderer context
menu.
On 2016/04/20 17:56:50, Ben Goodger (Google) wrote:
> your class names and comments should not refer to:
> - renderers
> - menus
> 
> in the ash layer.
> 
> -> LinkHandlerModelFactory.
> 
> Rather than having a delegate, can you just have the code in components/arc
> subclass this & override a do-nothing impl of CreateModel? Then you can have
the
> code in components/arc just set the property on Shell.

Done.

https://codereview.chromium.org/1760773004/diff/400001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/app/generated_r...
chrome/app/generated_resources.grd:737: +          </message>
On 2016/04/21 04:20:45, dcheng wrote:
> Nit: Hangouts

Done.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_factory.h (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory.h:19:
DISALLOW_COPY_AND_ASSIGN(OpenWithMenuFactory);
On 2016/04/20 04:40:18, hidehiko wrote:
> DISALLOW_IMPLICIT_CONSTRUCTORS.

Done.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:77:
ui::SimpleMenuModel* submenu = new ui::SimpleMenuModel(&submenu_delegate_);
On 2016/04/20 04:40:18, hidehiko wrote:
> I'd prefer
> std::unique_ptr here, too. Instead, std::move can be used at L83.

Done.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:87: if (i ==
kNumMainMenuCommands - 1) {
On 2016/04/20 04:40:18, hidehiko wrote:
> How about iterating between [i, kNumMainMenuCommands - 1), and doing this just
> after the loop?

Done.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:101:
command_id <= IDC_CONTENT_CONTEXT_OPEN_WITH_LAST;
On 2016/04/20 04:40:18, hidehiko wrote:
> indent?

This is by git cl format actually..

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash.cc:119: void
ModelChanged(
On 2016/04/20 18:10:18, hidehiko wrote:
> As we discussed offline, if this is called twice or more, it could be
> problematic. Would you mind to fix?

Done.

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://codereview.chromium.org/1760773004/diff/400001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/render_view_context_menu.h:198:
scoped_ptr<RenderViewContextMenuObserver> open_with_menu_observer_;
On 2016/04/21 04:20:45, dcheng wrote:
> Nit: std::unique_ptr

Done.

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
File components/arc/intent_helper/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
components/arc/intent_helper/arc_intent_helper_bridge.cc:30:
ash::Shell::GetInstance()->open_with_menu_controller()->SetDelegate(nullptr);
On 2016/04/20 18:10:18, hidehiko wrote:
> Could you verify this is always called before this instance's destruction?
> 
> Could you delete the instance returned by CreateModel?
> It has the reference to ArcBridgeService via raw ptr, but there looks no path
to
> destruct it on ABS's destruction?

Done. Copied line 30 to the destructor following a suggestion from Luis.

(Luis: since calling the ash method with nullptr twice is okay, I didn't
introduce any local boolean flags for this.)

As for the second question, I stopped passing ABS* raw pointer to the model impl
class (also based on Luis' suggestion.)

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
File components/arc/intent_helper/link_handler_model_impl.cc (right):

https://codereview.chromium.org/1760773004/diff/400001/components/arc/intent_...
components/arc/intent_helper/link_handler_model_impl.cc:47: NotifyObserver();
On 2016/04/21 04:20:45, dcheng wrote:
> Btw, it seems weird that we have to call NotifyObserver() here. Is there no
> synchronization guarantee that we'll call AddObserver() before calling into
> ARC++ to update the URL handler list?

Done, deleted.

[dcheng, 2016-04-24 19:27:57]

https://codereview.chromium.org/1760773004/diff/490001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_factory_ash_unittest.cc
(right):

https://codereview.chromium.org/1760773004/diff/490001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash_unittest.cc:33:
ash::LinkHandlerInfo info = {base::StringPrintf("App %zu", i), image, i};
%zu should be %" PRIuS "

[Yusuke Sato, 2016-04-25 15:07:46]

Please take another look.

https://codereview.chromium.org/1760773004/diff/490001/chrome/browser/rendere...
File chrome/browser/renderer_context_menu/open_with_menu_factory_ash_unittest.cc
(right):

https://codereview.chromium.org/1760773004/diff/490001/chrome/browser/rendere...
chrome/browser/renderer_context_menu/open_with_menu_factory_ash_unittest.cc:33:
ash::LinkHandlerInfo info = {base::StringPrintf("App %zu", i), image, i};
On 2016/04/24 19:27:57, dcheng wrote:
> %zu should be %" PRIuS "

Done.

[Yusuke Sato, 2016-04-26 15:46:56]

Ben, please take another look.

[Ben Goodger (Google), 2016-04-26 16:36:31]

lgtm

[dcheng, 2016-04-26 19:40:01]

(still) lgtm

[hidehiko, 2016-04-27 06:51:43]

components/arc LGTM.

[Yusuke Sato, 2016-04-27 06:54:37]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-04-27 06:54:37]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1760773004/#ps510001
(title: "Address dcheng's comment")

[commit-bot: I haz the power, 2016-04-27 06:54:54]

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

[commit-bot: I haz the power, 2016-04-27 07:52:26]

Committed patchset #21 (id:510001)

[commit-bot: I haz the power, 2016-04-27 07:53:58]

Description was changed from

==========
Add "Open with <ARC-app-name>" items to the context menu

so that the user can browse a URL with an ARC app.

BUG=591780
TEST=ran unittest, manually checked with a Chromebook too
==========

to

==========
Add "Open with <ARC-app-name>" items to the context menu

so that the user can browse a URL with an ARC app.

BUG=591780
TEST=ran unittest, manually checked with a Chromebook too

Committed: https://crrev.com/1168edac81b324d505851ca7d4982ccf305d0226
Cr-Commit-Position: refs/heads/master@{#390015}
==========

[commit-bot: I haz the power, 2016-04-27 07:54:00]

Patchset 21 (id:??) landed as
https://crrev.com/1168edac81b324d505851ca7d4982ccf305d0226
Cr-Commit-Position: refs/heads/master@{#390015}