Add app.window.setIcon

Add app.window.setLauncherIcon

All this does currently is set a member of ShellWindow and notify ShellWindowLauncherController listeners.

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

[stevenjb, 2012-11-30 23:31:26]

In the process of adding this code, and the implementation code that uses it (CL
to follow), it occurred to me that I am not sure whether this should be
setLauncherIcon, or just setIcon.

In Ash, the Launcher is currently the only place that I can think of where the
app icon is used, but on other platforms we might use it elsewhere, e.g. in the
dock. I would need assistance implementing that on Win and OSX, but it seems
like that may be a better option, rather than providing functionality that is
currently Ash specific?

Thoughts? (Please add anyone else who might want to weigh in on this to the CL).

[jeremya, 2012-12-02 23:20:02]

On 2012/11/30 23:31:26, stevenjb (chromium) wrote:
> In the process of adding this code, and the implementation code that uses it
(CL
> to follow), it occurred to me that I am not sure whether this should be
> setLauncherIcon, or just setIcon.
> 
> In Ash, the Launcher is currently the only place that I can think of where the
> app icon is used, but on other platforms we might use it elsewhere, e.g. in
the
> dock. I would need assistance implementing that on Win and OSX, but it seems
> like that may be a better option, rather than providing functionality that is
> currently Ash specific?
> 
> Thoughts? (Please add anyone else who might want to weigh in on this to the
CL).

I definitely prefer setIcon, and yes, we should implement this on Windows and
Mac (though the Mac implementation may prove difficult as we're still a bit
scrappy there w.r.t dock integration)

[jeremya, 2012-12-03 00:09:34]

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:16:
namespace SetLauncherIcon = api::app_current_window_internal::SetLauncherIcon;
Why move these inside 'namespace extensions'?

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_window/app_window_apitest.cc (right):

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_apitest.cc:86:
EXPECT_FALSE(shell_window->launcher_icon_url().is_empty());
Perhaps we could have a slightly bolder assertion that the icon URL is actually
what was set?

https://codereview.chromium.org/11316292/diff/2002/chrome/test/data/extension...
File
chrome/test/data/extensions/platform_apps/windows_api_set_launcher_icon/background.js
(right):

https://codereview.chromium.org/11316292/diff/2002/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/windows_api_set_launcher_icon/background.js:7:
win.setLauncherIcon("icon.png");
Is this allowed to be a remote resource?

[asargent_no_longer_on_chrome, 2012-12-03 18:17:08]

I get the impression it may be a while before we can support this on Windows and
Mac. Given that, would it make sense to keep it experimental / dev-channel only
/ behind a flag or something?

[stevenjb, 2012-12-03 18:22:31]

On 2012/12/03 18:17:08, Antony Sargent wrote:
> I get the impression it may be a while before we can support this on Windows
and
> Mac. Given that, would it make sense to keep it experimental / dev-channel
only
> / behind a flag or something?

I would be fine with that. What would be the easiest way to do that?

[stevenjb, 2012-12-03 19:02:41]

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:16:
namespace SetLauncherIcon = api::app_current_window_internal::SetLauncherIcon;
On 2012/12/03 00:09:34, jeremya wrote:
> Why move these inside 'namespace extensions'?

It seemed simpler since they all have extensions:: in their namespace. I don't
think it especially matters, can move it back if you prefer.

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
File chrome/browser/extensions/api/app_window/app_window_apitest.cc (right):

https://codereview.chromium.org/11316292/diff/2002/chrome/browser/extensions/...
chrome/browser/extensions/api/app_window/app_window_apitest.cc:86:
EXPECT_FALSE(shell_window->launcher_icon_url().is_empty());
On 2012/12/03 00:09:34, jeremya wrote:
> Perhaps we could have a slightly bolder assertion that the icon URL is
actually
> what was set?

That introduces a dependency on the .js in the test, but I guess we already have
that expecting "IconSet". Will change,

https://codereview.chromium.org/11316292/diff/2002/chrome/test/data/extension...
File
chrome/test/data/extensions/platform_apps/windows_api_set_launcher_icon/background.js
(right):

https://codereview.chromium.org/11316292/diff/2002/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/windows_api_set_launcher_icon/background.js:7:
win.setLauncherIcon("icon.png");
On 2012/12/03 00:09:34, jeremya wrote:
> Is this allowed to be a remote resource?
No, a remote resource would have to be fetched with XMLHttpRequest() then passed
with window.webkitURL.createObjectURL(). (This is already enforced by Chrome).

[stevenjb, 2012-12-03 22:33:59]

+sky@/ben@ (Specifically for chrome/browser/ui/ash/launcher/ OWNER, but also for
API change in general)

PTAL

[Ben Goodger (Google), 2012-12-04 00:03:37]

lgtm

https://codereview.chromium.org/11316292/diff/3033/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.h (right):

https://codereview.chromium.org/11316292/diff/3033/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.h:43: virtual
void OnShellWindowIconUpdated(ShellWindow* shell_window) OVERRIDE;
2 cents: we have a stronger tradition of using the term "Changed" for this type
of thing in Chrome vs. "Updated", but ignore this feedback if the ShellWindow
stuff already uses Updated in other places.

[stevenjb, 2012-12-04 04:31:02]

https://codereview.chromium.org/11316292/diff/3033/chrome/browser/ui/ash/laun...
File chrome/browser/ui/ash/launcher/shell_window_launcher_controller.h (right):

https://codereview.chromium.org/11316292/diff/3033/chrome/browser/ui/ash/laun...
chrome/browser/ui/ash/launcher/shell_window_launcher_controller.h:43: virtual
void OnShellWindowIconUpdated(ShellWindow* shell_window) OVERRIDE;
On 2012/12/04 00:03:37, Ben Goodger (Google) wrote:
> 2 cents: we have a stronger tradition of using the term "Changed" for this
type
> of thing in Chrome vs. "Updated", but ignore this feedback if the ShellWindow
> stuff already uses Updated in other places.
Makes sense. Done.

[stevenjb, 2012-12-04 23:03:56]

Note extensions test changes from patch 6 to 7. I added an
ExperimentalPlatformAppBrowserTest class for testing experimental APIs.

[stevenjb, 2012-12-04 23:05:46]

Updated the issue sublect.

P.S. I will await feedback / lg from someone on the apps team before I commit
this.

[jeremya, 2012-12-04 23:56:33]

lgtm

https://codereview.chromium.org/11316292/diff/7023/chrome/common/extensions/a...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/11316292/diff/7023/chrome/common/extensions/a...
chrome/common/extensions/api/app_window.idl:124: // Set the app icon.
Can you document that this is a) experimental and b) only works on Ash?

Or possibly better (or as well), mark it [nodoc].

[asargent_no_longer_on_chrome, 2012-12-05 00:30:53]

lgtm

[stevenjb, 2012-12-05 00:56:16]

https://codereview.chromium.org/11316292/diff/7023/chrome/common/extensions/a...
File chrome/common/extensions/api/app_window.idl (right):

https://codereview.chromium.org/11316292/diff/7023/chrome/common/extensions/a...
chrome/common/extensions/api/app_window.idl:124: // Set the app icon.
On 2012/12/04 23:56:34, jeremya wrote:
> Can you document that this is a) experimental and b) only works on Ash?
> 
> Or possibly better (or as well), mark it [nodoc].

Added experimental, better comment, TODO, and [nodoc]

[commit-bot: I haz the power, 2012-12-05 00:57:00]

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

[commit-bot: I haz the power, 2012-12-05 05:25:28]

Change committed as 171163