Add extension API to change window show state using chrome.windows.update().

Add extension API to change window show state using chrome.windows.update().

Valid states are normal, minimized and maximized. Actual behavior depends on OS and window type.

BUG=32345
TEST=ExtensionApiTest.UpdateWindowShowState


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

[jennb, 2011-11-18 01:04:53]

ben for BrowserWindow API changes.
asargent for extension API changes.

Thanks,
Jenn

[asargent_no_longer_on_chrome, 2011-11-18 18:52:23]

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_util.cc (right):

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_util.cc:63: std::string
ExtensionTabUtil::GetWindowShowStateText(const Browser*browser) {
nit: need a space after "const Browser*" here

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_apitest.cc (right):

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_apitest.cc:30:
DISABLED_UpdateWindowShowState
:(

It's a bummer to add a new test that starts out disabled.  Can we fix the test
to work in all platforms?

Actually, an interesting alternative for this might be to rewrite all these
tests (or at least the test of your new properties / params) using our
relatively new chrome/browser/extensions/extension_function_test_utils.h stuff -
see that file and the small number of new tests that we've written using it.

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:545:
EXTENSION_FUNCTION_VALIDATE(false);
nit: instead of  EXTENSION_FUNCTION_VALIDATE(false), it might be better to do
something like: EXTENSION_FUNCTION_ERROR("invalid value for state");

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:548: // Bounds information is
ignored if minimized or maximized state is requested.
Instead of ignoring, I wonder if it would make sense to consider it an error if
bounds information is passed along with setting state to minimized/maximized.
The benefit is that developers who miss reading the documentation about ignoring
bounds values would get an explicit error message instead of potentially being
surprised at the ignore behavior.

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:607: }
Same question here: would it be better to return an error if the developer made
a nonsensical request (active=true and state=minimized, or active=false and
state=maximimzed)?

[jennb, 2011-11-19 01:44:33]

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tab_util.cc (right):

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tab_util.cc:63: std::string
ExtensionTabUtil::GetWindowShowStateText(const Browser*browser) {
On 2011/11/18 18:52:24, Antony Sargent wrote:
> nit: need a space after "const Browser*" here

Done.

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_apitest.cc (right):

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_apitest.cc:30:
DISABLED_UpdateWindowShowState
On 2011/11/18 18:52:24, Antony Sargent wrote:
> :(
> 
> It's a bummer to add a new test that starts out disabled.  Can we fix the test
> to work in all platforms?
> 
> Actually, an interesting alternative for this might be to rewrite all these
> tests (or at least the test of your new properties / params) using our
> relatively new chrome/browser/extensions/extension_function_test_utils.h stuff
-
> see that file and the small number of new tests that we've written using it. 

I tried using the extension_function_test_utils.h stuff to invoke the state
changes, but even after a message loop RunAllPending, the state wasn't updated
on linux yet. Manual verification shows the window does change state. I ended up
leaving the existing test alone. On the bright side, I added more tests using
extension_function_test_utils.h stuff to check the invalid cases.

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:545:
EXTENSION_FUNCTION_VALIDATE(false);
On 2011/11/18 18:52:24, Antony Sargent wrote:
> nit: instead of  EXTENSION_FUNCTION_VALIDATE(false), it might be better to do
> something like: EXTENSION_FUNCTION_ERROR("invalid value for state");
> 
> 

Done.

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:548: // Bounds information is
ignored if minimized or maximized state is requested.
On 2011/11/18 18:52:24, Antony Sargent wrote:
> Instead of ignoring, I wonder if it would make sense to consider it an error
if
> bounds information is passed along with setting state to minimized/maximized.
> The benefit is that developers who miss reading the documentation about
ignoring
> bounds values would get an explicit error message instead of potentially being
> surprised at the ignore behavior.

Done.

http://codereview.chromium.org/8586045/diff/1001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_tabs_module.cc:607: }
On 2011/11/18 18:52:24, Antony Sargent wrote:
> Same question here: would it be better to return an error if the developer
made
> a nonsensical request (active=true and state=minimized, or active=false and
> state=maximimzed)? 

Done.

[Ben Goodger (Google), 2011-11-21 16:55:43]

BrowserWindow changes LGTM.

[asargent_no_longer_on_chrome, 2011-11-21 19:04:05]

lgtm

Can you create a bug for the linux issue if there isn't one already?

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

http://codereview.chromium.org/8586045/diff/13001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.cc:618:
EXTENSION_FUNCTION_ERROR(keys::kInvalidWindowStateError);
nit: EXTENSION_FUNCTION_ERROR sets bad_message_ = true, which ends up killing
the renderer process. The idea here is this should only be used for cases where
we got data that should have already been caught by the schema validation in the
renderer process (eg a missing param that isn't marked optional in
extension_api.json). For cases where we just want to return an error message but
not terminate the renderer, the right thing to do is:


error_ = keys::kInvalidWindowStateError;
return false;


(Also we have the ExtensionErrorUtils::FormatErrorMessage function for cases
where you want to have a paramaeterized error message)

[commit-bot: I haz the power, 2011-11-21 21:08:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8586045/23002

[commit-bot: I haz the power, 2011-11-21 21:08:15]

Can't apply patch for file chrome/browser/ui/cocoa/browser_window_cocoa.h.
While running patch -p1 --forward --force;
patching file chrome/browser/ui/cocoa/browser_window_cocoa.h
Hunk #1 FAILED at 55.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/cocoa/browser_window_cocoa.h.rej

[commit-bot: I haz the power, 2011-11-21 21:47:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8586045/28001

[commit-bot: I haz the power, 2011-11-21 22:44:57]

Try job failure for 8586045-28001 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2011-11-22 00:13:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jennb@chromium.org/8586045/28001

[commit-bot: I haz the power, 2011-11-22 01:31:12]

Change committed as 111071