Merge arc/common/settings.mojom into intent_helper.mojom

Merge arc/common/settings.mojom into intent_helper.mojom

This is for ag/848680 and ag/848681.

BUG=577929
TEST=http, https, and mailto URLs are still passed from ARC.
 CrOS setting changes are still sent to ARC.

Committed: https://crrev.com/d85dbd46dd8e9b4bc268b45ba2593ca761fde4f3
Cr-Commit-Position: refs/heads/master@{#371067}

[Yusuke Sato, 2016-01-21 01:18:53]

Description was changed from

==========
Merge arc/common/settings.mojom into intent_helper.mojom

BUG=577929
TEST=http, https, and mailto URLs are still passed from ARC.
 CrOS setting changes are still sent to ARC.
==========

to

==========
Merge arc/common/settings.mojom into intent_helper.mojom

This is for ag/848680 and ag/848681.

BUG=577929
TEST=http, https, and mailto URLs are still passed from ARC.
 CrOS setting changes are still sent to ARC.
==========

[Yusuke Sato, 2016-01-21 01:23:18]

yusukes@chromium.org changed reviewers:
+ benhansen@chromium.org, lhchavez@chromium.org

[Yusuke Sato, 2016-01-21 01:23:19]

PTAL

[benhansen1, 2016-01-21 02:41:04]

benhansen@google.com changed reviewers:
+ benhansen@google.com

[benhansen1, 2016-01-21 02:41:05]

LGTM. Thanks for doing all this work to merge them!

https://codereview.chromium.org/1610813003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/settings_bridge.cc (right):

https://codereview.chromium.org/1610813003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/settings_bridge.cc:161: if (delegate_)
Should we DCHECK delegate instead since it should always be set?

[Yusuke Sato, 2016-01-21 22:42:57]

PTAL

https://codereview.chromium.org/1610813003/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/settings_bridge.cc (right):

https://codereview.chromium.org/1610813003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/settings_bridge.cc:161: if (delegate_)
On 2016/01/21 02:41:05, benhansen1 wrote:
> Should we DCHECK delegate instead since it should always be set?

Done.

[benhansen1, 2016-01-21 22:45:38]

Great. Thanks again. Still LGTM

[Yusuke Sato, 2016-01-21 22:54:34]

(btw, settings.mojom is still marked as 'D' in rietveld, but it _is_ added
back.)

[Luis Héctor Chávez, 2016-01-21 23:32:06]

LGTM with a nit. You might want to rebase since the ArcService change landed.

https://codereview.chromium.org/1610813003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/settings_bridge.h (right):

https://codereview.chromium.org/1610813003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/settings_bridge.h:87: Delegate* delegate_;
nit: Can you make this Delegate* const delegate_;?

[Yusuke Sato, 2016-01-22 00:57:56]

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

[Yusuke Sato, 2016-01-22 00:57:57]

Thanks for the review!

dcheng/xiyuan: could you take a look at *.mojom and c/b/chromeos/ changes? This
CL is for moving a function in settings.mojom to intent_helper.mojom so we can
remove the former in the near future.

https://codereview.chromium.org/1610813003/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/settings_bridge.h (right):

https://codereview.chromium.org/1610813003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/settings_bridge.h:87: Delegate* delegate_;
On 2016/01/21 23:32:06, Luis Héctor Chávez wrote:
> nit: Can you make this Delegate* const delegate_;?

Done.

[xiyuan, 2016-01-22 16:09:59]

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc:30: // to be ready.
The comment seems suggesting that we would have a similar problem with
IntentHelperInstance as in http://crbug.com/575873. Should we do something
similar to what Luis did? That is, invoke OnIntentHelperInstanceReady() right
after AddObserver() in ctor.

e.g.
  arc_bridge_service()->AddObserver(this);
  if (arc_bridge_service()->intent_helper_instance())
    OnIntentHelperInstanceReady();

[lhc(google), 2016-01-22 16:13:44]

lhchavez@google.com changed reviewers:
+ lhchavez@google.com

[lhc(google), 2016-01-22 16:13:44]

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc:30: // to be ready.
On 2016/01/22 16:09:59, xiyuan wrote:
> The comment seems suggesting that we would have a similar problem with
> IntentHelperInstance as in http://crbug.com/575873. Should we do something
> similar to what Luis did? That is, invoke OnIntentHelperInstanceReady() right
> after AddObserver() in ctor.
> 
> e.g.
>   arc_bridge_service()->AddObserver(this);
>   if (arc_bridge_service()->intent_helper_instance())
>     OnIntentHelperInstanceReady();

I already fixed that by merging the race-prevention code into AddObserver, so
you always get notified:
https://code.google.com/p/chromium/codesearch#chromium/src/components/arc/arc...

I would probably remove this function and instead override
OnIntentHelperInstanceClosed(), which is a more reliable way of knowing that the
instance closed the connection (since the instance might reconnect later without
the whole bridge going down).

[xiyuan, 2016-01-22 16:21:12]

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc:30: // to be ready.
On 2016/01/22 16:13:44, lhc(google) wrote:
> On 2016/01/22 16:09:59, xiyuan wrote:
> > The comment seems suggesting that we would have a similar problem with
> > IntentHelperInstance as in http://crbug.com/575873. Should we do something
> > similar to what Luis did? That is, invoke OnIntentHelperInstanceReady()
right
> > after AddObserver() in ctor.
> > 
> > e.g.
> >   arc_bridge_service()->AddObserver(this);
> >   if (arc_bridge_service()->intent_helper_instance())
> >     OnIntentHelperInstanceReady();
> 
> I already fixed that by merging the race-prevention code into AddObserver, so
> you always get notified:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/arc/arc...
> 
> I would probably remove this function and instead override
> OnIntentHelperInstanceClosed(), which is a more reliable way of knowing that
the
> instance closed the connection (since the instance might reconnect later
without
> the whole bridge going down).

Thanks for the clarification. Apparently, I did not follow the ArcService CL
closely. :p

Let's do what Luis suggested, replace this with OnIntentHelperInstanceClosed().

[Yusuke Sato, 2016-01-22 18:04:10]

Please take another look.

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc (right):

https://codereview.chromium.org/1610813003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_intent_helper_bridge.cc:30: // to be ready.
Sounds good! Thanks for the suggestion. Done.

[dcheng, 2016-01-22 18:30:52]

lgtm

[xiyuan, 2016-01-22 18:40:38]

lgtm

Cool.

[Yusuke Sato, 2016-01-22 22:55:13]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-01-22 22:55:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org, benhansen@google.com
Link to the patchset: https://codereview.chromium.org/1610813003/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2016-01-22 22:56:46]

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

[commit-bot: I haz the power, 2016-01-22 23:06:35]

Description was changed from

==========
Merge arc/common/settings.mojom into intent_helper.mojom

This is for ag/848680 and ag/848681.

BUG=577929
TEST=http, https, and mailto URLs are still passed from ARC.
 CrOS setting changes are still sent to ARC.
==========

to

==========
Merge arc/common/settings.mojom into intent_helper.mojom

This is for ag/848680 and ag/848681.

BUG=577929
TEST=http, https, and mailto URLs are still passed from ARC.
 CrOS setting changes are still sent to ARC.
==========

[commit-bot: I haz the power, 2016-01-22 23:06:36]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-01-22 23:08:05]

Description was changed from

==========
Merge arc/common/settings.mojom into intent_helper.mojom

This is for ag/848680 and ag/848681.

BUG=577929
TEST=http, https, and mailto URLs are still passed from ARC.
 CrOS setting changes are still sent to ARC.
==========

to

==========
Merge arc/common/settings.mojom into intent_helper.mojom

This is for ag/848680 and ag/848681.

BUG=577929
TEST=http, https, and mailto URLs are still passed from ARC.
 CrOS setting changes are still sent to ARC.

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

[commit-bot: I haz the power, 2016-01-22 23:08:06]

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