Add a way to fetch window frame metrics from NativeShellWindow

Add a way to fetch window frame metrics from NativeShellWindow

This adds default implementations to the Cocoa and Gtk shell windows. The views shell window is currently the only class that implements these in a non-default way; the Cocoa and Gtk will get their own implementations in a follow-up patch.

This is needed to have the app window setBounds() and getBounds() APIs operate
on content bounds rather than window frame bounds.

R=ben@chromium.org,rsesek@chromium.org,estade@chromium.org

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

[Evan Stade, 2012-11-14 21:20:59]

https://codereview.chromium.org/11369237/diff/3001/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/11369237/diff/3001/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:717: return window_bounds;
please at least add a comment of some sort to make it clear this isn't a real
implementation. I don't understand from your CL description whether you intend
to ever implement this one, or just the ShellWindowGtk one. If you dont' plan to
implement this one, why not just return gfx::Rect() to make it clear it's wrong
(instead of making it almost right)?

[jeremya, 2012-11-14 23:35:59]

https://codereview.chromium.org/11369237/diff/3001/chrome/browser/ui/gtk/brow...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://codereview.chromium.org/11369237/diff/3001/chrome/browser/ui/gtk/brow...
chrome/browser/ui/gtk/browser_window_gtk.cc:717: return window_bounds;
On 2012/11/14 21:20:59, Evan Stade wrote:
> please at least add a comment of some sort to make it clear this isn't a real
> implementation. I don't understand from your CL description whether you intend
> to ever implement this one, or just the ShellWindowGtk one. If you dont' plan
to
> implement this one, why not just return gfx::Rect() to make it clear it's
wrong
> (instead of making it almost right)?

Done, made it NOTIMPLEMENTED().

[stevenjb, 2012-11-14 23:54:09]

https://codereview.chromium.org/11369237/diff/4001/chrome/browser/ui/base_win...
File chrome/browser/ui/base_window.h (right):

https://codereview.chromium.org/11369237/diff/4001/chrome/browser/ui/base_win...
chrome/browser/ui/base_window.h:91: const gfx::Rect& content_bounds) const = 0;
I was wondering if you had considered going with something like:

  virtual gfx::Insets GetFrameInsets() const = 0;

instead?

[jeremya, 2012-11-15 02:21:13]

http://codereview.chromium.org/11369237/diff/4001/chrome/browser/ui/base_wind...
File chrome/browser/ui/base_window.h (right):

http://codereview.chromium.org/11369237/diff/4001/chrome/browser/ui/base_wind...
chrome/browser/ui/base_window.h:91: const gfx::Rect& content_bounds) const = 0;
On 2012/11/14 23:54:10, stevenjb (chromium) wrote:
> I was wondering if you had considered going with something like:
> 
>   virtual gfx::Insets GetFrameInsets() const = 0;
> 
> instead?

I hadn't thought of that. That's nice because it means there's only one method
on BaseWindow, but it forces the callers to do a little more work.

Can I pretty please have a default implementation? :(

[jeremya, 2012-11-15 02:58:29]

Updated to use GetFrameInsets() and a default implementation.

[stevenjb, 2012-11-15 18:22:47]

https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/base_win...
File chrome/browser/ui/base_window.h (right):

https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/base_win...
chrome/browser/ui/base_window.h:86: }
If we have a default implementation, we need to ensure that none of the
inheriting classes derive from any non-interface classes (i.e. no multiple
inheritance). I believe this is true.

The implementation also needs to go in a .cc file, not in the header, and the
class needs a constructor and virtual destructor.

With those conditions, I have no problem with providing a default
implementation.

https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:413:
window_bounds.Inset(-GetFrameInsets());
Even though this is more verbose, I think it is more readable.

https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/shell_window_views.cc:580: return
gfx::Insets(kCaptionHeight, 0, 0, 0);
And this turned out much easier to read, so I think this is a win.

[jeremya, 2012-11-16 00:12:05]

https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/base_win...
File chrome/browser/ui/base_window.h (right):

https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/base_win...
chrome/browser/ui/base_window.h:86: }
On 2012/11/15 18:22:47, stevenjb (chromium) wrote:
> If we have a default implementation, we need to ensure that none of the
> inheriting classes derive from any non-interface classes (i.e. no multiple
> inheritance). I believe this is true.

It isn't quite.
ShellWindowViews < views::WidgetDelegateView (non-interface)
ShellWindowViews < NativeShellWindow (interface) < BaseWindow

so if BaseWindow were non-interface, SWV would inherit both it and views::WDV.

Thoughts?

[stevenjb, 2012-11-16 00:34:58]

On 2012/11/16 00:12:05, jeremya wrote:
>
https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/base_win...
> File chrome/browser/ui/base_window.h (right):
> 
>
https://codereview.chromium.org/11369237/diff/7001/chrome/browser/ui/base_win...
> chrome/browser/ui/base_window.h:86: }
> On 2012/11/15 18:22:47, stevenjb (chromium) wrote:
> > If we have a default implementation, we need to ensure that none of the
> > inheriting classes derive from any non-interface classes (i.e. no multiple
> > inheritance). I believe this is true.
> 
> It isn't quite.
> ShellWindowViews < views::WidgetDelegateView (non-interface)
> ShellWindowViews < NativeShellWindow (interface) < BaseWindow
> 
> so if BaseWindow were non-interface, SWV would inherit both it and views::WDV.
> 
> Thoughts?

Ah. Hmm.

I know we make a few exceptions to the multiple inheritance rule in the UI, but
I think we should try to avoid it.

My personal preference would be to put the trivial implementation in each
inheritor, since it is only the one method where a default would be convenient,
but Scott or Ben may wish to weigh in here.

sky@, ben@, thoughts?

[jeremya, 2012-11-19 04:33:39]

I removed the default implementation and implemented it in every descendant.

[stevenjb, 2012-11-19 17:36:37]

Thanks. Tedious I know, but better here Hi think.
LGTM

[Evan Stade, 2012-11-19 19:30:42]

FWIW, I don't think that it stops being an interface just because you have a
dummy implementation that does nothing or returns an empty value. That is,
multiple inheritance of these kinds of interfaces would be allowed. That said,
it's nice to keep this interface pure-virtual only.

[jeremya, 2012-11-19 23:35:56]

Ping sky@ for OWNERS rubber stamp :)

[sky, 2012-11-20 00:14:55]

Is there a reason this needs to be in BaseWindow and not some where closer to
app windows, say NativeShellWindow?

[jeremya, 2012-11-20 04:50:59]

On 2012/11/20 00:14:55, sky wrote:
> Is there a reason this needs to be in BaseWindow and not some where closer to
> app windows, say NativeShellWindow?

Done. (ShellWindow::GetBaseWindow() now returns a NativeShellWindow*, which is
also a BaseWindow* so existing callsites are unaffected barring an extra
#include.)

[sky, 2012-11-20 16:38:50]

LGTM

[stevenjb, 2012-11-20 18:34:07]

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

https://codereview.chromium.org/11369237/diff/9003/chrome/browser/extensions/...
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:7:
#include "chrome/browser/ui/extensions/native_shell_window.h"
This shouldn't be necessary, should it?

https://codereview.chromium.org/11369237/diff/9003/chrome/browser/extensions/...
File chrome/browser/extensions/shell_window_registry.cc (right):

https://codereview.chromium.org/11369237/diff/9003/chrome/browser/extensions/...
chrome/browser/extensions/shell_window_registry.cc:7: #include
"chrome/browser/ui/extensions/native_shell_window.h"
Unnecessary?

[jeremya, 2012-11-20 22:08:43]

On 2012/11/20 18:34:07, stevenjb (chromium) wrote:
>
https://codereview.chromium.org/11369237/diff/9003/chrome/browser/extensions/...
> File
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc
> (right):
> 
>
https://codereview.chromium.org/11369237/diff/9003/chrome/browser/extensions/...
>
chrome/browser/extensions/api/app_current_window_internal/app_current_window_internal_api.cc:7:
> #include "chrome/browser/ui/extensions/native_shell_window.h"
> This shouldn't be necessary, should it?
> 
>
https://codereview.chromium.org/11369237/diff/9003/chrome/browser/extensions/...
> File chrome/browser/extensions/shell_window_registry.cc (right):
> 
>
https://codereview.chromium.org/11369237/diff/9003/chrome/browser/extensions/...
> chrome/browser/extensions/shell_window_registry.cc:7: #include
> "chrome/browser/ui/extensions/native_shell_window.h"
> Unnecessary?

Without those #includes, it's not known than NativeShellWindow derives from
BaseWindow.

[stevenjb, 2012-11-21 00:43:35]

Right, of course, nevermind. Sorry about that.
LGTM

[commit-bot: I haz the power, 2012-11-21 00:46:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/9003

[commit-bot: I haz the power, 2012-11-21 01:11:45]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[jeremya, 2012-11-22 00:06:55]

My previous patch didn't account for ash, where the size of the frame is
different to Windows. I've updated the code to use a different calculation for
frame inset size, PTAL :)

[jeremya, 2012-11-22 00:07:22]

(in shell_window_views.cc)

[stevenjb, 2012-11-27 01:12:47]

https://codereview.chromium.org/11369237/diff/22030/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

https://codereview.chromium.org/11369237/diff/22030/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/shell_window_views.cc:580: gfx::Rect
client_bounds = gfx::Rect(100, 100);
I take it 100 x 100 is arbitrary? I assume it just has to be larger than the max
inset size? We should add a comment here stating as such and/or use constants.

[jeremya, 2012-11-27 02:50:16]

https://codereview.chromium.org/11369237/diff/22030/chrome/browser/ui/views/e...
File chrome/browser/ui/views/extensions/shell_window_views.cc (right):

https://codereview.chromium.org/11369237/diff/22030/chrome/browser/ui/views/e...
chrome/browser/ui/views/extensions/shell_window_views.cc:580: gfx::Rect
client_bounds = gfx::Rect(100, 100);
On 2012/11/27 01:12:47, stevenjb (chromium) wrote:
> I take it 100 x 100 is arbitrary? I assume it just has to be larger than the
max
> inset size? We should add a comment here stating as such and/or use constants.

it just needs to be large enough that GetWindowBoundsForClientBounds isn't going
to decide that it's too small and return something larger than the real insets,
in trying to enforce that the window size be large enough for the frame controls
to fit. Perhaps it should actually be (1000,1000).

I've upped the size and added a comment.

[stevenjb, 2012-11-28 00:03:06]

Thanks, LGTM

[sky, 2012-11-28 00:10:23]

SLGTM

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/9026

[commit-bot: I haz the power, 2012-11-28 01:23:31]

Failed to trigger a try job on ios_dbg_simulator
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2012-11-28 01:23:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/15025

[commit-bot: I haz the power, 2012-11-28 01:57:05]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-28 02:18:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/24014

[commit-bot: I haz the power, 2012-11-28 04:56:31]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-28 06:28:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/24014

[commit-bot: I haz the power, 2012-11-28 08:41:38]

Retried try job too often on linux_chromeos for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-28 23:57:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/16026

[commit-bot: I haz the power, 2012-11-29 00:43:32]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-29 00:44:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/16026

[commit-bot: I haz the power, 2012-11-30 04:02:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/16026

[commit-bot: I haz the power, 2012-11-30 04:02:31]

Failed to apply patch for
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
  Hunk #1 FAILED at 110.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h.rej

Patch:       chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
Index: chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
diff --git a/chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
b/chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
index
dd9a08ac34354cbe96022f471eaf23dc02db8c6b..bd241f76a36901fd43809e79a611d63ef4bbe80d
100644
--- a/chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
+++ b/chrome/browser/ui/cocoa/extensions/shell_window_cocoa.h
@@ -110,6 +110,7 @@ class ShellWindowCocoa : public NativeShellWindow {
   virtual void HandleKeyboardEvent(
       const content::NativeWebKeyboardEvent& event) OVERRIDE;
   virtual void RenderViewHostChanged() OVERRIDE {}
+  virtual gfx::Insets GetFrameInsets() const OVERRIDE;
 
  private:
   virtual ~ShellWindowCocoa();

[commit-bot: I haz the power, 2012-11-30 05:46:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/22051

[commit-bot: I haz the power, 2012-11-30 06:03:28]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-03 03:37:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/18031

[commit-bot: I haz the power, 2012-12-03 03:37:38]

Failed to apply patch for chrome/browser/ui/extensions/shell_window.h:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/extensions/shell_window.h
  Hunk #1 FAILED at 54.
  Hunk #2 succeeded at 96 (offset 8 lines).
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/ui/extensions/shell_window.h.rej

Patch:       chrome/browser/ui/extensions/shell_window.h
Index: chrome/browser/ui/extensions/shell_window.h
diff --git a/chrome/browser/ui/extensions/shell_window.h
b/chrome/browser/ui/extensions/shell_window.h
index
0a7a43ffe80681434da1d7e5af5a82a103abd914..db1db331a9a6306bb960f07e6a9817bf6ffc6b5f
100644
--- a/chrome/browser/ui/extensions/shell_window.h
+++ b/chrome/browser/ui/extensions/shell_window.h
@@ -54,9 +54,9 @@ class ShellWindow : public content::NotificationObserver,
     ~CreateParams();
 
     Frame frame;
-    // Specify the initial bounds of the window. INT_MIN designates
-    // 'unspecified' for any coordinate, and should be replaced with a default
-    // value.
+    // Specify the initial content bounds of the window (excluding any window
+    // decorations). INT_MIN designates 'unspecified' for any coordinate, and
+    // should be replaced with a default value.
     gfx::Rect bounds;
 
     gfx::Size minimum_size;
@@ -88,10 +88,8 @@ class ShellWindow : public content::NotificationObserver,
   Profile* profile() const { return profile_; }
   const gfx::Image& app_icon() const { return app_icon_; }
 
-  BaseWindow* GetBaseWindow();
-  gfx::NativeWindow GetNativeWindow() {
-    return GetBaseWindow()->GetNativeWindow();
-  }
+  NativeAppWindow* GetBaseWindow();
+  gfx::NativeWindow GetNativeWindow();
 
   // NativeAppWindows should call this to determine what the window's title
   // is on startup and from within UpdateWindowTitle().

[commit-bot: I haz the power, 2012-12-03 04:53:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/24020

[commit-bot: I haz the power, 2012-12-03 04:55:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-03 05:31:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremya@chromium.org/11369237/24020

[commit-bot: I haz the power, 2012-12-03 08:33:56]

Change committed as 170713