Add the app.windows.getBounds method

Add the app.windows.getBounds method 

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

[jeremya, 2012-10-19 00:26:45]

http://codereview.chromium.org/11193049/diff/1/chrome/browser/ui/extensions/s...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/11193049/diff/1/chrome/browser/ui/extensions/s...
chrome/browser/ui/extensions/shell_window.cc:487: SendBoundsUpdate();
I don't think SaveWindowPosition is sufficient here. In Views, this only gets
called from SizeChanged, Close and ActivationChanged (but not when the window
moves).

http://codereview.chromium.org/11193049/diff/1/chrome/common/extensions/api/a...
File chrome/common/extensions/api/app_window.idl (right):

http://codereview.chromium.org/11193049/diff/1/chrome/common/extensions/api/a...
chrome/common/extensions/api/app_window.idl:59: long x;
as discussed in chat, left/top probably makes more sense here :)

http://codereview.chromium.org/11193049/diff/1/chrome/renderer/resources/exte...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

http://codereview.chromium.org/11193049/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:87:
chromeHidden.updateAppWindowBounds = function(info) {
Hm, I should probably use this instead of my bespoke solution for
OnAppWindowClosed.

http://codereview.chromium.org/11193049/diff/1/chrome/renderer/resources/exte...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:91: data.x =
info.x;
Perhaps this should be in data.bounds?

[asargent_no_longer_on_chrome, 2012-10-24 00:27:54]

Hey Jeremy, have another look. 

I'm still working on the other pieces for the bug, as well as a browser test,
but it might be worth getting this first piece in now.

[jeremya, 2012-10-24 04:18:24]

http://codereview.chromium.org/11193049/diff/10002/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/11193049/diff/10002/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:613:
shell_window_->SaveWindowPosition();
We might not want to save the window position every time the window moves (that
results in a write to disc). Perhaps both this and SaveWindowPosition could call
SendBoundsUpdate?

[asargent_no_longer_on_chrome, 2012-10-24 04:42:03]

On 2012/10/24 04:18:24, jeremya wrote:
>
http://codereview.chromium.org/11193049/diff/10002/chrome/browser/ui/views/ex...
> File chrome/browser/ui/views/extensions/shell_window_views.cc (right):
> 
>
http://codereview.chromium.org/11193049/diff/10002/chrome/browser/ui/views/ex...
> chrome/browser/ui/views/extensions/shell_window_views.cc:613:
> shell_window_->SaveWindowPosition();
> We might not want to save the window position every time the window moves
(that
> results in a write to disc). Perhaps both this and SaveWindowPosition could
call
> SendBoundsUpdate?

Every time we call ShellWindowGeometryCache::SaveGeometry it just starts/resets
a 1 second timer to do the write, so in practice the number of writes to disc
should be relatively small. Its probably still better to avoid unnecessary
writes though, so I'll make the change as you suggest.

[asargent_no_longer_on_chrome, 2012-10-31 00:19:31]

PTAL

[jeremya, 2012-10-31 00:32:46]

lgtm :)

http://codereview.chromium.org/11193049/diff/21001/chrome/common/extensions/a...
File chrome/common/extensions/api/app_window.idl (right):

http://codereview.chromium.org/11193049/diff/21001/chrome/common/extensions/a...
chrome/common/extensions/api/app_window.idl:131: // Returns the current <a
href="#type-AppWindow">AppWindow</a>.
I don't think this really makes clear the behaviour of current() -- perhaps
something like 'returns the AppWindow of the calling context'? Or maybe that's
too technical...

http://codereview.chromium.org/11193049/diff/21001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

http://codereview.chromium.org/11193049/diff/21001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:89: width:
bounds.width, height:bounds.height };
nit: missing space

[jeremya, 2012-10-31 03:12:21]

http://codereview.chromium.org/11193049/diff/21001/chrome/common/extensions/a...
File chrome/common/extensions/api/app_window.idl (right):

http://codereview.chromium.org/11193049/diff/21001/chrome/common/extensions/a...
chrome/common/extensions/api/app_window.idl:62: long height;
These should be optional. I think it makes sense to call eg.

win.setBounds({ width: 100, height: 100, left: 20 });

[asargent_no_longer_on_chrome, 2012-10-31 05:37:35]

http://codereview.chromium.org/11193049/diff/21001/chrome/common/extensions/a...
File chrome/common/extensions/api/app_window.idl (right):

http://codereview.chromium.org/11193049/diff/21001/chrome/common/extensions/a...
chrome/common/extensions/api/app_window.idl:62: long height;
On 2012/10/31 03:12:21, jeremya wrote:
> These should be optional. I think it makes sense to call eg.
> 
> win.setBounds({ width: 100, height: 100, left: 20 });

Done.

http://codereview.chromium.org/11193049/diff/21001/chrome/common/extensions/a...
chrome/common/extensions/api/app_window.idl:131: // Returns the current <a
href="#type-AppWindow">AppWindow</a>.
On 2012/10/31 00:32:46, jeremya wrote:
> I don't think this really makes clear the behaviour of current() -- perhaps
> something like 'returns the AppWindow of the calling context'? Or maybe that's
> too technical...

I tried rewording it to use similar language to the comments for the
chrome.extension.getViews and chrome.runtime.getBackgroundPage methods. Feel
free to change it if you can think of something better. :)

http://codereview.chromium.org/11193049/diff/21001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right):

http://codereview.chromium.org/11193049/diff/21001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/app_window_custom_bindings.js:89: width:
bounds.width, height:bounds.height };
On 2012/10/31 00:32:46, jeremya wrote:
> nit: missing space

Done.

[commit-bot: I haz the power, 2012-10-31 05:37:43]

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

[commit-bot: I haz the power, 2012-10-31 05:37:48]

Presubmit check for 11193049-31001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/views
    chrome/browser/ui/extensions

Presubmit checks took 2.4s to calculate.

[asargent_no_longer_on_chrome, 2012-10-31 05:44:00]

+sky for chrome/browser/ui/views/
+mihaip for chrome/browser/ui/extensions/

[asargent_no_longer_on_chrome, 2012-10-31 05:44:46]

+sky for chrome/browser/ui/views/

+mihaip for chrome/browser/ui/extensions/

[jeremya, 2012-10-31 05:47:31]

(hm, I should probably be in OWNERS for c/b/ui/extensions, since I wrote
most of 3 out of the 5 files there.)


On Wed, Oct 31, 2012 at 4:44 PM, <asargent@chromium.org> wrote:

> +sky for chrome/browser/ui/views/
>
> +mihaip for chrome/browser/ui/extensions/
>
>
>
https://chromiumcodereview.**appspot.com/11193049/<https://chromiumcodereview...
>

[sky, 2012-10-31 14:11:14]

LGTM - but do you want to notify when resized too?

[asargent_no_longer_on_chrome, 2012-10-31 16:46:09]

It seems like we already get notified about resizes via
ShellWindowViews::SaveWindowPlacement

On Wed, Oct 31, 2012 at 7:11 AM, <sky@chromium.org> wrote:

> LGTM - but do you want to notify when resized too?
>
>
https://chromiumcodereview.**appspot.com/11193049/<https://chromiumcodereview...
>

[sky, 2012-10-31 20:44:46]

Ah, ok. LGTM2

[Mihai Parparita -not on Chrome, 2012-11-02 20:05:14]

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:459: DictionaryValue* update = new
DictionaryValue();
Can you use the generated C++ class for Bounds in the IDL file here?

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:687: void 
ShellWindowViews::OnWidgetMove() {
Nit: extra space after void.

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:688:
shell_window_->SendBoundsUpdate();
Shouldn't this call It SaveWindowPlacement too?

In general, it seems awkward to have two (public) position-related methods in
ShellWindow that subclasses need to invoke. Can we just have a generic
OnWindowBoundsChanged that subclasses call, and then ShellWindow decides what to
do (save the persisted bounds, send them to the renderer, etc.)

[asargent_no_longer_on_chrome, 2012-11-02 20:55:50]

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:459: DictionaryValue* update = new
DictionaryValue();
On 2012/11/02 20:05:14, Mihai Parparita wrote:
> Can you use the generated C++ class for Bounds in the IDL file here?

Done.

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:687: void 
ShellWindowViews::OnWidgetMove() {
On 2012/11/02 20:05:14, Mihai Parparita wrote:
> Nit: extra space after void.

Done.

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:688:
shell_window_->SendBoundsUpdate();
On 2012/11/02 20:05:14, Mihai Parparita wrote:
> Shouldn't this call It SaveWindowPlacement too?

I had it that way initially, but in patch 3 of this review Jeremy was concerned
about doing lots of writes to disk (link: http://goo.gl/ALg4t). The
ShellWindowGeometryCache does wait for 1sec of quiescence before doing any
writes. 

> In general, it seems awkward to have two (public) position-related methods in
> ShellWindow that subclasses need to invoke. Can we just have a generic
> OnWindowBoundsChanged that subclasses call, and then ShellWindow decides what
to
> do (save the persisted bounds, send them to the renderer, etc.)

Yeah, that's sort of a consequence of the above. Maybe better the way I had it
in patch set 3?

[Mihai Parparita -not on Chrome, 2012-11-02 22:12:56]

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:688:
shell_window_->SendBoundsUpdate();
On 2012/11/02 20:55:50, Antony Sargent wrote:
> On 2012/11/02 20:05:14, Mihai Parparita wrote:
> > Shouldn't this call It SaveWindowPlacement too?
> 
> I had it that way initially, but in patch 3 of this review Jeremy was
concerned
> about doing lots of writes to disk (link: http://goo.gl/ALg4t). The
> ShellWindowGeometryCache does wait for 1sec of quiescence before doing any
> writes. 
> 
> > In general, it seems awkward to have two (public) position-related methods
in
> > ShellWindow that subclasses need to invoke. Can we just have a generic
> > OnWindowBoundsChanged that subclasses call, and then ShellWindow decides
what
> to
> > do (save the persisted bounds, send them to the renderer, etc.)
> 
> Yeah, that's sort of a consequence of the above. Maybe better the way I had it
> in patch set 3?

Yeah, I like that better (since it doesn't actually lead to any disk writes). I
would just suggest renaming SaveWindowPlacement to something a bit more generic,
and maybe making it private, with NativeShellWindow as a friend.

[asargent_no_longer_on_chrome, 2012-11-05 23:37:28]

-miahaip, +miket for chrome/browser/ui/extensions/ OWNER

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

http://codereview.chromium.org/11193049/diff/31001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:688:
shell_window_->SendBoundsUpdate();
On 2012/11/02 22:12:56, Mihai Parparita wrote:
> On 2012/11/02 20:55:50, Antony Sargent wrote:
> > On 2012/11/02 20:05:14, Mihai Parparita wrote:
> > > Shouldn't this call It SaveWindowPlacement too?
> > 
> > I had it that way initially, but in patch 3 of this review Jeremy was
> concerned
> > about doing lots of writes to disk (link: http://goo.gl/ALg4t). The
> > ShellWindowGeometryCache does wait for 1sec of quiescence before doing any
> > writes. 
> > 
> > > In general, it seems awkward to have two (public) position-related methods
> in
> > > ShellWindow that subclasses need to invoke. Can we just have a generic
> > > OnWindowBoundsChanged that subclasses call, and then ShellWindow decides
> what
> > to
> > > do (save the persisted bounds, send them to the renderer, etc.)
> > 
> > Yeah, that's sort of a consequence of the above. Maybe better the way I had
it
> > in patch set 3?
> 
> Yeah, I like that better (since it doesn't actually lead to any disk writes).
I
> would just suggest renaming SaveWindowPlacement to something a bit more
generic,
> and maybe making it private, with NativeShellWindow as a friend.

Ok, I went back to just having the 1 method, but left it called
"SaveWindowPosition" for now to avoid having to add more owners to this review,
and have filed crbug.com/159508 for renaming it to "OnNativeBoundsChanged" and
having it call SaveWindowPosition and SendBoundsUpdate. 

Also, while the geometry cache won't do writes more frequently than after 1 sec
of quiescence, I've also filed crbug.com/159509 to make sure it doesn't do any
redundant writes if it gets updates that already match what is cached.

[jeremya, 2012-11-05 23:59:52]

lgtm

[miket_OOO, 2012-11-06 01:45:31]

lgtm

[commit-bot: I haz the power, 2012-11-06 17:52:17]

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

[commit-bot: I haz the power, 2012-11-06 19:52:11]

Change committed as 166252