Add setBounds method and browsertest for get/set bounds to app window API

Add setBounds method and browsertest for get/set bounds to app window API

Also fixed a bug where the window bounds were often not set after creation until the window was moved/resized.

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

[asargent_no_longer_on_chrome, 2012-11-02 00:30:14]

This still has the not-yet-committed changes from
http://codereview.chromium.org/11193049/, but the setBounds specific stuff and
browser test is ready for you to have a look.

[asargent_no_longer_on_chrome, 2012-11-02 00:31:19]

Oops, just realized I meant "Add *setBounds*..." not "Add getBounds..." for the
CL description.

[jeremya, 2012-11-02 00:47:45]

On 2012/11/02 00:31:19, Antony Sargent wrote:
> Oops, just realized I meant "Add *setBounds*..." not "Add getBounds..." for
the
> CL description.

BTW, you can give 'git cl upload' a commit to diff against if you don't want it
to include other changes :)

[jeremya, 2012-11-02 05:06:41]

http://codereview.chromium.org/11369039/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/app_window/app_window_apitest.cc (right):

http://codereview.chromium.org/11369039/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/app_window/app_window_apitest.cc:31: int slop =
10;
What other platforms are these functions inaccurate on? It'd be good to have
setBounds(b); assert(getBounds() == b);.

Perhaps add a TODO here to clarify or fix behaviour, and maybe file a bug?

http://codereview.chromium.org/11369039/diff/1/chrome/test/data/extensions/pl...
File chrome/test/data/extensions/platform_apps/windows_api_bounds/main.js
(right):

http://codereview.chromium.org/11369039/diff/1/chrome/test/data/extensions/pl...
chrome/test/data/extensions/platform_apps/windows_api_bounds/main.js:18:
setTimeout(checkBounds, 50);
Can you add a TODO here to rewrite when bounds changed events are implemented?

[asargent_no_longer_on_chrome, 2012-11-09 20:25:34]

Ok, new version up. Sorry for rebasing in middle of review, but should be easier
to see changes specific to this CL now. Also, note that I also included a fix
and test for the bounds being 0 until move/resize problem in this CL.

http://codereview.chromium.org/11369039/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/app_window/app_window_apitest.cc (right):

http://codereview.chromium.org/11369039/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/app_window/app_window_apitest.cc:31: int slop =
10;
On 2012/11/02 05:06:41, jeremya wrote:
> What other platforms are these functions inaccurate on? It'd be good to have
> setBounds(b); assert(getBounds() == b);.
> 
> Perhaps add a TODO here to clarify or fix behaviour, and maybe file a bug?

Done.

http://codereview.chromium.org/11369039/diff/1/chrome/test/data/extensions/pl...
File chrome/test/data/extensions/platform_apps/windows_api_bounds/main.js
(right):

http://codereview.chromium.org/11369039/diff/1/chrome/test/data/extensions/pl...
chrome/test/data/extensions/platform_apps/windows_api_bounds/main.js:18:
setTimeout(checkBounds, 50);
On 2012/11/02 05:06:41, jeremya wrote:
> Can you add a TODO here to rewrite when bounds changed events are implemented?

Done.

[jeremya, 2012-11-13 06:16:30]

Oops, sorry I missed this yesterday!

http://codereview.chromium.org/11369039/diff/3003/chrome/browser/extensions/a...
File
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
(right):

http://codereview.chromium.org/11369039/diff/3003/chrome/browser/extensions/a...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:103:
window->GetBaseWindow()->SetBounds(bounds);
I think this ought to set the *content* bounds, not the window bounds, but I can
look at that in a separate patch. (I'm doing some other cleanups in that area,
trying to standardize everything on content bounds in light of
http://code.google.com/p/chromium/issues/detail?id=155240#c14

http://codereview.chromium.org/11369039/diff/3003/chrome/browser/extensions/a...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

http://codereview.chromium.org/11369039/diff/3003/chrome/browser/extensions/a...
chrome/browser/extensions/api/app_window/app_window_api.cc:212:
result->Set("bounds", boundsValue);
This is kind of unrelated to this patch, but if it's easier to leave it in than
to split it out then I'm fine with it going in with the rest of this stuff.

http://codereview.chromium.org/11369039/diff/3003/chrome/renderer/resources/e...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

http://codereview.chromium.org/11369039/diff/3003/chrome/renderer/resources/e...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:120: //
updated value.
What happens if the bounds you set aren't the bounds that eventually the window
has? e.g. on OS X if you position a window at 0,0 it will be moved down below
the menu bar. I'm not sure this is the right behaviour.

http://codereview.chromium.org/11369039/diff/3003/chrome/test/data/extensions...
File chrome/test/data/extensions/platform_apps/windows_api_bounds/background.js
(right):

http://codereview.chromium.org/11369039/diff/3003/chrome/test/data/extensions...
chrome/test/data/extensions/platform_apps/windows_api_bounds/background.js:15:
window.close();
Why win.close() AND window.close()?

[asargent_no_longer_on_chrome, 2012-11-14 05:22:12]

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

https://codereview.chromium.org/11369039/diff/3003/chrome/browser/extensions/...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:103:
window->GetBaseWindow()->SetBounds(bounds);
On 2012/11/13 06:16:30, jeremya wrote:
> I think this ought to set the *content* bounds, not the window bounds, but I
can
> look at that in a separate patch. (I'm doing some other cleanups in that area,
> trying to standardize everything on content bounds in light of
> http://code.google.com/p/chromium/issues/detail?id=155240#c14

Ok.

BTW, if we're maintaining cached values of both the contentBounds and
frameBounds in the renderer, and we get an API call to change the contentBounds,
are we even able to immediately calculate the new frameBounds in the renderer
and write both through to the cache, or do we need to wait to get some
information back from the browser process to know the new frameBounds?

https://codereview.chromium.org/11369039/diff/3003/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

https://codereview.chromium.org/11369039/diff/3003/chrome/renderer/resources/...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:120: //
updated value.
On 2012/11/13 06:16:30, jeremya wrote:
> What happens if the bounds you set aren't the bounds that eventually the
window
> has? e.g. on OS X if you position a window at 0,0 it will be moved down below
> the menu bar. I'm not sure this is the right behaviour.

We won't know synchronously that this has happened though, right? If such a
repositioning happens, we should get our updateAppWindowBounds called via an IPC
from the browser process shortly thereafter, so we get a sort of 'eventual
consistency'. Are you suggesting we just not update our cache here? It seemed
like in comment 12 on the bug
(https://code.google.com/p/chromium/issues/detail?id=134070#c12) you stated that
calling setBounds followed immediately by getBounds should return the same
values we just set.

BTW, I also have a CL I'll put up for review shortly which implements
onBoundsChanged, onMinimized, onMaximized, and onRestored events.

https://codereview.chromium.org/11369039/diff/3003/chrome/test/data/extension...
File chrome/test/data/extensions/platform_apps/windows_api_bounds/background.js
(right):

https://codereview.chromium.org/11369039/diff/3003/chrome/test/data/extension...
chrome/test/data/extensions/platform_apps/windows_api_bounds/background.js:15:
window.close();
On 2012/11/13 06:16:30, jeremya wrote:
> Why win.close() AND window.close()?

This makes the test fail faster. If we just closed the app window, then the
background page wouldn't close until the transient background page timeout which
is something like 10 seconds.

Note that I'm not using the apitest.js stuff in this test because I need to pass
expected bounds information back to the app window's main.js via the
ExtensionTestMessageListener, so I can't just call chrome.test.fail() here.

[asargent_no_longer_on_chrome, 2012-11-17 06:20:49]

Ok, as discussed offline, I removed the caching of values from setBounds and
updated the test accordingly.

[jeremya, 2012-11-18 22:21:39]

lgtm

[commit-bot: I haz the power, 2012-11-22 03:55:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/11369039/18001

[commit-bot: I haz the power, 2012-11-22 03:55:13]

Failed to apply patch for
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
  Hunk #1 FAILED at 5.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc.rej

Patch:      
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
Index:
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
diff --git
a/chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
b/chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
index
5619b65ae38f145f31215d73e5a5ced08ba9ea43..e14bf9a7cda429077044bc1489fc94c42db4b9db
100644
---
a/chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
+++
b/chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
@@ -5,8 +5,12 @@
 #include
"chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
 #include "chrome/browser/ui/extensions/shell_window.h"
+#include "chrome/common/extensions/api/app_current_window_internal.h"
 #include "chrome/common/extensions/extension_error_utils.h"
 
+namespace SetBounds = extensions::api::app_current_window_internal::SetBounds;
+using extensions::api::app_current_window_internal::Bounds;
+
 namespace extensions {
 
 namespace {
@@ -80,4 +84,24 @@ bool AppCurrentWindowInternalHideFunction::RunWithWindow(
   return true;
 }
 
+bool AppCurrentWindowInternalSetBoundsFunction::RunWithWindow(
+    ShellWindow* window) {
+  // Start with the current bounds, and change any values that are specified in
+  // the incoming parameters.
+  gfx::Rect bounds = window->GetBaseWindow()->GetBounds();
+  scoped_ptr<SetBounds::Params> params(SetBounds::Params::Create(*args_));
+  CHECK(params.get());
+  if (params->bounds.left)
+    bounds.set_x(*(params->bounds.left));
+  if (params->bounds.top)
+    bounds.set_y(*(params->bounds.top));
+  if (params->bounds.width)
+    bounds.set_width(*(params->bounds.width));
+  if (params->bounds.height)
+    bounds.set_height(*(params->bounds.height));
+
+  window->GetBaseWindow()->SetBounds(bounds);
+  return true;
+}
+
 }  // namespace extensions

[commit-bot: I haz the power, 2012-11-24 22:09:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asargent@chromium.org/11369039/31001

[commit-bot: I haz the power, 2012-11-25 04:28:32]

Change committed as 169391