Implement fullscreen info bubble on Win and Mac

Implement fullscreen info bubble on Win and Mac.

When a site enters fullscreen a bubble should appear, both to give the user the option to exit fullscreen and to inform them that the site has entered fullscreen. This patch implements that behaviour on Windows and Mac.

XIB changes: (FullScreenExitBubble.xib)
* Added explanatory text field.
* Added "allow" and "deny" buttons.
* Changed from View into Window.


BUG=73923
TEST=

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

[koz (OOO until 15th September), 2011-08-26 09:25:26]

Hi guys,

This change adds an implementation of the M15 UI for notifying a user that the
site they are on has entered fullscreen mode.

jochen, could you please look at the content settings stuff?

kasting, could you please look at the browser and infobar stuff?

Please excuse any slight errors might be in here - I made some last minute
additions that I don't have time to check just at the moment.

Thanks!

[Peter Kasting, 2011-08-26 18:09:02]

I talked with Glen about this UI and what we're trying to do, as well as some of
my concerns with the existing proposal.  Here are the results of that
conversation.

I think instead of implementing this with infobars for M15 and then ripping that
right back out and implementing a different UI in M16, it would be better to
implement the final UI* now.  The reason is that getting this infobar version
right is going to require more changes than what you have here (I don't think
the trigger code in Browser is in the right place, the normal fullscreen exit
bubble needs to behave differently in order to not cause problems for the
infobar, etc.), whereas doing the M16 "modified fullscreen bubble" UI should
actually be simpler.  Plus the UX is more consistent and nicer so all else being
equal we should go for that.

I think implementing that UI should be a matter of adding a bit more code to the
existing fullscreen exit bubble so that it can display a different string in the
site-fullscreen case, and can also show some buttons and not auto-hide when
we're in the permission case.

*Glen and I also want to do a content setting icon that appears in the "denied"
case, but that can wait until M16.

Also, the strings used here have problems.  "Allow" and "Deny" on their own
don't make sense as after-action words.  So I propose the following content for
the "permission" bubble case:

www.google.com has gone fullscreen.  [Always allow] [Never allow]  X

The "informative" case that auto-hides can be:

www.google.com has gone fullscreen.  [Never allow]

We can leave the existing bubble as-is for when it reappears after hiding.

I really don't think this should be too hard to accomplish.  If necessary I can
try and help get this done (although I do have a number of other things on my
plate ATM).

[koz (OOO until 15th September), 2011-08-29 07:35:30]

On 2011/08/26 18:09:02, Peter Kasting wrote:
> I talked with Glen about this UI and what we're trying to do, as well as some
of
> my concerns with the existing proposal.  Here are the results of that
> conversation.
> 
> I think instead of implementing this with infobars for M15 and then ripping
that
> right back out and implementing a different UI in M16, it would be better to
> implement the final UI* now.  The reason is that getting this infobar version
> right is going to require more changes than what you have here (I don't think
> the trigger code in Browser is in the right place, the normal fullscreen exit
> bubble needs to behave differently in order to not cause problems for the
> infobar, etc.), whereas doing the M16 "modified fullscreen bubble" UI should
> actually be simpler.  Plus the UX is more consistent and nicer so all else
being
> equal we should go for that.
> 
> I think implementing that UI should be a matter of adding a bit more code to
the
> existing fullscreen exit bubble so that it can display a different string in
the
> site-fullscreen case, and can also show some buttons and not auto-hide when
> we're in the permission case.
> 
> *Glen and I also want to do a content setting icon that appears in the
"denied"
> case, but that can wait until M16.
> 
> Also, the strings used here have problems.  "Allow" and "Deny" on their own
> don't make sense as after-action words.  So I propose the following content
for
> the "permission" bubble case:
> 
> http://www.google.com has gone fullscreen.  [Always allow] [Never allow]  X
> 
> The "informative" case that auto-hides can be:
> 
> http://www.google.com has gone fullscreen.  [Never allow]
> 
> We can leave the existing bubble as-is for when it reappears after hiding.
> 
> I really don't think this should be too hard to accomplish.  If necessary I
can
> try and help get this done (although I do have a number of other things on my
> plate ATM).

Thanks for your comments, Peter. You raise a good point - I'd also rather get
the final UI done for M15 and with the points you raise I realise that it
probably isn't quite as much work as we had thought. Unfortunately the deadline
for us is Tuesday afternoon (24 hours from now) as we have an eng offsite from
Wednesday to Friday, and so won't be able to get it over the line.

Jeremy and I have spent today working on implementing the new UI you suggested
and can hopefully have something that needs cleaning up by tomorrow afternoon
(I'll upload our (very) rough progress so you can let us know if we're on the
right track - please note this doesn't include the work we've done on Mac, which
has no exit bubble to modify). Perhaps if we get enough of the work done you'd
be able to get it over the line after Tuesday?

[Peter Kasting, 2011-08-29 18:06:29]

I can help you get stuff done.

In this case, I think if we have something limping along by the branch point,
that's good enough.  Normally we try not to do significant work to anything
that's branched, but the alternative here seems like delaying shipping the JS
fullscreen API to M16, and I'm assuming that's strongly undesirable.  (If not,
then we should really consider going ahead and punting that to M16 to enable, in
which case you have time to do the work here without pressure.)

[koz (OOO until 15th September), 2011-10-06 03:34:40]

Hi Nico,

Jeremy and I have been working on this and we now have the mac version nearly
finished, we have some questions though:

* the fullscreen exit bubble is a window, which has implications for focus and
cursor styles eg:
  - if you click on the bubble, your next click in the renderer will be
swallowed to give the renderer focus.
  - mousing over the link in the bubble won't change the cursor to a hand
because the window doesn't have focus
  - if the bubble doesn't have focus, clicking the link doesn't work (clicking
the buttons does work, however)
Is it possible to work around these problems? We used a window to get the shadow
and to reuse code in info_bubble_window.mm.

* the text in the "site has gone fullscreen" label and the "press f11 to exit"
link don't seem to be centered, but it looks like they should be from the xib.
Is such a discrepancy common? Should we make it look wrong in the xib to get the
desired look in the real app or is there a better way to vertically center the
text?

* we put the buttons, the link and the label into GTMWidthBasedTweakers, but
they have dynamic content, which requires a relayout of some kind. We tried
calling GTMUILocalizerAndLayoutTweaker.tweakUI but that doesn't appear to work
(see fullscreen_exit_bubble_controller.mm).

Thanks for your help!

[Nico, 2011-10-06 03:51:44]

> - if you click on the bubble, your next click in the renderer will be
> swallowed to give the renderer focus.

Can you programmatically focus the renderer on window close?

No great suggestions for the other two questions.

> * the text in the "site has gone fullscreen" label and the "press f11 to exit"
> link don't seem to be centered, but it looks like they should be from the xib.
> Is such a discrepancy common? Should we make it look wrong in the xib to get
the
> desired look in the real app or is there a better way to vertically center the
> text?

Can you tell the text field to center the text? IIRC Jeremy added some padding
to the text field, and if the text is left-aligned that might cause this.

> * we put the buttons, the link and the label into GTMWidthBasedTweakers, but
> they have dynamic content, which requires a relayout of some kind. We tried
> calling GTMUILocalizerAndLayoutTweaker.tweakUI but that doesn't appear to work
> (see fullscreen_exit_bubble_controller.mm).

http://www.chromium.org/developers/design-documents/ui-localization/mac-notes...
:

"""Dynamic UIs

There are cases where the data that ends up being displayed is dynamic, in that
it includes data that comes from webpages, servers, etc.  For these, we will
probably always need to do custom code to help with the layout.

Examples: See src/chrome/browser/cocoa/infobar*."""

[Peter Kasting, 2011-10-06 04:15:33]

On 2011/10/06 03:34:40, koz wrote:
> Is it possible to work around these problems? We used a window to get the
shadow
> and to reuse code in info_bubble_window.mm.

Are there conceivable ways to do this without windows?  If so maybe we can
convert to one of those to avoid these problems (which I think are worth fixing
in the long term, though we could probably ship them in the V1 implementation). 
This might mean modifying the existing info bubble code.

[jeremya, 2011-10-06 04:22:23]

On 2011/10/06 03:51:44, Nico wrote:
> > - if you click on the bubble, your next click in the renderer will be
> > swallowed to give the renderer focus.
> 
> Can you programmatically focus the renderer on window close?
> 
> No great suggestions for the other two questions.

Hm, I think those are deal breakers. Perhaps we need to use a view -- if we do,
how can we get it to render a drop shadow?

> > * the text in the "site has gone fullscreen" label and the "press f11 to
exit"
> > link don't seem to be centered, but it looks like they should be from the
xib.
> > Is such a discrepancy common? Should we make it look wrong in the xib to get
> the
> > desired look in the real app or is there a better way to vertically center
the
> > text?
> 
> Can you tell the text field to center the text? IIRC Jeremy added some padding
> to the text field, and if the text is left-aligned that might cause this.

Er- we meant vertical alignment, not horizontal alignment.

[jeremya, 2011-10-06 04:23:08]

On 2011/10/06 04:15:33, Peter Kasting wrote:
> On 2011/10/06 03:34:40, koz wrote:
> > Is it possible to work around these problems? We used a window to get the
> shadow
> > and to reuse code in info_bubble_window.mm.
> 
> Are there conceivable ways to do this without windows?  If so maybe we can
> convert to one of those to avoid these problems (which I think are worth
fixing
> in the long term, though we could probably ship them in the V1
implementation). 
> This might mean modifying the existing info bubble code.

I don't know of any way to get the drop shadow that info bubble has in a view.

[Nico, 2011-10-06 04:35:45]

On Thu, Oct 6, 2011 at 4:22 AM,  <jeremya@chromium.org> wrote:
> On 2011/10/06 03:51:44, Nico wrote:
>>
>> > - if you click on the bubble, your next click in the renderer will be
>> > swallowed to give the renderer focus.
>
>> Can you programmatically focus the renderer on window close?
>
>> No great suggestions for the other two questions.
>
> Hm, I think those are deal breakers.

With "no great suggestions" I meant that you have to try a couple
things, not that it's impossible.
http://stackoverflow.com/questions/128015/make-osx-application-respond-to-fir...
might be something to try, for example.


> Er- we meant vertical alignment, not horizontal alignment.

The bubble replaces the text view in the xib with a text field at load
time, right? Maybe the two heights don't match? In that case, add the
difference between the two heights / 2 to the new view's y right after
the swap.

[yzshen1, 2011-10-10 19:09:30]

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1720: window_->SetFullscreen(entering_fullscreen,
GURL(), false);
I am not familiar with Mac implementation, but it seems SetPresentationMode
should be used.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3919: void
Browser::OnAcceptFullscreenPermission(const GURL& url) {
On Windows:
After clicking "Allow", restarting browser will cause LOG(DFATAL) in
PrefProvider::CanonicalizeContentSettingsExceptions.

Have you met the same issue?

[koz (OOO until 15th September), 2011-10-10 22:49:58]

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1720: window_->SetFullscreen(entering_fullscreen,
GURL(), false);
On 2011/10/10 19:09:30, yzshen1 wrote:
> I am not familiar with Mac implementation, but it seems SetPresentationMode
> should be used.

Done.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3919: void
Browser::OnAcceptFullscreenPermission(const GURL& url) {
On 2011/10/10 19:09:30, yzshen1 wrote:
> On Windows:
> After clicking "Allow", restarting browser will cause LOG(DFATAL) in
> PrefProvider::CanonicalizeContentSettingsExceptions.
> 
> Have you met the same issue?

No, I can't reproduce this. Can you repro if you delete your user data
directory?

[yzshen1, 2011-10-10 22:55:58]

On 2011/10/10 22:49:58, koz wrote:
> http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.c...
> chrome/browser/ui/browser.cc:1720: window_->SetFullscreen(entering_fullscreen,
> GURL(), false);
> On 2011/10/10 19:09:30, yzshen1 wrote:
> > I am not familiar with Mac implementation, but it seems SetPresentationMode
> > should be used.
> 
> Done.
> 
>
http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/browser.c...
> chrome/browser/ui/browser.cc:3919: void
> Browser::OnAcceptFullscreenPermission(const GURL& url) {
> On 2011/10/10 19:09:30, yzshen1 wrote:
> > On Windows:
> > After clicking "Allow", restarting browser will cause LOG(DFATAL) in
> > PrefProvider::CanonicalizeContentSettingsExceptions.
> > 
> > Have you met the same issue?
> 
> No, I can't reproduce this. Can you repro if you delete your user data
> directory?
If I delete the user data dir, I won't get that issue (until I click "allow" in
fullscreen mode again).

Are you running Debug build?

[Scott Hess - ex-Googler, 2011-10-10 23:19:50]

Reviewed the bit I'd spoken with Jeremy about.

Per our offline conversation, most bubbles dismiss when you click the action,
which will call -resignKeyWindow, which will revert key back to the next window
(expect to be the parent window), which put the firstResponder for that window
back in charge, basically where you were before clicking in the bubble in the
first place.  In this case, the bubble is re-purposed, so it could revert things
manually by sending -becomeKeyWindow to the parent window.  But it sounds like
there are other reasons to change how key and first responder work in bubbles.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:10: #import
"third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
Can this #import be moved into the .mm file by using a @class line in this file?

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:96: CGFloat maxY =
NSHeight([bwc window].frame);
Capture the frame once in an NSRect then use NSWidth() and NSHeight() inline.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:116: origin.y =
maxY - NSHeight([self window].frame);
Likewise here, capture the frame once then work on it.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:161: return
static_cast<AnimatableView*>([self window]);
Is this still a valid conversion?

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:193: [NSFont
systemFontSizeForControlSize:NSRegularControlSize]];
Merge error?

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:47: return YES;
Needs to be conditional.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/inf...
File chrome/browser/ui/cocoa/info_bubble_window.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/inf...
chrome/browser/ui/cocoa/info_bubble_window.mm:139: return NO;
This needs to be conditional.  Avi could probably tell you if doing this will
cause an accessibility fail.

[jeremya, 2011-10-11 05:28:48]

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:10: #import
"third_party/GTM/AppKit/GTMUILocalizerAndLayoutTweaker.h"
On 2011/10/10 23:19:50, shess wrote:
> Can this #import be moved into the .mm file by using a @class line in this
file?

Done.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:96: CGFloat maxY =
NSHeight([bwc window].frame);
On 2011/10/10 23:19:50, shess wrote:
> Capture the frame once in an NSRect then use NSWidth() and NSHeight() inline.

Done.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:116: origin.y =
maxY - NSHeight([self window].frame);
On 2011/10/10 23:19:50, shess wrote:
> Likewise here, capture the frame once then work on it.

Done.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:161: return
static_cast<AnimatableView*>([self window]);
On 2011/10/10 23:19:50, shess wrote:
> Is this still a valid conversion?

Nope :) Removed.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:193: [NSFont
systemFontSizeForControlSize:NSRegularControlSize]];
On 2011/10/10 23:19:50, shess wrote:
> Merge error?

I don't think so... what makes you say that?

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:47: return YES;
On 2011/10/10 23:19:50, shess wrote:
> Needs to be conditional.

Done.

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/inf...
File chrome/browser/ui/cocoa/info_bubble_window.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/inf...
chrome/browser/ui/cocoa/info_bubble_window.mm:139: return NO;
On 2011/10/10 23:19:50, shess wrote:
> This needs to be conditional.  Avi could probably tell you if doing this will
> cause an accessibility fail.

Avi who? :)

[Scott Hess - ex-Googler, 2011-10-11 05:32:30]

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:193: [NSFont
systemFontSizeForControlSize:NSRegularControlSize]];
On 2011/10/11 05:28:48, jeremya wrote:
> On 2011/10/10 23:19:50, shess wrote:
> > Merge error?
> 
> I don't think so... what makes you say that?

Sorry, brace-counting fail across the lines, compounded by excess instances of
"size".

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/inf...
File chrome/browser/ui/cocoa/info_bubble_window.mm (right):

http://codereview.chromium.org/7740044/diff/41001/chrome/browser/ui/cocoa/inf...
chrome/browser/ui/cocoa/info_bubble_window.mm:139: return NO;
On 2011/10/11 05:28:48, jeremya wrote:
> On 2011/10/10 23:19:50, shess wrote:
> > This needs to be conditional.  Avi could probably tell you if doing this
will
> > cause an accessibility fail.
> 
> Avi who? :)

Drissman, avi@chromium.org, I'm pretty sure he groks the OSX accessibility
stuff.    He's in MTV this week, if you're conveniently located.

[Nico, 2011-10-11 06:15:16]

(You're not waiting for comments from me here, are you? Since you hadn't replied
to my last reply, I assumed you're still iterating on the code.)

[jeremya, 2011-10-11 21:46:31]

On 2011/10/11 06:15:16, Nico wrote:
> (You're not waiting for comments from me here, are you? Since you hadn't
replied
> to my last reply, I assumed you're still iterating on the code.)

The Mac code is now in a reviewable state -- shess has been looking at the UI
code in particular, would love to get some more feedback on the code around it
(i.e. the code in browser_window_controller)

Thanks!

[Peter Kasting, 2011-10-11 23:08:31]

http://codereview.chromium.org/7740044/diff/52001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13862: You have gone fullscreen
Why do the two messages above have a trailing period but this does not?

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1639: // TODO Change this to an enum.
Nit: Change what to an enum?

Also please give an owner ("TODO(pkasting):"). (2 places)

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1652: if (for_tab) {
Nit: Less duplication:

  GURL requesting_url;
  bool ask_permission = false;
  if (for_tab) {
    url = GetSelectedTabContents()->GetURL();
    ask_permission = !url.SchemeIsFile() &&
        (GetFullscreenSetting(url) == CONTENT_SETTING_ASK);
  }
  window_->SetFullscreen(entering_fullscreen, url, ask_permission);

Also applies to TogglePresentationMode() below.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1654: bool ask_permission = (!url.SchemeIsFile())
&&
Nit: No parens around unary.  Put them around the binary condition instead.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3896: "", CONTENT_SETTING_ALLOW);
Nit: "" -> std::string() (2 places)

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:139: virtual void SetFullscreen(bool
fullscreen, const GURL& url,
Nit: One arg per line, aligned

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:50: 
Nit: Just remove blank lines between related functions that don't have comments.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:249: void Panel::SetFullscreen(bool
fullscreen, const GURL& url,
Nit: One arg per line, aligned

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:88: virtual void SetFullscreen(bool fullscreen,
const GURL& url,
Nit: One arg per line, aligned

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:753: void
BrowserView::SetFullscreen(bool fullscreen, const GURL& url,
Nit: One arg per line, aligned

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:2170: void
BrowserView::ProcessFullscreen(bool fullscreen, const GURL& url,
Nit: One arg per line, aligned

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:2221: browser_.get(),
Nit: I'd just put all these on one line and indent 4

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.h:257: virtual void
SetFullscreen(bool fullscreen, const GURL& url,
Nit: One arg per line, aligned

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (left):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:129:
popup_->SetOpacity(static_cast<unsigned char>(0xff * kOpacity));
If you're removing this you should kill the kOpacity constant entirely. 
Furthermore, the popup doesn't need to be transparent, and in fact we could
maybe (?) get by without a separate popup at all and just use a completely
standard bubble.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:30: public
views::ButtonListener {
Nit: Must be aligned with other parent class declaration.  You'll probably need
to move the ": public views::View" part down a line and indent it 4.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:41: //
views::ButtonListener
Nit: Can this be private?

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:44: virtual void
HideButtons();
Nit: Is this really virtual?  Seems like no?  Also needs a comment about what it
does (or more precisely why).

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:48: // Space between the
site info label and the buttons / link.
Nit: Blank line above this

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:53: // virtual void
OnPaint(gfx::Canvas* canvas);
Don't comment out declarations -- remove it entirely.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:89:
link_.SetNormalColor(SK_ColorBLACK);
There are potential accessibility issues here.  You're not explicitly setting
the colors of the labels, just the link.  That means you should probably set the
link to use the same colors as the message label instead of assuming it should
be black.

http://codereview.chromium.org/8221027/ will be modifying this code and the code
that you need to call in order to implement my suggestion above.  Once that
lands you'll need to explicitly give the link and the label the appropriate
background color.  The best way for you to accomplish that and my other
suggestions is for you to move the set_background() call below to the top of
this constructor, then do something like:

  link_. ...(set up other stuff)...
  link_.SetBackgroundColor(background()->get_color());
  link_.SetNormalColor(message_label_.enabled_color());
  link_.SetPressedColor(message_label_.enabled_color());

  message_label_. ...(set up other stuff)...
  message_label_.SetBackgroundColor(background()->get_color());

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:100:
views::BubbleBorder* bubble_border =
Nit: Instead of manually creating a bubble border, can we just use the bubble
class outright?  That would simplify this file.  (See also below.)

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:116: if (!show_buttons_)
{
Nit: No {}

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:123: UTF16ToWide(
This line is erroneous.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:127: } else if
(url.SchemeIsFile()) {
Nit: No else after return (2 places)

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:173: gfx::Size
link_preferred_size(link_.GetPreferredSize());
Did you consider using a layout manager like GridLayout to take care of your
layout and sizing functions?

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:192:
link_.SetBounds(link_x,
Nit: Wrapping here is odd

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:211: views::Widget*
frame,
Nit: Move up to previous line and align other args even

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:227: this,
UTF16ToWideHack(accelerator.GetShortcutText()), url,
Nit: I think you can fit this all on 2 lines if you move the first arg up to the
previous line

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:241:
popup_->GetRootView()->SetLayoutManager(NULL);
Why is this needed?

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:336: int
animation_height = static_cast<int>(
Nit: Use size_animation_.CurrentValueBetween().

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:340: if
(animation_height <= kPopupTopPx) {
Nit: Simpler:

  int y_offset = std::min(animation_height, kPopupTopPx);
  size.set_height(size.height() - animation_height +   y_offset);
  origin.set_y(origin.y() - y_offset);

You can probably get even simpler by making animation_height go 0->total_height
instead of total_height->0, starting |origin| at the screen y, and reversing the
signs of various calculations above.

However, I'm concerned with this -- it seems like it means the bubble will slide
to the top of the screen and then compress down until it disappears instead of
sliding offscreen entirely, which would presumably look better (and the reverse
on showing rather than hiding).  Plus I don't know if the bubble border draws
correctly at very small heights.

Shouldn't instead this just animate the bubble on/offscreen by moving the y
coordinate?

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:27: views::Widget* frame,
Nit: Move this up to the previous line and align other args with it

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:35: protected:
Nit: Blank line above protected:

[koz (OOO until 15th September), 2011-10-12 05:38:23]

Thanks for the thorough review, Peter!

http://codereview.chromium.org/7740044/diff/52001/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/app/generated_resour...
chrome/app/generated_resources.grd:13862: You have gone fullscreen
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Why do the two messages above have a trailing period but this does not?

Oops, just a typo. Fixed.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1639: // TODO Change this to an enum.
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Change what to an enum?
> 
> Also please give an owner ("TODO(pkasting):"). (2 places)

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1652: if (for_tab) {
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Less duplication:
> 
>   GURL requesting_url;
>   bool ask_permission = false;
>   if (for_tab) {
>     url = GetSelectedTabContents()->GetURL();
>     ask_permission = !url.SchemeIsFile() &&
>         (GetFullscreenSetting(url) == CONTENT_SETTING_ASK);
>   }
>   window_->SetFullscreen(entering_fullscreen, url, ask_permission);
> 
> Also applies to TogglePresentationMode() below.

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1654: bool ask_permission = (!url.SchemeIsFile())
&&
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: No parens around unary.  Put them around the binary condition instead.

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3896: "", CONTENT_SETTING_ALLOW);
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: "" -> std::string() (2 places)

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser_w...
File chrome/browser/ui/browser_window.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/browser_w...
chrome/browser/ui/browser_window.h:139: virtual void SetFullscreen(bool
fullscreen, const GURL& url,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: One arg per line, aligned

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:50: 
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Just remove blank lines between related functions that don't have
comments.

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.cc:249: void Panel::SetFullscreen(bool
fullscreen, const GURL& url,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: One arg per line, aligned

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
File chrome/browser/ui/panels/panel.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/panels/pa...
chrome/browser/ui/panels/panel.h:88: virtual void SetFullscreen(bool fullscreen,
const GURL& url,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: One arg per line, aligned

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:753: void
BrowserView::SetFullscreen(bool fullscreen, const GURL& url,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: One arg per line, aligned

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:2170: void
BrowserView::ProcessFullscreen(bool fullscreen, const GURL& url,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: One arg per line, aligned

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:2221: browser_.get(),
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: I'd just put all these on one line and indent 4

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.h:257: virtual void
SetFullscreen(bool fullscreen, const GURL& url,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: One arg per line, aligned

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (left):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:129:
popup_->SetOpacity(static_cast<unsigned char>(0xff * kOpacity));
On 2011/10/11 23:08:32, Peter Kasting wrote:
> If you're removing this you should kill the kOpacity constant entirely. 
> Furthermore, the popup doesn't need to be transparent, and in fact we could
> maybe (?) get by without a separate popup at all and just use a completely
> standard bubble.

I've removed the constant (thanks), but as I talked about above the standard
bubble doesn't seem to be suitable for this UI.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:30: public
views::ButtonListener {
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Must be aligned with other parent class declaration.  You'll probably
need
> to move the ": public views::View" part down a line and indent it 4.

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:41: //
views::ButtonListener
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Can this be private?

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:44: virtual void
HideButtons();
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Is this really virtual?  Seems like no?  Also needs a comment about what
it
> does (or more precisely why).

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:48: // Space between the
site info label and the buttons / link.
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Blank line above this

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:53: // virtual void
OnPaint(gfx::Canvas* canvas);
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Don't comment out declarations -- remove it entirely.

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:89:
link_.SetNormalColor(SK_ColorBLACK);
On 2011/10/11 23:08:32, Peter Kasting wrote:
> There are potential accessibility issues here.  You're not explicitly setting
> the colors of the labels, just the link.  That means you should probably set
the
> link to use the same colors as the message label instead of assuming it should
> be black.
> 
> http://codereview.chromium.org/8221027/ will be modifying this code and the
code
> that you need to call in order to implement my suggestion above.  Once that
> lands you'll need to explicitly give the link and the label the appropriate
> background color.  The best way for you to accomplish that and my other
> suggestions is for you to move the set_background() call below to the top of
> this constructor, then do something like:
> 
>   link_. ...(set up other stuff)...
>   link_.SetBackgroundColor(background()->get_color());
>   link_.SetNormalColor(message_label_.enabled_color());
>   link_.SetPressedColor(message_label_.enabled_color());
> 
>   message_label_. ...(set up other stuff)...
>   message_label_.SetBackgroundColor(background()->get_color());

Cool. I'll make this change once your patch lands.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:100:
views::BubbleBorder* bubble_border =
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Instead of manually creating a bubble border, can we just use the bubble
> class outright?  That would simplify this file.  (See also below.)

There were a few reasons why we abandoned our attempts to use Bubble here:
- Bubble animates differently to how we want to here - it fades but we slide
- Bubble auto-cleans itself up when it is hidden, but we want to be able to
slide the view in and out.
- Bubble auto-hides when it loses focus (we did get around this by adding a flag
to Bubble)

It became quite tricky when we tried to instantiate new Bubbles whenever the
base class (FullscreenExitBubble) called Show() / Hide(), as the base class also
needs to measure the size of the exit bubble when it is in the hidden state to
implement the auto-hide logic.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:116: if (!show_buttons_)
{
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:123: UTF16ToWide(
On 2011/10/11 23:08:32, Peter Kasting wrote:
> This line is erroneous.

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:127: } else if
(url.SchemeIsFile()) {
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: No else after return (2 places)

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:173: gfx::Size
link_preferred_size(link_.GetPreferredSize());
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Did you consider using a layout manager like GridLayout to take care of your
> layout and sizing functions?

I'm not sure if that would work here as we need to make button_box_width the
larger of the two buttons and the link behind it. Also, hiding the buttons and
showing the link shouldn't cause the bubble to change size.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:192:
link_.SetBounds(link_x,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Wrapping here is odd

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:211: views::Widget*
frame,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Move up to previous line and align other args even

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:227: this,
UTF16ToWideHack(accelerator.GetShortcutText()), url,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: I think you can fit this all on 2 lines if you move the first arg up to
the
> previous line

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:241:
popup_->GetRootView()->SetLayoutManager(NULL);
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Why is this needed?

I've added a comment.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:336: int
animation_height = static_cast<int>(
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Use size_animation_.CurrentValueBetween().

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:340: if
(animation_height <= kPopupTopPx) {
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Simpler:
> 
>   int y_offset = std::min(animation_height, kPopupTopPx);
>   size.set_height(size.height() - animation_height +   y_offset);
>   origin.set_y(origin.y() - y_offset);
> 
> You can probably get even simpler by making animation_height go
0->total_height
> instead of total_height->0, starting |origin| at the screen y, and reversing
the
> signs of various calculations above.
> 
> However, I'm concerned with this -- it seems like it means the bubble will
slide
> to the top of the screen and then compress down until it disappears instead of
> sliding offscreen entirely, which would presumably look better (and the
reverse
> on showing rather than hiding).  Plus I don't know if the bubble border draws
> correctly at very small heights.
> 
> Shouldn't instead this just animate the bubble on/offscreen by moving the y
> coordinate?

Yes, that would be the best way to do it, but we were concerned that a negative
y coordinate would put the popup outside the browser (eg: multiple monitor
environments) - will it not do that?

Also, because the border is on the contents view and we clear the layout manager
for the popup, the border won't change size while the popup shrinks.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.h (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:27: views::Widget* frame,
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Move this up to the previous line and align other args with it

Done.

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.h:35: protected:
On 2011/10/11 23:08:32, Peter Kasting wrote:
> Nit: Blank line above protected:

Done.

[Peter Kasting, 2011-10-12 07:38:40]

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:340: if
(animation_height <= kPopupTopPx) {
On 2011/10/12 05:38:23, koz wrote:
> > Shouldn't instead this just animate the bubble on/offscreen by moving the y
> > coordinate?
> 
> Yes, that would be the best way to do it, but we were concerned that a
negative
> y coordinate would put the popup outside the browser (eg: multiple monitor
> environments) - will it not do that?

Test and see.  Grab two monitors and tell Windows that one is positioned above
the other, then change the patch to do something like this and see what happens.

[Nico, 2011-10-12 16:23:09]

mac stuff looks mostly ok

Please provide a real change description, and include a "xib changes:"
paragraph(
http://svnsearch.org/svnsearch/repos/CHROMIUM/search?logMessage=%22xib%20chan...
)

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.h:59: bool ask_permission);
Consider using an enum instead of a bool – makes the calling sites more
readable. Also below.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.h:366:
askPermission:(BOOL)ask_permission;
interCaps in objc code

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.h:381:
askPermission:(BOOL)ask_permission;
same

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1908:
askPermission:(BOOL)ask_permission {
and here (and below)

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1958: [self
setFullscreen:presentationMode forURL:url askPermission:ask_permission];
80 cols

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1990:
askPermission:ask_permission];
align :

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:69: show_buttons_ =
ask_permission;
showButtons_

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:117: [super
awakeFromNib];
"You should call the super implementation of awakeFromNib only if you know for
certain that your superclass provides an implementation."  -
http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Applica...

you probably don't need this call

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:140: // The star
currently triggers on mouse down, not mouse up.
star?

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:142:
gtm_setDuration:0.2  // TODO: Constantify.
do

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:212: [exitLabel_
setFrameSize:NSMakeSize(1.0e7, 1.0e7)];
:-/

Is window width/height big enough? (if we ever turn on layer backing, this will
alloc a huge backing store temporarily else)

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:218:
labelFrame.origin.x += labelFrame.size.width - textFrame.size.width;
NSWidth(...) - NSWidth(...)

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:47: return YES;
this is an unintentional change, right?

[yzshen1, 2011-10-12 17:36:41]

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1659: window_->SetFullscreen(entering_fullscreen,
url, ask_permission);
Do we cancel the tab fullscreen mode when navigating to another location?

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3783: void
Browser::ToggleFullscreenModeForTab(TabContents* tab,
Please consider the following scenario:
1) enter browser fullscreen (currently we show the fullscreen bubble without any
button).
2) the page calls ToggleFullscreenModeForTab to enter tab fullscreen. We may
need to change the bubble contents to show permission buttons. (I assume that
user permission is still needed for tab fullscreen, even if in browser
fullscreen mode, is that correct?)

Similarly, we may also need to change the bubble contents when switching from
tab fullscreen back to browser fullscreen.
What do you think?

Besides, currently the messages are confusing to users when browser fullscreen
is involved. Consider the following scenario:
1) enter browser fullscreen: "This page has gone full screen. Press ESC to
exit."
2) the page requests to enter tab fullscreen: "This page has gone full screen .
[Allow] [Exit full screen]"
3) user clicks "[Exit full screen]", the browser returns to browser fullscreen
mode, the only visible change to the user is the message changes back to "This
page has gone full screen. Press ESC to exit.".

[Peter Kasting, 2011-10-12 23:09:21]

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:89:
link_.SetNormalColor(SK_ColorBLACK);
On 2011/10/12 05:38:23, koz wrote:
> On 2011/10/11 23:08:32, Peter Kasting wrote:
> > There are potential accessibility issues here.  You're not explicitly
setting
> > the colors of the labels, just the link.  That means you should probably set
> the
> > link to use the same colors as the message label instead of assuming it
should
> > be black.
> > 
> > http://codereview.chromium.org/8221027/ will be modifying this code and the
> code
> > that you need to call in order to implement my suggestion above.  Once that
> > lands you'll need to explicitly give the link and the label the appropriate
> > background color.  The best way for you to accomplish that and my other
> > suggestions is for you to move the set_background() call below to the top of
> > this constructor, then do something like:
> > 
> >   link_. ...(set up other stuff)...
> >   link_.SetBackgroundColor(background()->get_color());
> >   link_.SetNormalColor(message_label_.enabled_color());
> >   link_.SetPressedColor(message_label_.enabled_color());
> > 
> >   message_label_. ...(set up other stuff)...
> >   message_label_.SetBackgroundColor(background()->get_color());
> 
> Cool. I'll make this change once your patch lands.

This has now landed.

[koz (OOO until 15th September), 2011-10-13 05:11:16]

On 2011/10/12 07:38:40, Peter Kasting wrote:
>
http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
> File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):
> 
>
http://codereview.chromium.org/7740044/diff/52001/chrome/browser/ui/views/ful...
> chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:340: if
> (animation_height <= kPopupTopPx) {
> On 2011/10/12 05:38:23, koz wrote:
> > > Shouldn't instead this just animate the bubble on/offscreen by moving the
y
> > > coordinate?
> > 
> > Yes, that would be the best way to do it, but we were concerned that a
> negative
> > y coordinate would put the popup outside the browser (eg: multiple monitor
> > environments) - will it not do that?
> 
> Test and see.  Grab two monitors and tell Windows that one is positioned above
> the other, then change the patch to do something like this and see what
happens.

We tried this and the popup did indeed appear on the second monitor.

[koz (OOO until 15th September), 2011-10-13 05:12:07]

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1659: window_->SetFullscreen(entering_fullscreen,
url, ask_permission);
On 2011/10/12 17:36:41, yzshen1 wrote:
> Do we cancel the tab fullscreen mode when navigating to another location?

Not currently, but I've got a patch that implements that (link:
http://codereview.chromium.org/7966031/ - it got rolled back because of flaky
tests).

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:3783: void
Browser::ToggleFullscreenModeForTab(TabContents* tab,
On 2011/10/12 17:36:41, yzshen1 wrote:
> Please consider the following scenario:
> 1) enter browser fullscreen (currently we show the fullscreen bubble without
any
> button).
> 2) the page calls ToggleFullscreenModeForTab to enter tab fullscreen. We may
> need to change the bubble contents to show permission buttons. (I assume that
> user permission is still needed for tab fullscreen, even if in browser
> fullscreen mode, is that correct?)
> 
> Similarly, we may also need to change the bubble contents when switching from
> tab fullscreen back to browser fullscreen.
> What do you think?
> 
> Besides, currently the messages are confusing to users when browser fullscreen
> is involved. Consider the following scenario:
> 1) enter browser fullscreen: "This page has gone full screen. Press ESC to
> exit."
> 2) the page requests to enter tab fullscreen: "This page has gone full screen
.
> [Allow] [Exit full screen]"
> 3) user clicks "[Exit full screen]", the browser returns to browser fullscreen
> mode, the only visible change to the user is the message changes back to "This
> page has gone full screen. Press ESC to exit.".

As discussed, I'll leave this for you in your mouselook change.

[jeremya, 2011-10-13 05:16:54]

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.h (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.h:59: bool ask_permission);
On 2011/10/12 16:23:09, Nico wrote:
> Consider using an enum instead of a bool – makes the calling sites more
> readable. Also below.

I think the problem was mainly that the SetFullscreen(false, ...) case was
confusing, since the GURL and the bool were unused in that case. I've split it
into EnterFullscreen and ExitFullscreen, which I think makes the callsites more
readable even without an enum.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.h:366:
askPermission:(BOOL)ask_permission;
On 2011/10/12 16:23:09, Nico wrote:
> interCaps in objc code

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.h:381:
askPermission:(BOOL)ask_permission;
On 2011/10/12 16:23:09, Nico wrote:
> same

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1908:
askPermission:(BOOL)ask_permission {
On 2011/10/12 16:23:09, Nico wrote:
> and here (and below)

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1958: [self
setFullscreen:presentationMode forURL:url askPermission:ask_permission];
On 2011/10/12 16:23:09, Nico wrote:
> 80 cols

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.mm:1990:
askPermission:ask_permission];
On 2011/10/12 16:23:09, Nico wrote:
> align :

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:69: show_buttons_ =
ask_permission;
On 2011/10/12 16:23:09, Nico wrote:
> showButtons_

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:117: [super
awakeFromNib];
On 2011/10/12 16:23:09, Nico wrote:
> "You should call the super implementation of awakeFromNib only if you know for
> certain that your superclass provides an implementation."  -
>
http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/Applica...
> 
> you probably don't need this call

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:140: // The star
currently triggers on mouse down, not mouse up.
On 2011/10/12 16:23:09, Nico wrote:
> star?

I don't even know. Removed.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:142:
gtm_setDuration:0.2  // TODO: Constantify.
On 2011/10/12 16:23:09, Nico wrote:
> do

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:212: [exitLabel_
setFrameSize:NSMakeSize(1.0e7, 1.0e7)];
On 2011/10/12 16:23:09, Nico wrote:
> :-/
> 
> Is window width/height big enough? (if we ever turn on layer backing, this
will
> alloc a huge backing store temporarily else)

window width/height should be enough. I'm surprised that layer backing would be
allocated on setFrameSize and not be deferred until draw...

Changed anyway.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:218:
labelFrame.origin.x += labelFrame.size.width - textFrame.size.width;
On 2011/10/12 16:23:09, Nico wrote:
> NSWidth(...) - NSWidth(...)

Done.

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/hyp...
File chrome/browser/ui/cocoa/hyperlink_text_view.mm (right):

http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/cocoa/hyp...
chrome/browser/ui/cocoa/hyperlink_text_view.mm:47: return YES;
On 2011/10/12 16:23:09, Nico wrote:
> this is an unintentional change, right?

Yep, removed.

[koz (OOO until 15th September), 2011-10-13 05:20:41]

On 2011/10/13 05:12:07, koz wrote:
> http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.cc
> File chrome/browser/ui/browser.cc (right):
> 
>
http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.c...
> chrome/browser/ui/browser.cc:1659: window_->SetFullscreen(entering_fullscreen,
> url, ask_permission);
> On 2011/10/12 17:36:41, yzshen1 wrote:
> > Do we cancel the tab fullscreen mode when navigating to another location?
> 
> Not currently, but I've got a patch that implements that (link:
> http://codereview.chromium.org/7966031/ - it got rolled back because of flaky
> tests).
> 
>
http://codereview.chromium.org/7740044/diff/48011/chrome/browser/ui/browser.c...
> chrome/browser/ui/browser.cc:3783: void
> Browser::ToggleFullscreenModeForTab(TabContents* tab,
> On 2011/10/12 17:36:41, yzshen1 wrote:
> > Please consider the following scenario:
> > 1) enter browser fullscreen (currently we show the fullscreen bubble without
> any
> > button).
> > 2) the page calls ToggleFullscreenModeForTab to enter tab fullscreen. We may
> > need to change the bubble contents to show permission buttons. (I assume
that
> > user permission is still needed for tab fullscreen, even if in browser
> > fullscreen mode, is that correct?)
> > 
> > Similarly, we may also need to change the bubble contents when switching
from
> > tab fullscreen back to browser fullscreen.
> > What do you think?
> > 
> > Besides, currently the messages are confusing to users when browser
fullscreen
> > is involved. Consider the following scenario:
> > 1) enter browser fullscreen: "This page has gone full screen. Press ESC to
> > exit."
> > 2) the page requests to enter tab fullscreen: "This page has gone full
screen
> .
> > [Allow] [Exit full screen]"
> > 3) user clicks "[Exit full screen]", the browser returns to browser
fullscreen
> > mode, the only visible change to the user is the message changes back to
"This
> > page has gone full screen. Press ESC to exit.".
> 
> As discussed, I'll leave this for you in your mouselook change.

This change needs to land ASAP for M16. Is there anything more that needs to be
done for LGTM?

[Nico, 2011-10-13 05:24:10]

still missing a cl description

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:262: }
newline

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.h:380: url:(const GURL&)url
why forURL: above and url: here? Use url: above too

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:56: // must override
if they do not hide the buttons.
There are no subclasses, are there? Are there ok or cancel buttons even?

[koz (OOO until 15th September), 2011-10-13 06:09:43]

Thanks, Nico! I've updated the description.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:262: }
On 2011/10/13 05:24:10, Nico wrote:
> newline

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_controller.h:380: url:(const GURL&)url
On 2011/10/13 05:24:10, Nico wrote:
> why forURL: above and url: here? Use url: above too

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:56: // must override
if they do not hide the buttons.
On 2011/10/13 05:24:10, Nico wrote:
> There are no subclasses, are there? Are there ok or cancel buttons even?

Ah, copy/paste fail. Fixed.

[Nico, 2011-10-13 17:15:19]

(did you upload the new snapshot?)

[Peter Kasting, 2011-10-13 18:18:25]

On 2011/10/13 05:11:16, koz wrote:
> On 2011/10/12 07:38:40, Peter Kasting wrote:
> > Test and see.  Grab two monitors and tell Windows that one is positioned
above
> > the other, then change the patch to do something like this and see what
> happens.
> 
> We tried this and the popup did indeed appear on the second monitor.

Thanks for testing this.  It would probably be good to add a comment to the code
like "// We can't just animate vertically up off the monitor, because if another
monitor is above it, the bubble will appear there."

[Scott Hess - ex-Googler, 2011-10-13 20:30:27]

Another pass at the mac-specific stuff.  I'm not sure I understand the overall
stuff well enough to review it.  And I should probably step out of Nico's way.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:517:
askPermission:ask_permission];
Indentation.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:18: } // namespace
fullscreen_exit_bubble
I don't really get why you have this out here, rather than as an anonymous
namespace in the .mm file?

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:63: ofType:@"nib"];
Indentation.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:96: NSWindow*
window = [self window];  // completes nib load
I'd make this InfoBubbleWindow*, then use that in preference to [self window]
below.

Note that you should be using static_cast<InfoBubbleWindow*>().  Sorry.  Also,
I'd at least throw in a DCHECK() that the instance is of the expected class,
though that might make more sense in -awakFromNib.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:98:
[(InfoBubbleWindow*)[self window] setCanBecomeKey:NO];
If I were REALLY pedantic I'd say -setCanBecomeKeyWindow:, to match
-canBecomeKeyWindow.  Yeah, Cocoa is wordy that way.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:121: NSPoint origin
= windowFrame.origin;
You just overwrite origin below.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:139:
gtm_setDuration:kHideDurationMs/1000.0
Any reason not to just have kHideDuration just be a float in the first place, in
seconds?  Yeah, I have no idea why the previous code did kConstant/1000.0.  But
I can't think of a good reason why to do it, really.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:143: return;
Extraneous return.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:197:
NSHeight(windowFrame))];
How about windowFrame.size?

[tony, 2011-10-13 22:34:39]

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:37: // Space between the popup and
the top of the screen.
Nit: The other comments are on the same line.  Seems like this should be
consistent or we should change all the other comments to be above the variable.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:39: // Height of region triggering
slide-in
Nit: This comment appears to belong to kSlideInRegionHeightPx.  Maybe move it
above.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:15: #include
"chrome/browser/ui/browser.h"
Nit: sort header

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:51: // hbox
Nit: I would remove this comment.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:57: // Add buttons
Nit: This too, or make it more descriptive.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:58: GtkWidget* deny_button =
gtk_button_new_with_label("Deny");
Should this string be localized?

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:60: GtkWidget* allow_button
= gtk_button_new_with_label("Allow");
This too?

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:61:
gtk_box_pack_end(GTK_BOX(hbox_), allow_button, FALSE, FALSE, 0);
Do we need to add any extra text here?  It looks like right now it just does
"Exit fullscreen (F11) [Deny] [Allow]".

If the plan is to do this in a follow up patch, do we need this code at all?

[koz (OOO until 15th September), 2011-10-13 23:42:41]

On 2011/10/13 17:15:19, Nico wrote:
> (did you upload the new snapshot?)

Sorry, my bad. I forgot to upload before I left yesterday. New patch is up now.

[Peter Kasting, 2011-10-13 23:48:06]

Non-Mac/non-GTK stuff LGTM once you at least fix the accidental removal of
"link_.SetEnabledColor()", and I suggest also looking at my comment in Layout()
about making things look more like they're sliding in and out.

My comments are on patch set 22 instead of 23 that you just uploaded, but
nothing relevant has changed in patch set 23.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1660: if (entering_fullscreen) {
Nit: No {}

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:37: // Space between the popup and
the top of the screen.
On 2011/10/13 22:34:39, tony wrote:
> Nit: The other comments are on the same line.  Seems like this should be
> consistent or we should change all the other comments to be above the
variable.

Yeah, in this case I'd just do:

static const int foo;  // Blah blah blah
                       // ...blah blah

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:52: static const int
kMiddlePaddingPx = 30;
Nit: Some compilers get angsty about defining class member constants at their
declaration point.  To avoid any hassle here, I'd just stick this at file-scope
inside an anonymous namespace.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:88:
message_label_.SetBackgroundColor(background()->get_color());
Nit: You should call enabled_color() before calling this (as this can change its
result).  You can either move this line down lower, or you can just do:

  SkColor default_text_color(message_label_.enabled_color());
  message_label_.SetBackgroundColor(background()->get_color());
  ...
  link_.SetEnabledColor(default_text_color);

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:104:
link_.SetPressedColor(message_label_.enabled_color());
You still want to SetEnabledColor() here like the code used to.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:106:
AddChildView(&message_label_);
Nit: Go ahead and do this right after setting up |message_label_| above, for
consistency.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:116:
accept_button_->set_focusable(false);
Nit: Can you put these two set_focusable() calls above the AddChildView() calls?

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:118:
link_.SetVisible(false);
Nit: Put this up with the rest of the |link_| setup code

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:119: if (!show_buttons_)
Nit: Blank line above this

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:154: gfx::Size
link_preferred_size(link_.GetPreferredSize());
Nit: Feel free to remove "preferred_" out of all these variable names here and
in Layout() to save on verbosity -- we use "foo_size" for this kind of thing in
other code.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:178: int inner_height =
height() - insets.height();
Nit: One small issue with this layout is that it looks as if, as you animate the
bubble size, all the controls are going to expand from/contract to the middle of
the bubble (due to your "y / 2" calculations).  If instead you position all the
controls based "height() - padding - control.height()" (effectively "pinning
them" to the bottom), like the old code did, then it will look more like the
bubble contents are sliding onscreen and sliding off, which should look a lot
better.  (I like to test this sort of thing by slowing the slide animation down
to take like 5 seconds total, so I can see the animation very clearly.)

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:187:
message_label_.SetBounds(insets.left() + kPaddingPx,
Nit: I think if you just use SetPosition() all these controls will be sized to
their preferred sizes by default.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:338: int
animation_height =
Nit: Another way to wrap this is:

    int animation_height = size_animation_->CurrentValueBetween(
        static_cast<double>(total_height), 0.0f);

Also, |animation_height| is kind of a weird and confusing name here.  I don't
really know what name would be better, maybe this just needs a comment about
what it means or something.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:420: // TODO(koz):
Implement fullscreen content settings UI.
Nit: Is there a bug for this?  Seems like this should be covered by a bug rather
than a TODO...

[koz (OOO until 15th September), 2011-10-14 00:35:16]

Working through the comments...

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:1660: if (entering_fullscreen) {
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/bro...
chrome/browser/ui/cocoa/browser_window_cocoa.mm:517:
askPermission:ask_permission];
On 2011/10/13 20:30:27, shess wrote:
> Indentation.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.h:18: } // namespace
fullscreen_exit_bubble
On 2011/10/13 20:30:27, shess wrote:
> I don't really get why you have this out here, rather than as an anonymous
> namespace in the .mm file?

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:96: NSWindow*
window = [self window];  // completes nib load
On 2011/10/13 20:30:27, shess wrote:
> I'd make this InfoBubbleWindow*, then use that in preference to [self window]
> below.
> 
> Note that you should be using static_cast<InfoBubbleWindow*>().  Sorry.  Also,
> I'd at least throw in a DCHECK() that the instance is of the expected class,
> though that might make more sense in -awakFromNib.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:98:
[(InfoBubbleWindow*)[self window] setCanBecomeKey:NO];
On 2011/10/13 20:30:27, shess wrote:
> If I were REALLY pedantic I'd say -setCanBecomeKeyWindow:, to match
> -canBecomeKeyWindow.  Yeah, Cocoa is wordy that way.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:121: NSPoint origin
= windowFrame.origin;
On 2011/10/13 20:30:27, shess wrote:
> You just overwrite origin below.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:139:
gtm_setDuration:kHideDurationMs/1000.0
On 2011/10/13 20:30:27, shess wrote:
> Any reason not to just have kHideDuration just be a float in the first place,
in
> seconds?  Yeah, I have no idea why the previous code did kConstant/1000.0. 
But
> I can't think of a good reason why to do it, really.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:197:
NSHeight(windowFrame))];
On 2011/10/13 20:30:27, shess wrote:
> How about windowFrame.size?

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble.h (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:37: // Space between the popup and
the top of the screen.
On 2011/10/13 22:34:39, tony wrote:
> Nit: The other comments are on the same line.  Seems like this should be
> consistent or we should change all the other comments to be above the
variable.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:37: // Space between the popup and
the top of the screen.
On 2011/10/13 23:48:06, Peter Kasting wrote:
> On 2011/10/13 22:34:39, tony wrote:
> > Nit: The other comments are on the same line.  Seems like this should be
> > consistent or we should change all the other comments to be above the
> variable.
> 
> Yeah, in this case I'd just do:
> 
> static const int foo;  // Blah blah blah
>                        // ...blah blah

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.h:39: // Height of region triggering
slide-in
On 2011/10/13 22:34:39, tony wrote:
> Nit: This comment appears to belong to kSlideInRegionHeightPx.  Maybe move it
> above.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:15: #include
"chrome/browser/ui/browser.h"
On 2011/10/13 22:34:39, tony wrote:
> Nit: sort header

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:61:
gtk_box_pack_end(GTK_BOX(hbox_), allow_button, FALSE, FALSE, 0);
On 2011/10/13 22:34:39, tony wrote:
> Do we need to add any extra text here?  It looks like right now it just does
> "Exit fullscreen (F11) [Deny] [Allow]".
> 
> If the plan is to do this in a follow up patch, do we need this code at all?

Yeah, the current plan is to do the plumbing for GTK on this patch and fix up
the UI in a follow-up. We'll revert these UI changes.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:52: static const int
kMiddlePaddingPx = 30;
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Some compilers get angsty about defining class member constants at their
> declaration point.  To avoid any hassle here, I'd just stick this at
file-scope
> inside an anonymous namespace.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:52: static const int
kMiddlePaddingPx = 30;
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Some compilers get angsty about defining class member constants at their
> declaration point.  To avoid any hassle here, I'd just stick this at
file-scope
> inside an anonymous namespace.

Done.

[koz (OOO until 15th September), 2011-10-14 01:06:13]

Thanks for your comments, all. They are very much appreciated.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:88:
message_label_.SetBackgroundColor(background()->get_color());
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: You should call enabled_color() before calling this (as this can change
its
> result).  You can either move this line down lower, or you can just do:
> 
>   SkColor default_text_color(message_label_.enabled_color());
>   message_label_.SetBackgroundColor(background()->get_color());
>   ...
>   link_.SetEnabledColor(default_text_color);

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:104:
link_.SetPressedColor(message_label_.enabled_color());
On 2011/10/13 23:48:06, Peter Kasting wrote:
> You still want to SetEnabledColor() here like the code used to.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:106:
AddChildView(&message_label_);
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Go ahead and do this right after setting up |message_label_| above, for
> consistency.

We've moved message_label_'s SetBackgroundColor() down, so this is immediately
after it now.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:116:
accept_button_->set_focusable(false);
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Can you put these two set_focusable() calls above the AddChildView()
calls?

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:118:
link_.SetVisible(false);
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Put this up with the rest of the |link_| setup code

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:119: if (!show_buttons_)
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Blank line above this

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:154: gfx::Size
link_preferred_size(link_.GetPreferredSize());
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Feel free to remove "preferred_" out of all these variable names here and
> in Layout() to save on verbosity -- we use "foo_size" for this kind of thing
in
> other code.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:178: int inner_height =
height() - insets.height();
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: One small issue with this layout is that it looks as if, as you animate
the
> bubble size, all the controls are going to expand from/contract to the middle
of
> the bubble (due to your "y / 2" calculations).  If instead you position all
the
> controls based "height() - padding - control.height()" (effectively "pinning
> them" to the bottom), like the old code did, then it will look more like the
> bubble contents are sliding onscreen and sliding off, which should look a lot
> better.  (I like to test this sort of thing by slowing the slide animation
down
> to take like 5 seconds total, so I can see the animation very clearly.)

The bubble size doesn't actually get animated - the containing widget (ie: the
popup)'s height gets animated to zero, but we NULL out its LayoutManager so it
doesn't cause the FullScreenExitBubbleView to change size.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:187:
message_label_.SetBounds(insets.left() + kPaddingPx,
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: I think if you just use SetPosition() all these controls will be sized to
> their preferred sizes by default.

Done.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:338: int
animation_height =
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Another way to wrap this is:
> 
>     int animation_height = size_animation_->CurrentValueBetween(
>         static_cast<double>(total_height), 0.0f);
> 
> Also, |animation_height| is kind of a weird and confusing name here.  I don't
> really know what name would be better, maybe this just needs a comment about
> what it means or something.

How about popup_bottom?

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:420: // TODO(koz):
Implement fullscreen content settings UI.
On 2011/10/13 23:48:06, Peter Kasting wrote:
> Nit: Is there a bug for this?  Seems like this should be covered by a bug
rather
> than a TODO...

Actually, I don't think there are any plans to do this. This was speculative
comment. Removed.

[Peter Kasting, 2011-10-14 01:23:19]

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:178: int inner_height =
height() - insets.height();
On 2011/10/14 01:06:13, koz wrote:
> The bubble size doesn't actually get animated - the containing widget (ie: the
> popup)'s height gets animated to zero, but we NULL out its LayoutManager so it
> doesn't cause the FullScreenExitBubbleView to change size.

I missed that, oops.

Does that do the right thing?  Won't the contained view then  just always get
laid out at the same y-coordinate, so it will look like the top is pinned and
the bottom is being rolled up, rather than like the whole thing is sliding
offscreen?

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:338: int
animation_height =
On 2011/10/14 01:06:13, koz wrote:
> On 2011/10/13 23:48:06, Peter Kasting wrote:
> > Nit: Another way to wrap this is:
> > 
> >     int animation_height = size_animation_->CurrentValueBetween(
> >         static_cast<double>(total_height), 0.0f);
> > 
> > Also, |animation_height| is kind of a weird and confusing name here.  I
don't
> > really know what name would be better, maybe this just needs a comment about
> > what it means or something.
> 
> How about popup_bottom?

That would be fine if this were the onscreen y-coordinate of the popup bottom.

I think the core problem here is that |animation_height| means something weird
and so it's hard to find a good name for it.  Perhaps changing its name and its
meaning would make reading this code easier to understand.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:420: // TODO(koz):
Implement fullscreen content settings UI.
On 2011/10/14 01:06:13, koz wrote:
> On 2011/10/13 23:48:06, Peter Kasting wrote:
> > Nit: Is there a bug for this?  Seems like this should be covered by a bug
> rather
> > than a TODO...
> 
> Actually, I don't think there are any plans to do this. This was speculative
> comment. Removed.

Um, I don't think that's OK.  This needs content settings UI.

[jeremya, 2011-10-14 02:01:49]

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:178: int inner_height =
height() - insets.height();
On 2011/10/14 01:23:19, Peter Kasting wrote:
> On 2011/10/14 01:06:13, koz wrote:
> > The bubble size doesn't actually get animated - the containing widget (ie:
the
> > popup)'s height gets animated to zero, but we NULL out its LayoutManager so
it
> > doesn't cause the FullScreenExitBubbleView to change size.
> 
> I missed that, oops.
> 
> Does that do the right thing?  Won't the contained view then  just always get
> laid out at the same y-coordinate, so it will look like the top is pinned and
> the bottom is being rolled up, rather than like the whole thing is sliding
> offscreen?

We manually set the y-coordinate of the contained view in AnimationProgressed.

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/content_settings_handler.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/content_settings_handler.cc:420: // TODO(koz):
Implement fullscreen content settings UI.
On 2011/10/14 01:23:19, Peter Kasting wrote:
> On 2011/10/14 01:06:13, koz wrote:
> > On 2011/10/13 23:48:06, Peter Kasting wrote:
> > > Nit: Is there a bug for this?  Seems like this should be covered by a bug
> > rather
> > > than a TODO...
> > 
> > Actually, I don't think there are any plans to do this. This was speculative
> > comment. Removed.
> 
> Um, I don't think that's OK.  This needs content settings UI.

http://crbug.com/100292

I don't think it's urgent to have that UI for M16, so that bug is marked
Mstone-17.

[Nico, 2011-10-14 06:21:23]

cocoa parts lgtm

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/cocoa/ful...
File chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm (right):

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:28: namespace {
const has implicit internal linkage, no need for the unnamed namespace

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/cocoa/ful...
chrome/browser/ui/cocoa/fullscreen_exit_bubble_controller.mm:102: [bubble_
setArrowLocation:info_bubble::kNoArrow];
infoBubble

[tony, 2011-10-14 17:29:56]

gtk changes LGTM

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/fullscree...
File chrome/browser/ui/fullscreen_exit_bubble.cc (right):

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/fullscree...
chrome/browser/ui/fullscreen_exit_bubble.cc:43: // | _  _  _  _ | Exit full
screen mode (F11) | _  _  _  _ |  Slide-in region
Nit: You may want to update this text or change it to some placeholder value.

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/gtk/fulls...
File chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc (right):

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/gtk/fulls...
chrome/browser/ui/gtk/fullscreen_exit_bubble_gtk.cc:50: 
Nit: stray blank line.  Maybe convert to a TODO() with a bug link to implement
the new UI.

[yzshen1, 2011-10-14 17:32:39]

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/gtk/brows...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/gtk/brows...
chrome/browser/ui/gtk/browser_window_gtk.cc:1441:
fullscreen_exit_bubble_.reset(new FullscreenExitBubbleGtk(
Can this line compile?

[Peter Kasting, 2011-10-14 17:36:52]

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/70001/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:178: int inner_height =
height() - insets.height();
On 2011/10/14 02:01:49, jeremya wrote:
> On 2011/10/14 01:23:19, Peter Kasting wrote:
> > Does that do the right thing?  Won't the contained view then  just always
get
> > laid out at the same y-coordinate, so it will look like the top is pinned
and
> > the bottom is being rolled up, rather than like the whole thing is sliding
> > offscreen?
> 
> We manually set the y-coordinate of the contained view in AnimationProgressed.

I see.  Dang, this is really confusing.  There needs to be a big comment
somewhere (maybe in GetPopupRect()?) that explains the entire animation process
and layout design, and things we do elsewhere can have comments referring
readers over to that.

[Peter Kasting, 2011-10-14 20:33:28]

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/views/ful...
File chrome/browser/ui/views/fullscreen_exit_bubble_views.cc (right):

http://codereview.chromium.org/7740044/diff/83002/chrome/browser/ui/views/ful...
chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:104:
link_.SetPressedColor(message_label_.enabled_color());
Nit: Add a comment like:

  // Get the message's enabled_color() before setting its background color, as
doing that can change the enabled color, and we want both labels to have the
same underlying "desired" color.

[commit-bot: I haz the power, 2011-10-15 00:27:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7740044/83002

[commit-bot: I haz the power, 2011-10-15 00:28:05]

Can't apply patch for file
chrome/browser/content_settings/content_settings_utils.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/content_settings/content_settings_utils.cc
Hunk #1 FAILED at 26.
Hunk #2 FAILED at 43.
Hunk #3 succeeded at 24 (offset -33 lines).
2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/content_settings/content_settings_utils.cc.rej

[Nico, 2011-10-15 00:28:07]

Sounds like they're mostly nits on the windows part, and the rest has lgs…i'ma
land this, and koz will do a follow-up for the remaining comments.

[commit-bot: I haz the power, 2011-10-15 00:28:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7740044/83002

[commit-bot: I haz the power, 2011-10-15 00:29:17]

Can't apply patch for file
chrome/browser/content_settings/content_settings_utils.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/content_settings/content_settings_utils.cc
Hunk #1 FAILED at 26.
Hunk #2 FAILED at 43.
Hunk #3 succeeded at 24 (offset -33 lines).
2 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/content_settings/content_settings_utils.cc.rej

[Ben Goodger (Google), 2011-10-15 00:52:44]

owners rubber stamp LGTM

[commit-bot: I haz the power, 2011-10-15 01:53:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/koz@chromium.org/7740044/94001

[commit-bot: I haz the power, 2011-10-15 01:53:45]

Presubmit check for 7740044-94001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
New code should not use wstrings.  If you are calling an API that accepts a
wstring, fix the API.
    chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:39
    chrome/browser/ui/views/fullscreen_exit_bubble_views.cc:72

Presubmit checks took 6.2s to calculate.