Add ContentSuggestionsMediator

Add ContentSuggestionsMediator

This CL adds a Mediator level in ContentSuggestions.
The mediator classes are used to create a interface between the data source in
C++ and the UI in Objective-C.
This CL only creates the classes to see the interactions from an architectural point.
Most of the methods are no-op for now.

BUG=679369
Review-Url: https://codereview.chromium.org/2638413006
Cr-Commit-Position: refs/heads/master@{#447511}
Committed: https://chromium.googlesource.com/chromium/src/+/0ac7f3e7f45b25cf8b3283a7df27bf68b3a72157

[gambard, 2017-01-20 12:40:38]

Description was changed from

==========
Add ContentSuggestionsMediator

This CL adds a Mediator level in ContentSuggestions.
The mediator classes are used to create a interface between the data source in
C++ and the UI in Objective-C.

BUG=679369
==========

to

==========
Add ContentSuggestionsMediator

This CL adds a Mediator level in ContentSuggestions.
The mediator classes are used to create a interface between the data source in
C++ and the UI in Objective-C.
This CL only creates the classes to see the interactions from an architectural
point.
Most of the methods are no-op for now.

BUG=679369
==========

[gambard, 2017-01-25 12:53:49]

gambard@chromium.org changed reviewers:
+ lpromero@chromium.org, marq@chromium.org

[gambard, 2017-01-25 12:53:49]

PTAL.

https://codereview.chromium.org/2638413006/diff/20001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (left):

https://codereview.chromium.org/2638413006/diff/20001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:43: NOTREACHED();
This is being fixed in another CL.

[lpromero, 2017-01-26 10:10:21]

I like this global picture we talked about where the UI code basically describes
the language it needs via protocols, and the coordinator/model layer just speaks
its language.

UI layer is hermetic to any Chromium code. It will be easy to integrate with
Showcase (which is not per se a goal, but a litmus test).

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:18: //
BrowserState uesd to create the ContentSuggestionFactory.
used*

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:41:
initWithContentService:IOSChromeContentSuggestionsServiceFactory::
Should you just pass the ContentService to this coordinator?
Mark, what is your global vision on this? New Arch coordinators all have a
browserState, does that mean that the coordinators are also responsible to
create the right service they need?
I don't yet see any issue with both approaches. Just trying to sync with the big
picture.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:18: //
ntp_snippets::ContentSuggestionsService and the objective-c services using
Nit: Objective-C.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (left):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:43: NOTREACHED();
What is the context of this change? Is it related to the CL goal?

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:26:
@property(nonatomic, strong) id<ContentSuggestionsDataSource> dataSource;
Can we risk creating a cycle if the one creating and retaining the updater is
the data source? Worth mentioning in the header that this object has a strong
reference to the data source, so the caller should take action to break a
potential cycle when needed.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:53:
// Loads the data with the dataSource instead of hard coded value.
Mark this with // PLACEHOLDER

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:12:
// format compatible with objective-c.
Nit: Objective-C

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:16:
@property(nonatomic, weak) id<ContentSuggestionsDataSink> dataSink;
It's basically a dataSource delegate?

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:19:
- (NSArray<id<ContentSuggestionsData>>*)getData;
s/getData/currentData

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:21:
// Returns the new data.
When dataSink receives the dataAvailable message, does getData/currentData
already contains the new data? I think it should.

Do you need the diff?

If yes, then keep this but s/getMoreData/availableData.
If no, remove this second method and just requery getData/currentData.

[marq (ping after 24h), 2017-01-27 12:44:56]

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:41:
initWithContentService:IOSChromeContentSuggestionsServiceFactory::
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> Should you just pass the ContentService to this coordinator?
> Mark, what is your global vision on this? New Arch coordinators all have a
> browserState, does that mean that the coordinators are also responsible to
> create the right service they need?
> I don't yet see any issue with both approaches. Just trying to sync with the
big
> picture.

Globally the idea was that coordinators would manipulate browserStates and pass
specific services into non-coordinator objects. The general idea is to keep the
public API of coordinators simple enough so we don't need to pass specific
services up and down the chain.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h:12:
@protocol ContentSuggestionsServiceBridgeObserver
Remove 'Bridge" from the protocol name.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h:13:

You still need comments for each method declaration; compare to
web_state_observer_bridge.h

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h:14:
- (void)onNewSuggestions:(ntp_snippets::Category)category;
The usual pattern for delegate and observer protocols is to pass the delegating
or observed object as the first parameter, so I would expect to see the
ContentSuggestionsService object in each of these methods.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:30:
NS_DESIGNATED_INITIALIZER;
Nit: newline after each method declaration.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:26:
@property(nonatomic, strong) id<ContentSuggestionsDataSource> dataSource;
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> Can we risk creating a cycle if the one creating and retaining the updater is
> the data source? Worth mentioning in the header that this object has a strong
> reference to the data source, so the caller should take action to break a
> potential cycle when needed.

Does this need to be strong? Delegates usually aren't.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:53:
// Loads the data with the dataSource instead of hard coded value.
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> Mark this with // PLACEHOLDER

Also, clarify that this is a change that needs to be made ("change this to load
the data ... ").

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:116:
[self.dataSource getMoreData];
Either delete the dataSource call or assign the result to something; the risk
here is that looks like it's causing the dataSource to do something, but the
current method definition returns an array. Better to code defensively even in
placeholder code.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data.h:11:
@protocol ContentSuggestionsData
Avoid very general words in name like 'Data', "Value', 'Object', 'Result'. This
sounds like it can just be a 'ContentSuggestion', maybe?

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data.h:12: 
I'm also not sure about the merits of defining data containers like this with
protocols. A superclass seems fine as long as it remains unencumbered by
dependencies.

Protocols are great for defining interfaces with behavior behind them, but they
are limited in how we can type-check them, and -conformsToProtocol can actually
have performance issues in some cases. So *maybe* it's better to just use a
class here.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h:12: //
Notifies the Data Sink that new data are available.
Super nit: 'data' is the plural of the singular 'datum', but it is usually
treated like a grammatically singular mass noun (like 'water', 'dust', 'food').
So: 'new data is available'.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h:13: -
(void)dataAvailable;
This is interesting to consider and I don't have any concrete answers yet -- but
should this protocol push the data, or just alert the sink and let it pull via
the DataSource protocol? No change needed, but something to consider as this
evolves.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:16:
@property(nonatomic, weak) id<ContentSuggestionsDataSink> dataSink;
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> It's basically a dataSource delegate?

The 'dataSink' naming is something I proposed a while back. This seems like a
good place to see how it feels.

(in general, I like defining specialized cases of the delegate pattern and
giving them their own conventions; the data source/delegate distinction in table
and collection views seems like a good precedent for this).

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:18:
// Returns all the data currently available.
What is returned if no data is available? (I would suggest an empty array).

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:19:
- (NSArray<id<ContentSuggestionsData>>*)getData;
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> s/getData/currentData

Except the important noun is 'suggestions', not 'data'. I would prefer
-allSuggestions here.

('get' in method names is reserved the indirect return of values via in-out
parameters, such as [NSBezierPath getLineDash:count:phase:])

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:21:
// Returns the new data.
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> When dataSink receives the dataAvailable message, does getData/currentData
> already contains the new data? I think it should.
> 
> Do you need the diff?
> 
> If yes, then keep this but s/getMoreData/availableData.
> If no, remove this second method and just requery getData/currentData.

Definitey consider this approach.

Also I would prefer if the method name was more specific about what suggestion
it was fetching; 'moreData' feels a bit like NSSet's 'anyObject', where you have
no assurances about what will be returned. So maybe one of:

- recentSuggestions
- updatedSuggestions
- latestSuggestions

(please avoid 'newSuggestions', since that has implications for memory
semantics).

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:22:
- (NSArray<id<ContentSuggestionsData>>*)getMoreData;
Document what is returned when there is no further data; I would suggest an
empty array rather than nil.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h:24:
NS_DESIGNATED_INITIALIZER;
nit: newline.

[gambard, 2017-01-30 15:19:18]

Thanks, PTAL.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:18: //
BrowserState uesd to create the ContentSuggestionFactory.
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> used*

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:41:
initWithContentService:IOSChromeContentSuggestionsServiceFactory::
On 2017/01/27 12:44:55, marq wrote:
> On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> > Should you just pass the ContentService to this coordinator?
> > Mark, what is your global vision on this? New Arch coordinators all have a
> > browserState, does that mean that the coordinators are also responsible to
> > create the right service they need?
> > I don't yet see any issue with both approaches. Just trying to sync with the
> big
> > picture.
> 
> Globally the idea was that coordinators would manipulate browserStates and
pass
> specific services into non-coordinator objects. The general idea is to keep
the
> public API of coordinators simple enough so we don't need to pass specific
> services up and down the chain.

Acknowledged.
I keep the browserState.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.h:18: //
ntp_snippets::ContentSuggestionsService and the objective-c services using
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> Nit: Objective-C.

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
File
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h:12:
@protocol ContentSuggestionsServiceBridgeObserver
On 2017/01/27 12:44:55, marq wrote:
> Remove 'Bridge" from the protocol name.

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h:13:

On 2017/01/27 12:44:55, marq wrote:
> You still need comments for each method declaration; compare to
> web_state_observer_bridge.h

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_service_bridge_observer.h:14:
- (void)onNewSuggestions:(ntp_snippets::Category)category;
On 2017/01/27 12:44:55, marq wrote:
> The usual pattern for delegate and observer protocols is to pass the
delegating
> or observed object as the first parameter, so I would expect to see the
> ContentSuggestionsService object in each of these methods.

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/image_fetcher_impl.mm (left):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/image_fetcher_impl.mm:43: NOTREACHED();
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> What is the context of this change? Is it related to the CL goal?

It will be handled in https://codereview.chromium.org/2652893005/

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.h:30:
NS_DESIGNATED_INITIALIZER;
On 2017/01/27 12:44:55, marq wrote:
> Nit: newline after each method declaration. 

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:26:
@property(nonatomic, strong) id<ContentSuggestionsDataSource> dataSource;
On 2017/01/27 12:44:55, marq wrote:
> On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> > Can we risk creating a cycle if the one creating and retaining the updater
is
> > the data source? Worth mentioning in the header that this object has a
strong
> > reference to the data source, so the caller should take action to break a
> > potential cycle when needed.
> 
> Does this need to be strong? Delegates usually aren't.

Yes, the data source should be kept alive by the coordinator.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:53:
// Loads the data with the dataSource instead of hard coded value.
On 2017/01/27 12:44:55, marq wrote:
> On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> > Mark this with // PLACEHOLDER
> 
> Also, clarify that this is a change that needs to be made ("change this to
load
> the data ... ").

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_collection_updater.mm:116:
[self.dataSource getMoreData];
On 2017/01/27 12:44:55, marq wrote:
> Either delete the dataSource call or assign the result to something; the risk
> here is that looks like it's causing the dataSource to do something, but the
> current method definition returns an array. Better to code defensively even in
> placeholder code.

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data.h:11:
@protocol ContentSuggestionsData
On 2017/01/27 12:44:55, marq wrote:
> Avoid very general words in name like 'Data', "Value', 'Object', 'Result'.
This
> sounds like it can just be a 'ContentSuggestion', maybe?

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data.h:12: 
On 2017/01/27 12:44:55, marq wrote:
> I'm also not sure about the merits of defining data containers like this with
> protocols. A superclass seems fine as long as it remains unencumbered by
> dependencies.
> 
> Protocols are great for defining interfaces with behavior behind them, but
they
> are limited in how we can type-check them, and -conformsToProtocol can
actually
> have performance issues in some cases. So *maybe* it's better to just use a
> class here.

I will use the class directly for now. If it causes trouble I will revert back
to protocol.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h:12: //
Notifies the Data Sink that new data are available.
On 2017/01/27 12:44:55, marq wrote:
> Super nit: 'data' is the plural of the singular 'datum', but it is usually
> treated like a grammatically singular mass noun (like 'water', 'dust',
'food').
> So: 'new data is available'.

Oh. Ok, I always treated it as plural.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_sink.h:13: -
(void)dataAvailable;
On 2017/01/27 12:44:55, marq wrote:
> This is interesting to consider and I don't have any concrete answers yet --
but
> should this protocol push the data, or just alert the sink and let it pull via
> the DataSource protocol? No change needed, but something to consider as this
> evolves.

By your own documentation: "The dataSource pattern is pull, not push." :)
I will see how it evolves.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:12:
// format compatible with objective-c.
On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> Nit: Objective-C

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:18:
// Returns all the data currently available.
On 2017/01/27 12:44:55, marq wrote:
> What is returned if no data is available? (I would suggest an empty array).

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:19:
- (NSArray<id<ContentSuggestionsData>>*)getData;
On 2017/01/27 12:44:55, marq wrote:
> On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> > s/getData/currentData
> 
> Except the important noun is 'suggestions', not 'data'. I would prefer
> -allSuggestions here.
> 
> ('get' in method names is reserved the indirect return of values via in-out
> parameters, such as [NSBezierPath getLineDash:count:phase:])

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:21:
// Returns the new data.
On 2017/01/27 12:44:55, marq wrote:
> On 2017/01/26 10:10:20, lpromero (ooo til Feb9) wrote:
> > When dataSink receives the dataAvailable message, does getData/currentData
> > already contains the new data? I think it should.
> > 
> > Do you need the diff?
> > 
> > If yes, then keep this but s/getMoreData/availableData.
> > If no, remove this second method and just requery getData/currentData.
> 
> Definitey consider this approach.
> 
> Also I would prefer if the method name was more specific about what suggestion
> it was fetching; 'moreData' feels a bit like NSSet's 'anyObject', where you
have
> no assurances about what will be returned. So maybe one of:
> 
> - recentSuggestions
> - updatedSuggestions
> - latestSuggestions
> 
> (please avoid 'newSuggestions', since that has implications for memory
> semantics).

I will remove this method for now as I am not sure of how the data sink asks for
more data.
The question being: who keeps a cache with the downloaded data. If it is the
data source this methods does not make sense.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_data_source.h:22:
- (NSArray<id<ContentSuggestionsData>>*)getMoreData;
On 2017/01/27 12:44:55, marq wrote:
> Document what is returned when there is no further data; I would suggest an
> empty array rather than nil.

Done.

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h
(right):

https://codereview.chromium.org/2638413006/diff/40001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.h:24:
NS_DESIGNATED_INITIALIZER;
On 2017/01/27 12:44:56, marq wrote:
> nit: newline.

Done.

[marq (ping after 24h), 2017-02-01 09:55:40]

https://codereview.chromium.org/2638413006/diff/80001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2638413006/diff/80001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:73: -
(void)contentSuggestionsService:
Some sort of formatting chaos has happened.

[gambard, 2017-02-01 13:38:07]

Thanks, PTAL.

https://codereview.chromium.org/2638413006/diff/80001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm
(right):

https://codereview.chromium.org/2638413006/diff/80001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_mediator.mm:73: -
(void)contentSuggestionsService:
On 2017/02/01 09:55:40, marq wrote:
> Some sort of formatting chaos has happened.

No idea what happened. git cl format gave me the same formatting until I
hand-formatted it...

[marq (ping after 24h), 2017-02-01 13:45:33]

lgtm

[gambard, 2017-02-01 13:45:55]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-02-01 13:46:04]

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-02-01 14:53:18]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485956755133160,
"parent_rev": "3c7603c430a4d41e9ffb7d0cc733e90a00020150", "commit_rev":
"0ac7f3e7f45b25cf8b3283a7df27bf68b3a72157"}

[commit-bot: I haz the power, 2017-02-01 14:53:52]

Description was changed from

==========
Add ContentSuggestionsMediator

This CL adds a Mediator level in ContentSuggestions.
The mediator classes are used to create a interface between the data source in
C++ and the UI in Objective-C.
This CL only creates the classes to see the interactions from an architectural
point.
Most of the methods are no-op for now.

BUG=679369
==========

to

==========
Add ContentSuggestionsMediator

This CL adds a Mediator level in ContentSuggestions.
The mediator classes are used to create a interface between the data source in
C++ and the UI in Objective-C.
This CL only creates the classes to see the interactions from an architectural
point.
Most of the methods are no-op for now.

BUG=679369

Review-Url: https://codereview.chromium.org/2638413006
Cr-Commit-Position: refs/heads/master@{#447511}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ac7f3e7f45b25cf8b3283a7df27...
==========

[commit-bot: I haz the power, 2017-02-01 14:53:53]

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