Linux: Fix regression with --kiosk.

Linux: Fix regression with --kiosk.

BUG=470265
TEST=Manual, --kiosk should open in kiosk mode.

[Lei Zhang, 2015-03-27 22:14:34]

thestig@chromium.org changed reviewers:
+ pkasting@chromium.org

[Lei Zhang, 2015-03-27 22:14:34]

The problem with the existing code is that BrowserList::GetLastActive() returns
NULL. Immediately after Browser creation, BrowserList::SetLastActive() hasn't
been called yet.

[Peter Kasting, 2015-03-27 22:18:00]

I gather this was easier than explicitly calling SetLastActive() as the old CL
did?

LGTM

https://codereview.chromium.org/1044523002/diff/1/chrome/browser/ui/startup/s...
File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):

https://codereview.chromium.org/1044523002/diff/1/chrome/browser/ui/startup/s...
chrome/browser/ui/startup/startup_browser_creator_impl.h:104: // Returens the
Browser that processed |urls_to_open|.
Returns

[Lei Zhang, 2015-03-27 22:25:46]

On 2015/03/27 22:18:00, Peter Kasting wrote:
> I gather this was easier than explicitly calling SetLastActive() as the old CL
> did?

The code never called SetLastActive(). Before, OpenTabsInBrowser() just used the
Browser* it got, and now the Browser* is being passed back up.

>
https://codereview.chromium.org/1044523002/diff/1/chrome/browser/ui/startup/s...
> File chrome/browser/ui/startup/startup_browser_creator_impl.h (right):
> 
>
https://codereview.chromium.org/1044523002/diff/1/chrome/browser/ui/startup/s...
> chrome/browser/ui/startup/startup_browser_creator_impl.h:104: // Returens the
> Browser that processed |urls_to_open|.
> Returns

Done.

[Peter Kasting, 2015-03-27 22:29:43]

On 2015/03/27 22:25:46, Lei Zhang wrote:
> On 2015/03/27 22:18:00, Peter Kasting wrote:
> > I gather this was easier than explicitly calling SetLastActive() as the old
CL
> > did?
> 
> The code never called SetLastActive(). Before, OpenTabsInBrowser() just used
the
> Browser* it got, and now the Browser* is being passed back up.

No, I mean the fix in https://codereview.chromium.org/2847090 to make sure Linux
Views was calling SetLastActive() the way Windows Views was.

[Lei Zhang, 2015-03-27 22:47:02]

On 2015/03/27 22:29:43, Peter Kasting wrote:
> On 2015/03/27 22:25:46, Lei Zhang wrote:
> > On 2015/03/27 22:18:00, Peter Kasting wrote:
> > > I gather this was easier than explicitly calling SetLastActive() as the
old
> CL
> > > did?
> > 
> > The code never called SetLastActive(). Before, OpenTabsInBrowser() just used
> the
> > Browser* it got, and now the Browser* is being passed back up.
> 
> No, I mean the fix in https://codereview.chromium.org/2847090 to make sure
Linux
> Views was calling SetLastActive() the way Windows Views was.

Ah, let me take another look at this then.

[Lei Zhang, 2015-03-27 23:08:04]

So on Windows, BrowserFrame::OnNativeWidgetActivationChanged() or
BrowserView::OnWidgetActivationChanged() gets called before we look for the
kiosk switch? On Linux, they get called after. I see
https://codereview.chromium.org/2847090/ got reverted, but it's not obvious why.
Are you suggesting trying to reapply it, rather than doing this CL?

[Lei Zhang, 2015-03-27 23:14:21]

On 2015/03/27 23:08:04, Lei Zhang wrote:
> So on Windows, BrowserFrame::OnNativeWidgetActivationChanged() or
> BrowserView::OnWidgetActivationChanged() gets called before we look for the
> kiosk switch? On Linux, they get called after. I see
> https://codereview.chromium.org/2847090/ got reverted, but it's not obvious
why.
> Are you suggesting trying to reapply it, rather than doing this CL?

Adding BrowserList::SetLastActive() works for me, and it looks like
BrowserWindowCocoa::Show() calls BrowserList::SetLastActive() as well.

[Peter Kasting, 2015-03-27 23:46:55]

On 2015/03/27 23:14:21, Lei Zhang wrote:
> On 2015/03/27 23:08:04, Lei Zhang wrote:
> > So on Windows, BrowserFrame::OnNativeWidgetActivationChanged() or
> > BrowserView::OnWidgetActivationChanged() gets called before we look for the
> > kiosk switch? On Linux, they get called after. I see
> > https://codereview.chromium.org/2847090/ got reverted, but it's not obvious
> why.
> > Are you suggesting trying to reapply it, rather than doing this CL?
> 
> Adding BrowserList::SetLastActive() works for me, and it looks like
> BrowserWindowCocoa::Show() calls BrowserList::SetLastActive() as well.

I'm all for changes that make the three platforms more consistent.  If it
happens to obviate the need for the rest of this CL, that's great.

The CL as you've written it doesn't seem wrong, either; I'm not opposed to it if
you think it's a win, but I figured the other change might be simpler and might
avoid any future edge cases that expected an active window sooner.

[Lei Zhang, 2015-03-28 02:06:48]

On 2015/03/27 23:46:55, Peter Kasting wrote:
> On 2015/03/27 23:14:21, Lei Zhang wrote:
> > On 2015/03/27 23:08:04, Lei Zhang wrote:
> > > So on Windows, BrowserFrame::OnNativeWidgetActivationChanged() or
> > > BrowserView::OnWidgetActivationChanged() gets called before we look for
the
> > > kiosk switch? On Linux, they get called after. I see
> > > https://codereview.chromium.org/2847090/ got reverted, but it's not
obvious
> > why.
> > > Are you suggesting trying to reapply it, rather than doing this CL?
> > 
> > Adding BrowserList::SetLastActive() works for me, and it looks like
> > BrowserWindowCocoa::Show() calls BrowserList::SetLastActive() as well.
> 
> I'm all for changes that make the three platforms more consistent.  If it
> happens to obviate the need for the rest of this CL, that's great.
> 
> The CL as you've written it doesn't seem wrong, either; I'm not opposed to it
if
> you think it's a win, but I figured the other change might be simpler and
might
> avoid any future edge cases that expected an active window sooner.

Sounds good. Closing this CL.