linux_aura: Port the linux first run dialog to aura.

linux_aura: Port the linux first run dialog to aura.

On Linux, we need to show a dialog to get permission to collect UMA data
and to change the default browser.

BUG=234637
R=msw@chromium.org, sky@chromium.org

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

[msw, 2014-01-15 20:22:41]

Nice work, I'm sorry I dragged my feet on this. You'll want to get review Scott
or someone else that understands the nuance of showing a Views dialog before a
browser window exists, there are issues with ref counting there iirc, see
<http://crrev.com/200732> for example.

https://codereview.chromium.org/137393008/diff/1/chrome/browser/shell_integra...
File chrome/browser/shell_integration_linux.cc (left):

https://codereview.chromium.org/137393008/diff/1/chrome/browser/shell_integra...
chrome/browser/shell_integration_linux.cc:342:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
Why remove this?

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
File chrome/browser/ui/views/first_run_dialog.cc (right):

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:53: bool show_reporting_dialog =
!metrics_reporting_pref ||
nit: this could be inlined

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:58:
CreateBrowserModalDialogViews(dialog, NULL)->Show();
Will this show over an actual running browser window? If so, can you try to find
it for parenting here? Otherwise, this should probably use
DialogDelegate::CreateDialogWidget (and supply a context if one is available).

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:88: void FirstRunDialog::Init() {
nit: this could be inlined to the ctor

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:109: //
layout->StartRowWithPadding(0, 0, 0, related_y);
nit: remove commented code.

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:120: void FirstRunDialog::OnClosed()
{
Should this also set should_show_dialog_ to false?

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:125: should_show_dialog_= false;
nit: add a space before '='.

[Elliot Glaysher, 2014-01-15 20:41:31]

+sky for review

https://codereview.chromium.org/137393008/diff/1/chrome/browser/shell_integra...
File chrome/browser/shell_integration_linux.cc (left):

https://codereview.chromium.org/137393008/diff/1/chrome/browser/shell_integra...
chrome/browser/shell_integration_linux.cc:342:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2014/01/15 20:22:41, msw wrote:
> Why remove this?

Because its wrong.

I assume that the reason its here is because it goes out to the filesystem and
most times that this code would be called, its called while the main browser
window is up and a file thread exists. This isn't the case during the early
bring up.

I verified that it triggers today in debug gtk builds with --force-first-run
that this is erroniously triggering. It's wrong today.

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
File chrome/browser/ui/views/first_run_dialog.cc (right):

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:58:
CreateBrowserModalDialogViews(dialog, NULL)->Show();
On 2014/01/15 20:22:41, msw wrote:
> Will this show over an actual running browser window? If so, can you try to
find
> it for parenting here? Otherwise, this should probably use
> DialogDelegate::CreateDialogWidget (and supply a context if one is available).

I previously did that and had some focus issues. I copied this because its used
in simple_message_box_views.cc, which we also use in an early pre-Browser world
to show the root error dialog.

[msw, 2014-01-15 20:59:37]

LGTM with a minor comment.

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
File chrome/browser/ui/views/first_run_dialog.cc (right):

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:58:
CreateBrowserModalDialogViews(dialog, NULL)->Show();
On 2014/01/15 20:41:31, Elliot Glaysher wrote:
> On 2014/01/15 20:22:41, msw wrote:
> > Will this show over an actual running browser window? If so, can you try to
> find
> > it for parenting here? Otherwise, this should probably use
> > DialogDelegate::CreateDialogWidget (and supply a context if one is
available).
> 
> I previously did that and had some focus issues. I copied this because its
used
> in simple_message_box_views.cc, which we also use in an early pre-Browser
world
> to show the root error dialog.

Hmm, then doesn't this just equate to DialogDelegate::CreateDialogWidget(dialog,
NULL, NULL)? I would avoid using CreateBrowserModalDialogViews for a dialog that
is not browser modal; you can also then remove the constrained_window_views.h
include.

[sky, 2014-01-15 21:36:11]

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/first_run_dialog.h (right):

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_dialog.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_FIRST_RUN_DIALOG_H_
Should we name this first_run_dialog_aurax11 as that is the only place it's
used?

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_dialog.h:21: public
base::MessageLoop::Dispatcher {
Use MessagePumpDispatcher (MessageLoop::Dispatcher is a typedef and I'm in the
process of nuking it).

[Elliot Glaysher, 2014-01-15 22:01:39]

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
File chrome/browser/ui/views/first_run_dialog.cc (right):

https://codereview.chromium.org/137393008/diff/1/chrome/browser/ui/views/firs...
chrome/browser/ui/views/first_run_dialog.cc:58:
CreateBrowserModalDialogViews(dialog, NULL)->Show();
On 2014/01/15 20:59:37, msw wrote:
> On 2014/01/15 20:41:31, Elliot Glaysher wrote:
> > On 2014/01/15 20:22:41, msw wrote:
> > > Will this show over an actual running browser window? If so, can you try
to
> > find
> > > it for parenting here? Otherwise, this should probably use
> > > DialogDelegate::CreateDialogWidget (and supply a context if one is
> available).
> > 
> > I previously did that and had some focus issues. I copied this because its
> used
> > in simple_message_box_views.cc, which we also use in an early pre-Browser
> world
> > to show the root error dialog.
> 
> Hmm, then doesn't this just equate to
DialogDelegate::CreateDialogWidget(dialog,
> NULL, NULL)? I would avoid using CreateBrowserModalDialogViews for a dialog
that
> is not browser modal; you can also then remove the constrained_window_views.h
> include.

Tested again. Worked this time. Done.

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/first_run_dialog.h (right):

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_dialog.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_FIRST_RUN_DIALOG_H_
On 2014/01/15 21:36:11, sky wrote:
> Should we name this first_run_dialog_aurax11 as that is the only place it's
> used?

There isn't anything conceptually x11-ish about it. There isn't anything
conceptually linux-ish about it either. (There is something conceptually
aura-ish about it, but at this point, views implies aura so I see that as a
wash.)

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_dialog.h:21: public
base::MessageLoop::Dispatcher {
On 2014/01/15 21:36:11, sky wrote:
> Use MessagePumpDispatcher (MessageLoop::Dispatcher is a typedef and I'm in the
> process of nuking it).

Done.

[sky, 2014-01-16 00:29:39]

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/first_run_dialog.h (right):

https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/first_run_dialog.h:5: #ifndef
CHROME_BROWSER_UI_VIEWS_FIRST_RUN_DIALOG_H_
On 2014/01/15 22:01:40, Elliot Glaysher wrote:
> On 2014/01/15 21:36:11, sky wrote:
> > Should we name this first_run_dialog_aurax11 as that is the only place it's
> > used?
> 
> There isn't anything conceptually x11-ish about it. There isn't anything
> conceptually linux-ish about it either. (There is something conceptually
> aura-ish about it, but at this point, views implies aura so I see that as a
> wash.)

True. My rationale for this request is two fold:
. Folks can readily see this is only used on aura-x11.
. AFAIK this file is only needed on aura-x11.

If I'm wrong on the second point, then I agree it's fine to keep it here.

[Elliot Glaysher, 2014-01-16 00:49:43]

On 2014/01/16 00:29:39, sky wrote:
>
https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
> File chrome/browser/ui/views/first_run_dialog.h (right):
> 
>
https://codereview.chromium.org/137393008/diff/100001/chrome/browser/ui/views...
> chrome/browser/ui/views/first_run_dialog.h:5: #ifndef
> CHROME_BROWSER_UI_VIEWS_FIRST_RUN_DIALOG_H_
> On 2014/01/15 22:01:40, Elliot Glaysher wrote:
> > On 2014/01/15 21:36:11, sky wrote:
> > > Should we name this first_run_dialog_aurax11 as that is the only place
it's
> > > used?
> > 
> > There isn't anything conceptually x11-ish about it. There isn't anything
> > conceptually linux-ish about it either. (There is something conceptually
> > aura-ish about it, but at this point, views implies aura so I see that as a
> > wash.)
> 
> True. My rationale for this request is two fold:
> . Folks can readily see this is only used on aura-x11.
> . AFAIK this file is only needed on aura-x11.
> 
> If I'm wrong on the second point, then I agree it's fine to keep it here.

The file is only needed today on aura on "desktop linux" OSes, regardless of
what actual OS (linux/bsd/solaris) or windowing system they use. Intel is
actively working on a Wayland version through ozone, and this file should be
those builds.

[sky, 2014-01-16 01:32:01]

Ok, LGTM

[commit-bot: I haz the power, 2014-01-16 02:10:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/erg@chromium.org/137393008/200001

[commit-bot: I haz the power, 2014-01-16 04:54:36]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Elliot Glaysher, 2014-01-16 18:20:10]

Committed patchset #3 manually as r245259 (presubmit successful).