First part of remembering platform app window geometry.

First part of remembering platform app window geometry.
This adds the infastructure necesary to remember and restore window sizes and positions, but doesn't include the platform-specific bits to actually remember window geometry yet.

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

[Marijn Kruisselbrink, 2012-08-27 18:11:22]

The platform neutral parts of restoring window geometry, I'll upload separate
CL's for the GTK and Mac specific parts.

[asargent_no_longer_on_chrome, 2012-08-28 18:49:41]

+cc jeremya since he's probably interested to see these changes

Looks great overall, just a few nit picky things. Also, we'll need to get
someone from the chrome/browser/ui/extensions/OWNERS file to review the changes
in there - probably mihaip is a good choice.


Overall note: make sure to follow the following rule from the style guide (I
tried to point out most instances where I saw them)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

"Comments should usually be written as complete sentences with proper
capitalization and periods at the end. Shorter comments, such as comments at the
end of a line of code, can sometimes be less formal, but you should be
consistent with your style. Complete sentences are more readable, and they
provide some assurance that the comment is complete and not an unfinished
thought."

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/app_window/app_window_api.cc:63:
create_params.window_key = *options->id;
nit: can you enforce that the id is some reasonable length here? (maybe 256 or
less?)

BTW, we will eventually have "automagic" support for declaring this in the .idl
file and having it be enforced by the bindings: crbug.com/131617

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/shell_window_geometry_cache.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.cc:91: // Not in the map
means loading data for the extension didn't finish yet
nit: end sentence with a period. 

Also, it could be not found in the map because we simply haven't opened any
windows for this app before, right?

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.cc:148:
std::map<std::string, gfx::Rect>& geometry = cache_[extension_id];
It might be good to see if cache_ already contains an entry for extension_id. If
it does, then that means that the app started up quickly and has already set a
window geometry that we don't want to clobber with the value coming out of the
database.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/shell_window_geometry_cache.h (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.h:28: // geometry change
nit: finish paragraph with a period.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.h:37: void
SaveWindowGeometry(const std::string& extension_id,
optional suggestion: "SaveGeometry" and "GetGeometry" might be more concise
function names, since "Window" is already in the class name.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:34: using
ShellWindowGeometryCache::OnExtensionUnloaded;
As mentioned in person, the rest of chrome code uses the pattern of adding a
friend declaration in the parent class for this purpose, so even though this is
more concise we should follow the general convention.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/shell_window.h (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
chrome/browser/ui/extensions/shell_window.h:55: // Specify if bounds should be
restored from a previous time
nit: period at end of sentence

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
chrome/browser/ui/extensions/shell_window.h:158: // Read window geometry from
storage
nit: period at end of sentence

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
chrome/browser/ui/extensions/shell_window.h:167: // used for saving/restoring
window geometry
nit: make full sentence, with period at end

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/common/extensio...
chrome/common/extensions/api/app_window.idl:23: 
We should add a comment somewhere explaining to developers the difference
between width/height and defaultWidth/defaultHeight (similarly for left/top and
defaultLeft/defaultTop). Is it an error to specify both a regular and default
value? etc.

You can add this comment either above line 8 where we start the definition of
hte CreateWindowOptions dictionary, or later in the file above the declaration
of the |create| function.

[Marijn Kruisselbrink, 2012-08-28 22:14:57]

Hopefully fixed all your comments. Also I seem to need an OWNER for chrome/
because I added a new unit test class?

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/app_window/app_window_api.cc:63:
create_params.window_key = *options->id;
On 2012/08/28 18:49:41, Antony Sargent wrote:
> nit: can you enforce that the id is some reasonable length here? (maybe 256 or
> less?)
> 
> BTW, we will eventually have "automagic" support for declaring this in the
.idl
> file and having it be enforced by the bindings: crbug.com/131617

Done.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/shell_window_geometry_cache.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.cc:91: // Not in the map
means loading data for the extension didn't finish yet
On 2012/08/28 18:49:41, Antony Sargent wrote:
> nit: end sentence with a period. 
> 
> Also, it could be not found in the map because we simply haven't opened any
> windows for this app before, right?
Currently GeometryFromStorage creates an entry in the cache_ map for each
extension even if there are no windows for it, maybe that doesn't make sense
though.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.cc:148:
std::map<std::string, gfx::Rect>& geometry = cache_[extension_id];
On 2012/08/28 18:49:41, Antony Sargent wrote:
> It might be good to see if cache_ already contains an entry for extension_id.
If
> it does, then that means that the app started up quickly and has already set a
> window geometry that we don't want to clobber with the value coming out of the
> database.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/shell_window_geometry_cache.h (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.h:28: // geometry change
On 2012/08/28 18:49:41, Antony Sargent wrote:
> nit: finish paragraph with a period.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache.h:37: void
SaveWindowGeometry(const std::string& extension_id,
On 2012/08/28 18:49:41, Antony Sargent wrote:
> optional suggestion: "SaveGeometry" and "GetGeometry" might be more concise
> function names, since "Window" is already in the class name. 
> 

Done.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/shell_window_geometry_cache_unittest.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/extensi...
chrome/browser/extensions/shell_window_geometry_cache_unittest.cc:34: using
ShellWindowGeometryCache::OnExtensionUnloaded;
On 2012/08/28 18:49:41, Antony Sargent wrote:
> As mentioned in person, the rest of chrome code uses the pattern of adding a
> friend declaration in the parent class for this purpose, so even though this
is
> more concise we should follow the general convention.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
File chrome/browser/ui/extensions/shell_window.h (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
chrome/browser/ui/extensions/shell_window.h:55: // Specify if bounds should be
restored from a previous time
On 2012/08/28 18:49:41, Antony Sargent wrote:
> nit: period at end of sentence

Done.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
chrome/browser/ui/extensions/shell_window.h:158: // Read window geometry from
storage
On 2012/08/28 18:49:41, Antony Sargent wrote:
> nit: period at end of sentence
oops, that entire method here is just a leftover from an earlier attempt

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/browser/ui/exte...
chrome/browser/ui/extensions/shell_window.h:167: // used for saving/restoring
window geometry
On 2012/08/28 18:49:41, Antony Sargent wrote:
> nit: make full sentence, with period at end

Done.

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/common/extensio...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10871086/diff/1/chrome/common/extensio...
chrome/common/extensions/api/app_window.idl:23: 
On 2012/08/28 18:49:41, Antony Sargent wrote:
> We should add a comment somewhere explaining to developers the difference
> between width/height and defaultWidth/defaultHeight (similarly for left/top
and
> defaultLeft/defaultTop). Is it an error to specify both a regular and default
> value? etc.
> 
> You can add this comment either above line 8 where we start the definition of
> hte CreateWindowOptions dictionary, or later in the file above the declaration
> of the |create| function. 
Okay, attempted to write some sort of explanation, not sure if it all makes
sense or is the best way to explain it all...
Also adding it above line 8 didn't make it show up in the generated docs at all
(also I'm not sure if I should be using html <p> tags for the paragraphs,
existing docs don't seem to, but then paragraphs get all merged together in the
generated html)

[asargent_no_longer_on_chrome, 2012-08-29 19:54:18]

LGTM w/ one more nit. 

For the chrome/ OWNERS reviewer, yeah, it's the change to the .gypi files. Maybe
add sky@chromium.org?

https://chromiumcodereview.appspot.com/10871086/diff/16002/chrome/browser/ext...
File chrome/browser/extensions/api/app_window/app_window_api.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/16002/chrome/browser/ext...
chrome/browser/extensions/api/app_window/app_window_api.cc:65:
create_params.window_key = options->id->substr(0, 256);
Instead of silently truncating, let's instead return return an error.

[Marijn Kruisselbrink, 2012-08-29 21:31:49]

mihai: can you review the chrome/browser/ui/extensions changes?

sky: can you review the chrome/chrome_tests.gypi changes?

[Mihai Parparita -not on Chrome, 2012-08-29 21:39:14]

LGTM

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:114: // restore geometry
Extraneous comment.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:412: if (!window_key_.empty()) {
Nit: turn this into an early return, so that the body is less indented.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.h (right):

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.h:57: bool restorePosition;
Nit: should be called restore_position and restore_size.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.h:91: virtual void
SaveWindowPosition();
Nit: Add an empty newline after this.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/common/exte...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/common/exte...
chrome/common/extensions/api/app_window.idl:61: // in which case a default size
and platform dependent position will be used.
Nit: some lines appear to be > 80 characters.

[Marijn Kruisselbrink, 2012-08-29 22:31:33]

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:114: // restore geometry
On 2012/08/29 21:39:14, Mihai Parparita wrote:
> Extraneous comment.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:412: if (!window_key_.empty()) {
On 2012/08/29 21:39:14, Mihai Parparita wrote:
> Nit: turn this into an early return, so that the body is less indented.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.h (right):

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.h:57: bool restorePosition;
On 2012/08/29 21:39:14, Mihai Parparita wrote:
> Nit: should be called restore_position and restore_size.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.h:91: virtual void
SaveWindowPosition();
On 2012/08/29 21:39:14, Mihai Parparita wrote:
> Nit: Add an empty newline after this.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/common/exte...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10871086/diff/10007/chrome/common/exte...
chrome/common/extensions/api/app_window.idl:61: // in which case a default size
and platform dependent position will be used.
On 2012/08/29 21:39:14, Mihai Parparita wrote:
> Nit: some lines appear to be > 80 characters.

Done.

[sky, 2012-08-30 00:03:18]

LGTM

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/chrome_brow...
File chrome/chrome_browser_extensions.gypi (right):

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/chrome_brow...
chrome/chrome_browser_extensions.gypi:548:
'browser/extensions/shell_window_geometry_cache.cc',
sort.

[jeremya, 2012-08-30 04:49:33]

lgtm

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:119: &cached_bounds)) {
nit: indentation

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:127:
native_window_->SetBounds(bounds);
Will this set the bounds after the window has already been shown? Perhaps we
should fetch this before creating the window and update the create params with
the restored bounds.

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:416: shell_window_geometry_cache();
nit: 4 spaces here too, i believe

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/common/exte...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/common/exte...
chrome/common/extensions/api/app_window.idl:9: // Id to identify the window.
Perhaps this comment should be expanded a little? This is the first thing
visible on the docs (http://developer.chrome.com/apps/app.window.html)

[Marijn Kruisselbrink, 2012-08-30 17:15:05]

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:119: &cached_bounds)) {
On 2012/08/30 04:49:33, jeremya wrote:
> nit: indentation

Done.

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:127:
native_window_->SetBounds(bounds);
On 2012/08/30 04:49:33, jeremya wrote:
> Will this set the bounds after the window has already been shown? Perhaps we
> should fetch this before creating the window and update the create params with
> the restored bounds.
In theory I guess this could indeed result in the window moving after it has
been shown (although I haven't noticed such behavior). There is a problem with
just passing updated bounds to NativeShellWindow::Create though: most
NativeShellWindow implementations interpret bounds passed in the create params
differently from those passed to SetBounds (which is why if you look at the OSX
specific bits, I even added a call to SetBounds in the case where there are no
coordinates to be restored
(https://chromiumcodereview.appspot.com/10893020/diff/1/chrome/browser/ui/exte....
I suppose these inconsistency should probably be fixed, but that requires to
first know what the expected behavior is (with regard to size&position including
or not including potential window chrome).

If updating the create params would be the way to go though, would it be better
to pass
params by value to ShellWindow::Init, or make a copy inside the method (I
suspect the second way is the preferred method).

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:416: shell_window_geometry_cache();
On 2012/08/30 04:49:33, jeremya wrote:
> nit: 4 spaces here too, i believe
Yeah, I wasn't sure about indentation here. Is that 4 spaces as in 2 more, or 2
less?

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/chrome_brow...
File chrome/chrome_browser_extensions.gypi (right):

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/chrome_brow...
chrome/chrome_browser_extensions.gypi:548:
'browser/extensions/shell_window_geometry_cache.cc',
On 2012/08/30 00:03:19, sky wrote:
> sort.

Done.

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/common/exte...
File chrome/common/extensions/api/app_window.idl (right):

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/common/exte...
chrome/common/extensions/api/app_window.idl:9: // Id to identify the window.
On 2012/08/30 04:49:33, jeremya wrote:
> Perhaps this comment should be expanded a little? This is the first thing
> visible on the docs (http://developer.chrome.com/apps/app.window.html)
Yeah, I tried to add the comment below for the create() method to actually apply
to the CreateWindowOptions dictionary, but comments there seem to be ignored by
the docs generator. Expanded this comment at least a little bit now.

[jeremya, 2012-08-31 04:36:11]

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
File chrome/browser/ui/extensions/shell_window.cc (right):

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:127:
native_window_->SetBounds(bounds);
On 2012/08/30 17:15:05, Marijn Kruisselbrink wrote:
> On 2012/08/30 04:49:33, jeremya wrote:
> > Will this set the bounds after the window has already been shown? Perhaps we
> > should fetch this before creating the window and update the create params
with
> > the restored bounds.
> In theory I guess this could indeed result in the window moving after it has
> been shown (although I haven't noticed such behavior). There is a problem with
> just passing updated bounds to NativeShellWindow::Create though: most
> NativeShellWindow implementations interpret bounds passed in the create params
> differently from those passed to SetBounds (which is why if you look at the
OSX
> specific bits, I even added a call to SetBounds in the case where there are no
> coordinates to be restored

Yes, this should be fixed :) (please add a TODO here so we don't forget).

The desired behaviour for chrome.app.window.create() w.r.t. the dimensions is
that the width/height passed in should refer to the innerWidth/innerHeight of
the created window. And obviously if you create a window and then set its bounds
to exactly the size you told it to be in create(), it shouldn't change size :)

Non-frameless windows will be going away soon (in favour of HTML titlebars, see
http://codereview.chromium.org/10896032/).

>
(https://chromiumcodereview.appspot.com/10893020/diff/1/chrome/browser/ui/exte....
> I suppose these inconsistency should probably be fixed, but that requires to
> first know what the expected behavior is (with regard to size&position
including
> or not including potential window chrome).
> 
> If updating the create params would be the way to go though, would it be
better
> to pass
> params by value to ShellWindow::Init, or make a copy inside the method (I
> suspect the second way is the preferred method).

Hm, all the options seem a little bit gross here. I think the grossness comes
from the fact that we use CreateParams to get data from the extension API call
into ShellWindow::Init as well as to pass data into the NativeShellWindow
constructor.

Once the chrome-style frame disappears, the only things the native windows will
need access to from the CreateParams are the initial bounds and the min/max
size. Perhaps we could just pass in the initial bounds to the NativeShellWindow
constructor, then set the min/max size with a method?

https://chromiumcodereview.appspot.com/10871086/diff/18001/chrome/browser/ui/...
chrome/browser/ui/extensions/shell_window.cc:416: shell_window_geometry_cache();
On 2012/08/30 17:15:05, Marijn Kruisselbrink wrote:
> On 2012/08/30 04:49:33, jeremya wrote:
> > nit: 4 spaces here too, i believe
> Yeah, I wasn't sure about indentation here. Is that 4 spaces as in 2 more, or
2
> less?

I meant 2 more :)

[commit-bot: I haz the power, 2012-08-31 15:08:21]

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

[commit-bot: I haz the power, 2012-08-31 17:16:58]

Change committed as 154451