Convert banned calls to use Navigate in ash app list.

Convert banned calls to use Navigate in ash app list.

The ash app list uses FindLastActiveWithProfile and FindOrCreateTabbedBrowser, which are banned. This change replaces them with calls to chrome::Navigate.

BUG=138632
TEST=Check the app list on ChromeOS can be used to show application options and links from the search box, with and without chrome browser windows open.


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

[benwells, 2012-08-13 08:47:20]

I am removing these calls as I want to move these files and the presubmit queue
complains if I try to do so.

When testing by building with chromeos=1 on Linux this triggers some openGL
assertion, which I don't think has anything to do with the change.

[benwells, 2012-08-13 08:47:52]

-beng, +Ben Goodger

[Ben Goodger (Google), 2012-08-13 15:32:07]

LGTM. Note that in practice, right now, this resolves to the same banned call,
used by Navigate(), but at least that's only one banned place, rather than
several!

[xiyuan, 2012-08-13 16:20:55]

https://chromiumcodereview.appspot.com/10825317/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/app_list/search_builder.cc (right):

https://chromiumcodereview.appspot.com/10825317/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/app_list/search_builder.cc:245: params.disposition =
chrome::DispositionFromEventFlags(event_flags);
Think we still need the special case for disposition so that we open the link in
a new tab instead of replacing current tab. See http://crbug.com/132326

[xiyuan, 2012-08-13 17:49:00]

https://chromiumcodereview.appspot.com/10825317/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/app_list/search_builder.cc (right):

https://chromiumcodereview.appspot.com/10825317/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/app_list/search_builder.cc:245: params.disposition =
chrome::DispositionFromEventFlags(event_flags);
One simple work around might be like this:

params.disposition = event_flags == ui::EF_NONE ?
    NEW_FOREGROUND_TAB :
    chrome::DispositionFromEventFlags(event_flags);

This should cover 99% of the case and not depending on banned calls.

[jennb, 2012-08-14 00:33:01]

drive-by...

http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/ap...
File chrome/browser/ui/views/ash/app_list/search_builder.cc (right):

http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/ap...
chrome/browser/ui/views/ash/app_list/search_builder.cc:242:
chrome::NavigateParams params(NULL,
Does it work to pass a NULL browser here? One of the first things
chrome::Navigate() does is deref params->browser->profile().

[benwells, 2012-08-14 02:46:35]

http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/ap...
File chrome/browser/ui/views/ash/app_list/search_builder.cc (right):

http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/ap...
chrome/browser/ui/views/ash/app_list/search_builder.cc:242:
chrome::NavigateParams params(NULL,
On 2012/08/14 00:33:01, jennb wrote:
> Does it work to pass a NULL browser here? One of the first things
> chrome::Navigate() does is deref params->browser->profile().

This code from chrome::Navigate() makes it safe:
  params->browser = GetBrowserForDisposition(params);

  if (!params->browser)
    return;

Note that there is a block above which might try dereferencing
params->browser->is_type_panel, but only if params->source_contents is not NULL.

Having said that, I was getting a crash when using this, it is crashing in the
open gl layer where it tries to delete a context. I am assuming this crash is to
do with building / running chromeos directly on Linux.

[jennb, 2012-08-14 06:23:36]

On Mon, Aug 13, 2012 at 7:46 PM, <benwells@chromium.org> wrote:

>
> http://codereview.chromium.**org/10825317/diff/1/chrome/**
>
browser/ui/views/ash/app_list/**search_builder.cc<http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/app_list/search_builder.cc>
> File chrome/browser/ui/views/ash/**app_list/search_builder.cc (right):
>
> http://codereview.chromium.**org/10825317/diff/1/chrome/**
>
browser/ui/views/ash/app_list/**search_builder.cc#newcode242<http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/app_list/search_builder.cc#newcode242>
> chrome/browser/ui/views/ash/**app_list/search_builder.cc:**242:
> chrome::NavigateParams params(NULL,
> On 2012/08/14 00:33:01, jennb wrote:
>
>> Does it work to pass a NULL browser here? One of the first things
>> chrome::Navigate() does is deref params->browser->profile().
>>
>
> This code from chrome::Navigate() makes it safe:
>   params->browser = GetBrowserForDisposition(**params);
>

Above that line is this call:

if (!AdjustNavigateParamsForURL(params))

    return;


In AdjustNavigateParamsForURL is the params->browser->profile() line.

Jenn


>
>   if (!params->browser)
>     return;
>
> Note that there is a block above which might try dereferencing
> params->browser->is_type_**panel, but only if params->source_contents is
> not NULL.
>
> Having said that, I was getting a crash when using this, it is crashing
> in the open gl layer where it tries to delete a context. I am assuming
> this crash is to do with building / running chromeos directly on Linux.
>
>
http://codereview.chromium.**org/10825317/<http://codereview.chromium.org/108...
>

[benwells, 2012-08-14 07:12:08]

On 2012/08/14 06:23:36, jennb wrote:
> On Mon, Aug 13, 2012 at 7:46 PM, <mailto:benwells@chromium.org> wrote:
> 
> >
> > http://codereview.chromium.**org/10825317/diff/1/chrome/**
> >
>
browser/ui/views/ash/app_list/**search_builder.cc<http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/app_list/search_builder.cc>
> > File chrome/browser/ui/views/ash/**app_list/search_builder.cc (right):
> >
> > http://codereview.chromium.**org/10825317/diff/1/chrome/**
> >
>
browser/ui/views/ash/app_list/**search_builder.cc#newcode242<http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/app_list/search_builder.cc#newcode242>
> > chrome/browser/ui/views/ash/**app_list/search_builder.cc:**242:
> > chrome::NavigateParams params(NULL,
> > On 2012/08/14 00:33:01, jennb wrote:
> >
> >> Does it work to pass a NULL browser here? One of the first things
> >> chrome::Navigate() does is deref params->browser->profile().
> >>
> >
> > This code from chrome::Navigate() makes it safe:
> >   params->browser = GetBrowserForDisposition(**params);
> >
> 
> Above that line is this call:
> 
> if (!AdjustNavigateParamsForURL(params))
> 
>     return;
> 
> 
> In AdjustNavigateParamsForURL is the params->browser->profile() line.
> 
> Jenn
> 
> 
> >
> >   if (!params->browser)
> >     return;
> >
> > Note that there is a block above which might try dereferencing
> > params->browser->is_type_**panel, but only if params->source_contents is
> > not NULL.
> >
> > Having said that, I was getting a crash when using this, it is crashing
> > in the open gl layer where it tries to delete a context. I am assuming
> > this crash is to do with building / running chromeos directly on Linux.
> >
> >
>
http://codereview.chromium.**org/10825317/%3Chttp://codereview.chromium.org/1...>
> >

Oh yeah, thanks for catching that! I think that assumption about it being openGL
was wrong, I guess I shouldn't trust the stack dump!

I'll look at making NavigateParams able to handle a NULL browser.

[benwells, 2012-08-14 08:49:03]

The assumption about params.browser != NULL was all over the code :(. I think
I've caught all the problems.

jennb: I believe you want to be able to navigate without a browser too, could
you see if your code works with this patch?

beng: please have another look, it's a much heavier change now. I think this is
worthwhile to help get rid of the banned functions as a common use case for them
seems to be for navigation, but maybe you have a better idea of how to remove
the banned functions for this use case.

https://chromiumcodereview.appspot.com/10825317/diff/1/chrome/browser/ui/view...
File chrome/browser/ui/views/ash/app_list/search_builder.cc (right):

https://chromiumcodereview.appspot.com/10825317/diff/1/chrome/browser/ui/view...
chrome/browser/ui/views/ash/app_list/search_builder.cc:245: params.disposition =
chrome::DispositionFromEventFlags(event_flags);
On 2012/08/13 17:49:00, xiyuan wrote:
> One simple work around might be like this:
> 
> params.disposition = event_flags == ui::EF_NONE ?
>     NEW_FOREGROUND_TAB :
>     chrome::DispositionFromEventFlags(event_flags);
> 
> This should cover 99% of the case and not depending on banned calls.

The navigate function will change disposition from CURRENT_TAB to
NEW_FOREGROUND_TAB in NormalizeDisposition if params.source_contents is NULL
(which it is in our case).

However the behavior is a bit different now, as if the current tab is the NTP it
won't be used. Is that OK? It might make sense to update the Navigate call to
handle this in NormalizeDisposition, but that scares me a little.

[xiyuan, 2012-08-14 16:18:17]

http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/ap...
File chrome/browser/ui/views/ash/app_list/search_builder.cc (right):

http://codereview.chromium.org/10825317/diff/1/chrome/browser/ui/views/ash/ap...
chrome/browser/ui/views/ash/app_list/search_builder.cc:245: params.disposition =
chrome::DispositionFromEventFlags(event_flags);
Okay. Good to know that NormalizeDisposition will do that.

The behavior change should be okay. As long as we don't override the current
tab, it should cover most use cases. Let's see if tester/UX/user complains about
it.

[jennb, 2012-08-14 18:16:51]

> jennb: I believe you want to be able to navigate without a browser too, 
> could you see if your code works with this patch?

Tested with this patch and it works! I'll wait for your commit before I land my
patch so that I can avoid using banned FindOrCreateTabbedBrowser.

Thanks,
Jenn

[jennb, 2012-08-15 18:47:20]

beng - ping on re-review?

Hoping benwells can land this patch soon. I'm waiting on this change before I
can enable browserless panels by default in dev.

Thanks!

[Ben Goodger (Google), 2012-08-16 17:55:29]

lgtm

[commit-bot: I haz the power, 2012-08-16 23:53:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10825317/15002

[commit-bot: I haz the power, 2012-08-17 02:18:01]

Change committed as 152018