[ios] Adds code to support the iOS Handoff feature.

[ios] Adds code to support the iOS Handoff feature.

BUG=662936
Committed: https://crrev.com/f5225f68f44886f5d020d9e92a3dba6fac23a1ab
Cr-Commit-Position: refs/heads/master@{#430766}

[rohitrao (ping after 24h), 2016-11-07 16:17:56]

rohitrao@chromium.org changed reviewers:
+ erikchen@chromium.org, sdefresne@chromium.org

[rohitrao (ping after 24h), 2016-11-07 16:17:57]

+erikchen for review and OWNERS approval of the DEPS addition

+sdefresne FYI

[erikchen, 2016-11-07 19:02:58]

Before I do a full review, can you respond to my comment?

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/handoff_manager.h (right):

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/handoff_manager.h:18: @interface
HandoffManager : NSObject
isn't there already a HandoffManager?

components/handoff/handoff_manager.h

[rohitrao (ping after 24h), 2016-11-07 19:13:25]

On 2016/11/07 19:02:58, erikchen wrote:
> Before I do a full review, can you respond to my comment?
> 
>
https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
> File ios/chrome/browser/device_sharing/handoff_manager.h (right):
> 
>
https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
> ios/chrome/browser/device_sharing/handoff_manager.h:18: @interface
> HandoffManager : NSObject
> isn't there already a HandoffManager?
> 
> components/handoff/handoff_manager.h

Huh, yes, but that file is only compiled on Mac, so we apparently have our own
version on iOS.

Let me audit the differences between the two files and see if I can unify them.

[erikchen, 2016-11-07 22:02:08]

lgtm on the premise that the two HandoffManagers will be refactored sometime in
the not-so-distant future.

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/device_sharing_manager.h (right):

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/device_sharing_manager.h:29: //
|-updateBrowserState:NULL| before |browserState| is destroyed.
s/NULL/nullptr

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/handoff_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/handoff_manager.mm:18: GURL _activeURL;
I think you should remove this line and add an explicit synthesize.

[erikchen, 2016-11-07 22:04:34]

On 2016/11/07 22:02:08, erikchen wrote:
> lgtm on the premise that the two HandoffManagers will be refactored sometime
in
> the not-so-distant future.

Maybe add a TODO and a link to a crbug?

[rohitrao (ping after 24h), 2016-11-08 02:02:44]

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

[commit-bot: I haz the power, 2016-11-08 02:03:16]

Dry run: 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-11-08 02:11:10]

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

[commit-bot: I haz the power, 2016-11-08 02:11:10]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[rohitrao (ping after 24h), 2016-11-08 02:11:41]

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

[commit-bot: I haz the power, 2016-11-08 02:12:20]

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

[rohitrao (ping after 24h), 2016-11-08 02:15:31]

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

[commit-bot: I haz the power, 2016-11-08 02:16:30]

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

[rohitrao (ping after 24h), 2016-11-08 02:19:42]

Updated to use the existing HandoffManager.  Do these changes look ok to you? 
How do you feel about putting the prefs registration behind OS_IOS?

I manually verified that the iOS version of HandoffManager is the same as the
components/ version, except for a few small differences like ivars converted to
properties.

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/device_sharing_manager.h (right):

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/device_sharing_manager.h:29: //
|-updateBrowserState:NULL| before |browserState| is destroyed.
On 2016/11/07 22:02:08, erikchen wrote:
> s/NULL/nullptr

Done.

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/handoff_manager.h (right):

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/handoff_manager.h:18: @interface
HandoffManager : NSObject
On 2016/11/07 19:02:58, erikchen wrote:
> isn't there already a HandoffManager?
> 
> components/handoff/handoff_manager.h

Acknowledged.

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/handoff_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/20001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/handoff_manager.mm:18: GURL _activeURL;
On 2016/11/07 22:02:08, erikchen wrote:
> I think you should remove this line and add an explicit synthesize.

No longer relevant, this file has been deleted from the CL.

[commit-bot: I haz the power, 2016-11-08 03:26:40]

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

[commit-bot: I haz the power, 2016-11-08 03:26:41]

Dry run: This issue passed the CQ dry run.

[erikchen, 2016-11-08 18:13:55]

lgtm with nits

https://codereview.chromium.org/2484703003/diff/70001/components/handoff/hand...
File components/handoff/handoff_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/70001/components/handoff/hand...
components/handoff/handoff_manager.mm:14: #include
"components/pref_registry/pref_registry_syncable.h"  // nogncheck
why do you need nogncheck?

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/OWNERS (right):

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/OWNERS:1: erikchen@chromium.org
I'm an OWNER of this?

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/device_sharing_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/device_sharing_manager.mm:93: // TODO(msarda):
Handle nearby sharing pref changes.
msarda is getting rid of this downstream:
https://chromereviews.googleplex.com/538247013/

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/pref...
File ios/chrome/browser/pref_names.h (right):

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/pref...
ios/chrome/browser/pref_names.h:8: // TODO(rohitrao): Remove these includes once
embedders are migrated to use the
add a link to a crbug

[rohitrao (ping after 24h), 2016-11-08 19:57:36]

https://codereview.chromium.org/2484703003/diff/70001/components/handoff/hand...
File components/handoff/handoff_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/70001/components/handoff/hand...
components/handoff/handoff_manager.mm:14: #include
"components/pref_registry/pref_registry_syncable.h"  // nogncheck
On 2016/11/08 18:13:55, erikchen wrote:
> why do you need nogncheck?

I added the components/pref_registry dependency only to the iOS build, so this
include would fail the GN header check on mac.

I could alternatively add the dependency on all platforms, even though it's not
needed on mac.

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/OWNERS (right):

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/OWNERS:1: erikchen@chromium.org
On 2016/11/08 18:13:55, erikchen wrote:
> I'm an OWNER of this?

SURPRISE!

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
File ios/chrome/browser/device_sharing/device_sharing_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/devi...
ios/chrome/browser/device_sharing/device_sharing_manager.mm:93: // TODO(msarda):
Handle nearby sharing pref changes.
On 2016/11/08 18:13:55, erikchen wrote:
> msarda is getting rid of this downstream:
> https://chromereviews.googleplex.com/538247013/

Done.

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/pref...
File ios/chrome/browser/pref_names.h (right):

https://codereview.chromium.org/2484703003/diff/70001/ios/chrome/browser/pref...
ios/chrome/browser/pref_names.h:8: // TODO(rohitrao): Remove these includes once
embedders are migrated to use the
On 2016/11/08 18:13:55, erikchen wrote:
> add a link to a crbug

Done.

[rohitrao (ping after 24h), 2016-11-08 20:02:20]

The CQ bit was checked by rohitrao@chromium.org

[rohitrao (ping after 24h), 2016-11-08 20:02:21]

The patchset sent to the CQ was uploaded after l-g-t-m from
erikchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2484703003/#ps90001
(title: "Review.")

[commit-bot: I haz the power, 2016-11-08 20:03:10]

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-11-08 20:12:13]

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

[commit-bot: I haz the power, 2016-11-08 20:12:14]

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

[rohitrao (ping after 24h), 2016-11-08 20:13:39]

rohitrao@chromium.org changed reviewers:
+ gab@chromium.org

[rohitrao (ping after 24h), 2016-11-08 20:13:40]

+gab for OWNERS approval for the new DEPS entry from components/handoff ->
components/pref_registry

[gab, 2016-11-08 21:26:22]

prefs DEPS addition rs-lgtm

https://codereview.chromium.org/2484703003/diff/90001/components/handoff/hand...
File components/handoff/handoff_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/90001/components/handoff/hand...
components/handoff/handoff_manager.mm:14: #include
"components/pref_registry/pref_registry_syncable.h"  // nogncheck
What does // nogncheck do?

[rohitrao (ping after 24h), 2016-11-08 23:37:29]

The CQ bit was checked by rohitrao@chromium.org

[commit-bot: I haz the power, 2016-11-08 23:38:21]

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

[rohitrao (ping after 24h), 2016-11-08 23:39:33]

https://codereview.chromium.org/2484703003/diff/90001/components/handoff/hand...
File components/handoff/handoff_manager.mm (right):

https://codereview.chromium.org/2484703003/diff/90001/components/handoff/hand...
components/handoff/handoff_manager.mm:14: #include
"components/pref_registry/pref_registry_syncable.h"  // nogncheck
On 2016/11/08 21:26:22, gab wrote:
> What does // nogncheck do?

The pref_registry dependency is added conditionally, only on iOS, and the header
is similarly in an OS_IOS block.  But "gn check" doesn't understand how to
process #ifdefs, so when it runs on mac, it sees this include and complains
(correctly, on Mac) that the handoff target doesn't depend on pref_registry.

// nogncheck tells the checker to ignore this include.  It's generally used to
work around this kind of scenario.

[commit-bot: I haz the power, 2016-11-08 23:45:04]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2016-11-08 23:48:56]

Description was changed from

==========
[ios] Adds code to support the iOS Handoff feature.

BUG=662936
==========

to

==========
[ios] Adds code to support the iOS Handoff feature.

BUG=662936

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

[commit-bot: I haz the power, 2016-11-08 23:48:57]

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