Create an Obj-C bridge for MostVisitedSitesObserver

Create an Obj-C bridge for MostVisitedSitesObserver

BUG=707754
Review-Url: https://codereview.chromium.org/2791253002
Cr-Commit-Position: refs/heads/master@{#461648}
Committed: https://chromium.googlesource.com/chromium/src/+/faf8e94ac52d828621c9ad0d3a491a147c701de7

[gambard, 2017-04-03 13:50:40]

gambard@chromium.org changed reviewers:
+ sfiera@chromium.org

[gambard, 2017-04-03 13:50:41]

PTAL.

[sfiera, 2017-04-03 14:03:15]

This looks good, but:
- Can it already replace ios/chrome/browser/ui/ntp/google_landing_controller.mm?
- Why here and not ios/chrome/browser/ntp_tiles?

[sfiera, 2017-04-03 14:03:40]

On 2017/04/03 14:03:15, sfiera wrote:
> This looks good, but:
> - Can it already replace
ios/chrome/browser/ui/ntp/google_landing_controller.mm?

(not the whole file, I mean, but the class with identical functionality there)

[gambard, 2017-04-03 14:18:49]

+sdefresne for file location.
I have used it in Google Landing Controller.

[gambard, 2017-04-03 14:19:10]

gambard@chromium.org changed reviewers:
+ sdefresne@chromium.org

[gambard, 2017-04-03 14:19:11]

Actually adding sdefresne.

[sfiera, 2017-04-03 14:23:29]

LGTM

I was suggesting ios/chrome/browser/ntp_tiles (next to
IOSMostVisitedSitesFactory), not components/ntp_tiles/ios. Given that the class
is just an ObjC binding, and not really tied to an embedder, I'm OK with the
component too.

[sdefresne, 2017-04-03 14:25:49]

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
File components/ntp_tiles/ios/most_visited_sites_bridge_observer.h (right):

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:1: // Copyright
2017 The Chromium Authors. All rights reserved.
I think this file should be named after the main class, so
most_visited_sites_bridge.h or if you rename the class as suggested
most_visited_sites_observer_bridge.h.

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:14: @protocol
MostVisitedSitesObserver
marq & rohitrao recommendation regarding Objective-C protocol to bridge C++
observer interfaces would be to name this @protocol MostVisitedSitesObserving.

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:29: class
MostVisitedSitesBridge : public MostVisitedSites::Observer {
I would call this MostVisitedSitesObserverBridge.

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:37:
__unsafe_unretained id<MostVisitedSitesObserver> observer_;
I think we should avoid using __unsafe_unretained if possible, what about using
base::WeakNSProtocol<id<MostvisitedSitesObserver>> here?

[sdefresne, 2017-04-03 14:26:01]

lgtm with comments

[gambard, 2017-04-03 15:17:37]

Thanks, I have moved the files to ios/

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
File components/ntp_tiles/ios/most_visited_sites_bridge_observer.h (right):

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:1: // Copyright
2017 The Chromium Authors. All rights reserved.
On 2017/04/03 14:25:49, sdefresne wrote:
> I think this file should be named after the main class, so
> most_visited_sites_bridge.h or if you rename the class as suggested
> most_visited_sites_observer_bridge.h.

Done.

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:14: @protocol
MostVisitedSitesObserver
On 2017/04/03 14:25:49, sdefresne wrote:
> marq & rohitrao recommendation regarding Objective-C protocol to bridge C++
> observer interfaces would be to name this @protocol MostVisitedSitesObserving.

Done.

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:29: class
MostVisitedSitesBridge : public MostVisitedSites::Observer {
On 2017/04/03 14:25:49, sdefresne wrote:
> I would call this MostVisitedSitesObserverBridge.

Done.

https://codereview.chromium.org/2791253002/diff/60001/components/ntp_tiles/io...
components/ntp_tiles/ios/most_visited_sites_bridge_observer.h:37:
__unsafe_unretained id<MostVisitedSitesObserver> observer_;
On 2017/04/03 14:25:49, sdefresne wrote:
> I think we should avoid using __unsafe_unretained if possible, what about
using
> base::WeakNSProtocol<id<MostvisitedSitesObserver>> here?

Done.

[gambard, 2017-04-03 15:22:08]

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

[commit-bot: I haz the power, 2017-04-03 15:22:39]

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, 2017-04-03 16:27:56]

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

[commit-bot: I haz the power, 2017-04-03 16:27:57]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-04-04 06:51:12]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-04-04 06:51:13]

The patchset sent to the CQ was uploaded after l-g-t-m from sfiera@chromium.org,
sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2791253002/#ps100001
(title: "Move files to ios/")

[commit-bot: I haz the power, 2017-04-04 06:51:30]

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, 2017-04-04 06:55:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491288672590120,
"parent_rev": "be4aafe5cf2c03d863ed96d7fc3d3e6b66a86d0f", "commit_rev":
"faf8e94ac52d828621c9ad0d3a491a147c701de7"}

[commit-bot: I haz the power, 2017-04-04 06:56:13]

Description was changed from

==========
Create an Obj-C bridge for MostVisitedSitesObserver

BUG=707754
==========

to

==========
Create an Obj-C bridge for MostVisitedSitesObserver

BUG=707754

Review-Url: https://codereview.chromium.org/2791253002
Cr-Commit-Position: refs/heads/master@{#461648}
Committed:
https://chromium.googlesource.com/chromium/src/+/faf8e94ac52d828621c9ad0d3a49...
==========

[commit-bot: I haz the power, 2017-04-04 06:56:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/faf8e94ac52d828621c9ad0d3a49...