Host-side implementation of ARC Intent handler

Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}

Committed: https://crrev.com/03af3e520814e764eba70ce6512f0a0f98e3e18e
Cr-Commit-Position: refs/heads/master@{#370487}

[Yusuke Sato, 2016-01-15 02:16:11]

Description was changed from

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC
==========

to

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC
==========

[Yusuke Sato, 2016-01-15 02:16:11]

yusukes@chromium.org changed reviewers:
+ elijahtaylor@chromium.org, hidehiko@chromium.org, lhchavez@lhchavez.com

[Yusuke Sato, 2016-01-15 02:16:25]

PTAL

[hidehiko, 2016-01-15 09:04:55]

https://codereview.chromium.org/1590723003/diff/20001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:168: FOR_EACH_OBSERVER(Observer,
observer_list(), OnIntentHelperInstanceReady());
Please call this after set_connection_error_handler().
(Other methods are fixed on ToT).

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/a...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/a...
components/arc/common/arc_bridge.mojom:36:
OnIntentHelperInstanceReady@111(IntentHelperInstance instance_ptr);
nit: MinVersion = 4 or later? (Please re-check this when submitting).

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/i...
File components/arc/common/intent.mojom (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/i...
components/arc/common/intent.mojom:8: OnOpenUrl@0(string url);
Could you document what this does?

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:18: virtual
~ArcIntentHelperBridge() = 0;
s/ = 0//

[Luis Héctor Chávez, 2016-01-15 16:51:20]

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

[Luis Héctor Chávez, 2016-01-15 16:51:20]

Adding the correct lhchavez

[Luis Héctor Chávez, 2016-01-15 17:26:47]

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/i...
File components/arc/common/intent.mojom (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/i...
components/arc/common/intent.mojom:7: interface IntentHelperHost {
Also document the purpose of this interface.

Now that I think of it, the Settings interface just sends broadcast intents that
are listened by ArcSettings package.

Given that it makes more sense for the IntentHelper to also handle broadcast
intents, can you sync up with benhansen@ to see if it is possible to drop the
settings interface and have the IntentHelperInstance also handle broadcasting
intents?

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:16: class
ArcIntentHelperBridge {
This is the third time this exact interface has been introduced, and the
constructor for the ArcServiceManager is getting out of hand.

How about you add

components/arc/arc_service.h:

namespace arc {

class ArcService {
 public:
  virtual ~ArcService() {}
  virtual void Initialize(ArcBridgeService* service) = 0;

 private:
  DISALLOW_COPY_AND_ASSIGN(ArcService);
};

}  // namespace

And make ArcServiceManager receive a std::initializer_list<ArcService>[1] and
store that to a std::vector<ArcService> instead of having to modify that class
every time we want to add a new service. The rest of the services constructed in
the ArcServiceManager can also be modified to implement this interface (except
|arc_notification_manager_|), but that can be done in a separate change.

1: I *think* this should work with std::vector::emplace{_,back}, but if you run
into any issues, fall back to a std::initializer_list<scoped_ptr<ArcService>>.

[Luis Héctor Chávez, 2016-01-15 19:10:22]

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:16: class
ArcIntentHelperBridge {
On 2016/01/15 17:26:47, Luis Héctor Chávez wrote:
> This is the third time this exact interface has been introduced, and the
> constructor for the ArcServiceManager is getting out of hand.
> 
> How about you add
> 
> components/arc/arc_service.h:
> 
> namespace arc {
> 
> class ArcService {
>  public:
>   virtual ~ArcService() {}
>   virtual void Initialize(ArcBridgeService* service) = 0;
> 
>  private:
>   DISALLOW_COPY_AND_ASSIGN(ArcService);
> };
> 
> }  // namespace
> 
> And make ArcServiceManager receive a std::initializer_list<ArcService>[1] and
> store that to a std::vector<ArcService> instead of having to modify that class
> every time we want to add a new service. The rest of the services constructed
in
> the ArcServiceManager can also be modified to implement this interface (except
> |arc_notification_manager_|), but that can be done in a separate change.
> 
> 1: I *think* this should work with std::vector::emplace{_,back}, but if you
run
> into any issues, fall back to a std::initializer_list<scoped_ptr<ArcService>>.

That won't work, since std::initializer_list always makes a copy :( Let me think
this again.

[Luis Héctor Chávez, 2016-01-15 21:55:00]

On 2016/01/15 19:10:22, Luis Héctor Chávez wrote:
>
https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
> File components/arc/intent_helper/arc_intent_helper_bridge.h (right):
> 
>
https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
> components/arc/intent_helper/arc_intent_helper_bridge.h:16: class
> ArcIntentHelperBridge {
> On 2016/01/15 17:26:47, Luis Héctor Chávez wrote:
> > This is the third time this exact interface has been introduced, and the
> > constructor for the ArcServiceManager is getting out of hand.
> > 
> > How about you add
> > 
> > components/arc/arc_service.h:
> > 
> > namespace arc {
> > 
> > class ArcService {
> >  public:
> >   virtual ~ArcService() {}
> >   virtual void Initialize(ArcBridgeService* service) = 0;
> > 
> >  private:
> >   DISALLOW_COPY_AND_ASSIGN(ArcService);
> > };
> > 
> > }  // namespace
> > 
> > And make ArcServiceManager receive a std::initializer_list<ArcService>[1]
and
> > store that to a std::vector<ArcService> instead of having to modify that
class
> > every time we want to add a new service. The rest of the services
constructed
> in
> > the ArcServiceManager can also be modified to implement this interface
(except
> > |arc_notification_manager_|), but that can be done in a separate change.
> > 
> > 1: I *think* this should work with std::vector::emplace{_,back}, but if you
> run
> > into any issues, fall back to a
std::initializer_list<scoped_ptr<ArcService>>.
> 
> That won't work, since std::initializer_list always makes a copy :( Let me
think
> this again.

Ended up doing it myself since it is a large change:
https://codereview.chromium.org/1596663002/

[Yusuke Sato, 2016-01-16 00:11:07]

PTAL at patch set #3.

Again, please ignore the diff for ime.mojom. Sorry about that.

https://codereview.chromium.org/1590723003/diff/20001/components/arc/arc_brid...
File components/arc/arc_bridge_service.cc (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/arc_brid...
components/arc/arc_bridge_service.cc:168: FOR_EACH_OBSERVER(Observer,
observer_list(), OnIntentHelperInstanceReady());
On 2016/01/15 09:04:55, hidehiko wrote:
> Please call this after set_connection_error_handler().
> (Other methods are fixed on ToT).

Done.

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/a...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/a...
components/arc/common/arc_bridge.mojom:36:
OnIntentHelperInstanceReady@111(IntentHelperInstance instance_ptr);
On 2016/01/15 09:04:55, hidehiko wrote:
> nit: MinVersion = 4 or later? (Please re-check this when submitting).

Will do when I submit this.

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/i...
File components/arc/common/intent.mojom (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/i...
components/arc/common/intent.mojom:7: interface IntentHelperHost {
On 2016/01/15 17:26:47, Luis Héctor Chávez wrote:
> Also document the purpose of this interface.

Done.
 
> Now that I think of it, the Settings interface just sends broadcast intents
that
> are listened by ArcSettings package.
> 
> Given that it makes more sense for the IntentHelper to also handle broadcast
> intents, can you sync up with benhansen@ to see if it is possible to drop the
> settings interface and have the IntentHelperInstance also handle broadcasting
> intents?

Chatted with Ben and we agreed to merge settings.mojom into mine. Let me do that
in a separate CL (I'm going to work on that right after this CL.) For now, added
a TODO.

Ben also told me that LaunchApp() in app.mojom might be a candidate to move to
intent.mojo, but the interface can probably stay in app.mojom because LaunchApp
does not send intent/broadcast at all. It just issues a binder call to our home
screen app.

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/i...
components/arc/common/intent.mojom:8: OnOpenUrl@0(string url);
On 2016/01/15 09:04:55, hidehiko wrote:
> Could you document what this does?

Done.

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
File components/arc/intent_helper/arc_intent_helper_bridge.h (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:16: class
ArcIntentHelperBridge {
On 2016/01/15 19:10:22, Luis Héctor Chávez wrote:
> On 2016/01/15 17:26:47, Luis Héctor Chávez wrote:
> > This is the third time this exact interface has been introduced, and the
> > constructor for the ArcServiceManager is getting out of hand.
> > 
> > How about you add
> > 
> > components/arc/arc_service.h:
> > 
> > namespace arc {
> > 
> > class ArcService {
> >  public:
> >   virtual ~ArcService() {}
> >   virtual void Initialize(ArcBridgeService* service) = 0;
> > 
> >  private:
> >   DISALLOW_COPY_AND_ASSIGN(ArcService);
> > };
> > 
> > }  // namespace
> > 
> > And make ArcServiceManager receive a std::initializer_list<ArcService>[1]
and
> > store that to a std::vector<ArcService> instead of having to modify that
class
> > every time we want to add a new service. The rest of the services
constructed
> in
> > the ArcServiceManager can also be modified to implement this interface
(except
> > |arc_notification_manager_|), but that can be done in a separate change.
> > 
> > 1: I *think* this should work with std::vector::emplace{_,back}, but if you
> run
> > into any issues, fall back to a
std::initializer_list<scoped_ptr<ArcService>>.
> 
> That won't work, since std::initializer_list always makes a copy :( Let me
think
> this again.

Will rebase this on top of your refactoring CL as needed.

https://codereview.chromium.org/1590723003/diff/20001/components/arc/intent_h...
components/arc/intent_helper/arc_intent_helper_bridge.h:18: virtual
~ArcIntentHelperBridge() = 0;
On 2016/01/15 09:04:55, hidehiko wrote:
> s/ = 0//

Done. Is there a standard coding guideline for this? (I followed
components/arc/settings/arc_settings_bridge.h.)

[elijahtaylor1, 2016-01-16 06:52:04]

removing myself from the review, this already has enough coverage in
components/arc

[elijahtaylor1, 2016-01-16 06:52:17]

elijahtaylor@chromium.org changed reviewers:
- elijahtaylor@chromium.org

[hidehiko, 2016-01-19 05:16:57]

LGTM assuming this lands before Luis's conflicting CLs.

Note: you'll need an OWNER review for .mojom files, too.

[Yusuke Sato, 2016-01-19 20:31:41]

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/a...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/1590723003/diff/20001/components/arc/common/a...
components/arc/common/arc_bridge.mojom:36:
OnIntentHelperInstanceReady@111(IntentHelperInstance instance_ptr);
On 2016/01/15 09:04:55, hidehiko wrote:
> nit: MinVersion = 4 or later? (Please re-check this when submitting).

Done.

[Yusuke Sato, 2016-01-19 20:32:21]

yusukes@chromium.org changed reviewers:
+ dcheng@chromium.org, xiyuan@chromium.org

[Yusuke Sato, 2016-01-19 20:34:31]

Luis: PTAL
Xiyuan, Daniel: Could you take a look at c/b/chromeos/ and mojom files?

[Luis Héctor Chávez, 2016-01-19 20:35:22]

lgtm

[xiyuan, 2016-01-19 21:52:16]

c/b/chromeos/ LGTM

[dcheng, 2016-01-19 22:08:39]

lgtm

[Yusuke Sato, 2016-01-19 22:52:27]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-01-19 22:52:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/1590723003/#ps80001
(title: "Add MinVersion=4")

[commit-bot: I haz the power, 2016-01-19 22:53:23]

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

[commit-bot: I haz the power, 2016-01-20 00:39:16]

Description was changed from

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC
==========

to

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC
==========

[commit-bot: I haz the power, 2016-01-20 00:39:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-20 00:41:41]

Description was changed from

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC
==========

to

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}
==========

[commit-bot: I haz the power, 2016-01-20 00:41:42]

Patchset 5 (id:??) landed as
https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}

[raymes, 2016-01-20 02:54:56]

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

[raymes, 2016-01-20 02:54:57]

I think this may have broken the ChromeOS trybots:
https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64...

[raymes, 2016-01-20 02:57:32]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1607603005/ by raymes@chromium.org.

The reason for reverting is: This may have broken the ChromeOS trybots: 

https://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64...

../../chrome/browser/chromeos/arc/arc_intent_helper_bridge_impl.cc: In member
function 'virtual void arc::ArcIntentHelperBridgeImpl::OnOpenUrl(const
mojo::String&)':
../../chrome/browser/chromeos/arc/arc_intent_helper_bridge_impl.cc:39:16: error:
no matching function for call to 'GURL::GURL(const mojo::String&)'
   GURL gurl(url);.

[Yusuke Sato, 2016-01-20 18:37:53]

Description was changed from

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}
==========

to

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}
==========

[Yusuke Sato, 2016-01-20 19:00:58]

Thanks Raymes for taking care of this. The compile error was a mid-air collision
with
https://chromium.googlesource.com/chromium/src/+/dfbcc3b7926990bbf5232112261f...
(GURL interface change).

Xiyuan, could you take another look at
https://codereview.chromium.org/1590723003/diff2/80001:100001/chrome/browser/...
? Other files are unchanged.

[xiyuan, 2016-01-20 19:37:14]

lgtm

[Yusuke Sato, 2016-01-20 19:42:50]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-01-20 19:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org, lhchavez@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1590723003/#ps100001
(title: "Support new GURL interface at
https://chromium.googlesource.com/chromium/src/+/dfbcc3b7926990bbf523…")

[commit-bot: I haz the power, 2016-01-20 19:43:50]

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

[commit-bot: I haz the power, 2016-01-20 20:15:06]

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

[commit-bot: I haz the power, 2016-01-20 20:15:07]

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

[Yusuke Sato, 2016-01-20 20:20:49]

The CQ bit was checked by yusukes@chromium.org

[commit-bot: I haz the power, 2016-01-20 20:21:46]

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

[commit-bot: I haz the power, 2016-01-20 21:08:17]

Description was changed from

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}
==========

to

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}
==========

[commit-bot: I haz the power, 2016-01-20 21:08:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-20 21:10:01]

Description was changed from

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}
==========

to

==========
Host-side implementation of ARC Intent handler

The new interface, OnOpenUrl(), is implemented in
arc_intent_helper_bridge_impl.cc. ARC-side CLs for
the interfacve are ag/845051/ and ag/845044/.

BUG=577929
TEST=http, https, and mailto URLs are passed from ARC

Committed: https://crrev.com/379e0814ff680203c0b4d88cc77d339f0d03f70f
Cr-Commit-Position: refs/heads/master@{#370240}

Committed: https://crrev.com/03af3e520814e764eba70ce6512f0a0f98e3e18e
Cr-Commit-Position: refs/heads/master@{#370487}
==========

[commit-bot: I haz the power, 2016-01-20 21:10:03]

Patchset 6 (id:??) landed as
https://crrev.com/03af3e520814e764eba70ce6512f0a0f98e3e18e
Cr-Commit-Position: refs/heads/master@{#370487}