Made launcher hidden when kicking off chrome in app mode.

Made launcher hidden when kicking off chrome in app mode.

A new switch --force-app-mode has been added for this purpose. On ChromeOS, this switch will also force app install in the next CL.

BUG=172988, 173008
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179762

[oshima, 2013-01-30 00:28:31]

https://codereview.chromium.org/12077055/diff/3001/ash/wm/shelf_layout_manage...
File ash/wm/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12077055/diff/3001/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:256: default:
case SHELF_AUTO_HIDE_BEHAVIOR_NEVER:

and no default so that compilation fails if someone added new enum item

https://codereview.chromium.org/12077055/diff/3001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12077055/diff/3001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:1215: // to
completely hide it when we run app mode.
NOTREACHED()

https://codereview.chromium.org/12077055/diff/3001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/12077055/diff/3001/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:195: extern const char kForceAppMode[];
sort

[Mr4D (OOO till 08-26), 2013-01-30 00:55:00]

Please see comments. Maybe even two utility functions (for kiosk as well)?

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:638:
command_line->HasSwitch(switches::kForceAppMode)) {
Please create a utility query for the command line switch check.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc
(right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:663:
CommandLine* command_line = CommandLine::ForCurrentProcess();
Please create a utility query for the command line switch check.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:2084: command_line->HasSwitch(switches::kAppId));
You could use that here as well.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:396:
command_line->HasSwitch(switches::kAppId));
And so on.

[zel, 2013-01-30 01:01:34]

https://codereview.chromium.org/12077055/diff/3001/ash/wm/shelf_layout_manage...
File ash/wm/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12077055/diff/3001/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:256: default:
On 2013/01/30 00:28:31, oshima wrote:
> case SHELF_AUTO_HIDE_BEHAVIOR_NEVER:
> 
> and no default so that compilation fails if someone added new enum item

Done.

https://codereview.chromium.org/12077055/diff/3001/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12077055/diff/3001/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:1215: // to
completely hide it when we run app mode.
On 2013/01/30 00:28:31, oshima wrote:
> NOTREACHED()

Done.

https://codereview.chromium.org/12077055/diff/3001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.h (right):

https://codereview.chromium.org/12077055/diff/3001/chrome/common/chrome_switc...
chrome/common/chrome_switches.h:195: extern const char kForceAppMode[];
On 2013/01/30 00:28:31, oshima wrote:
> sort

Done.

[oshima, 2013-01-30 01:17:15]

lgtm my bits with one nit.
I agree that it's better to have a predicate function to tell
if it's app mode.

https://codereview.chromium.org/12077055/diff/2002/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12077055/diff/2002/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:692: const char kForceAppMode[]                
 = "force-app-mode";
is this only for chromeos? if so, add comment.

[zel, 2013-01-30 01:46:32]

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:638:
command_line->HasSwitch(switches::kForceAppMode)) {
On 2013/01/30 00:55:00, Mr4D wrote:
> Please create a utility query for the command line switch check.

Where do you guys think I should place such utility function into?

https://codereview.chromium.org/12077055/diff/2002/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/12077055/diff/2002/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:692: const char kForceAppMode[]                
 = "force-app-mode";
On 2013/01/30 01:17:15, oshima wrote:
> is this only for chromeos? if so, add comment.

not 100% sure that this is going to be CrOS-only feature yet

[Mr4D (OOO till 08-26), 2013-01-30 02:11:41]

I would put it there.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:638:
command_line->HasSwitch(switches::kForceAppMode)) {
Maybe chrome/browser/ui/ash/ash_util.cc?

[zel, 2013-01-30 03:03:44]

PTAL

I moved that switch check to chrome/common/switch_utils.cc to
IsRunningInAppMode(). 

I've also changed browser lifetime management for forced app mode in ash_init.cc
as well. We will now close chrome when all app-specific surfaces close.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc
(right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:638:
command_line->HasSwitch(switches::kForceAppMode)) {
On 2013/01/30 02:11:41, Mr4D wrote:
> Maybe chrome/browser/ui/ash/ash_util.cc?

Done.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_app.cc:638:
command_line->HasSwitch(switches::kForceAppMode)) {
On 2013/01/30 02:11:41, Mr4D wrote:
> Maybe chrome/browser/ui/ash/ash_util.cc?

Done.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc
(right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_per_browser.cc:663:
CommandLine* command_line = CommandLine::ForCurrentProcess();
On 2013/01/30 00:55:00, Mr4D wrote:
> Please create a utility query for the command line switch check.

Done.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/browser....
chrome/browser/ui/browser.cc:2084: command_line->HasSwitch(switches::kAppId));
On 2013/01/30 00:55:00, Mr4D wrote:
> You could use that here as well.

Done.

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/fullscre...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12077055/diff/2002/chrome/browser/ui/fullscre...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:396:
command_line->HasSwitch(switches::kAppId));
On 2013/01/30 00:55:00, Mr4D wrote:
> And so on.

Done.

[Mr4D (OOO till 08-26), 2013-01-30 04:05:27]

lgtm

lgtm with the tiny nits.

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate.cc:589: if
(switches::IsRunningInAppMode()) {
No brackets needed

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:393: #if
!defined(OS_MACOSX)  // Kiosk mode not available on Mac.
This comment is now a bit confusing.

[oshima, 2013-01-30 06:14:35]

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/ash...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/ash...
chrome/browser/ui/ash/ash_init.cc:112: browser::StartKeepAlive();
I think you should skip EndKeepAlive
in application_lifetime_aura.cc

[Mattias Nissler (ping if slow), 2013-01-30 10:46:14]

Looks reasonable. I would be more comfortable if we brought up a shell that
doesn't even instantiate any UI objects other than the root windows - otherwise
this will need fixing all the time as people add more features to ash (and we've
certainly made that experience with retail mode). I'm everything but an Ash
expert though - so feel free to ignore.

[zel, 2013-01-30 17:06:43]

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/ash...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/ash...
chrome/browser/ui/ash/ash_init.cc:112: browser::StartKeepAlive();
On 2013/01/30 06:14:35, oshima wrote:
> I think you should skip EndKeepAlive
> in application_lifetime_aura.cc

Done.

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/chr...
File chrome/browser/ui/ash/chrome_shell_delegate.cc (right):

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/ash/chr...
chrome/browser/ui/ash/chrome_shell_delegate.cc:589: if
(switches::IsRunningInAppMode()) {
On 2013/01/30 04:05:27, Mr4D wrote:
> No brackets needed

Done.

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/fullscr...
File chrome/browser/ui/fullscreen/fullscreen_controller.cc (right):

https://codereview.chromium.org/12077055/diff/10002/chrome/browser/ui/fullscr...
chrome/browser/ui/fullscreen/fullscreen_controller.cc:393: #if
!defined(OS_MACOSX)  // Kiosk mode not available on Mac.
On 2013/01/30 04:05:27, Mr4D wrote:
> This comment is now a bit confusing.

Done.

[oshima, 2013-01-30 22:08:20]

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/lifetime/ap...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/lifetime/ap...
chrome/browser/lifetime/application_lifetime_aura.cc:51:
NotifyAndTerminate(true);
just FYI: I'm 100% sure if you can skip this one or not. Make sure
chrome properly exist, but not by getting killed by session_manager.

[oshima, 2013-01-30 22:14:17]

lgtm, if you tested and confirmed it works.

On 2013/01/30 22:08:20, oshima wrote:
>
https://codereview.chromium.org/12077055/diff/9014/chrome/browser/lifetime/ap...
> File chrome/browser/lifetime/application_lifetime_aura.cc (right):
> 
>
https://codereview.chromium.org/12077055/diff/9014/chrome/browser/lifetime/ap...
> chrome/browser/lifetime/application_lifetime_aura.cc:51:
> NotifyAndTerminate(true);
> just FYI: I'm 100% sure if you can skip this one or not. Make sure
> chrome properly exist, but not by getting killed by session_manager.

[zel, 2013-01-30 22:26:52]

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/lifetime/ap...
File chrome/browser/lifetime/application_lifetime_aura.cc (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/lifetime/ap...
chrome/browser/lifetime/application_lifetime_aura.cc:51:
NotifyAndTerminate(true);
On 2013/01/30 22:08:20, oshima wrote:
> just FYI: I'm 100% sure if you can skip this one or not. Make sure
> chrome properly exist, but not by getting killed by session_manager.

works fine with this in place

[sky, 2013-01-30 22:27:10]

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
File ash/wm/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:250: ShelfVisibilityState
ShelfLayoutManager::GetShelfVisibilityState() {
Make position match header, and name this more clearly, maybe
CalculateShelfVisibilityStateWhenMaximized or something. A getter implies a
simple return value and is easily confused with auto_hide_state().

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:266: } else if (gesture_drag_status_ ==
GESTURE_DRAG_COMPLETE_IN_PROGRESS) {
Are you sure we can't hit a gesture_drag_status_ or immersive mode?

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:280: SetState(GetShelfVisibilityState());
fix indentation while you're here.

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_init.cc:111: !switches::IsRunningInAppMode()) {
Does running in app mode trigger session restore? If so and nothing is restored
I suspect you'll like a CHECK. Can you test this.

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils.h
File chrome/common/switch_utils.h (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils...
chrome/common/switch_utils.h:20: bool IsRunningInAppMode();
switch_utils isn't a good place for this function. Move it some where closer to
where app mode is kept.

[zel, 2013-01-30 22:55:16]

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
File ash/wm/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:250: ShelfVisibilityState
ShelfLayoutManager::GetShelfVisibilityState() {
On 2013/01/30 22:27:10, sky wrote:
> Make position match header, and name this more clearly, maybe
> CalculateShelfVisibilityStateWhenMaximized or something. A getter implies a
> simple return value and is easily confused with auto_hide_state().

Moved. Renamed to CalculateShelfVisibility()... it's used for default state too,
not just maximized. Added CalculateShelfVisibilityWhenDragging() for dragging
case.

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:266: } else if (gesture_drag_status_ ==
GESTURE_DRAG_COMPLETE_IN_PROGRESS) {
On 2013/01/30 22:27:10, sky wrote:
> Are you sure we can't hit a gesture_drag_status_ or immersive mode?

Good point. I've added a method for dragging - that can happen. Immersive mode
is not an option when auto_hide_behavior_ == SHELF_AUTO_HIDE_ALWAYS_HIDDEN. I've
added DCHECK for that one.

https://codereview.chromium.org/12077055/diff/9014/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:280: SetState(GetShelfVisibilityState());
On 2013/01/30 22:27:10, sky wrote:
> fix indentation while you're here.

Done.

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_init.cc:111: !switches::IsRunningInAppMode()) {
On 2013/01/30 22:27:10, sky wrote:
> Does running in app mode trigger session restore? If so and nothing is
restored
> I suspect you'll like a CHECK. Can you test this.

Nope. No session restore needed for this mode.

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils.h
File chrome/common/switch_utils.h (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils...
chrome/common/switch_utils.h:20: bool IsRunningInAppMode();
On 2013/01/30 22:27:10, sky wrote:
> switch_utils isn't a good place for this function. Move it some where closer
to
> where app mode is kept.

This is used all over now:
   chrome/browser/ui/ash/launcher/*, and
   chrome/browser/ui/views/frame/*, and
   chrome/browser/lifetime

and a CL is coming where I need it under chrome/browser/extensions/* too.

chrome/common/switch_utils.h/cc seemed like a good to me from what was suggested
by previous reviews. do you want me to move this under chrome/browser/* ? if so,
any good choice there or should I create a new file.

[sky, 2013-01-30 23:29:56]

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_init.cc:111: !switches::IsRunningInAppMode()) {
On 2013/01/30 22:55:16, zel wrote:
> On 2013/01/30 22:27:10, sky wrote:
> > Does running in app mode trigger session restore? If so and nothing is
> restored
> > I suspect you'll like a CHECK. Can you test this.
> 
> Nope. No session restore needed for this mode.

But this runs on chromeos which automatically triggers session restore, right?
Is there an early out some where else that avoid session restore for this mode?

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils.h
File chrome/common/switch_utils.h (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils...
chrome/common/switch_utils.h:20: bool IsRunningInAppMode();
On 2013/01/30 22:55:16, zel wrote:
> On 2013/01/30 22:27:10, sky wrote:
> > switch_utils isn't a good place for this function. Move it some where closer
> to
> > where app mode is kept.
> 
> This is used all over now:
>    chrome/browser/ui/ash/launcher/*, and
>    chrome/browser/ui/views/frame/*, and
>    chrome/browser/lifetime
> 
> and a CL is coming where I need it under chrome/browser/extensions/* too.
> 
> chrome/common/switch_utils.h/cc seemed like a good to me from what was
suggested
> by previous reviews. do you want me to move this under chrome/browser/* ? if
so,
> any good choice there or should I create a new file.

IMO this file is intended for utilities relating to maintaining switches, not
whether a switch is set. In fact whether a feature is enabled shouldn't be part
of switches, it should be close to the other code related to the feature. I
would be inclined to put this in c/b/u/app_mode.

[zel, 2013-01-30 23:56:57]

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
File chrome/browser/ui/ash/ash_init.cc (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/browser/ui/ash/ash_...
chrome/browser/ui/ash/ash_init.cc:111: !switches::IsRunningInAppMode()) {
On 2013/01/30 23:29:56, sky wrote:
> On 2013/01/30 22:55:16, zel wrote:
> > On 2013/01/30 22:27:10, sky wrote:
> > > Does running in app mode trigger session restore? If so and nothing is
> > restored
> > > I suspect you'll like a CHECK. Can you test this.
> > 
> > Nope. No session restore needed for this mode.
> 
> But this runs on chromeos which automatically triggers session restore, right?
> Is there an early out some where else that avoid session restore for this
mode?

Nope, we won't trigger session restore in forced app mode. I am working on
startup/shutdown changes for this mode in another CL - this part was only
covering UI tweaks.

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils.h
File chrome/common/switch_utils.h (right):

https://codereview.chromium.org/12077055/diff/9014/chrome/common/switch_utils...
chrome/common/switch_utils.h:20: bool IsRunningInAppMode();
On 2013/01/30 23:29:56, sky wrote:
> On 2013/01/30 22:55:16, zel wrote:
> > On 2013/01/30 22:27:10, sky wrote:
> > > switch_utils isn't a good place for this function. Move it some where
closer
> > to
> > > where app mode is kept.
> > 
> > This is used all over now:
> >    chrome/browser/ui/ash/launcher/*, and
> >    chrome/browser/ui/views/frame/*, and
> >    chrome/browser/lifetime
> > 
> > and a CL is coming where I need it under chrome/browser/extensions/* too.
> > 
> > chrome/common/switch_utils.h/cc seemed like a good to me from what was
> suggested
> > by previous reviews. do you want me to move this under chrome/browser/* ? if
> so,
> > any good choice there or should I create a new file.
> 
> IMO this file is intended for utilities relating to maintaining switches, not
> whether a switch is set. In fact whether a feature is enabled shouldn't be
part
> of switches, it should be close to the other code related to the feature. I
> would be inclined to put this in c/b/u/app_mode.

moved to chrome/browser/app_mode/app_mode_utils.(h|cc)

[sky, 2013-01-31 00:58:13]

https://codereview.chromium.org/12077055/diff/8004/ash/wm/shelf_layout_manage...
File ash/wm/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12077055/diff/8004/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:279:
SetState(CalculateShelfVisibilityWhileDragging());
Should we instead ignore gestures when hidden.

[zel, 2013-01-31 01:07:27]

https://codereview.chromium.org/12077055/diff/8004/ash/wm/shelf_layout_manage...
File ash/wm/shelf_layout_manager.cc (right):

https://codereview.chromium.org/12077055/diff/8004/ash/wm/shelf_layout_manage...
ash/wm/shelf_layout_manager.cc:279:
SetState(CalculateShelfVisibilityWhileDragging());
On 2013/01/31 00:58:13, sky wrote:
> Should we instead ignore gestures when hidden.

I've added TODO here, there will me many quirks like this one that I will have
to test on the device.

[sky, 2013-01-31 01:08:17]

LGTM with a TODO and the following fixes.

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/app_mode/ap...
File chrome/browser/app_mode/app_mode_utils.h (right):

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/app_mode/ap...
chrome/browser/app_mode/app_mode_utils.h:8: #include <map>
remove all includes from here.

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/app_mode/ap...
chrome/browser/app_mode/app_mode_utils.h:13: namespace app_mode_utils {
put this in chrome namespace (see thread to chromium-dev on namespaces).

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:574: // return false;
Fix this. Either remove if (if not needed), or remove comment.

[zel, 2013-01-31 01:19:44]

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/app_mode/ap...
File chrome/browser/app_mode/app_mode_utils.h (right):

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/app_mode/ap...
chrome/browser/app_mode/app_mode_utils.h:8: #include <map>
On 2013/01/31 01:08:17, sky wrote:
> remove all includes from here.

Done.

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/app_mode/ap...
chrome/browser/app_mode/app_mode_utils.h:13: namespace app_mode_utils {
On 2013/01/31 01:08:17, sky wrote:
> put this in chrome namespace (see thread to chromium-dev on namespaces).

Done.

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/ui/startup/...
File chrome/browser/ui/startup/startup_browser_creator.cc (right):

https://codereview.chromium.org/12077055/diff/8004/chrome/browser/ui/startup/...
chrome/browser/ui/startup/startup_browser_creator.cc:574: // return false;
On 2013/01/31 01:08:17, sky wrote:
> Fix this. Either remove if (if not needed), or remove comment.

whoops! this wasn't suppose to be here at all. reverted.