Implement SearchTabHelperDelegate.

Implement SearchTabHelperDelegate.

This CL,
- Removes chrome::FindBrowser...() function calls from search_tab_helper.cc.	
- Creates a SearchTabHelperDelegate interface and have the Browser object implement that interface to provide the necessary functionality to SearchTabHelper.

BUG=272583
TEST=none

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

[kmadhusu, 2014-04-04 20:34:16]

Please review.

Thanks.

[Jered, 2014-04-04 22:39:21]

On 2014/04/04 20:34:16, kmadhusu wrote:
> Please review.
> 
> Thanks.

Do you need to update any tests for this CL?

[Jered, 2014-04-04 22:39:27]

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:102: public SearchTabHelperDelegate,
(no action required) Ugh what is a HelperDelegate? Who comes up with these
names?

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:670: virtual void
RemoveMostVisitedItemsMatchingOpenTabs(
This seems like pushing too much into Browser. Instead, can we have it return
the information we need to do the pruning we want to do? e.g. return a
set<string> urls but still do the MV pruning in SearchTabHelper.

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:520: OmniboxView* omnibox =
delegate_ ? delegate_->GetOmniboxView() : NULL;
Extract a helper GetOmniboxView() and have it return this, instead of
replicating the ? : expression.

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper_delegate.h (right):

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper_delegate.h:22: class
SearchTabHelperDelegate {
Why not make this abstract? A possible answer would be to make tests easier, but
I don't see any in this CL?

[kmadhusu, 2014-04-07 21:44:09]

Addressed comments. No need to update the existing tests.

PTAL.Thanks.

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:102: public SearchTabHelperDelegate,
On 2014/04/04 22:39:27, Jered wrote:
> (no action required) Ugh what is a HelperDelegate? Who comes up with these
> names?

If you prefer, we can rename this to "SearchTabDelegate". WDYT?

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:670: virtual void
RemoveMostVisitedItemsMatchingOpenTabs(
On 2014/04/04 22:39:27, Jered wrote:
> This seems like pushing too much into Browser. Instead, can we have it return
> the information we need to do the pruning we want to do? e.g. return a
> set<string> urls but still do the MV pruning in SearchTabHelper.

Done.

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:520: OmniboxView* omnibox =
delegate_ ? delegate_->GetOmniboxView() : NULL;
On 2014/04/04 22:39:27, Jered wrote:
> Extract a helper GetOmniboxView() and have it return this, instead of
> replicating the ? : expression.

Done.

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper_delegate.h (right):

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper_delegate.h:22: class
SearchTabHelperDelegate {
On 2014/04/04 22:39:27, Jered wrote:
> Why not make this abstract? A possible answer would be to make tests easier,
but
> I don't see any in this CL?

In future, TabAndroid will implement this interface. Most of these functions
will have an empty implementation for TabAndroid. That's why I didn't make this
abstract.

[Jered, 2014-04-08 12:32:04]

On 2014/04/07 21:44:09, kmadhusu wrote:
> Addressed comments. No need to update the existing tests.
> 
> PTAL.Thanks.
> 
>
https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browser.h
> File chrome/browser/ui/browser.h (right):
> 
>
https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browse...
> chrome/browser/ui/browser.h:102: public SearchTabHelperDelegate,
> On 2014/04/04 22:39:27, Jered wrote:
> > (no action required) Ugh what is a HelperDelegate? Who comes up with these
> > names?
> 
> If you prefer, we can rename this to "SearchTabDelegate". WDYT?
> 
>
https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browse...
> chrome/browser/ui/browser.h:670: virtual void
> RemoveMostVisitedItemsMatchingOpenTabs(
> On 2014/04/04 22:39:27, Jered wrote:
> > This seems like pushing too much into Browser. Instead, can we have it
return
> > the information we need to do the pruning we want to do? e.g. return a
> > set<string> urls but still do the MV pruning in SearchTabHelper.
> 
> Done.
> 
>
https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
> File chrome/browser/ui/search/search_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
> chrome/browser/ui/search/search_tab_helper.cc:520: OmniboxView* omnibox =
> delegate_ ? delegate_->GetOmniboxView() : NULL;
> On 2014/04/04 22:39:27, Jered wrote:
> > Extract a helper GetOmniboxView() and have it return this, instead of
> > replicating the ? : expression.
> 
> Done.
> 
>
https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
> File chrome/browser/ui/search/search_tab_helper_delegate.h (right):
> 
>
https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/search...
> chrome/browser/ui/search/search_tab_helper_delegate.h:22: class
> SearchTabHelperDelegate {
> On 2014/04/04 22:39:27, Jered wrote:
> > Why not make this abstract? A possible answer would be to make tests easier,
> but
> > I don't see any in this CL?
> 
> In future, TabAndroid will implement this interface. Most of these functions
> will have an empty implementation for TabAndroid. That's why I didn't make
this
> abstract.

lgtm

[Jered, 2014-04-08 12:32:10]

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

https://codereview.chromium.org/222923007/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser.h:102: public SearchTabHelperDelegate,
On 2014/04/07 21:44:15, kmadhusu wrote:
> On 2014/04/04 22:39:27, Jered wrote:
> > (no action required) Ugh what is a HelperDelegate? Who comes up with these
> > names?
> 
> If you prefer, we can rename this to "SearchTabDelegate". WDYT?

I guess it is the delegate of SearchTabHelper, so it is fine as it is. I'll just
have to live with it. :)

https://codereview.chromium.org/222923007/diff/120001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/120001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1782: DCHECK(open_urls);
optional: You could also have this method return a set<string>. Return-value
optimization should make that just as efficient without needing a
pointer/dcheck.

[kmadhusu, 2014-04-08 18:57:52]

https://codereview.chromium.org/222923007/diff/120001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/120001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1782: DCHECK(open_urls);
On 2014/04/08 12:32:10, Jered wrote:
> optional: You could also have this method return a set<string>. Return-value
> optimization should make that just as efficient without needing a
> pointer/dcheck.

Done.

[kmadhusu, 2014-04-08 18:58:38]

pkasting@: Please do the OWNER's check for browser.* changes.

Thanks.

[Peter Kasting, 2014-04-08 20:47:51]

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1760: content::PAGE_TRANSITION_AUTO_BOOKMARK);
What kinds of navigations are these?  I want to be sure AUTO_BOOKMARK is
actually correct.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1778: return location_bar ?
location_bar->GetOmniboxView() : NULL;
Can |window_| ever not have a location bar, e.g. for App windows or something? 
If not, you could simplify to:

  return window_ ? window_->GetLocationBar()->GetOmniboxView() : NULL;

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1784: NOTREACHED();
Do not handle DCHECK failure.

Either just DCHECK that the variables are non-NULL (or do nothing, if there's no
reason for a reader to wonder about them), or if they can legitimately be NULL,
remove the NOTREACHED().

[kmadhusu, 2014-04-08 21:11:35]

pkasting@: Addressed comments. PTAL.

Thanks.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1760: content::PAGE_TRANSITION_AUTO_BOOKMARK);
On 2014/04/08 20:47:52, Peter Kasting wrote:
> What kinds of navigations are these?  I want to be sure AUTO_BOOKMARK is
> actually correct.

When an user clicks on a most visited thumbnail, we invoke this function to
navigate the page to the most visited item url.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1778: return location_bar ?
location_bar->GetOmniboxView() : NULL;
On 2014/04/08 20:47:52, Peter Kasting wrote:
> Can |window_| ever not have a location bar, e.g. for App windows or something?

> If not, you could simplify to:
> 
>   return window_ ? window_->GetLocationBar()->GetOmniboxView() : NULL;

Done. SearchTabHelper invokes this function. window_->GetLocationBar() will not
be NULL. Simplified as per your suggestion.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1784: NOTREACHED();
On 2014/04/08 20:47:52, Peter Kasting wrote:
> Do not handle DCHECK failure.
> 
> Either just DCHECK that the variables are non-NULL (or do nothing, if there's
no
> reason for a reader to wonder about them), or if they can legitimately be
NULL,
> remove the NOTREACHED().

Done.

[Peter Kasting, 2014-04-08 21:45:43]

LGTM with comments

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1760: content::PAGE_TRANSITION_AUTO_BOOKMARK);
On 2014/04/08 21:11:36, kmadhusu wrote:
> On 2014/04/08 20:47:52, Peter Kasting wrote:
> > What kinds of navigations are these?  I want to be sure AUTO_BOOKMARK is
> > actually correct.
> 
> When an user clicks on a most visited thumbnail, we invoke this function to
> navigate the page to the most visited item url.

You may want to encode that in the function name, or at least its comments: e.g.
name it "NavigateOnThumbnailClick()" or something, then write a comment in here
that we're guaranteed that AUTO_BOOKMARK is the right transition since this only
gets called for thumbnail clicks.

This will discourage people in the future from seeing this as "ah ha, an easy
way to navigate in response to anything!" and adding more callers which want a
different transition.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1778: return location_bar ?
location_bar->GetOmniboxView() : NULL;
On 2014/04/08 21:11:36, kmadhusu wrote:
> On 2014/04/08 20:47:52, Peter Kasting wrote:
> > Can |window_| ever not have a location bar, e.g. for App windows or
something?
> 
> > If not, you could simplify to:
> > 
> >   return window_ ? window_->GetLocationBar()->GetOmniboxView() : NULL;
> 
> Done. SearchTabHelper invokes this function. window_->GetLocationBar() will
not
> be NULL. Simplified as per your suggestion.

Also make sure to think through:
* Startup/shutdown, especially race conditions in processing events during
shutdown
* Testing

I'm happy if we can legally simplify this, I just want to make sure doing so is
truly robust.  When I asked the original question, I honestly didn't know the
answer.

https://codereview.chromium.org/222923007/diff/180001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/180001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1782: DCHECK(top_sites);
Hmm.  In this case maybe we should simplify even more and just do this below:

  chrome::GetOpenUrls(*tab_strip_model_, *profile_->GetTopSites(), &open_urls);

[kmadhusu, 2014-04-09 00:00:42]

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1760: content::PAGE_TRANSITION_AUTO_BOOKMARK);
On 2014/04/08 21:45:46, Peter Kasting wrote:
> On 2014/04/08 21:11:36, kmadhusu wrote:
> > On 2014/04/08 20:47:52, Peter Kasting wrote:
> > > What kinds of navigations are these?  I want to be sure AUTO_BOOKMARK is
> > > actually correct.
> > 
> > When an user clicks on a most visited thumbnail, we invoke this function to
> > navigate the page to the most visited item url.
> 
> You may want to encode that in the function name, or at least its comments:
e.g.
> name it "NavigateOnThumbnailClick()" or something, then write a comment in
here
> that we're guaranteed that AUTO_BOOKMARK is the right transition since this
only
> gets called for thumbnail clicks.
> 
> This will discourage people in the future from seeing this as "ah ha, an easy
> way to navigate in response to anything!" and adding more callers which want a
> different transition.

Renamed "NavigateToURL" to "NavigateOnClick" and added a detailed comment
regarding the page transition.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1778: return location_bar ?
location_bar->GetOmniboxView() : NULL;
On 2014/04/08 21:45:46, Peter Kasting wrote:
> On 2014/04/08 21:11:36, kmadhusu wrote:
> > On 2014/04/08 20:47:52, Peter Kasting wrote:
> > > Can |window_| ever not have a location bar, e.g. for App windows or
> something?
> > 
> > > If not, you could simplify to:
> > > 
> > >   return window_ ? window_->GetLocationBar()->GetOmniboxView() : NULL;
> > 
> > Done. SearchTabHelper invokes this function. window_->GetLocationBar() will
> not
> > be NULL. Simplified as per your suggestion.
> 
> Also make sure to think through:
> * Startup/shutdown, especially race conditions in processing events during
> shutdown
> * Testing
> 
> I'm happy if we can legally simplify this, I just want to make sure doing so
is
> truly robust.  When I asked the original question, I honestly didn't know the
> answer.

In the current code
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...),
we assumed that "window_->GetLocationBar()" is always non-null. I also ran
several tests to make sure this change doesn't regress the current behavior.
Having said that, the simplified code is safe when this function is invoked from
SearchTabHelper. 

But after committing this CL, this function can be invoked by any other code.
Just to be on the safer side, I prefer to go back to my previous version of code
and validate "window_->GetLocationBar()" return value.

https://codereview.chromium.org/222923007/diff/180001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/180001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1782: DCHECK(top_sites);
On 2014/04/08 21:45:46, Peter Kasting wrote:
> Hmm.  In this case maybe we should simplify even more and just do this below:
> 
>   chrome::GetOpenUrls(*tab_strip_model_, *profile_->GetTopSites(),
&open_urls);

Same here. As of now, this function is called only from SearchTabHelper. When
invoked from SearchTabHelper, |top_sites| is always non-null. But in future, any
one can call this function. If this function is invoked for an Incoginto
profile, |top_sites| is going to be NULL. Reverted DCHECK to an if-condition.

[Peter Kasting, 2014-04-09 00:03:24]

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1760: content::PAGE_TRANSITION_AUTO_BOOKMARK);
On 2014/04/09 00:00:42, kmadhusu wrote:
> On 2014/04/08 21:45:46, Peter Kasting wrote:
> > On 2014/04/08 21:11:36, kmadhusu wrote:
> > > On 2014/04/08 20:47:52, Peter Kasting wrote:
> > > > What kinds of navigations are these?  I want to be sure AUTO_BOOKMARK is
> > > > actually correct.
> > > 
> > > When an user clicks on a most visited thumbnail, we invoke this function
to
> > > navigate the page to the most visited item url.
> > 
> > You may want to encode that in the function name, or at least its comments:
> e.g.
> > name it "NavigateOnThumbnailClick()" or something, then write a comment in
> here
> > that we're guaranteed that AUTO_BOOKMARK is the right transition since this
> only
> > gets called for thumbnail clicks.
> > 
> > This will discourage people in the future from seeing this as "ah ha, an
easy
> > way to navigate in response to anything!" and adding more callers which want
a
> > different transition.
> 
> Renamed "NavigateToURL" to "NavigateOnClick" and added a detailed comment
> regarding the page transition. 

I don't think that's a great function name -- link clicks are clicks, but we'd
expect them to use PAGE_TRANSITION_LINK.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1778: return location_bar ?
location_bar->GetOmniboxView() : NULL;
On 2014/04/09 00:00:42, kmadhusu wrote:
> But after committing this CL, this function can be invoked by any other code.
> Just to be on the safer side, I prefer to go back to my previous version of
code
> and validate "window_->GetLocationBar()" return value. 

If your code expects this to be non-NULL, then don't check for NULL "just to be
safe".  If you have to add anything, add a DCHECK to imply that callers are not
allowed to call you if this is NULL.  But adding nothing at all seems
preferable.

[kmadhusu, 2014-04-09 01:31:03]

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1760: content::PAGE_TRANSITION_AUTO_BOOKMARK);
On 2014/04/09 00:03:24, Peter Kasting wrote:
> On 2014/04/09 00:00:42, kmadhusu wrote:
> > On 2014/04/08 21:45:46, Peter Kasting wrote:
> > > On 2014/04/08 21:11:36, kmadhusu wrote:
> > > > On 2014/04/08 20:47:52, Peter Kasting wrote:
> > > > > What kinds of navigations are these?  I want to be sure AUTO_BOOKMARK
is
> > > > > actually correct.
> > > > 
> > > > When an user clicks on a most visited thumbnail, we invoke this function
> to
> > > > navigate the page to the most visited item url.
> > > 
> > > You may want to encode that in the function name, or at least its
comments:
> > e.g.
> > > name it "NavigateOnThumbnailClick()" or something, then write a comment in
> > here
> > > that we're guaranteed that AUTO_BOOKMARK is the right transition since
this
> > only
> > > gets called for thumbnail clicks.
> > > 
> > > This will discourage people in the future from seeing this as "ah ha, an
> easy
> > > way to navigate in response to anything!" and adding more callers which
want
> a
> > > different transition.
> > 
> > Renamed "NavigateToURL" to "NavigateOnClick" and added a detailed comment
> > regarding the page transition. 
> 
> I don't think that's a great function name -- link clicks are clicks, but we'd
> expect them to use PAGE_TRANSITION_LINK.

Renamed "NavigateOnClick" to "NavigateOnThumbnailClick".

(repeating our offline conversation for reference): As of now, this function is
called to navigate to thumbnail URLs and to privileged Chrome URLs. Ideally, any
page transitions initiating from the search results page should be marked as
"LINK" instead of "AUTO_BOOKMARK". I will submit a follow up CL to fix this.

https://codereview.chromium.org/222923007/diff/160001/chrome/browser/ui/brows...
chrome/browser/ui/browser.cc:1778: return location_bar ?
location_bar->GetOmniboxView() : NULL;
On 2014/04/09 00:03:24, Peter Kasting wrote:
> On 2014/04/09 00:00:42, kmadhusu wrote:
> > But after committing this CL, this function can be invoked by any other
code.
> > Just to be on the safer side, I prefer to go back to my previous version of
> code
> > and validate "window_->GetLocationBar()" return value. 
> 
> If your code expects this to be non-NULL, then don't check for NULL "just to
be
> safe".  If you have to add anything, add a DCHECK to imply that callers are
not
> allowed to call you if this is NULL.  But adding nothing at all seems
> preferable.

Done.

[kmadhusu, 2014-04-09 21:20:25]

The CQ bit was checked by kmadhusu@chromium.org

[commit-bot: I haz the power, 2014-04-09 21:20:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/222923007/280001

[commit-bot: I haz the power, 2014-04-10 00:11:01]

Change committed as 262871