Linux/Mac Views: Make BrowserWindow::Show() behave like Windows.

Linux/Mac Views: Make BrowserWindow::Show() behave like Windows.

Calling BrowserWindow::Show() should synchronously call
BrowserList::SetLastActive().

This fixes a --kiosk startup regression.

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

Committed: https://crrev.com/8d447bb8d38166299820c3ec8410d06d0e1ddbbd
Cr-Commit-Position: refs/heads/master@{#322940}

[Lei Zhang, 2015-03-28 02:31:41]

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

[Lei Zhang, 2015-03-28 02:31:41]

Based on discussion from https://codereview.chromium.org/1044523002/

[Peter Kasting, 2015-03-28 02:35:38]

LGTM

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:687: // A similar block also
appears in BrowserWindowCocoa::Show().
Nit: I would replace the last sentence of the comment below with this one and
change "Linux Views" below to "other platforms".

Then the comment is just as applicable to both Linux and Mac both now and in any
presumptive Mac Views-based future.

[tapted, 2015-03-28 03:10:57]

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:688: #if !defined(OS_WIN)
add `&& !defined(OS_CHROMEOS)`?

The downside of not excluding ChromeOS is probably minor, but non-desktop Aura
widgets activate synchronously, like Windows.

The same would apply for browser windows on Linux running inside an Ash desktop,
but I doubt it's worth singling that out.

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:695: // Mac Views will likely have
the same problem, so do this for Mac as well.
yup - activation on Mac is always asynchronous.

[Peter Kasting, 2015-03-28 03:12:52]

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:688: #if !defined(OS_WIN)
On 2015/03/28 03:10:57, tapted wrote:
> add `&& !defined(OS_CHROMEOS)`?
> 
> The downside of not excluding ChromeOS is probably minor, but non-desktop Aura
> widgets activate synchronously, like Windows.
> 
> The same would apply for browser windows on Linux running inside an Ash
desktop,
> but I doubt it's worth singling that out.

Hmm, is OS_CHROMEOS right then?  It sounds like this has more to do with some
kind of AURA or ASH #define...

[tapted, 2015-03-28 03:35:32]

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1048473002/diff/1/chrome/browser/ui/views/fra...
chrome/browser/ui/views/frame/browser_view.cc:688: #if !defined(OS_WIN)
On 2015/03/28 03:12:52, Peter Kasting wrote:
> On 2015/03/28 03:10:57, tapted wrote:
> > add `&& !defined(OS_CHROMEOS)`?
> > 
> > The downside of not excluding ChromeOS is probably minor, but non-desktop
Aura
> > widgets activate synchronously, like Windows.
> > 
> > The same would apply for browser windows on Linux running inside an Ash
> desktop,
> > but I doubt it's worth singling that out.
> 
> Hmm, is OS_CHROMEOS right then?  It sounds like this has more to do with some
> kind of AURA or ASH #define...

Yeah - it would be a complexity/correctness tradeoff...

Another option could be to override NativeWidgetPrivate::Activate() (which
*should* be called via the frame_->Show() call just above).
DesktopBrowserFrameAuraLinux (and BrowserFrameMac*) already exist and I think
they already have access to do something like

  DesktopBrowserFrameAura::Activate();
  // comment
  BrowserList::SetLastActive(browser_view()->browser());


*I can keep this on my radar and just worry about Mac later

[Lei Zhang, 2015-03-30 22:42:16]

In patch set 2, it's still #if !defined(OS_WIN), but inside I've added a check
to see if the desktop is Ash.

[tapted, 2015-03-30 23:01:04]

noticed an early exit case, it might make sense to move the code earlier in the
function - see what you think (otherwise lgtm)

https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:662: if (frame_->IsVisible()) {
just noticed this early exit condition.. it's probably not relevant for kiosk
mode, but it might explain why error bubbles provided by
GlobalErrorServiceFactory sometimes get lost on session restore..

[Lei Zhang, 2015-03-30 23:16:56]

On 2015/03/30 23:01:04, tapted wrote:
> noticed an early exit case, it might make sense to move the code earlier in
the
> function - see what you think (otherwise lgtm)

I don't have a strong opinion.

>
https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/frame/browser_view.cc (right):
> 
>
https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/frame/browser_view.cc:662: if (frame_->IsVisible()) {
> just noticed this early exit condition.. it's probably not relevant for kiosk
> mode, but it might explain why error bubbles provided by
> GlobalErrorServiceFactory sometimes get lost on session restore..

Is there a bug for this? How about we land this CL as is, and whoever is looking
into this problem can consider moving the call to the top?

[Peter Kasting, 2015-03-30 23:22:21]

On 2015/03/30 23:16:56, Lei Zhang wrote:
> On 2015/03/30 23:01:04, tapted wrote:
> > noticed an early exit case, it might make sense to move the code earlier in
> the
> > function - see what you think (otherwise lgtm)
> 
> I don't have a strong opinion.
> 
> >
>
https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
> > chrome/browser/ui/views/frame/browser_view.cc:662: if (frame_->IsVisible())
{
> > just noticed this early exit condition.. it's probably not relevant for
kiosk
> > mode, but it might explain why error bubbles provided by
> > GlobalErrorServiceFactory sometimes get lost on session restore..
> 
> Is there a bug for this? How about we land this CL as is, and whoever is
looking
> into this problem can consider moving the call to the top?

Why would that be preferable?  If we're adding this call anyway, it's just as
easy to add it at the top as the bottom, and the top seems more correct to me.

[Lei Zhang, 2015-03-30 23:32:27]

On 2015/03/30 23:22:21, Peter Kasting wrote:
> Why would that be preferable?  If we're adding this call anyway, it's just as
> easy to add it at the top as the bottom, and the top seems more correct to me.

To the top it goes. There might be some cases where we end up calling
SetLastActive() twice as a result, but I suppose that won't hurt.

[tapted, 2015-03-30 23:40:05]

slgtm :)

On 2015/03/30 23:16:56, Lei Zhang wrote:
> On 2015/03/30 23:01:04, tapted wrote:
> > noticed an early exit case, it might make sense to move the code earlier in
> the
> > function - see what you think (otherwise lgtm)
> 
> I don't have a strong opinion.
> 
> >
>
https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/frame/browser_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/1048473002/diff/20001/chrome/browser/ui/views...
> > chrome/browser/ui/views/frame/browser_view.cc:662: if (frame_->IsVisible())
{
> > just noticed this early exit condition.. it's probably not relevant for
kiosk
> > mode, but it might explain why error bubbles provided by
> > GlobalErrorServiceFactory sometimes get lost on session restore..
> 
> Is there a bug for this? How about we land this CL as is, and whoever is
looking
> into this problem can consider moving the call to the top?

http://crbug.com/383644 perhaps. Or http://crbug.com/376161. I'm really just
thinking aloud about a hunch - it's pretty elusive and hard to tell whether it
was intended behavior. It's more often "extension Foo needs new permissions"
bubbles, so leave it with me to prod further.

It would relate to the browser()->OnWindowDidShow(); call further down, so
orthogonal to this CL in any case.

[Lei Zhang, 2015-03-31 01:28:03]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2015-03-31 01:28:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1048473002/#ps40001
(title: "Move to the top of BrowserView::Show()")

[commit-bot: I haz the power, 2015-03-31 01:29:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1048473002/40001

[commit-bot: I haz the power, 2015-03-31 01:34:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-31 01:36:02]

Patchset 3 (id:??) landed as
https://crrev.com/8d447bb8d38166299820c3ec8410d06d0e1ddbbd
Cr-Commit-Position: refs/heads/master@{#322940}