Add the suggestions UI in chrome

Add the suggestions UI in chrome

This CL adds an entry to the toolbar menu to enter the suggestions UI.
The position in the toolbar menu is not ideal but it is useful to test the UI
in the context of Chrome. It will be changed.
This entry is accessible using an experimental setting.

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

[gambard, 2017-01-18 13:07:49]

Description was changed from

==========
Add the suggestions UI in chrome

This CL adds an entry to the toolbar menu to enter the suggestions UI.
This entry is accessible using an experimental setting.

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

to

==========
Add the suggestions UI in chrome

This CL adds an entry to the toolbar menu to enter the suggestions UI.
The position in the toolbar menu is not ideal but it is useful to test the UI
in the context of Chrome. It will be changed.
This entry is accessible using an experimental setting.

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

[gambard, 2017-01-18 13:12:26]

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

[gambard, 2017-01-18 13:12:27]

PTAL.

[lpromero, 2017-01-18 13:32:15]

lgtm

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:1529: Suggestions UI
Maybe just "Suggestions"? In case the term stays.

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/BUILD.gn (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/BUILD.gn:24: "//ios/chrome/browser",
Out of curiosity: why this one?

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:45: completion:nil];
Optional: _navigationController.reset();

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4191: -
(void)clearPresentedStateWithCompletion:(ProceduralBlock)completion {
You probably need to add a call to stop there (or simply reset the coordinator).

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4377: - (void)showSuggestionsUI
{
This could be called twice in a row (for example when the UI is lagging and the
user taps twice, thinking the first one didn't get called). In that case, we
seem to call -start twice, no?

[gambard, 2017-01-18 14:34:47]

Thanks, noyau@: PTAL.

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:1529: Suggestions UI
On 2017/01/18 13:32:15, lpromero wrote:
> Maybe just "Suggestions"? In case the term stays.

Done.

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/BUILD.gn (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/BUILD.gn:24: "//ios/chrome/browser",
On 2017/01/18 13:32:15, lpromero wrote:
> Out of curiosity: why this one?

for ChromeCoordinator

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:45: completion:nil];
On 2017/01/18 13:32:15, lpromero wrote:
> Optional: _navigationController.reset();

Done.

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4191: -
(void)clearPresentedStateWithCompletion:(ProceduralBlock)completion {
On 2017/01/18 13:32:15, lpromero wrote:
> You probably need to add a call to stop there (or simply reset the
coordinator).

Done.

https://codereview.chromium.org/2643753002/diff/20001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:4377: - (void)showSuggestionsUI
{
On 2017/01/18 13:32:15, lpromero wrote:
> This could be called twice in a row (for example when the UI is lagging and
the
> user taps twice, thinking the first one didn't get called). In that case, we
> seem to call -start twice, no?

Fixed in the coordinator.

[lpromero, 2017-01-18 14:37:07]

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:23: // Prevent this
coordinator to be started twice in a row.
I am not sure if we want to philosophically do it that way. Because that would
mean doing it in all coordinators, or having two types of coordinators. (That
could be a setting in the context though.)

Mark, do you have feedback on that?

[noyau (Ping after 24h), 2017-01-18 15:08:35]

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/reso...
File ios/chrome/browser/resources/Settings.bundle/Experimental.plist (right):

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/reso...
ios/chrome/browser/resources/Settings.bundle/Experimental.plist:341:
<string>PSToggleSwitchSpecifier</string>
Never do a toggle is what I learned over time. Always do enable/disable/default.
This is more flexible and you'll thank me in a few months for it...

[gambard, 2017-01-18 16:54:05]

Thanks, PTAL.

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/reso...
File ios/chrome/browser/resources/Settings.bundle/Experimental.plist (right):

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/reso...
ios/chrome/browser/resources/Settings.bundle/Experimental.plist:341:
<string>PSToggleSwitchSpecifier</string>
On 2017/01/18 15:08:35, noyau wrote:
> Never do a toggle is what I learned over time. Always do
enable/disable/default.
> This is more flexible and you'll thank me in a few months for it...

Done.

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/40001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:23: // Prevent this
coordinator to be started twice in a row.
On 2017/01/18 14:37:07, lpromero wrote:
> I am not sure if we want to philosophically do it that way. Because that would
> mean doing it in all coordinators, or having two types of coordinators. (That
> could be a setting in the context though.)
> 
> Mark, do you have feedback on that?

I don't know if we want to do it that way but we are already doing it (in
AlertCoordinator for example).

[noyau (Ping after 24h), 2017-01-18 16:57:55]

noyau@chromium.org changed reviewers:
+ marq@chromium.org

[noyau (Ping after 24h), 2017-01-18 16:57:56]

lgtm, but adding marq as reviewer to answer the question.

[marq (ping after 24h), 2017-01-19 16:32:32]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:1528: <message
name="IDS_IOS_TOOLS_MENU_SUGGESTIONS_UI" desc="The iOS menu item for displaying
the Suggestions UI [Length: 15em] [iOS only]">
Remove '_UI' from the string name, to match other similar strings.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
File ios/chrome/browser/experimental_flags.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
ios/chrome/browser/experimental_flags.mm:315: if
(command_line->HasSwitch(switches::kEnableSuggestionsUI)) {
Is the usual pattern when both 'enable' and 'disable' flags are present to
enable?

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
ios/chrome/browser/experimental_flags.mm:317: } else if
(command_line->HasSwitch(switches::kDisableSuggestionsUI)) {
no 'else' after return; just

if ()
  return

if ()
  return

return

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
Ideally this would be the problem of the code that owns the coordinator.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:47: animated:YES
self.context.animated? Not sure if that landed yet.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/commands/ios_command_ids.h (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/commands/ios_command_ids.h:79: #define
IDC_SHOW_SUGGESTIONS_UI                        40955
Just SHOW_SUGGESTIONS; anything we show is UI.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:34: extern
NSString* const kToolsMenuSuggestionsUIId;
Readability nit: in every case here, 'Id' should be 'ID'.

But do remove 'UI' from the name.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:185: if
(!experimental_flags::IsSuggestionsUIEnabled()) {
Do you need experimental flag checks both here and in the BVC? It's more code to
remove when the experiment concludes.

[lpromero, 2017-01-19 17:42:40]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:47: animated:YES
On 2017/01/19 16:32:32, marq wrote:
> self.context.animated? Not sure if that landed yet.

Not yet in ios/chrome. I will make a new pass when I decide to move forward with
it in ios/chrome, so this can stay as is in this CL.

[gambard, 2017-01-19 17:50:12]

Thanks, PTAL.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/app/strings/...
File ios/chrome/app/strings/ios_strings.grd (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/app/strings/...
ios/chrome/app/strings/ios_strings.grd:1528: <message
name="IDS_IOS_TOOLS_MENU_SUGGESTIONS_UI" desc="The iOS menu item for displaying
the Suggestions UI [Length: 15em] [iOS only]">
On 2017/01/19 16:32:31, marq wrote:
> Remove '_UI' from the string name, to match other similar strings.

Done.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
File ios/chrome/browser/experimental_flags.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
ios/chrome/browser/experimental_flags.mm:315: if
(command_line->HasSwitch(switches::kEnableSuggestionsUI)) {
On 2017/01/19 16:32:31, marq wrote:
> Is the usual pattern when both 'enable' and 'disable' flags are present to
> enable? 

It is the one used in the other flags.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
ios/chrome/browser/experimental_flags.mm:317: } else if
(command_line->HasSwitch(switches::kDisableSuggestionsUI)) {
On 2017/01/19 16:32:31, marq wrote:
> no 'else' after return; just
> 
> if ()
>   return
> 
> if ()
>   return
> 
> return

Done.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/19 16:32:31, marq wrote:
> Ideally this would be the problem of the code that owns the coordinator.

I can add a "running" property and check it in the BVC. Is it better?

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/c...
File ios/chrome/browser/ui/commands/ios_command_ids.h (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/commands/ios_command_ids.h:79: #define
IDC_SHOW_SUGGESTIONS_UI                        40955
On 2017/01/19 16:32:32, marq wrote:
> Just SHOW_SUGGESTIONS; anything we show is UI.

Done.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:34: extern
NSString* const kToolsMenuSuggestionsUIId;
On 2017/01/19 16:32:32, marq wrote:
> Readability nit: in every case here, 'Id' should be 'ID'.
> 
> But do remove 'UI' from the name.

Done.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/ui/t...
ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:185: if
(!experimental_flags::IsSuggestionsUIEnabled()) {
On 2017/01/19 16:32:32, marq wrote:
> Do you need experimental flag checks both here and in the BVC? It's more code
to
> remove when the experiment concludes.

I used the same pattern as reading list

[lpromero, 2017-01-19 18:49:13]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/19 17:50:12, gambard wrote:
> On 2017/01/19 16:32:31, marq wrote:
> > Ideally this would be the problem of the code that owns the coordinator.
> 
> I can add a "running" property and check it in the BVC. Is it better?

I think there is a fundamental problem here: the BVC never resets its
coordinator ivar (scoped_nsobject), except when asked to clear all presented UI.
So in the normal case, how do you dismiss the Suggestions UI?

[gambard, 2017-01-20 07:37:44]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/19 18:49:13, lpromero wrote:
> On 2017/01/19 17:50:12, gambard wrote:
> > On 2017/01/19 16:32:31, marq wrote:
> > > Ideally this would be the problem of the code that owns the coordinator.
> > 
> > I can add a "running" property and check it in the BVC. Is it better?
> 
> I think there is a fundamental problem here: the BVC never resets its
> coordinator ivar (scoped_nsobject), except when asked to clear all presented
UI.
> So in the normal case, how do you dismiss the Suggestions UI?

In the normal case, the suggestion UI has a "done" button calling stop. See line
39.

[gambard, 2017-01-20 08:38:32]

I move suggestions to content_suggestions.

[lpromero, 2017-01-20 08:58:02]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/20 07:37:43, gambard wrote:
> On 2017/01/19 18:49:13, lpromero wrote:
> > On 2017/01/19 17:50:12, gambard wrote:
> > > On 2017/01/19 16:32:31, marq wrote:
> > > > Ideally this would be the problem of the code that owns the coordinator.
> > > 
> > > I can add a "running" property and check it in the BVC. Is it better?
> > 
> > I think there is a fundamental problem here: the BVC never resets its
> > coordinator ivar (scoped_nsobject), except when asked to clear all presented
> UI.
> > So in the normal case, how do you dismiss the Suggestions UI?
> 
> In the normal case, the suggestion UI has a "done" button calling stop. See
line
> 39.

Ah missed that one! I think I looked for " stop];" so I didn't see it. Thanks!

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/19 17:50:12, gambard wrote:
> On 2017/01/19 16:32:31, marq wrote:
> > Ideally this would be the problem of the code that owns the coordinator.
> 
> I can add a "running" property and check it in the BVC. Is it better?

At some point in the New Architecture we'll need this coordinator to report to
its parent, which will be the one calling -stop. Is it worth doing now, instead
of a workaround?

https://codereview.chromium.org/2643753002/diff/120001/ios/chrome/browser/con...
File ios/chrome/browser/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2643753002/diff/120001/ios/chrome/browser/con...
ios/chrome/browser/content_suggestions/BUILD.gn:15:
"//ios/chrome/browser/ui/suggestions",
Should this be renamed content_suggestions too? In a separate CL.

[gambard, 2017-01-20 12:22:17]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/20 08:58:02, lpromero wrote:
> On 2017/01/19 17:50:12, gambard wrote:
> > On 2017/01/19 16:32:31, marq wrote:
> > > Ideally this would be the problem of the code that owns the coordinator.
> > 
> > I can add a "running" property and check it in the BVC. Is it better?
> 
> At some point in the New Architecture we'll need this coordinator to report to
> its parent, which will be the one calling -stop. Is it worth doing now,
instead
> of a workaround?

For now the parent is BVC, not sure that adding code to it is a good idea.
I think it is OK for now to keep the guard here.

https://codereview.chromium.org/2643753002/diff/120001/ios/chrome/browser/con...
File ios/chrome/browser/content_suggestions/BUILD.gn (right):

https://codereview.chromium.org/2643753002/diff/120001/ios/chrome/browser/con...
ios/chrome/browser/content_suggestions/BUILD.gn:15:
"//ios/chrome/browser/ui/suggestions",
On 2017/01/20 08:58:02, lpromero wrote:
> Should this be renamed content_suggestions too? In a separate CL.

https://codereview.chromium.org/2644123003/

[marq (ping after 24h), 2017-01-20 12:34:46]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
File ios/chrome/browser/experimental_flags.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/expe...
ios/chrome/browser/experimental_flags.mm:315: if
(command_line->HasSwitch(switches::kEnableSuggestionsUI)) {
On 2017/01/19 17:50:12, gambard wrote:
> On 2017/01/19 16:32:31, marq wrote:
> > Is the usual pattern when both 'enable' and 'disable' flags are present to
> > enable? 
> 
> It is the one used in the other flags.

OK.

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/20 12:22:17, gambard wrote:
> On 2017/01/20 08:58:02, lpromero wrote:
> > On 2017/01/19 17:50:12, gambard wrote:
> > > On 2017/01/19 16:32:31, marq wrote:
> > > > Ideally this would be the problem of the code that owns the coordinator.
> > > 
> > > I can add a "running" property and check it in the BVC. Is it better?
> > 
> > At some point in the New Architecture we'll need this coordinator to report
to
> > its parent, which will be the one calling -stop. Is it worth doing now,
> instead
> > of a workaround?
> 
> For now the parent is BVC, not sure that adding code to it is a good idea.
> I think it is OK for now to keep the guard here.

Again, I think the object creating the coordinator should be able to keep track
of whether it has created and started one yet or not. I don't like the idea of
-start being a no-op sometimes.

[gambard, 2017-01-23 09:45:54]

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):

https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
coordinator to be started twice in a row.
On 2017/01/20 12:34:46, marq wrote:
> On 2017/01/20 12:22:17, gambard wrote:
> > On 2017/01/20 08:58:02, lpromero wrote:
> > > On 2017/01/19 17:50:12, gambard wrote:
> > > > On 2017/01/19 16:32:31, marq wrote:
> > > > > Ideally this would be the problem of the code that owns the
coordinator.
> > > > 
> > > > I can add a "running" property and check it in the BVC. Is it better?
> > > 
> > > At some point in the New Architecture we'll need this coordinator to
report
> to
> > > its parent, which will be the one calling -stop. Is it worth doing now,
> > instead
> > > of a workaround?
> > 
> > For now the parent is BVC, not sure that adding code to it is a good idea.
> > I think it is OK for now to keep the guard here.
> 
> Again, I think the object creating the coordinator should be able to keep
track
> of whether it has created and started one yet or not. I don't like the idea of
> -start being a no-op sometimes. 

As we discussed it, what solution do you suggest?

[marq (ping after 24h), 2017-01-23 15:31:23]

On 2017/01/23 09:45:54, gambard wrote:
>
https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
> File ios/chrome/browser/suggestions/suggestions_coordinator.mm (right):
> 
>
https://codereview.chromium.org/2643753002/diff/80001/ios/chrome/browser/sugg...
> ios/chrome/browser/suggestions/suggestions_coordinator.mm:27: // Prevent this
> coordinator to be started twice in a row.
> On 2017/01/20 12:34:46, marq wrote:
> > On 2017/01/20 12:22:17, gambard wrote:
> > > On 2017/01/20 08:58:02, lpromero wrote:
> > > > On 2017/01/19 17:50:12, gambard wrote:
> > > > > On 2017/01/19 16:32:31, marq wrote:
> > > > > > Ideally this would be the problem of the code that owns the
> coordinator.
> > > > > 
> > > > > I can add a "running" property and check it in the BVC. Is it better?
> > > > 
> > > > At some point in the New Architecture we'll need this coordinator to
> report
> > to
> > > > its parent, which will be the one calling -stop. Is it worth doing now,
> > > instead
> > > > of a workaround?
> > > 
> > > For now the parent is BVC, not sure that adding code to it is a good idea.
> > > I think it is OK for now to keep the guard here.
> > 
> > Again, I think the object creating the coordinator should be able to keep
> track
> > of whether it has created and started one yet or not. I don't like the idea
of
> > -start being a no-op sometimes
> 
> As we discussed it, what solution do you suggest?

For now, we can keep the no-op start (AlertCoordinator, for example, also has
no-op starts), and you can (again, following AlertCoordinator) implement a
-visible property on the coordinator.

[gambard, 2017-01-24 08:27:31]

Thanks, PTAL.

[lpromero, 2017-01-24 08:31:57]

lgtm

[marq (ping after 24h), 2017-01-24 10:09:23]

LGTM after comment fixes.

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:13:
@property(nonatomic, readonly) BOOL visible;
Add a comment. What are the expected conditions under which it is set?

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:29: //
Prevent this coordinator to be started twice in a row.
nit: "to be started" --> "from being started"

[gambard, 2017-01-25 08:09:50]

Thanks!

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:13:
@property(nonatomic, readonly) BOOL visible;
On 2017/01/24 10:09:22, marq wrote:
> Add a comment. What are the expected conditions under which it is set?

Done.

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2643753002/diff/160001/ios/chrome/browser/con...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:29: //
Prevent this coordinator to be started twice in a row.
On 2017/01/24 10:09:22, marq wrote:
> nit: "to be started" --> "from being started"

Done.

[gambard, 2017-01-25 08:12:57]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-01-25 08:12:57]

The patchset sent to the CQ was uploaded after l-g-t-m from noyau@chromium.org,
marq@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2643753002/#ps180001
(title: "Update comments")

[gambard, 2017-01-25 08:12:58]

The CQ bit was unchecked by gambard@chromium.org

[gambard, 2017-01-25 08:12:58]

The CQ bit was checked by gambard@chromium.org

[commit-bot: I haz the power, 2017-01-25 08:13:14]

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-01-25 09:14:24]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485331978963330,
"parent_rev": "d52b38109b89d346334cc0bc582328d6131fe720", "commit_rev":
"d2e44fbf6b351a10a698b1571019fe0a06bf132c"}

[commit-bot: I haz the power, 2017-01-25 09:15:00]

Description was changed from

==========
Add the suggestions UI in chrome

This CL adds an entry to the toolbar menu to enter the suggestions UI.
The position in the toolbar menu is not ideal but it is useful to test the UI
in the context of Chrome. It will be changed.
This entry is accessible using an experimental setting.

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

to

==========
Add the suggestions UI in chrome

This CL adds an entry to the toolbar menu to enter the suggestions UI.
The position in the toolbar menu is not ideal but it is useful to test the UI
in the context of Chrome. It will be changed.
This entry is accessible using an experimental setting.

BUG=679369

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

[commit-bot: I haz the power, 2017-01-25 09:15:01]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/d2e44fbf6b351a10a698b1571019...