[Mac] Retain location bar focus on switching to fullscreen and back.

[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740
Review-Url: https://codereview.chromium.org/2679273002
Cr-Original-Commit-Position: refs/heads/master@{#467074}
Committed: https://chromium.googlesource.com/chromium/src/+/2a657ecbdd2743e0cc7c4e5130a5b1caac88ec9e
Review-Url: https://codereview.chromium.org/2679273002
Cr-Commit-Position: refs/heads/master@{#469753}
Committed: https://chromium.googlesource.com/chromium/src/+/94f3ef666ed1521361039efefb5825b72e2b32f8

[shrike, 2017-02-07 20:04:01]

PTAL

[shrike, 2017-02-08 00:02:28]

Please hold off on this for now as I found a regression.

[shrike, 2017-02-08 01:43:17]

Description was changed from

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=avi@chromium.org
BUG=447740
==========

to

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740
==========

[shrike, 2017-02-08 01:43:18]

shrike@chromium.org changed reviewers:
+ spqchan@chromium.org

[shrike, 2017-02-08 01:43:59]

spqchan@ - PTAL for a sanity check.

[spqchan, 2017-02-08 19:21:51]

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:965: //
-setContentViewSubviews: changes the view hierarchy, and if the
"is focused will" -> "is focused, it will

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus) {
nit: No curly brackets required. Also maybe you can just directly put [self
locationBarHasFocus] in the if statement?

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
(right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:53: if
([browserController_ locationBarHasFocus]) {
Would it possible to move this out of fullscreen_toolbar_controller?
I prefer that this class is more focused on managing the different parts of
fullscreen and calculating the layout.

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:150: if
([visibilityLockController_ isToolbarVisibilityLockedForOwner:self]) {
Remove: This is redundant, L163 should cover this.

[shrike, 2017-02-08 19:51:42]

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:965: //
-setContentViewSubviews: changes the view hierarchy, and if the
On 2017/02/08 19:21:50, spqchan (SLOW Feb 8-9) wrote:
> "is focused will" -> "is focused, it will

I massaged it a bit more to avoid "it will cause it" which is confusing.

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus) {
On 2017/02/08 19:21:50, spqchan (SLOW Feb 8-9) wrote:
> nit: No curly brackets required. Also maybe you can just directly put [self
> locationBarHasFocus] in the if statement?

I removed the curly braces. Coding if statements without curly braces invites
bugs so I always add them, but I see that it is the style that you use.

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
(right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:53: if
([browserController_ locationBarHasFocus]) {
On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> Would it possible to move this out of fullscreen_toolbar_controller?
> I prefer that this class is more focused on managing the different parts of
> fullscreen and calculating the layout.

I could maybe do that with a fourth toolbar style (see my comment below),
otherwise I don't think it's possible. The fullscreen toolbar controller
controls when the toolbar is visible, so the code at this level needs to arrange
to keep the toolbar around. I can't arrange that from another class unless I use
a fourth toolbar style.

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:150: if
([visibilityLockController_ isToolbarVisibilityLockedForOwner:self]) {
On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> Remove: This is redundant, L163 should cover this.

I have to have this here, otherwise the code early outs on L157. The issue is
that there are three toolbar style modes (NONE, PRESENT, HIDDEN), but I need a
fourth style which would mean "keep the toolbar around as you go into fullscreen
mode". It seemed kind of ugly to add a fourth style so I opted to lock the
toolbar temporarily and to check for that lock in this method,

[spqchan, 2017-02-09 01:50:35]

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus) {
On 2017/02/08 19:51:42, shrike wrote:
> On 2017/02/08 19:21:50, spqchan (SLOW Feb 8-9) wrote:
> > nit: No curly brackets required. Also maybe you can just directly put [self
> > locationBarHasFocus] in the if statement?
> 
> I removed the curly braces. Coding if statements without curly braces invites
> bugs so I always add them, but I see that it is the style that you use.

Hey, to be fair, the reason I use this style in the first place is because it
got nitpicked by my reviewers  :)
According to rsesek and tapted, we shouldn't use curly braces if the if
statement only contains one line

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
(right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:53: if
([browserController_ locationBarHasFocus]) {
On 2017/02/08 19:51:42, shrike wrote:
> On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> > Would it possible to move this out of fullscreen_toolbar_controller?
> > I prefer that this class is more focused on managing the different parts of
> > fullscreen and calculating the layout.
> 
> I could maybe do that with a fourth toolbar style (see my comment below),
> otherwise I don't think it's possible. The fullscreen toolbar controller
> controls when the toolbar is visible, so the code at this level needs to
arrange
> to keep the toolbar around. I can't arrange that from another class unless I
use
> a fourth toolbar style.

Will something like this work?

In browser_window_controller_private.mm, change L785 to:

if ([self locationBarHasFocus])
    [self lockToolbarVisibilityForOwner:self withAnimation:NO];
[fullscreenToolbarController_ enterFullscreenMode];
[self releaseToolbarVisibilityForOwner:self withAnimation:NO];


But yeah, please don't introduce a fourth style.

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:150: if
([visibilityLockController_ isToolbarVisibilityLockedForOwner:self]) {
On 2017/02/08 19:51:42, shrike wrote:
> On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> > Remove: This is redundant, L163 should cover this.
> 
> I have to have this here, otherwise the code early outs on L157. The issue is
> that there are three toolbar style modes (NONE, PRESENT, HIDDEN), but I need a
> fourth style which would mean "keep the toolbar around as you go into
fullscreen
> mode". It seemed kind of ugly to add a fourth style so I opted to lock the
> toolbar temporarily and to check for that lock in this method,

I'm a bit confused, why do you want to show the toolbar when it's not supposed
to? AFAIK, if you fullscreen a youtube video, the toolbar is not supposed to
appear.

If it's supposed to appear though, we can just check the toolbar visibility
locks before checking the toolbar style, but I'm not sure why we want to show
the toolbar for TOOLBAR_NONE.

https://codereview.chromium.org/2679273002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus)
optional nit:  if ([self locationBarHasFocus]) and get rid of
|locationBarHasFocus|

https://codereview.chromium.org/2679273002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:1018: if
(locationBarHadFocus) {
Ditto from above

[shrike, 2017-02-09 02:18:29]

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus) {
On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> On 2017/02/08 19:51:42, shrike wrote:
> > On 2017/02/08 19:21:50, spqchan (SLOW Feb 8-9) wrote:
> > > nit: No curly brackets required. Also maybe you can just directly put
[self
> > > locationBarHasFocus] in the if statement?
> > 
> > I removed the curly braces. Coding if statements without curly braces
invites
> > bugs so I always add them, but I see that it is the style that you use.
> 
> Hey, to be fair, the reason I use this style in the first place is because it
> got nitpicked by my reviewers  :)
> According to rsesek and tapted, we shouldn't use curly braces if the if
> statement only contains one line

I hear you that your reviewers told you that curly braces were wrong. The style
guide allows either form. In my not-so-humble opinion, coding if statements
without curly braces is not right (and I've seen at least one bug in Chromium
source that resulted from this practice). The style guide says to adopt the
prevailing style of the surrounding code so I go with that, but when I can I
stick with curly braces even though the reviewers say it's a nit (it's not).

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
(right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:53: if
([browserController_ locationBarHasFocus]) {
On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> On 2017/02/08 19:51:42, shrike wrote:
> > On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> > > Would it possible to move this out of fullscreen_toolbar_controller?
> > > I prefer that this class is more focused on managing the different parts
of
> > > fullscreen and calculating the layout.
> > 
> > I could maybe do that with a fourth toolbar style (see my comment below),
> > otherwise I don't think it's possible. The fullscreen toolbar controller
> > controls when the toolbar is visible, so the code at this level needs to
> arrange
> > to keep the toolbar around. I can't arrange that from another class unless I
> use
> > a fourth toolbar style.
> 
> Will something like this work?
> 
> In browser_window_controller_private.mm, change L785 to:
> 
> if ([self locationBarHasFocus])
>     [self lockToolbarVisibilityForOwner:self withAnimation:NO];
> [fullscreenToolbarController_ enterFullscreenMode];
> [self releaseToolbarVisibilityForOwner:self withAnimation:NO];
> 
> 
> But yeah, please don't introduce a fourth style.

This may work, but unfortunately I still need a check inside of
mustShowFullscreenToolbar that would read

    if ([visibilityLockController_
isToolbarVisibilityLockedForOwner:browserWindowController_])...

so that would mean the fullscreen toolbar controller knowing that the browser
window controller might be locking visibility (ugh).

Or I could call lockToolbarVisibilityForOwner:... in BWC passing in the FTC as
the owner but that's also kind of yucky. More discussion below.

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:150: if
([visibilityLockController_ isToolbarVisibilityLockedForOwner:self]) {
On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> On 2017/02/08 19:51:42, shrike wrote:
> > On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> > > Remove: This is redundant, L163 should cover this.
> > 
> > I have to have this here, otherwise the code early outs on L157. The issue
is
> > that there are three toolbar style modes (NONE, PRESENT, HIDDEN), but I need
a
> > fourth style which would mean "keep the toolbar around as you go into
> fullscreen
> > mode". It seemed kind of ugly to add a fourth style so I opted to lock the
> > toolbar temporarily and to check for that lock in this method,
> 
> I'm a bit confused, why do you want to show the toolbar when it's not supposed
> to? AFAIK, if you fullscreen a youtube video, the toolbar is not supposed to
> appear.
> 
> If it's supposed to appear though, we can just check the toolbar visibility
> locks before checking the toolbar style, but I'm not sure why we want to show
> the toolbar for TOOLBAR_NONE.

If the toolbar is TOOLBAR_NONE then the toolbar should not appear. But if you go
into fullscreen mode with TOOLBAR_NONE and then you make the toolbar appear and
then click in the omnibox, the toolbar won't go away until you're done editing.
So even though the browser is configured for TOOLBAR_NONE, the toolbar needs to
be visible in this case. So far so good.

The problem is that we're trying to preserve first responder state in the
omnibox when you transition to fullscreen. So in this case we want the toolbar
to remain visible even though we're configured for TOOLBAR_NONE.

The crux of the problem is that -mustShowFullscreenToolbar determines whether or
not the toolbar should be visible at the start of fullscreen. Because we're
editing the omnibox we want it to return YES, but because we're configured for
TOOLBAR_NONE it returns NO. This happens even if the omnibox is locking
visibility because the visibility lock check occurs after we check for
TOOLBAR_NONE. So we need a special case to get -mustShowFullscreenToolbar to
return YES before it checks for TOOLBAR_NONE. I assume the check for
TOOLBAR_NONE is required before the general check for the visibility lock (i.e.
that you had a specific reason for ordering these checks this way), and that
moving the visibility check before the TOOLBAR_NONE check will break something.
If that's not the case, then I think that would be an easy fix.

https://codereview.chromium.org/2679273002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus)
On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> optional nit:  if ([self locationBarHasFocus]) and get rid of
> |locationBarHasFocus|

The problem is that [self setContentViewSubviews:subviews] changes the view
hierarchy and forces the location bar to resign first responder. If  [self
locationBarHasFocus] was true before the call to -setContentViewSubviews:, it
will be false afterwards.

[spqchan, 2017-02-09 05:09:36]

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus) {
On 2017/02/09 02:18:29, shrike wrote:
> On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> > On 2017/02/08 19:51:42, shrike wrote:
> > > On 2017/02/08 19:21:50, spqchan (SLOW Feb 8-9) wrote:
> > > > nit: No curly brackets required. Also maybe you can just directly put
> [self
> > > > locationBarHasFocus] in the if statement?
> > > 
> > > I removed the curly braces. Coding if statements without curly braces
> invites
> > > bugs so I always add them, but I see that it is the style that you use.
> > 
> > Hey, to be fair, the reason I use this style in the first place is because
it
> > got nitpicked by my reviewers  :)
> > According to rsesek and tapted, we shouldn't use curly braces if the if
> > statement only contains one line
> 
> I hear you that your reviewers told you that curly braces were wrong. The
style
> guide allows either form. In my not-so-humble opinion, coding if statements
> without curly braces is not right (and I've seen at least one bug in Chromium
> source that resulted from this practice). The style guide says to adopt the
> prevailing style of the surrounding code so I go with that, but when I can I
> stick with curly braces even though the reviewers say it's a nit (it's not).

Makes sense. Since the codebase will be changed so that the underscore will be
in the front of the variable instead of the end, perhaps this can go with it?

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm
(right):

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:53: if
([browserController_ locationBarHasFocus]) {
On 2017/02/09 02:18:29, shrike wrote:
> On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> > On 2017/02/08 19:51:42, shrike wrote:
> > > On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> > > > Would it possible to move this out of fullscreen_toolbar_controller?
> > > > I prefer that this class is more focused on managing the different parts
> of
> > > > fullscreen and calculating the layout.
> > > 
> > > I could maybe do that with a fourth toolbar style (see my comment below),
> > > otherwise I don't think it's possible. The fullscreen toolbar controller
> > > controls when the toolbar is visible, so the code at this level needs to
> > arrange
> > > to keep the toolbar around. I can't arrange that from another class unless
I
> > use
> > > a fourth toolbar style.
> > 
> > Will something like this work?
> > 
> > In browser_window_controller_private.mm, change L785 to:
> > 
> > if ([self locationBarHasFocus])
> >     [self lockToolbarVisibilityForOwner:self withAnimation:NO];
> > [fullscreenToolbarController_ enterFullscreenMode];
> > [self releaseToolbarVisibilityForOwner:self withAnimation:NO];
> > 
> > 
> > But yeah, please don't introduce a fourth style.
> 
> This may work, but unfortunately I still need a check inside of
> mustShowFullscreenToolbar that would read
> 
>     if ([visibilityLockController_
> isToolbarVisibilityLockedForOwner:browserWindowController_])...
> 
> so that would mean the fullscreen toolbar controller knowing that the browser
> window controller might be locking visibility (ugh).
> 
> Or I could call lockToolbarVisibilityForOwner:... in BWC passing in the FTC as
> the owner but that's also kind of yucky. More discussion below.

Ah gotcha, so we're locking the toolbar as a special case for the omnibox. Since
the toolbar is locked whenever something in it is focused, perhaps we should
show the toolbar regardless of the style if something on it is focused before it
entered fullscreen. A similar issue could happen with bookmarks if the user is
focusing on it before we enter fullscreen.

In that case, how about we change mustShowFullscreen so that it will always
return true if something, anything, is locking the toolbar? That way we don't
need to worry about checking if BWC is locked here and it'll fix the focusing
issue with other parts of the toolbar.

https://codereview.chromium.org/2679273002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/fullscreen/fullscreen_toolbar_controller.mm:150: if
([visibilityLockController_ isToolbarVisibilityLockedForOwner:self]) {
On 2017/02/09 02:18:29, shrike wrote:
> On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> > On 2017/02/08 19:51:42, shrike wrote:
> > > On 2017/02/08 19:21:51, spqchan (SLOW Feb 8-9) wrote:
> > > > Remove: This is redundant, L163 should cover this.
> > > 
> > > I have to have this here, otherwise the code early outs on L157. The issue
> is
> > > that there are three toolbar style modes (NONE, PRESENT, HIDDEN), but I
need
> a
> > > fourth style which would mean "keep the toolbar around as you go into
> > fullscreen
> > > mode". It seemed kind of ugly to add a fourth style so I opted to lock the
> > > toolbar temporarily and to check for that lock in this method,
> > 
> > I'm a bit confused, why do you want to show the toolbar when it's not
supposed
> > to? AFAIK, if you fullscreen a youtube video, the toolbar is not supposed to
> > appear.
> > 
> > If it's supposed to appear though, we can just check the toolbar visibility
> > locks before checking the toolbar style, but I'm not sure why we want to
show
> > the toolbar for TOOLBAR_NONE.
> 
> If the toolbar is TOOLBAR_NONE then the toolbar should not appear. But if you
go
> into fullscreen mode with TOOLBAR_NONE and then you make the toolbar appear
and
> then click in the omnibox, the toolbar won't go away until you're done
editing.
> So even though the browser is configured for TOOLBAR_NONE, the toolbar needs
to
> be visible in this case. So far so good.
> 
> The problem is that we're trying to preserve first responder state in the
> omnibox when you transition to fullscreen. So in this case we want the toolbar
> to remain visible even though we're configured for TOOLBAR_NONE.
> 
> The crux of the problem is that -mustShowFullscreenToolbar determines whether
or
> not the toolbar should be visible at the start of fullscreen. Because we're
> editing the omnibox we want it to return YES, but because we're configured for
> TOOLBAR_NONE it returns NO. This happens even if the omnibox is locking
> visibility because the visibility lock check occurs after we check for
> TOOLBAR_NONE. So we need a special case to get -mustShowFullscreenToolbar to
> return YES before it checks for TOOLBAR_NONE. I assume the check for
> TOOLBAR_NONE is required before the general check for the visibility lock
(i.e.
> that you had a specific reason for ordering these checks this way), and that
> moving the visibility check before the TOOLBAR_NONE check will break
something.
> If that's not the case, then I think that would be an easy fix.

See comment from above.

https://codereview.chromium.org/2679273002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller_private.mm (right):

https://codereview.chromium.org/2679273002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller_private.mm:971: if
(locationBarHadFocus)
On 2017/02/09 02:18:29, shrike wrote:
> On 2017/02/09 01:50:34, spqchan (SLOW Feb 8-9) wrote:
> > optional nit:  if ([self locationBarHasFocus]) and get rid of
> > |locationBarHasFocus|
> 
> The problem is that [self setContentViewSubviews:subviews] changes the view
> hierarchy and forces the location bar to resign first responder. If  [self
> locationBarHasFocus] was true before the call to -setContentViewSubviews:, it
> will be false afterwards.

Ah, gotcha

[shrike, 2017-04-11 21:46:11]

spqchan@ - I reworked this cl and now it's much simpler. PTAL.

[spqchan, 2017-04-11 22:13:03]

LGTM! Thanks for simplifying it

[shrike, 2017-04-11 22:38:09]

The CQ bit was checked by shrike@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 22:39:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 23:33:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 23:33:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Avi (use Gerrit), 2017-04-13 14:52:45]

lgtm

stamp

[shrike, 2017-04-25 18:40:28]

The CQ bit was checked by shrike@chromium.org

[commit-bot: I haz the power, 2017-04-25 18:41:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-25 19:25:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493145628476600,
"parent_rev": "35d6c61f99274d161d56edfcd83511a3da883833", "commit_rev":
"2a657ecbdd2743e0cc7c4e5130a5b1caac88ec9e"}

[commit-bot: I haz the power, 2017-04-25 19:25:58]

Description was changed from

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740
==========

to

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740

Review-Url: https://codereview.chromium.org/2679273002
Cr-Commit-Position: refs/heads/master@{#467074}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a657ecbdd2743e0cc7c4e5130a5...
==========

[commit-bot: I haz the power, 2017-04-25 19:25:59]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2a657ecbdd2743e0cc7c4e5130a5...

[hcarmona, 2017-04-25 20:54:00]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2840073002/ by hcarmona@chromium.org.

The reason for reverting is: Suspected of causing test failures:
https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac10.9%20Tests/...

BrowserWindowFullScreenControllerTest.TestActivate
BrowserWindowFullScreenControllerTest.TestFullscreen.

[findit-for-me, 2017-04-25 21:57:49]

Findit(https://goo.gl/kROfz5) identified this CL at revision 467074 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[shrike, 2017-05-01 22:34:42]

Description was changed from

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740

Review-Url: https://codereview.chromium.org/2679273002
Cr-Commit-Position: refs/heads/master@{#467074}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a657ecbdd2743e0cc7c4e5130a5...
==========

to

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740

Review-Url: https://codereview.chromium.org/2679273002
Cr-Commit-Position: refs/heads/master@{#467074}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a657ecbdd2743e0cc7c4e5130a5...
==========

[shrike, 2017-05-04 20:45:37]

spqchan@ - as part of my change in browser_window_controller_private.mm I call
focusLocationBar: to refocus the toolbar, but this is causing some fullscreen
tests to fail (BrowserWindowFullScreenControllerTest.TestActivate and
BrowserWindowFullScreenControllerTest.TestFullscreen). The tests complain about
windows not being cleaned up at the end of the test. I can't reproduce the
problem on my machine, and I believe the windows are being cleaned up, just not
completely freed (i.e. even though they are offscreen they still appear in
-[NSApplication windows]. I say that because there's a comment in the TearDown()
method in ui_cocoa_test_helper.mm about NSTextView keeping windows from freeing
and I'm thinking something like that is going on here. I tried increasing the
spins of the run loop from 2 to 10 in that code but it made no difference

I've tried a number of other changes to try to fix this problem. My current
change replaces focusLocationBar: with a no op from within the test. Is this
acceptable? If not, do you have any thoughts?

[spqchan, 2017-05-04 22:06:12]

On 2017/05/04 20:45:37, shrike wrote:
> spqchan@ - as part of my change in browser_window_controller_private.mm I call
> focusLocationBar: to refocus the toolbar, but this is causing some fullscreen
> tests to fail (BrowserWindowFullScreenControllerTest.TestActivate and
> BrowserWindowFullScreenControllerTest.TestFullscreen). The tests complain
about
> windows not being cleaned up at the end of the test. I can't reproduce the
> problem on my machine, and I believe the windows are being cleaned up, just
not
> completely freed (i.e. even though they are offscreen they still appear in
> -[NSApplication windows]. I say that because there's a comment in the
TearDown()
> method in ui_cocoa_test_helper.mm about NSTextView keeping windows from
freeing
> and I'm thinking something like that is going on here. I tried increasing the
> spins of the run loop from 2 to 10 in that code but it made no difference
> 
> I've tried a number of other changes to try to fix this problem. My current
> change replaces focusLocationBar: with a no op from within the test. Is this
> acceptable? If not, do you have any thoughts?

LGTM on this approach.

[shrike, 2017-05-05 19:04:24]

The CQ bit was checked by shrike@chromium.org

[shrike, 2017-05-05 19:04:25]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
spqchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2679273002/#ps340001
(title: "Undo previous change, add comment.")

[commit-bot: I haz the power, 2017-05-05 19:04:59]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-05 19:23:40]

CQ is committing da patch.

Bot data: {"patchset_id": 340001, "attempt_start_ts": 1494011064558280,
"parent_rev": "65260aad824952d720217771840f45bad9067507", "commit_rev":
"94f3ef666ed1521361039efefb5825b72e2b32f8"}

[commit-bot: I haz the power, 2017-05-05 19:23:48]

Description was changed from

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740

Review-Url: https://codereview.chromium.org/2679273002
Cr-Commit-Position: refs/heads/master@{#467074}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a657ecbdd2743e0cc7c4e5130a5...
==========

to

==========
[Mac] Retain location bar focus on switching to fullscreen and back.

If a window's location bar has focus before switching to fullscreen
mode, it no longer has it afterwards (and the same for the fullscreen-
to-normal transition). This cl ensures that the location bar retains
focus (if it has it) through both transitions.

R=spqchan@chromium.org,avi@chromium.org
BUG=447740

Review-Url: https://codereview.chromium.org/2679273002
Cr-Original-Commit-Position: refs/heads/master@{#467074}
Committed:
https://chromium.googlesource.com/chromium/src/+/2a657ecbdd2743e0cc7c4e5130a5...
Review-Url: https://codereview.chromium.org/2679273002
Cr-Commit-Position: refs/heads/master@{#469753}
Committed:
https://chromium.googlesource.com/chromium/src/+/94f3ef666ed1521361039efefb58...
==========

[commit-bot: I haz the power, 2017-05-05 19:23:49]

Committed patchset #18 (id:340001) as
https://chromium.googlesource.com/chromium/src/+/94f3ef666ed1521361039efefb58...