Aura: Support hovering restore & close buttons for full screen apps

Aura: Support hovering restore & close buttons for full screen apps.
BUG=None
TEST=None

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

[sky, 2012-02-09 01:04:17]

Mostly just nits.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:107: TYPE_APP = 4
Did you talk with the extension guys to make sure they want a new type like
this? Also, if we want to restore these session restore is going to need to be
updated.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc (right):

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:29: const int
kHoverFadeDurationMs = 250;
This seems like a long time.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:42: separator_ =
rb.GetBitmapNamed(IDR_AURA_WINDOW_FULLSCREEN_SEPARATOR);
Isn't GetBitmapNamed deprecated?

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:67:
views::Button* sender, const views::Event& event) OVERRIDE {
nit: each param on its own line.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:75: // Gets an
image representing 3 bitmaps laied out horizontally that will be
laied -> laid

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:85:
all_images->extractSubset(
This seems tedious. Is there a reason we're going with three images in a single
image here? We typically don't do this for buttons.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:105: const
SkBitmap* separator_;
Generally we prefer values for SkBitmap and not pointers.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:123: bool ret =
top_margin.Contains(screen_point) ||
Since you don't use this bool, just return the result.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:214: if
(animation == show_animation_.get()) {
Did you try using the layer animation code? In theory it should be easier than
this.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:239:
control_widget_->Close();
I suspect explicitly closing the widget here and in the destructor isn't
necessary (because it's a child), but this should be fine.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h (right):

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:28: BrowserFrame*
frame, BrowserView* browser_view);
no explicit

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:43: OVERRIDE;
Similar to const, don't leave OVERRIDE on a line by itself.

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:59: // Restore
the app window (will result in switching the frame_view back).
nit: newline between 58 and 59

http://codereview.chromium.org/9359022/diff/5001/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:68: ControlView*
control_view_;
Add descriptions for these.

[DaveMoore, 2012-02-10 23:05:55]

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/br...
File chrome/browser/ui/browser.h (right):

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/br...
chrome/browser/ui/browser.h:107: TYPE_APP = 4
I've added it to session restore
On 2012/02/09 01:04:17, sky wrote:
> Did you talk with the extension guys to make sure they want a new type like
> this? Also, if we want to restore these session restore is going to need to be
> updated.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:29: const int
kHoverFadeDurationMs = 250;
Cole sez 130ms.
On 2012/02/09 01:04:17, sky wrote:
> This seems like a long time.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:42: separator_ =
rb.GetBitmapNamed(IDR_AURA_WINDOW_FULLSCREEN_SEPARATOR);
On 2012/02/09 01:04:17, sky wrote:
> Isn't GetBitmapNamed deprecated?

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:67:
views::Button* sender, const views::Event& event) OVERRIDE {
On 2012/02/09 01:04:17, sky wrote:
> nit: each param on its own line.

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:75: // Gets an
image representing 3 bitmaps laied out horizontally that will be
On 2012/02/09 01:04:17, sky wrote:
> laied -> laid

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:85:
all_images->extractSubset(
I like this approach more. It reduces the number of files we manage and
guarantees that the images are self consistent. We do it in some of the other UX
as well (network strength, battery power).

On 2012/02/09 01:04:17, sky wrote:
> This seems tedious. Is there a reason we're going with three images in a
single
> image here? We typically don't do this for buttons.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:105: const
SkBitmap* separator_;
On 2012/02/09 01:04:17, sky wrote:
> Generally we prefer values for SkBitmap and not pointers.

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:123: bool ret =
top_margin.Contains(screen_point) ||
On 2012/02/09 01:04:17, sky wrote:
> Since you don't use this bool, just return the result.

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:214: if
(animation == show_animation_.get()) {
I didn't, but this didn't seem very difficult. Are you saying I should switch?
On 2012/02/09 01:04:17, sky wrote:
> Did you try using the layer animation code? In theory it should be easier than
> this.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h (right):

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:28: BrowserFrame*
frame, BrowserView* browser_view);
On 2012/02/09 01:04:17, sky wrote:
> no explicit

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:43: OVERRIDE;
On 2012/02/09 01:04:17, sky wrote:
> Similar to const, don't leave OVERRIDE on a line by itself.

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:59: // Restore
the app window (will result in switching the frame_view back).
On 2012/02/09 01:04:17, sky wrote:
> nit: newline between 58 and 59

Done.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:68: ControlView*
control_view_;
On 2012/02/09 01:04:17, sky wrote:
> Add descriptions for these.

Done.

[sky, 2012-02-13 16:01:32]

Can you add a test to make sure TYPE_APP is restored correctly too. A patch with
just that would be easier, but I'm fine with both in the same patch if you
prefer that.

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/5001/chrome/browser/ui/vi...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.cc:214: if
(animation == show_animation_.get()) {
On 2012/02/10 23:05:55, DaveMoore wrote:
> I didn't, but this didn't seem very difficult. Are you saying I should switch?
> On 2012/02/09 01:04:17, sky wrote:
> > Did you try using the layer animation code? In theory it should be easier
than
> > this.
> 

It should be cleaner, and hopefully at some point we can offload layer
animations to a separate thread.

https://chromiumcodereview.appspot.com/9359022/diff/16001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/16001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1386: type == Browser::TYPE_APP ||
nit: keep spacing consistent.

https://chromiumcodereview.appspot.com/9359022/diff/16001/chrome/browser/ui/v...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h (right):

https://chromiumcodereview.appspot.com/9359022/diff/16001/chrome/browser/ui/v...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:43: virtual
gfx::Rect GetBoundsForTabStrip(views::View* tabstrip) const
Similar to const, don't leave OVERRIDE on a line by itself. Here 47.

[DaveMoore, 2012-02-13 18:58:40]

- Added SessionServiceTest.RestoreApp test
- Changed to using ScopedLayerAnimationSettings

http://codereview.chromium.org/9359022/diff/16001/chrome/browser/sessions/ses...
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/9359022/diff/16001/chrome/browser/sessions/ses...
chrome/browser/sessions/session_service.cc:1386: type == Browser::TYPE_APP ||
On 2012/02/13 16:01:32, sky wrote:
> nit: keep spacing consistent.

Done.

http://codereview.chromium.org/9359022/diff/16001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h (right):

http://codereview.chromium.org/9359022/diff/16001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/app_non_client_frame_view_aura.h:43: virtual
gfx::Rect GetBoundsForTabStrip(views::View* tabstrip) const
On 2012/02/13 16:01:32, sky wrote:
> Similar to const, don't leave OVERRIDE on a line by itself. Here 47.

Done.

[sky, 2012-02-13 22:13:46]

LGTM

[DaveMoore, 2012-02-24 19:50:50]

I had a lot of failing tests so I changed the approach. It needs a new review.

I don't have a new type of TYPE_APP. Instead on chromeos I restore browsers of
TYPE_POPUP that have an app_name. That means I need to save and restore that
app_name.

I've also enabled tab_restore of closed app windows (on chromeos) and added a
test to confirm it.

[DaveMoore, 2012-02-24 20:51:36]

Adding Antony for the extension work.

[sky, 2012-02-24 20:56:04]

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_restore.cc:812: const std::string app_name) {
const std::string&

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:487: registrar_.Add(this,
chrome::NOTIFICATION_BROWSER_WINDOW_READY,
Can you add a comment that we can't use OPENED as is_app isn't set at the time
OPENED is invoked.

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1383: Browser* browser  = *i;
nit: "  " -> " "

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
File chrome/browser/sessions/session_service.h (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:125: Browser::Type type,
Use an enum for the boolean (can also be used in should_track_...

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:1050: bool return_show_state = !is_type_tabbed();
nit: how about just show_state. Initially when I saw 1054 I thought it was a
mistake.

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:1176: if (tab_restore_service && is_app())
Move this into it's own method with the ifdef so that we don't have to worry
about potentially invoking BrowserClosing twice. I'm thinking of something like:

if (ShouldNotifyTabRestoreServiceOnClose())
  tab_restore_service->Brow...

[asargent_no_longer_on_chrome, 2012-02-24 21:37:27]

lgtm for extensions

[DaveMoore, 2012-02-25 16:54:42]

I also modified tab_restore_service_browsertest.cc as the test was flaky.

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
File chrome/browser/sessions/session_restore.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_restore.cc:812: const std::string app_name) {
On 2012/02/24 20:56:04, sky wrote:
> const std::string&

Done.

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
File chrome/browser/sessions/session_service.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:487: registrar_.Add(this,
chrome::NOTIFICATION_BROWSER_WINDOW_READY,
On 2012/02/24 20:56:04, sky wrote:
> Can you add a comment that we can't use OPENED as is_app isn't set at the time
> OPENED is invoked.

Done.

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_service.cc:1383: Browser* browser  = *i;
On 2012/02/24 20:56:04, sky wrote:
> nit: "  " -> " "

Done.

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
File chrome/browser/sessions/session_service.h (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/sess...
chrome/browser/sessions/session_service.h:125: Browser::Type type,
On 2012/02/24 20:56:04, sky wrote:
> Use an enum for the boolean (can also be used in should_track_...

Done.

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/ui/b...
File chrome/browser/ui/browser.cc (right):

https://chromiumcodereview.appspot.com/9359022/diff/30001/chrome/browser/ui/b...
chrome/browser/ui/browser.cc:1050: bool return_show_state = !is_type_tabbed();
On 2012/02/24 20:56:04, sky wrote:
> nit: how about just show_state. Initially when I saw 1054 I thought it was a
> mistake.

Done.

[sky, 2012-02-27 16:02:22]

LGTM