[ios showcase]Configures Resource Bundle and adds Toolbar Coordinator

[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed since Resource bundle uses
current_process_commandline_ to calculate the device scale and load the right 
resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
Review-Url: https://codereview.chromium.org/2651833002
Cr-Commit-Position: refs/heads/master@{#446712}
Committed: https://chromium.googlesource.com/chromium/src/+/9d62fd77957ef8e486ad77c57e05b717c5a037d3

[sczs, 2017-01-23 22:22:23]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

Adds ApplicationState support to Showcase. 
Creates a coordinator for Toolbar.
Renames Settings coordinator.

BUG=683793
==========

[sczs, 2017-01-23 22:22:23]

sczs@chromium.org changed reviewers:
+ edchin@chromium.org, lpromero@chromium.org, marq@chromium.org

[sczs, 2017-01-23 22:49:29]

Patchset #1 (id:1) has been deleted

[sczs, 2017-01-23 23:17:04]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

Adds ApplicationState support to Showcase. 
Creates a coordinator for Toolbar.
Renames Settings coordinator.

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

Adds ApplicationState support to Showcase. 
Creates a coordinator for Toolbar.
Renames Showcase Settings coordinator.

BUG=683793
==========

[sczs, 2017-01-23 23:18:06]

PTAL
The coordinator that presents the Toolbar is very simple for now, might make
some changes in a later CL.

[edchin, 2017-01-24 00:18:52]

What aspects of applicationState are required for showcase, and why?

[sczs, 2017-01-24 00:23:00]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

Adds ApplicationState support to Showcase. 
Creates a coordinator for Toolbar.
Renames Showcase Settings coordinator.

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle, which is initialized by ApplicationState.
For this reason this CL configures the Application State with the same steps as 
ios/clean. We can probably get rid of some steps, but I wanted it to behave
similar to 
clean.

Adds ApplicationState support to Showcase. 
Creates a coordinator for Toolbar.
Renames Showcase Settings coordinator.

BUG=683793
==========

[sczs, 2017-01-24 00:29:52]

On 2017/01/24 00:18:52, edchin wrote:
> What aspects of applicationState are required for showcase, and why?

ToolBarVC uses ios/chrome/grit/ios_theme_resources.h for Images ID's, these are
stored in the ResourcesBundle singleton, which is initialized by the
StartChromeMain step of the ApplicationState configuration. 
I edited the description to add that we can get rid of some steps for this
specific use case, but we might need to add them later on. WDYT, should I only
run the necessary steps? Or try to configure showcase as closer to the clean
skeleton as possible?

[lpromero, 2017-01-24 01:54:27]

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

[lpromero, 2017-01-24 01:54:28]

I'd rather keep ApplicationState logic out of Showcase entirely.
Please just initialize what you need in didFinishLaunching.

+cc gambard who was also looking at this.

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/settings/B...
File ios/showcase/settings/BUILD.gn (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/settings/B...
ios/showcase/settings/BUILD.gn:7: "sc_settings_coordinator.h",
Thanks for doing this!

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
File ios/showcase/toolbar/sc_toolbar_coordinator.h (left):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.h:16: 
Add a new line.

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
File ios/showcase/toolbar/sc_toolbar_coordinator.mm (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.mm:14: @property(nonatomic, strong)
ToolbarViewController* viewController;
BTW, why do you keep a pointer to that controller?

[marq (ping after 24h), 2017-01-24 09:49:07]

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/core/app_d...
File ios/showcase/core/app_delegate.mm (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/core/app_d...
ios/showcase/core/app_delegate.mm:40: self.applicationState = [[ApplicationState
alloc] init];
Sorry for any confusion caused by my comment in chat; I was conflating
ApplicationState with just the bundle set-up. This is overkill for now.

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/core/main.mm
File ios/showcase/core/main.mm (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/core/main....
ios/showcase/core/main.mm:17: // is started. This needs to be stack allocated
and live for the lifetime of
None of this is needed; we should keep the Showcase main.mm as simple as
possible.

If at some point we want to adopt (say) the pseudo-RTL support, we should factor
that code into something shared rather than just duplicating.

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
File ios/showcase/toolbar/sc_toolbar_coordinator.mm (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.mm:22: self.viewController =
[[ToolbarViewController alloc] init];
The toolbar doesn't set its own height; it relies on the view controller
presenting/containing it to do that. So you might want to set
self.viewController.view.frame.size.height to something reasonable here so the
toolbar looks like a toolbar.

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.mm:24: [self.baseViewController
pushViewController:self.viewController animated:YES];
You should add a protocol alerter for <ToolbarCommands> and set it as the
ToolbarViewController's toolbarCommandHandler.

[sczs, 2017-01-24 18:32:22]

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/core/main.mm
File ios/showcase/core/main.mm (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/core/main....
ios/showcase/core/main.mm:17: // is started. This needs to be stack allocated
and live for the lifetime of
On 2017/01/24 09:49:07, marq wrote:
> None of this is needed; we should keep the Showcase main.mm as simple as
> possible.
> 
> If at some point we want to adopt (say) the pseudo-RTL support, we should
factor
> that code into something shared rather than just duplicating.

Removed everything but the AtExitManager, without it the App crashes on launch.

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
File ios/showcase/toolbar/sc_toolbar_coordinator.h (left):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.h:16: 
On 2017/01/24 01:54:27, lpromero wrote:
> Add a new line.

Done.

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
File ios/showcase/toolbar/sc_toolbar_coordinator.mm (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.mm:14: @property(nonatomic, strong)
ToolbarViewController* viewController;
On 2017/01/24 01:54:27, lpromero wrote:
> BTW, why do you keep a pointer to that controller?

Removed. 
I was going to start working on this file but thought I just add it for now and
leave its implementation for a different CL. So this is a leftover, will remove!

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.mm:22: self.viewController =
[[ToolbarViewController alloc] init];
On 2017/01/24 09:49:07, marq wrote:
> The toolbar doesn't set its own height; it relies on the view controller
> presenting/containing it to do that. So you might want to set
> self.viewController.view.frame.size.height to something reasonable here so the
> toolbar looks like a toolbar.

Sorry for not being clearer (Though this feedback helps me anyways). I was
intending for this CL to only add the application State and the basic files.

 I was thinking on working on the implementation of this file on a different CL,
to keep things simple. Should I add it here?

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.mm:24: [self.baseViewController
pushViewController:self.viewController animated:YES];
On 2017/01/24 09:49:07, marq wrote:
> You should add a protocol alerter for <ToolbarCommands> and set it as the
> ToolbarViewController's toolbarCommandHandler.

Ditto.

[sczs, 2017-01-24 18:43:07]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle, which is initialized by ApplicationState.
For this reason this CL configures the Application State with the same steps as 
ios/clean. We can probably get rid of some steps, but I wanted it to behave
similar to 
clean.

Adds ApplicationState support to Showcase. 
Creates a coordinator for Toolbar.
Renames Showcase Settings coordinator.

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Creates an AtExitManager in main.mm
-Inits IOSChromeMain and the Resource bundle singleton.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

[lpromero, 2017-01-25 08:07:30]

lgtm

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/app_d...
File ios/showcase/core/app_delegate.mm (right):

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/app_d...
ios/showcase/core/app_delegate.mm:36: std::string(), nullptr,
ResourceBundle::LOAD_COMMON_RESOURCES);
Can you move this before or right after the MDCTypography on line 25? In case
we’d want it before creating any UI.

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/main.mm
File ios/showcase/core/main.mm (right):

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/main....
ios/showcase/core/main.mm:16: // is started. This needs to be stack allocated
and live for the lifetime of
Remove the comment about the crash controller.

[marq (ping after 24h), 2017-01-25 09:29:22]

LGTM

Can you please explain in the CL description why IOSChromeMain and the at-exit
manager are necessary?

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
File ios/showcase/toolbar/sc_toolbar_coordinator.mm (right):

https://codereview.chromium.org/2651833002/diff/20001/ios/showcase/toolbar/sc...
ios/showcase/toolbar/sc_toolbar_coordinator.mm:22: self.viewController =
[[ToolbarViewController alloc] init];
On 2017/01/24 18:32:22, sczs wrote:
> On 2017/01/24 09:49:07, marq wrote:
> > The toolbar doesn't set its own height; it relies on the view controller
> > presenting/containing it to do that. So you might want to set
> > self.viewController.view.frame.size.height to something reasonable here so
the
> > toolbar looks like a toolbar.
> 
> Sorry for not being clearer (Though this feedback helps me anyways). I was
> intending for this CL to only add the application State and the basic files.
> 
>  I was thinking on working on the implementation of this file on a different
CL,
> to keep things simple. Should I add it here? 

That's fine. Small CLs are good!

[sczs, 2017-01-25 15:46:28]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Creates an AtExitManager in main.mm
-Inits IOSChromeMain and the Resource bundle singleton.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed so the Resource bundle can calculate the
screen size of the device and load the right resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

[sczs, 2017-01-25 15:49:12]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed so the Resource bundle can calculate the
screen size of the device and load the right resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed since Resource bundle uses
current_process_commandline_ to calculate the device scale and load the right 
resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

[edchin, 2017-01-25 16:01:05]

lgtm

[sczs, 2017-01-25 16:09:41]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed since Resource bundle uses
current_process_commandline_ to calculate the device scale and load the right 
resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed since Resource bundle uses
current_process_commandline_ to calculate the device scale and load the right 
resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

[sczs, 2017-01-25 16:10:24]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-01-25 16:10:25]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org,
lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2651833002/#ps60001
(title: "CL Feedback")

[commit-bot: I haz the power, 2017-01-25 16:10:35]

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 16:18:43]

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

[commit-bot: I haz the power, 2017-01-25 16:18:44]

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

[sczs, 2017-01-25 16:59:22]

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/app_d...
File ios/showcase/core/app_delegate.mm (right):

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/app_d...
ios/showcase/core/app_delegate.mm:36: std::string(), nullptr,
ResourceBundle::LOAD_COMMON_RESOURCES);
On 2017/01/25 08:07:30, lpromero wrote:
> Can you move this before or right after the MDCTypography on line 25? In case
> we’d want it before creating any UI.

Done.

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/main.mm
File ios/showcase/core/main.mm (right):

https://codereview.chromium.org/2651833002/diff/40001/ios/showcase/core/main....
ios/showcase/core/main.mm:16: // is started. This needs to be stack allocated
and live for the lifetime of
On 2017/01/25 08:07:30, lpromero wrote:
> Remove the comment about the crash controller.

Done.

[sczs, 2017-01-25 17:00:06]

The CQ bit was checked by sczs@chromium.org

[sczs, 2017-01-25 17:00:06]

The patchset sent to the CQ was uploaded after l-g-t-m from marq@chromium.org,
edchin@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2651833002/#ps80001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-25 17:00:27]

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 17:07:32]

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

[commit-bot: I haz the power, 2017-01-25 17:07:33]

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

[sczs, 2017-01-25 17:14:51]

sczs@chromium.org changed reviewers:
+ thakis@chromium.org

[sczs, 2017-01-25 17:19:59]

Thakis,

Could you please lgtm, I'm just adding "ui/base" to the showcase DEPS file.

Thanks!

[sczs, 2017-01-27 02:18:20]

On 2017/01/25 17:19:59, sczs wrote:
> Thakis,
> 
> Could you please lgtm, I'm just adding "ui/base" to the showcase DEPS file.
> 
> Thanks!

Thakis, just a friendly reminder. Should be really simple to review.

[Nico, 2017-01-27 02:19:13]

lgtm, thanks for the ping. Feel free to ping if I don't look within a few hours
(during east coast working hours)

[sczs, 2017-01-27 02:22:43]

On 2017/01/27 02:19:13, Nico wrote:
> lgtm, thanks for the ping. Feel free to ping if I don't look within a few
hours
> (during east coast working hours)

Thanks! I was expecting you to see the ping tomorrow morning, will keep it mind
for future reviews

[sczs, 2017-01-27 02:22:48]

The CQ bit was checked by sczs@chromium.org

[commit-bot: I haz the power, 2017-01-27 02:23:25]

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-27 04:34:35]

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

[commit-bot: I haz the power, 2017-01-27 04:34:37]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sczs, 2017-01-27 16:46:15]

The CQ bit was checked by sczs@chromium.org

[commit-bot: I haz the power, 2017-01-27 16:46:37]

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-27 18:20:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485535575390720,
"parent_rev": "60311d47f627e94d7d47dd5548ac684263ff0762", "commit_rev":
"9d62fd77957ef8e486ad77c57e05b717c5a037d3"}

[commit-bot: I haz the power, 2017-01-27 18:21:55]

Description was changed from

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed since Resource bundle uses
current_process_commandline_ to calculate the device scale and load the right 
resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793
==========

to

==========
[ios showcase]Configures ApplicationState and adds Toolbar Coordinator

In order to run the ToolbarVC in Showcase we need to access the Shared
ResourceBundle.

-Adds a DEP to ui/base
-Inits the Resource bundle singleton.
-Inits IOSChromeMain, this is needed since Resource bundle uses
current_process_commandline_ to calculate the device scale and load the right 
resources.
-Creates an AtExitManager in main.mm which is needed by IOSChromeMain.
-Creates a simple placeholder coordinator file for Toolbar.
-Renames Showcase Settings coordinator.

BUG=683793

Review-Url: https://codereview.chromium.org/2651833002
Cr-Commit-Position: refs/heads/master@{#446712}
Committed:
https://chromium.googlesource.com/chromium/src/+/9d62fd77957ef8e486ad77c57e05...
==========

[commit-bot: I haz the power, 2017-01-27 18:21:58]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9d62fd77957ef8e486ad77c57e05...

[sczs, 2017-01-30 22:16:44]

Patchset #5 (id:100001) has been deleted