Show a different icon in the launcher for incognito windows

Show a different icon in the launcher for incognito windows


BUG=116932
TEST=Open an incognito window and a normal window.  Check that the icons are different

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

[Zachary Kuznia, 2012-03-09 00:49:53]

Hello oshima,

Please review this at:
https://chromiumcodereview.appspot.com/9649013

Cheers,
-Zach

[oshima, 2012-03-09 17:54:02]

sky should review this.
Just general comments below, but sky's comments should supersedes mine.

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.h
File ash/launcher/launcher_types.h (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.h#n...
ash/launcher/launcher_types.h:48: bool private_tab;
we use incognito or otr to refer to "off the record".
this setting is per browser window, so _tab sounds odd too.

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_view.cc#n...
ash/launcher/launcher_view.cc:306: button->SetTabImage(item.image,
item.num_tabs, item.private_tab);
maybe pass LauncherItem? It doesn't sound odd for LauncherButton to know
LauncherItem, and it will make it easy to update LauncherItem.

http://codereview.chromium.org/9649013/diff/1/ash/launcher/tabbed_launcher_bu...
File ash/launcher/tabbed_launcher_button.h (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/tabbed_launcher_bu...
ash/launcher/tabbed_launcher_button.h:93: // provided and if the tab is private.
browser window is incognito (or off the record).

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:75: }
remove {}

[sky, 2012-03-09 18:18:07]

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.cc
File ash/launcher/launcher_types.cc (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.cc#...
ash/launcher/launcher_types.cc:19: private_tab(is_private),
Having to maintain these two constructors is becoming a pain. I say nuke this
one.

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:125:
LauncherUpdater* updater, bool is_private) {
Wrap is_private

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.h (right):

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.h:79: bool
is_private);
I don't like mysterious booleans. Use an enum.

[Zachary Kuznia, 2012-03-09 22:17:35]

This CL currently has programmer art.  Should I submit with this, or try to find
real art first?

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.cc
File ash/launcher/launcher_types.cc (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.cc#...
ash/launcher/launcher_types.cc:19: private_tab(is_private),
On 2012/03/09 18:18:08, sky wrote:
> Having to maintain these two constructors is becoming a pain. I say nuke this
> one.

Done.

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.h
File ash/launcher/launcher_types.h (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_types.h#n...
ash/launcher/launcher_types.h:48: bool private_tab;
On 2012/03/09 17:54:02, oshima wrote:
> we use incognito or otr to refer to "off the record".
> this setting is per browser window, so _tab sounds odd too.
> 

Done.

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_view.cc
File ash/launcher/launcher_view.cc (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/launcher_view.cc#n...
ash/launcher/launcher_view.cc:306: button->SetTabImage(item.image,
item.num_tabs, item.private_tab);
On 2012/03/09 17:54:02, oshima wrote:
> maybe pass LauncherItem? It doesn't sound odd for LauncherButton to know
> LauncherItem, and it will make it easy to update LauncherItem.

Done.

http://codereview.chromium.org/9649013/diff/1/ash/launcher/tabbed_launcher_bu...
File ash/launcher/tabbed_launcher_button.h (right):

http://codereview.chromium.org/9649013/diff/1/ash/launcher/tabbed_launcher_bu...
ash/launcher/tabbed_launcher_button.h:93: // provided and if the tab is private.
On 2012/03/09 17:54:02, oshima wrote:
> browser window is incognito (or off the record).

Done.

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:125:
LauncherUpdater* updater, bool is_private) {
On 2012/03/09 18:18:08, sky wrote:
> Wrap is_private

Done.

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.h (right):

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.h:79: bool
is_private);
On 2012/03/09 18:18:08, sky wrote:
> I don't like mysterious booleans. Use an enum.

Done.

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9649013/diff/1/chrome/browser/ui/views/ash/lau...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:75: }
On 2012/03/09 17:54:02, oshima wrote:
> remove {}

Done.

[sky, 2012-03-09 22:40:46]

Check in the images you have now until we get real ones.

http://codereview.chromium.org/9649013/diff/8002/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9649013/diff/8002/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:31:
LauncherUpdater::LauncherUpdater(aura::Window* window,
A LauncherUpdater is always going to be associated with an incognito or
non-incognito profile. It's cleaner to check it and set it as const on the class
in the constructor from the tabmodel.

[Zachary Kuznia, 2012-03-09 23:23:10]

http://codereview.chromium.org/9649013/diff/8002/chrome/browser/ui/views/ash/...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9649013/diff/8002/chrome/browser/ui/views/ash/...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:31:
LauncherUpdater::LauncherUpdater(aura::Window* window,
On 2012/03/09 22:40:46, sky wrote:
> A LauncherUpdater is always going to be associated with an incognito or
> non-incognito profile. It's cleaner to check it and set it as const on the
class
> in the constructor from the tabmodel.

Done.

[sky, 2012-03-10 00:13:11]

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:203: void
ChromeLauncherDelegate::ConvertAppToTabbed(ash::LauncherID id) {
Does this need to take incognito too?

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:43: if
(tab_model_->count() > 0) {
Use the profile from the TabStripModel, that way you don't need to worry about
an empty model. This should also allow you to make is_incognito_ const.

[Zachary Kuznia, 2012-03-10 00:28:35]

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:203: void
ChromeLauncherDelegate::ConvertAppToTabbed(ash::LauncherID id) {
On 2012/03/10 00:13:11, sky wrote:
> Does this need to take incognito too?

I don't think so.  The LauncherItem is updated by the caller, so it wouldn't do
anything with the value.

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
File chrome/browser/ui/views/ash/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9649013/diff/11004/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/launcher_updater.cc:43: if
(tab_model_->count() > 0) {
On 2012/03/10 00:13:11, sky wrote:
> Use the profile from the TabStripModel, that way you don't need to worry about
> an empty model. This should also allow you to make is_incognito_ const.

Done.

[sky, 2012-03-11 21:15:34]

LGTM

[Ilya Sherman, 2012-03-12 23:12:26]

Reverted this due to a compile failure on the waterfall:
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2...

[willchan no longer on Chromium, 2012-03-12 23:19:04]

Not to be too bitchy, but what happened here? I don't see a single trybot green
run. And apparently the commit queue wasn't used. Please use trybots and the
commit queue.

On 2012/03/12 23:12:26, Ilya Sherman wrote:
> Reverted this due to a compile failure on the waterfall:
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%2520ChromiumOS...

[Zachary Kuznia, 2012-03-12 23:35:14]

The trybots weren't working on this cl because there are binary files being
added.  I thought that I had done a complete build locally to test this, but I
was incorrect.

I'm now splitting the binary files into a separate CL, and will land that first
so that I can properly use the trybots.

On 2012/03/12 23:19:04, willchan wrote:
> Not to be too bitchy, but what happened here? I don't see a single trybot
green
> run. And apparently the commit queue wasn't used. Please use trybots and the
> commit queue.
> 
> On 2012/03/12 23:12:26, Ilya Sherman wrote:
> > Reverted this due to a compile failure on the waterfall:
> >
>
http://build.chromium.org/p/chromium.chromiumos/builders/Linux%252520Chromium...