Add an external protocol dialog for ARC

Add an external protocol dialog for ARC

BUG=647485
TEST=try, manually tested the case in b:31406784

Committed: https://crrev.com/1b711a96ffa4dc40501ae767a3ff9e6fd736ff20
Cr-Commit-Position: refs/heads/master@{#419286}

[Yusuke Sato, 2016-09-15 22:20:36]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 22:20:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2016-09-15 22:21:22]

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

[Yusuke Sato, 2016-09-15 22:21:22]

the pdd review is still ongoing (see b/31406784), but could you start taking a
look at other part? thanks!

[Luis Héctor Chávez, 2016-09-15 22:42:12]

b/31406784 is marked as M-54, so you want a crbug too.

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

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:35: constexpr int
kMinInstanceVersion = 3;  // RequestActivityIcons' MinVersion.
nit: uint32_t.

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:110: gfx::Image
icon;
This doesn't seem to be used.

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:184:
intent_helper->RequestUrlHandlerList(
What would happen if ARC were to completely crash before the callback fires?

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_external_protocol_dialog.h (right):

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.h:14: // Shows ARC
version of the dialog. Returns true if ARC is supported and
maybe add "and is run in a context where it is allowed to handle external
protocol"... or something like that that is not as verbose.

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (left):

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:83: const
ui::PageTransition transition =
Naïve question: shouldn't there be a if (ShouldIgnoreNavigation(...)) return
...PROCEED;  here?

[Yusuke Sato, 2016-09-15 22:50:47]

Description was changed from

==========
Add an external protocol dialog for ARC

BUG=b:29079617,b:31406784
TEST=try, manually tested the case in b:31406784
==========

to

==========
Add an external protocol dialog for ARC

BUG=647485
TEST=try, manually tested the case in b:31406784
==========

[Yusuke Sato, 2016-09-15 23:07:45]

The CQ bit was checked by yusukes@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 23:07:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Yusuke Sato, 2016-09-15 23:08:25]

ptal

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

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:35: constexpr int
kMinInstanceVersion = 3;  // RequestActivityIcons' MinVersion.
On 2016/09/15 22:42:11, Luis Héctor Chávez wrote:
> nit: uint32_t.

Done.

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:110: gfx::Image
icon;
On 2016/09/15 22:42:11, Luis Héctor Chávez wrote:
> This doesn't seem to be used.

Done.

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:184:
intent_helper->RequestUrlHandlerList(
On 2016/09/15 22:42:11, Luis Héctor Chávez wrote:
> What would happen if ARC were to completely crash before the callback fires?

In that case, no dialog will be shown for the user click, and I feel that's
acceptable. The Chrome OS' default dialog will be shown (because of the check at
line 173) if the user clicks the link again.

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_external_protocol_dialog.h (right):

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.h:14: // Shows ARC
version of the dialog. Returns true if ARC is supported and
On 2016/09/15 22:42:11, Luis Héctor Chávez wrote:
> maybe add "and is run in a context where it is allowed to handle external
> protocol"... or something like that that is not as verbose.

Done.

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_navigation_throttle.cc (left):

https://codereview.chromium.org/2348673002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:83: const
ui::PageTransition transition =
On 2016/09/15 22:42:11, Luis Héctor Chávez wrote:
> Naïve question: shouldn't there be a if (ShouldIgnoreNavigation(...)) return
> ...PROCEED;  here?

Done.

I moved it to line 247 but it was probably more difficult to read.

[Luis Héctor Chávez, 2016-09-15 23:12:18]

lgtm, but you might want to check with somebody more knowledgeable in browser
stuff.

[Yusuke Sato, 2016-09-15 23:59:29]

thanks, I'll ask for a chromeos/ owner for review once I get pdd approval.

[commit-bot: I haz the power, 2016-09-16 00:21:54]

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

[commit-bot: I haz the power, 2016-09-16 00:21:57]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-09-16 19:41:00]

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

[Yusuke Sato, 2016-09-16 19:41:01]

Xiyuan, could you do an owner review for chromeos/?

[xiyuan, 2016-09-16 20:38:35]

lgtm

https://codereview.chromium.org/2348673002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc (right):

https://codereview.chromium.org/2348673002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:161: // static
nit: remove since it is not really a static method

[Yusuke Sato, 2016-09-16 20:55:12]

thanks!

https://codereview.chromium.org/2348673002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc (right):

https://codereview.chromium.org/2348673002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_external_protocol_dialog.cc:161: // static
On 2016/09/16 20:38:35, xiyuan wrote:
> nit: remove since it is not really a static method

Done.

[Yusuke Sato, 2016-09-16 20:55:37]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-09-16 20:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org,
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2348673002/#ps40001
(title: "Address Xiyuan's comment")

[commit-bot: I haz the power, 2016-09-16 20:56:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-16 21:52:45]

Description was changed from

==========
Add an external protocol dialog for ARC

BUG=647485
TEST=try, manually tested the case in b:31406784
==========

to

==========
Add an external protocol dialog for ARC

BUG=647485
TEST=try, manually tested the case in b:31406784
==========

[commit-bot: I haz the power, 2016-09-16 21:52:46]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-16 21:54:26]

Description was changed from

==========
Add an external protocol dialog for ARC

BUG=647485
TEST=try, manually tested the case in b:31406784
==========

to

==========
Add an external protocol dialog for ARC

BUG=647485
TEST=try, manually tested the case in b:31406784

Committed: https://crrev.com/1b711a96ffa4dc40501ae767a3ff9e6fd736ff20
Cr-Commit-Position: refs/heads/master@{#419286}
==========

[commit-bot: I haz the power, 2016-09-16 21:54:28]

Patchset 3 (id:??) landed as
https://crrev.com/1b711a96ffa4dc40501ae767a3ff9e6fd736ff20
Cr-Commit-Position: refs/heads/master@{#419286}