[iOS Clean] Created OverlayService.

[iOS Clean] Created OverlayService.

This CL introduces OverlayService, a BrowerStateKeyedService that manages
enqueueing overlay coordinators and scheduling the overlays to be displayed.

Classes of note:

- OverlayService: Interface to the overlay system.
- OverlayServiceFactory: Factory interface that generates OverlayServices.
- OverlayQueue: FIFO queue of overlays that have been added to the service.
- BrowserOverlayQueue: OverlayQueue for overlays that need to be displayed
  over a particular Browser.
- WebStateOverlayQueue: OverlayQueue for overlays that need to be displayed
  over a particular WebState's content area.
- OverlayScheduler: Manages creating OverlayQueues and scheduling when
  their queued overlays can be displayed.  Also switches foreground tabs to
  display a WebState's content for WebStateOverlayQueues.

BUG=none

[kkhorimoto, 2017-06-02 02:15:18]

kkhorimoto@chromium.org changed reviewers:
+ edchin@chromium.org, marq@chromium.org

[kkhorimoto, 2017-06-02 02:15:19]

This is the implementation of the overlay queue and scheduler described in
go/chrome-ios-new-arch-dialogs

[kkhorimoto, 2017-06-02 02:23:06]

self review

[marq (ping after 24h), 2017-06-02 16:14:11]

Here are some initial comments, but I need to think through the design some
more. Monday is a holiday in France; I'll try to have more to say on this on
Tuesday.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/commands/web_overlay_commands.h (right):

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/web_overlay_commands.h:14: @class
WebOverlayCoordinator;
Not needed -- this should be a red flag for a command protocol, which should
only depend on (at most) model classes.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h
(right):

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h:16:
// An overlay coordinator requested by a WKWebView.  These coordinators own the
Nit: WKWebView is an implementation detail inside the web layer. The ui layer
generally deals with WebStates.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h:30:
- (void)startOverlayingWebCoordinator:(BrowserCoordinator*)webCoordinator;
Wouldn't this just be this coordinator's parent?

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h:35:
- (void)cancelWebKitCompletion;
This very much feels like it shouldn't be part of the public API. Coordinators
should avoid exposing implementation details as much as possible.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.mm
(right):

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.mm:29:
if ((self = [super init]))
Prefer to have {} around the conditional block in an initializer, where it is
very likely that more code will be added and there's a good chance of
introducing a wacky bug.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.mm:52:
// Notify the scheduler that the overlay has stopped.
Move this comment after [super stop].

[kkhorimoto, 2017-06-03 00:37:33]

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/commands/web_overlay_commands.h (right):

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/commands/web_overlay_commands.h:14: @class
WebOverlayCoordinator;
On 2017/06/02 16:14:11, marq (ping after 24h) wrote:
> Not needed -- this should be a red flag for a command protocol, which should
> only depend on (at most) model classes.

Removed.  This was leftover from a previous command that I didn't end up using.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
File ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h
(right):

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h:16:
// An overlay coordinator requested by a WKWebView.  These coordinators own the
On 2017/06/02 16:14:11, marq (ping after 24h) wrote:
> Nit: WKWebView is an implementation detail inside the web layer. The ui layer
> generally deals with WebStates.

I've generalized this and other WebOverlay classes to simply reference Overlays.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h:30:
- (void)startOverlayingWebCoordinator:(BrowserCoordinator*)webCoordinator;
On 2017/06/02 16:14:11, marq (ping after 24h) wrote:
> Wouldn't this just be this coordinator's parent?

When this is called, there is no parent yet.  All the overlay functionality is
private to BrowserCoordinator+Internal, so this is needed as an entry point to
properly set up the overlay state.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.h:35:
- (void)cancelWebKitCompletion;
On 2017/06/02 16:14:11, marq (ping after 24h) wrote:
> This very much feels like it shouldn't be part of the public API. Coordinators
> should avoid exposing implementation details as much as possible.

I've generalized this so that it no longer refers to WebKit completion blocks.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
File
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.mm
(right):

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.mm:29:
if ((self = [super init]))
On 2017/06/02 16:14:11, marq (ping after 24h) wrote:
> Prefer to have {} around the conditional block in an initializer, where it is
> very likely that more code will be added and there's a good chance of
> introducing a wacky bug.

Acknowledged.

https://codereview.chromium.org/2921833002/diff/20001/ios/clean/chrome/browse...
ios/clean/chrome/browser/ui/web_contents/overlays/web_overlay_coordinator.mm:52:
// Notify the scheduler that the overlay has stopped.
On 2017/06/02 16:14:11, marq (ping after 24h) wrote:
> Move this comment after [super stop].

Acknowledged.

[kkhorimoto, 2017-06-03 00:39:05]

Description was changed from

==========
[iOS Clean] Created WebOverlay queueing system.

This CL introduces the functionality to queue overlays requested by
WebKit and to switch the foreground Tab before displaying these ovelays.
Classes of note:

- WebOverlayCoordinators are coordinators for UI requested by WebKit.
  They own completion blocks that must be executed before deallocation.

- WebOverlayQueue is a per-WebState list of WebOverlayCoordinators.  It
  uses WebOverlaySchedulerCommands to communicate overlay presentation
  events.

- WebOverlayScheduler class responsible for switching the foreground
  and triggering the start of the next overlay in a WebOverlayQueue.

BUG=none
==========

to

==========
[iOS Clean] Created overlay queueing system.

This CL introduces the functionality to queue overlays requested by
WebKit and to switch the foreground Tab before displaying these overlays.
Classes of note:

- OverlayCoordinators are coordinators for UI requested by WebStates.
  They are not guaranteed to be presented, and can use |-cancelOverlay|
  to take care of cleanup tasks.

- OverlayQueue is a per-WebState list of OverlayCoordinators.  It
  uses OverlaySchedulerCommands to communicate overlay presentation
  events.

- OverlayScheduler class responsible for switching the foreground tab
  and triggering the start of the next overlay in a OverlayQueue.

BUG=none
==========

[kkhorimoto, 2017-06-07 08:02:11]

PTAL!

[kkhorimoto, 2017-06-07 18:43:11]

rebase + gn deps fix

[kkhorimoto, 2017-06-08 01:08:23]

Description was changed from

==========
[iOS Clean] Created overlay queueing system.

This CL introduces the functionality to queue overlays requested by
WebKit and to switch the foreground Tab before displaying these overlays.
Classes of note:

- OverlayCoordinators are coordinators for UI requested by WebStates.
  They are not guaranteed to be presented, and can use |-cancelOverlay|
  to take care of cleanup tasks.

- OverlayQueue is a per-WebState list of OverlayCoordinators.  It
  uses OverlaySchedulerCommands to communicate overlay presentation
  events.

- OverlayScheduler class responsible for switching the foreground tab
  and triggering the start of the next overlay in a OverlayQueue.

BUG=none
==========

to

==========
[iOS Clean] Created overlay queueing system.

This CL introduces the functionality to queue overlays requested by
WebKit and to switch the foreground Tab before displaying these overlays.

Classes of note:

- OverlayCoordinators are coordinators for UI requested by WebStates.
  They are not guaranteed to be presented, and can use |-cancelOverlay|
  to take care of cleanup tasks.

- OverlayQueue is a per-WebState list of OverlayCoordinators.  It
  uses OverlaySchedulerCommands to communicate overlay presentation
  events.

- OverlayScheduler is responsible for switching the foreground tab
  and triggering the start of the next overlay in a OverlayQueue.

BUG=none
==========

[kkhorimoto, 2017-06-09 21:02:15]

Cancel overlays on queue deallocation

[marq (ping after 24h), 2017-06-12 17:36:32]

Kurt, I'm way overdue on the the review for this and the subsequent CLs. I'll
give them some serious attention tomorrow.

[marq (ping after 24h), 2017-06-14 10:02:33]

Overall this is moving in the right direction; nice work. 

Moving url_protection_space should probably have just been a separate CL; feel
free to extract it, since this CL may still evolve.

The two biggest changes I suggest are (a) just ditching the overlay API in
BrowserCoordinator and making that externally managed by these classes, and (b)
having the scheduler observe the overlay queues via an observer mechanism rather
than using commands.

(removing the BC overlay API should probably also be in a separate CL that this
one depends on).

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/overlay_commands.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/overlay_commands.h:21: -
(void)visibleOverlayWasReplacedForWebState:(web::WebState*)webState;
This feels more like something I'd expect in an observer or delegate. I'm not
sure if I want to use commands for that kind of notifications or not.

Should the schedule observe the overlay queues of each webstate instead?  

So the sequence would be:

- as soon as an overlay is triggered, -scheduleOverlay is called, and the
scheduler catches it.
- The scheduler starts observing that webstate's overlay queue
- once an overlay queue is emptied (either from the last overlay being
displayed, or form -cancelOverlays), the scheduler stops observing the queue.

WDYT? What am I missing?

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/tab_grid_commands.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/tab_grid_commands.h:19: -
(void)showTabGridTabForActiveWebState;
If the caller knows the index of the active web state, does having this command
add anything (since they could use showTabGridTabAtIndex:)?

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_coordinator.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_coordinator.h:37: -
(void)cancelOverlay;
How is this different from -stop?

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_coordinator.mm:34: [coordinator
addOverlayCoordinator:self];
I think we don't need both the overlay mechanism inside BrowserCoordinator *and*
the queuing/scheduling stuff you have here. I like your approach better, and
removing the overlay support from BC means we can treat overlays as children,
regularizing this interface.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_coordinator.mm:39: // Implemented
by subclasses.
The header needs to express that this class is intended to be subclassed.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:52: // removed when they
are stopped or when they
This comment now has me on tenterhooks ... ;-)

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:54: // Whether the first
overlay in |overlays_| is started.
If |overlays_| is empty, what value does this have?

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_scheduler.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.h:15: // An object that
manages displaying a WebState's content view before starting
Document clearly how this class may trigger webstates to be activated.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.h:17: @interface
OverlayScheduler : NSObject<OverlaySchedulerCommands>
Unit tests for this class?

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:23:
std::list<web::WebState*> _webStates;
Several places below make assertions about what's at the head of this list; the
semantics of that should be spelled out here in comments.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:72:
DCHECK_EQ(_webStates.front(), webState);
This makes me slightly nervous, because it makes the correctness of a command
sent by a part of the code that has no information or guarantees about how the
comment is implemented coupled to an implementation detail of this class. 

In general, the point of the dispatcher is to strongly decouple command
implementations from the callers of those commands. Commands should generally be
interpreted as requests from other parts of the application, and the caller
should assume little about how, by who,, or if the command is actually
fulfilled.

(this is more of a reason to make this information communicated by observation,
but it still shouldn't depend on the semantics of _webStates).

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:82:
self.browser->dispatcher());
FYI: Dispatcher is moving out of browser. See
https://codereview.chromium.org/2935653002/

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:153: OverlayQueue*
queue = OverlayQueue::FromWebState(self.webState);
This might belong in a mediator. 

(The general idea is that anything that's not hooking up objects and starting
them is outside the scope of a coordinator -- specifically we don't want
coordinator bloat to happen.)

[kkhorimoto, 2017-06-15 08:26:29]

I've updated the approach here so that A) OverlayScheduler uses
OverlayQueueObservation to update the schedule, and B) OverlayQueue is an
abstract base class that can be used for both WebState-specific overlays and
browser-level overlays.

I'll also move the BrowserCoordinator+Internal code for overlays into
OverlayCoordinator, but this will occur in a separate CL.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/overlay_commands.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/overlay_commands.h:21: -
(void)visibleOverlayWasReplacedForWebState:(web::WebState*)webState;
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> This feels more like something I'd expect in an observer or delegate. I'm not
> sure if I want to use commands for that kind of notifications or not.
> 
> Should the schedule observe the overlay queues of each webstate instead?  
> 
> So the sequence would be:
> 
> - as soon as an overlay is triggered, -scheduleOverlay is called, and the
> scheduler catches it.
> - The scheduler starts observing that webstate's overlay queue
> - once an overlay queue is emptied (either from the last overlay being
> displayed, or form -cancelOverlays), the scheduler stops observing the queue.
> 
> WDYT? What am I missing?

I've converted this protocol to OverlayQueueObserver.  It works differently than
how you describe though: it's a WebStateListObserver and begins/stops observing
when WebStates are added/removed from the list.  That ways it can rely
completely on observer callbacks rather then requiring OverlayQueue to call
|-scheduleOverlay|.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/commands/tab_grid_commands.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/commands/tab_grid_commands.h:19: -
(void)showTabGridTabForActiveWebState;
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> If the caller knows the index of the active web state, does having this
command
> add anything (since they could use showTabGridTabAtIndex:)?

Yep, this is not necessary anymore; removed.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_coordinator.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_coordinator.h:37: -
(void)cancelOverlay;
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> How is this different from -stop?

It's possible for these coordinators to be cancelled without their UI ever being
started (e.g. if a background Tab enqueues a dialog, but is closed before the
dialog can  be displayed).

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_coordinator.mm:34: [coordinator
addOverlayCoordinator:self];
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> I think we don't need both the overlay mechanism inside BrowserCoordinator
*and*
> the queuing/scheduling stuff you have here. I like your approach better, and
> removing the overlay support from BC means we can treat overlays as children,
> regularizing this interface.

Acknowledged.  This patch still uses the existing overlay system, but it will be
rebased onto another CL that moves the BrowserCoordinator+Internal overlay
support code to OverlayCoordinator.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_coordinator.mm:39: // Implemented
by subclasses.
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> The header needs to express that this class is intended to be subclassed.

Done.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:52: // removed when they
are stopped or when they
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> This comment now has me on tenterhooks ... ;-)

Hah just trying to keep it interesting :)

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:54: // Whether the first
overlay in |overlays_| is started.
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> If |overlays_| is empty, what value does this have?

It returns false; comments have been updated.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_scheduler.h (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.h:15: // An object that
manages displaying a WebState's content view before starting
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> Document clearly how this class may trigger webstates to be activated.

Done.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.h:17: @interface
OverlayScheduler : NSObject<OverlaySchedulerCommands>
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> Unit tests for this class?

Ack.  Will be in next patch.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:23:
std::list<web::WebState*> _webStates;
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> Several places below make assertions about what's at the head of this list;
the
> semantics of that should be spelled out here in comments.

Done.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:72:
DCHECK_EQ(_webStates.front(), webState);
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> This makes me slightly nervous, because it makes the correctness of a command
> sent by a part of the code that has no information or guarantees about how the
> comment is implemented coupled to an implementation detail of this class. 
> 
> In general, the point of the dispatcher is to strongly decouple command
> implementations from the callers of those commands. Commands should generally
be
> interpreted as requests from other parts of the application, and the caller
> should assume little about how, by who,, or if the command is actually
> fulfilled.
> 
> (this is more of a reason to make this information communicated by
observation,
> but it still shouldn't depend on the semantics of _webStates).

It's been converted to an observer pattern now.  The new semantics make more
sense as to why these assumptions have been made.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/tab_grid/tab_grid_coordinator.mm:82:
self.browser->dispatcher());
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> FYI: Dispatcher is moving out of browser. See
> https://codereview.chromium.org/2935653002/

Acknowledged.  I'll rebase when your CL is landed.

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/100001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:153: OverlayQueue*
queue = OverlayQueue::FromWebState(self.webState);
On 2017/06/14 10:02:32, marq (ping after 24h) wrote:
> This might belong in a mediator. 
> 
> (The general idea is that anything that's not hooking up objects and starting
> them is outside the scope of a coordinator -- specifically we don't want
> coordinator bloat to happen.)

This is no longer necessary.

[marq (ping after 24h), 2017-06-21 09:41:54]

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.h
(right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.h:31:
// Called when the overay is stopped so that the OverlayService can be notified.
Spelling: overlay.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.mm
(right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.mm:29:
objc_setAssociatedObject(self, kOverlayQueuePropertyKey, ptrValue,
I don't think encapsulation is worth the complexity of associated objects. If
coordinators need this as a property, they should have it.

You can define the property in the +overlay_support header along with @dynamic
instead of @synthesize, and then define the ivar in the main.mm along with
@synthesize.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue.h (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:66:
NSMutableArray<BrowserCoordinator*>* overlays_;
Please explicitly mark ObjC member variables '__strong' (or '__weak') in C++
headers.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:68: // BrowserCoordinator
in |overlays_| is started.
s/is started/has been started/ to clarify what the causal relationship is.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.mm:64: for
(BrowserCoordinator* overlay_coordinator in overlays_) {
Braceless loops are okay, but be consistent with them in the file. I'd also
prefer to have a newline after the indented line in braceless loops for clarity.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue_observer.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue_observer.mm:5: #import
"ios/clean/chrome/browser/ui/overlays/overlay_queue_observer.h"
I think you can just put the defaults and empty blocks into the header; there's
no need for an .mm file (compare to web_state_observer.h).

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue_unittest.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue_unittest.mm:110: //
Missing comment?

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue_unittest.mm:114:
queue().AddOverlay(overlay);
Missing test expectations?

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:140: WebStateList&
web_state_list = browser_->web_state_list();
Rather than passing the whole browser into the scheduler, I'd prefer to
individually assign the WSL and the dispatcher (which will move out of the
browser soon).

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:143:
web_state_list.ActivateWebStateAt(new_active_index);
We should arrive at some consensus about how activating webstates (in the model
layer) interacts with displaying tabs. Some options:

- Activating a tab in the WSL is expected to be observed by (for example) the
tab grid mediator, which will take care of changing the active tab.

- Activating a tab in the WSL is *never* expected to directly trigger
coordinator-layer changes, and must be followed with something like a
-showActiveTab command.

- Showing a tab at a particular index via a command is expected to change the
active tab in the WSL.

We should also consider synchrony issues in this, since changing the active tab
may require animation.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:178:
WebStateOverlayQueue::FromWebState(web_state)->CancelOverlays();
If WSOQ::FromWebState(foo) returns nullptr, this will explode. Should be defend
against this, or is there clearly no code path where this can occur?

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_service.h (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_service.h:18: // OverlayService
allows for the easy presentation and dismissal of overlay
The division of responsibilities between the OverlayService and the
OverlayScheduler isn't clear to me.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/web_state_overlay_queue.h (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/web_state_overlay_queue.h:34:
BrowserCoordinator* parent_coordinator_;
_weak, I'm guessing; default would be strong.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:68: // Notify the
OverlayService that this coordinator is no long displaying
no longer

[kkhorimoto, 2017-06-23 06:10:35]

rebase & Mark's comments

[kkhorimoto, 2017-06-23 06:11:17]

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.h
(right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.h:23:
@property(nonatomic, readonly) BOOL supportsOverlaying;
Not sure if this is the best solution.  Originally, this was handled in
OverlayCoordinator's |-stop|, but that requires having all UI presented as an
overlay to subclass that class.  Moving the functionality to a category allows
more flexibility, but requires trusting users of the service to follow the
guidelines in the comment (i.e. if |supportsOverlaying| returns YES, but
|-overlayWasStopped| is not called, the overlay scheduler will never show the
next overlay).

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.h:31:
// Called when the overay is stopped so that the OverlayService can be notified.
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> Spelling: overlay.

Done.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.mm
(right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/browser_coordinator+overlay_support.mm:29:
objc_setAssociatedObject(self, kOverlayQueuePropertyKey, ptrValue,
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> I don't think encapsulation is worth the complexity of associated objects. If
> coordinators need this as a property, they should have it.
> 
> You can define the property in the +overlay_support header along with @dynamic
> instead of @synthesize, and then define the ivar in the main.mm along with
> @synthesize.

Done.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue.h (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:66:
NSMutableArray<BrowserCoordinator*>* overlays_;
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> Please explicitly mark ObjC member variables '__strong' (or '__weak') in C++
> headers.

Done.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.h:68: // BrowserCoordinator
in |overlays_| is started.
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> s/is started/has been started/ to clarify what the causal relationship is.

Done.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue.mm:64: for
(BrowserCoordinator* overlay_coordinator in overlays_) {
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> Braceless loops are okay, but be consistent with them in the file. I'd also
> prefer to have a newline after the indented line in braceless loops for
clarity.

If we need to add a newline after the loop, going braceless doesn't really add
anything since it takes up the same number of lines.  I'll just use braces.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue_observer.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue_observer.mm:5: #import
"ios/clean/chrome/browser/ui/overlays/overlay_queue_observer.h"
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> I think you can just put the defaults and empty blocks into the header;
there's
> no need for an .mm file (compare to web_state_observer.h).

Done.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_queue_unittest.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue_unittest.mm:110: //
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> Missing comment?

Whoops, yes. This test is fixed now!

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_queue_unittest.mm:114:
queue().AddOverlay(overlay);
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> Missing test expectations?

Added!

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:140: WebStateList&
web_state_list = browser_->web_state_list();
On 2017/06/21 09:41:54, marq (ping after 24h) wrote:
> Rather than passing the whole browser into the scheduler, I'd prefer to
> individually assign the WSL and the dispatcher (which will move out of the
> browser soon).

This object is a BrowserUserData, so I don't think there's a lot of harm in
referencing the Browser object it's attached to.  That being said, I can update
this to only store the WSL & dispatcher if you feel strongly about it.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:143:
web_state_list.ActivateWebStateAt(new_active_index);
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> We should arrive at some consensus about how activating webstates (in the
model
> layer) interacts with displaying tabs. Some options:
> 
> - Activating a tab in the WSL is expected to be observed by (for example) the
> tab grid mediator, which will take care of changing the active tab.
> 
> - Activating a tab in the WSL is *never* expected to directly trigger
> coordinator-layer changes, and must be followed with something like a
> -showActiveTab command.
> 
> - Showing a tab at a particular index via a command is expected to change the
> active tab in the WSL.
> 
> We should also consider synchrony issues in this, since changing the active
tab
> may require animation.

As I mentioned in the meeting this morning, I think that updating the model
layer first is the way to go.  I'm split on whether the UI should update through
observers or if it should be triggered manually via commands.  I'm tempted to
say that it should happen automatically through observers, as that would allow
for the loosest coupling between this model-layer BrowserUserData code and the
UI consumer layer.  That being said, it might be useful to manually dispatch the
commands, for example if we wanted to change multiple model objects at once and
then dispatch a command that performs an animation based on the batch changes. 
I think adding a |-broadcastVisibleWebState:| should be enough for
synchronization, regardless of whether it occurs automatically or via commands.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:178:
WebStateOverlayQueue::FromWebState(web_state)->CancelOverlays();
On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> If WSOQ::FromWebState(foo) returns nullptr, this will explode. Should be
defend
> against this, or is there clearly no code path where this can occur?

There is no codepath where that can occur.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_service.h (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_service.h:18: // OverlayService
allows for the easy presentation and dismissal of overlay
On 2017/06/21 09:41:54, marq (ping after 24h) wrote:
> The division of responsibilities between the OverlayService and the
> OverlayScheduler isn't clear to me. 

The main entry point to this overlay system is adding overlays to an
OverlayQueue.  I thought that it wasn't as clear, so defining a single service
interface helps expose a more understandable API for overlaying.  Also, since
this exists at a ChromeBrowserState-level, it can handle adding/removing
observers for browsers, and is easily accessible from any BrowserCoordinator. 
I've moved the implementation files into an internal/ directory to make it
clearer that clients of this system should include the service-level target.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/web_state_overlay_queue.h (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/web_state_overlay_queue.h:34:
BrowserCoordinator* parent_coordinator_;
On 2017/06/21 09:41:54, marq (ping after 24h) wrote:
> _weak, I'm guessing; default would be strong.

Done.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/web_contents/web_coordinator.mm:68: // Notify the
OverlayService that this coordinator is no long displaying
On 2017/06/21 09:41:54, marq (ping after 24h) wrote:
> no longer

Done.

[kkhorimoto, 2017-06-23 06:16:22]

Description was changed from

==========
[iOS Clean] Created overlay queueing system.

This CL introduces the functionality to queue overlays requested by
WebKit and to switch the foreground Tab before displaying these overlays.

Classes of note:

- OverlayCoordinators are coordinators for UI requested by WebStates.
  They are not guaranteed to be presented, and can use |-cancelOverlay|
  to take care of cleanup tasks.

- OverlayQueue is a per-WebState list of OverlayCoordinators.  It
  uses OverlaySchedulerCommands to communicate overlay presentation
  events.

- OverlayScheduler is responsible for switching the foreground tab
  and triggering the start of the next overlay in a OverlayQueue.

BUG=none
==========

to

==========
[iOS Clean] Created OverlayService.

This CL introduces OverlayService, a BrowerStateKeyedService that manages
enqueueing overlay coordinators and scheduling the overlays to be displayed.

Classes of note:

- OverlayService: Interface to the overlay system.
- OverlayServiceFactory: Factory interface that generates OverlayServices.
- OverlayQueue: FIFO queue of overlays that have been added to the service.
- BrowserOverlayQueue: OverlayQueue for overlays that need to be displayed
  over a particular Browser.
- WebStateOverlayQueue: OverlayQueue for overlays that need to be displayed
  over a particular WebState's content area.
- OverlayScheduler: Manages creating OverlayQueues and scheduling when
  their queued overlays can be displayed.  Also switches foreground tabs to
  display a WebState's content for WebStateOverlayQueues.

BUG=none
==========

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

I think we're getting closer.

Generally I think it's better to start with a smaller system that might have
some awkward interactions, and then refine those in future CLs; the overlay
service seems like a preemptive optimization.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:140: WebStateList&
web_state_list = browser_->web_state_list();
On 2017/06/23 06:11:16, kkhorimoto_ wrote:
> On 2017/06/21 09:41:54, marq (ping after 24h) wrote:
> > Rather than passing the whole browser into the scheduler, I'd prefer to
> > individually assign the WSL and the dispatcher (which will move out of the
> > browser soon).
> 
> This object is a BrowserUserData, so I don't think there's a lot of harm in
> referencing the Browser object it's attached to.  That being said, I can
update
> this to only store the WSL & dispatcher if you feel strongly about it. 

Dispatcher is moving out of browser once I have time to land that change, and
while it makes sense for this class to be BrowserData (that is, owned by a
Browser), I don't think that necessarily means that it internally should depend
on the browser. I'd also like to generally keep dependencies as specific as
possible, and restrict how much "service container" classes (like BrowserState
and Browser) are passed around, instead of passing around the specific services
from them.

As a further litmus test, I wouldn't want this class to pass a Browser instance
to anything else.

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_scheduler.mm:143:
web_state_list.ActivateWebStateAt(new_active_index);
On 2017/06/23 06:11:16, kkhorimoto_ wrote:
> On 2017/06/21 09:41:53, marq (ping after 24h) wrote:
> > We should arrive at some consensus about how activating webstates (in the
> model
> > layer) interacts with displaying tabs. Some options:
> > 
> > - Activating a tab in the WSL is expected to be observed by (for example)
the
> > tab grid mediator, which will take care of changing the active tab.
> > 
> > - Activating a tab in the WSL is *never* expected to directly trigger
> > coordinator-layer changes, and must be followed with something like a
> > -showActiveTab command.
> > 
> > - Showing a tab at a particular index via a command is expected to change
the
> > active tab in the WSL.
> > 
> > We should also consider synchrony issues in this, since changing the active
> tab
> > may require animation.
> 
> As I mentioned in the meeting this morning, I think that updating the model
> layer first is the way to go.  I'm split on whether the UI should update
through
> observers or if it should be triggered manually via commands.  I'm tempted to
> say that it should happen automatically through observers, as that would allow
> for the loosest coupling between this model-layer BrowserUserData code and the
> UI consumer layer.  That being said, it might be useful to manually dispatch
the
> commands, for example if we wanted to change multiple model objects at once
and
> then dispatch a command that performs an animation based on the batch changes.

> I think adding a |-broadcastVisibleWebState:| should be enough for
> synchronization, regardless of whether it occurs automatically or via
commands.

OK. I think I would like to start with pure model-driven propagation and then
see where the pain points are. So start with having this method just
ActivateWebStateAt().

(we might consider adding a WebStateList::ActivateWebState() convenience method
that wraps the GetIndexOf/ActivateWebStateAt sequence).

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlays/overlay_service.h (right):

https://codereview.chromium.org/2921833002/diff/140001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlays/overlay_service.h:18: // OverlayService
allows for the easy presentation and dismissal of overlay
On 2017/06/23 06:11:16, kkhorimoto_ wrote:
> On 2017/06/21 09:41:54, marq (ping after 24h) wrote:
> > The division of responsibilities between the OverlayService and the
> > OverlayScheduler isn't clear to me. 
> 
> The main entry point to this overlay system is adding overlays to an
> OverlayQueue.  I thought that it wasn't as clear, so defining a single service
> interface helps expose a more understandable API for overlaying.  Also, since
> this exists at a ChromeBrowserState-level, it can handle adding/removing
> observers for browsers, and is easily accessible from any BrowserCoordinator. 
> I've moved the implementation files into an internal/ directory to make it
> clearer that clients of this system should include the service-level target.

I don't agree that adding this service makes the necessary interactions clearer.
For example, I think it's fine for the web container coordinator to grab the
webstate overlay queue directly.

Also, remember that we don't want to assume a 1:1 correspondence between
BrowserState and browsers -- in the future we may have multiple browsers for the
same BrowserState.

I also don't like that we need to add six files (factory, service, service_impl,
.h and .mm) to do this.

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlay_service/BUILD.gn (right):

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlay_service/BUILD.gn:5:
source_set("overlay_service") {
Nit: I preferred just calling this directory 'overlays', since we already have
'commands', 'animators', etc.

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
File
ios/clean/chrome/browser/ui/overlay_service/browser_coordinator+overlay_support.h
(right):

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlay_service/browser_coordinator+overlay_support.h:25:
// - call |-overlayWasStopped| from its |-stop| so that the OverlayService can
It seems like these things could be guaranteed in either BrowserCoordinator
itself or in an OverlayCoordinator subclass. There's been enough changes in this
design that I've lost track of what the arguments against a subclass were.

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlay_service/browser_coordinator+overlay_support.h:40:
- (void)overlayWasStopped;
yeah, I'd rather have an OverlayCoordinator class whose -stop handles notifying
the service and (separately) unparenting.

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlay_service/internal/BUILD.gn (right):

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlay_service/internal/BUILD.gn:5:
source_set("internal") {
I'm not sure that the internal/ subdirectory is useful here, unless we start
enforcing dependencies. But overall I don't think we're at the point where we're
defining hard API boundaries like this yet.

(And wouldn't overlay_queue.h also need to be a public header?)

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
File ios/clean/chrome/browser/ui/overlay_service/internal/overlay_scheduler.h
(right):

https://codereview.chromium.org/2921833002/diff/160001/ios/clean/chrome/brows...
ios/clean/chrome/browser/ui/overlay_service/internal/overlay_scheduler.h:26:
class OverlayScheduler : public BrowserUserData<OverlayScheduler>,
Unit tests for this class?