Launch panels as popup windows in Aura, and add separate launcher icon logic for panels.

Launch panels as popup windows in Aura, and add separate launcher icon logic for panels.

BUG=115901
TEST=Scratchpad and GTalk apps should open popup windows with a launcher icon (currently the blown up favicon, that will change in a follow up CL).


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125080

[stevenjb, 2012-03-02 22:44:28]

Please ignore changes to browser_actions_container.cc, those are being addressed
in http://codereview.chromium.org/9535020/, just here to avoid crashes while
testing.

[sky, 2012-03-02 23:08:53]

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc:45: const char
kAppTypePanel[] = "panel";
sort.

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/launcher_updater.cc:202: //
TODO(stevenjb): Get a large favicon for the launcher.
How come you don't want the same logic as apps use?

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/launcher_updater.h (right):

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/launcher_updater.h:37: TYPE_PANEL,
Why does the launcher need to know about panel?

[stevenjb, 2012-03-03 00:02:06]

I've had a number of discussions with the panel/quasar folk, and with Dave and
Zel about the best strategy for setting panel launcher icons for R19. Using high
res favicons seems the easiest solution to implement in a short time frame (vs.
supplying an API for extensions to update a window's app icon).

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc:45: const char
kAppTypePanel[] = "panel";
On 2012/03/02 23:08:53, sky wrote:
> sort.

Done.

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/launcher_updater.cc:189: if (type_ ==
TYPE_APP)
Panels want to change their icon when the favicon changes; their behavior is
somewhere between App and Tabbed behavior.

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/launcher_updater.h (right):

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/launcher_updater.h:37: TYPE_PANEL,
On 2012/03/02 23:08:53, sky wrote:
> Why does the launcher need to know about panel?
See next comment.

[sky, 2012-03-03 00:13:04]

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/launcher_updater.cc (right):

http://codereview.chromium.org/9590001/diff/1001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/launcher_updater.cc:189: if (type_ ==
TYPE_APP)
On 2012/03/03 00:02:06, stevenjb (chromium) wrote:
> Panels want to change their icon when the favicon changes; their behavior is
> somewhere between App and Tabbed behavior.

Ok, makes sense. But I don't think the ash portion of the launcher needs to
distinguish between APP and PANEL, right? I'm saying keep TYPE_PANEL only to the
chrome side of things.

[stevenjb, 2012-03-03 23:33:20]

Yes... with a change in ChromeLauncherDelegate::SetAppImage() I was able to
eliminate ash::TYPE_PANEL, at least for now.

[sky, 2012-03-05 15:20:25]

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_helper.cc:67: const Extension*
ExtensionTabHelper::GetExtension(
Make order match header.

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc:370:
i->second.updater->type() != LauncherUpdater::TYPE_PANEL)) {
Rather than this if, could you avoid asking for the image for type panels?

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:629: if (!icon_updater_.get())
Why is this needed?

[sky, 2012-03-05 15:22:52]

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:629: if (!icon_updater_.get())
On 2012/03/05 15:20:25, sky wrote:
> Why is this needed?

I see why it's needed. Could you move the creation to a method and have both
Show and this invoke it.

[stevenjb, 2012-03-05 19:50:06]

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_helper.cc (right):

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_helper.cc:67: const Extension*
ExtensionTabHelper::GetExtension(
On 2012/03/05 15:20:25, sky wrote:
> Make order match header.

Done.

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/aura...
File chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc (right):

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/aura...
chrome/browser/ui/views/aura/launcher/chrome_launcher_delegate.cc:370:
i->second.updater->type() != LauncherUpdater::TYPE_PANEL)) {
On 2012/03/05 15:20:25, sky wrote:
> Rather than this if, could you avoid asking for the image for type panels?

Looking through the code again (with a bit more familiarity now), I see that I
can avoid calling app_icon_loader_->FetchImage() in
ChromeLauncherDelegate::CreateAppLauncherItem(), and will add that.

However, we could still have an app, a panel it created, and a popup window it
created all in the launcher, all with the same app_id, so we need a way of
preventing updates with the same app_id from overriding icons set by the panel.
We may need an explicit way of doing this, but for now skipping panels (which
will always set their icon from LauncherUpdater::UpdateLauncher()) seems
easiest.

I did realize that we store item.app_type (not to be confused with
item.item_type), which does differentiate between PANEL and APP, so I will use
that instead of looking at the updater type.

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/9590001/diff/9001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:629: if (!icon_updater_.get())
On 2012/03/05 15:22:52, sky wrote:
> On 2012/03/05 15:20:25, sky wrote:
> > Why is this needed?
> 
> I see why it's needed. Could you move the creation to a method and have both
> Show and this invoke it.

Done.

[sky, 2012-03-05 22:19:04]

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

http://codereview.chromium.org/9590001/diff/16003/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:366: return;
This could cause problems if the app id of a tab changes. See AppIDChanged.

http://codereview.chromium.org/9590001/diff/16003/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:369: if
(i->second.app_id == id && i->second.app_type != APP_TYPE_PANEL) {
Add a comment as to why it's ok to skip panel.

[stevenjb, 2012-03-05 23:03:22]

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

http://codereview.chromium.org/9590001/diff/16003/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:366: return;
On 2012/03/05 22:19:04, sky wrote:
> This could cause problems if the app id of a tab changes. See AppIDChanged.
Ah. Right, I see. This should be a problem with the new logic, I'll remove the
early exit.

http://codereview.chromium.org/9590001/diff/16003/chrome/browser/ui/views/ash...
chrome/browser/ui/views/ash/launcher/chrome_launcher_delegate.cc:369: if
(i->second.app_id == id && i->second.app_type != APP_TYPE_PANEL) {
On 2012/03/05 22:19:04, sky wrote:
> Add a comment as to why it's ok to skip panel.

Done.

[Mihai Parparita -not on Chrome, 2012-03-05 23:28:29]

http://codereview.chromium.org/9590001/diff/18001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9590001/diff/18001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:623:
tab->extension_tab_helper()->SetExtensionAppIconById(extension_id);
Drive-by: Why can't SetExtensionAppById be used? That also sets the icon.

[stevenjb, 2012-03-05 23:50:55]

http://codereview.chromium.org/9590001/diff/18001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/9590001/diff/18001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:623:
tab->extension_tab_helper()->SetExtensionAppIconById(extension_id);
On 2012/03/05 23:28:29, Mihai Parparita wrote:
> Drive-by: Why can't SetExtensionAppById be used? That also sets the icon.

I tried that first, but it triggered a DCHECK on
extension->GetFullLaunchURL().is_valid() (the url is empty). I opted to just set
the icon instead of changing the DCHECK and setting the extension, since it
could potentially have other subtle side effects that I would would like to
avoid debugging in R19. I'd rather save the debugging for when we convert panels
to not be browsers for R20.

[sky, 2012-03-06 00:42:16]

LGTM

[commit-bot: I haz the power, 2012-03-06 00:49:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9590001/21001

[commit-bot: I haz the power, 2012-03-06 02:47:53]

Change committed as 125080