Removal of TabContentsDelegate::GetBrowser() interface method.

Change removing method, GetBrowser from TabContentsDelegate, as this was breaking an abstraction layer.  This routine was originally added in CL 434046, which required the Browser* to construct extension popup views from within Chrome-Frame instances.

I changed all accesses to Browser instances from usage of the above method, to either iterating the BrowserList using the situation-specific profile as a search key, or modifying the appropriate delegate interfaces to provide the functionality that was previously used directly via the Browser.

BUG=None
TEST=None


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

[Jeff Timanus, 2010-07-08 18:50:07]

Hi All,

As per Ben G.'s comments in review http://codereview.chromium.org/434046/show, I
have removed TabContentsDelegate::GetBrowser(), and replaced all usage of it
with an appropriate call to a static method of BrowserList.  

All of you have made use of this API, so I've included you on the reviewer list
for this CL.

I believe that my changes are correct, but I am not absolutely certain if it is
possible that the browser returned by indexing on the current profile will not
necessarily be the same as was returned by the old API.

Thanks,

Jeff

[Ben Goodger (Google), 2010-07-08 19:18:19]

http://codereview.chromium.org/2941001/diff/1/9
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/2941001/diff/1/9#newcode241
chrome/browser/dom_ui/history_ui.cc:241: browser->OpenClearBrowsingDataDialog();
Brett and I do not want DOMUI to depend on Browser directly either. I will
discuss with him and come up with a suggestion for this.

http://codereview.chromium.org/2941001/diff/1/12
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2941001/diff/1/12#newcode534
chrome/browser/tab_contents/tab_contents.cc:534: true))
I believe this misses the point of the request I was making... nothing in
TabContents land should know anything about Browsers.

http://codereview.chromium.org/2941001/diff/1/14
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/1/14#newcode286
chrome/browser/views/tabs/browser_tab_strip_controller.cc:286: if (browser &&
browser->OpenAppsPanelAsNewTab())
I don't like this object being directly coupled to the Browser. This should be
implemented in the TabStripModel, and wired through to the Browser via
TabStripModelDelegate.

[Jeff Timanus, 2010-07-15 19:01:12]

http://codereview.chromium.org/2941001/diff/1/9
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/2941001/diff/1/9#newcode241
chrome/browser/dom_ui/history_ui.cc:241: browser->OpenClearBrowsingDataDialog();
On 2010/07/08 19:18:20, Ben Goodger wrote:
> Brett and I do not want DOMUI to depend on Browser directly either. I will
> discuss with him and come up with a suggestion for this.

Have you had any discussions?  I can look into modifying the history service
routines to remove the direct usage of the browser here.

http://codereview.chromium.org/2941001/diff/1/12
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/2941001/diff/1/12#newcode534
chrome/browser/tab_contents/tab_contents.cc:534: true))
On 2010/07/08 19:18:20, Ben Goodger wrote:
> I believe this misses the point of the request I was making... nothing in
> TabContents land should know anything about Browsers.

I removed the use of browser here, and everywhere else in this file.  I added a
new method to TabStripModelDelegate to determine if large icons are permitted.

http://codereview.chromium.org/2941001/diff/1/14
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/1/14#newcode286
chrome/browser/views/tabs/browser_tab_strip_controller.cc:286: if (browser &&
browser->OpenAppsPanelAsNewTab())
On 2010/07/08 19:18:20, Ben Goodger wrote:
> I don't like this object being directly coupled to the Browser. This should be
> implemented in the TabStripModel, and wired through to the Browser via
> TabStripModelDelegate.

I removed use of the browser, as you suggested, and modified the necessary
delegates.

[Ben Goodger (Google), 2010-07-15 19:11:21]

Cool. I have been out sick this week. I will make sure I get to this
on Monday morning.

-Ben

On Thu, Jul 15, 2010 at 12:01 PM,  <twiz@chromium.org> wrote:
>
> http://codereview.chromium.org/2941001/diff/1/9
> File chrome/browser/dom_ui/history_ui.cc (right):
>
> http://codereview.chromium.org/2941001/diff/1/9#newcode241
> chrome/browser/dom_ui/history_ui.cc:241:
> browser->OpenClearBrowsingDataDialog();
> On 2010/07/08 19:18:20, Ben Goodger wrote:
>>
>> Brett and I do not want DOMUI to depend on Browser directly either. I
>
> will
>>
>> discuss with him and come up with a suggestion for this.
>
> Have you had any discussions?  I can look into modifying the history
> service routines to remove the direct usage of the browser here.
>
> http://codereview.chromium.org/2941001/diff/1/12
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
> http://codereview.chromium.org/2941001/diff/1/12#newcode534
> chrome/browser/tab_contents/tab_contents.cc:534: true))
> On 2010/07/08 19:18:20, Ben Goodger wrote:
>>
>> I believe this misses the point of the request I was making... nothing
>
> in
>>
>> TabContents land should know anything about Browsers.
>
> I removed the use of browser here, and everywhere else in this file.  I
> added a new method to TabStripModelDelegate to determine if large icons
> are permitted.
>
> http://codereview.chromium.org/2941001/diff/1/14
> File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):
>
> http://codereview.chromium.org/2941001/diff/1/14#newcode286
> chrome/browser/views/tabs/browser_tab_strip_controller.cc:286: if
> (browser && browser->OpenAppsPanelAsNewTab())
> On 2010/07/08 19:18:20, Ben Goodger wrote:
>>
>> I don't like this object being directly coupled to the Browser. This
>
> should be
>>
>> implemented in the TabStripModel, and wired through to the Browser via
>> TabStripModelDelegate.
>
> I removed use of the browser, as you suggested, and modified the
> necessary delegates.
>
> http://codereview.chromium.org/2941001/show
>

[Jeff Timanus, 2010-07-21 01:43:13]

On 2010/07/15 19:11:21, Ben Goodger wrote:
> Cool. I have been out sick this week. I will make sure I get to this
> on Monday morning.

Hi Ben,

Any chance you'll have a moment to look at this change?  I'm finding that people
are continuing to add use of the API that I've removed.

Thanks,

Jeff

> 
> -Ben
> 
> On Thu, Jul 15, 2010 at 12:01 PM,  <mailto:twiz@chromium.org> wrote:
> >
> > http://codereview.chromium.org/2941001/diff/1/9
> > File chrome/browser/dom_ui/history_ui.cc (right):
> >
> > http://codereview.chromium.org/2941001/diff/1/9#newcode241
> > chrome/browser/dom_ui/history_ui.cc:241:
> > browser->OpenClearBrowsingDataDialog();
> > On 2010/07/08 19:18:20, Ben Goodger wrote:
> >>
> >> Brett and I do not want DOMUI to depend on Browser directly either. I
> >
> > will
> >>
> >> discuss with him and come up with a suggestion for this.
> >
> > Have you had any discussions? &nbsp;I can look into modifying the history
> > service routines to remove the direct usage of the browser here.
> >
> > http://codereview.chromium.org/2941001/diff/1/12
> > File chrome/browser/tab_contents/tab_contents.cc (right):
> >
> > http://codereview.chromium.org/2941001/diff/1/12#newcode534
> > chrome/browser/tab_contents/tab_contents.cc:534: true))
> > On 2010/07/08 19:18:20, Ben Goodger wrote:
> >>
> >> I believe this misses the point of the request I was making... nothing
> >
> > in
> >>
> >> TabContents land should know anything about Browsers.
> >
> > I removed the use of browser here, and everywhere else in this file. &nbsp;I
> > added a new method to TabStripModelDelegate to determine if large icons
> > are permitted.
> >
> > http://codereview.chromium.org/2941001/diff/1/14
> > File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):
> >
> > http://codereview.chromium.org/2941001/diff/1/14#newcode286
> > chrome/browser/views/tabs/browser_tab_strip_controller.cc:286: if
> > (browser && browser->OpenAppsPanelAsNewTab())
> > On 2010/07/08 19:18:20, Ben Goodger wrote:
> >>
> >> I don't like this object being directly coupled to the Browser. This
> >
> > should be
> >>
> >> implemented in the TabStripModel, and wired through to the Browser via
> >> TabStripModelDelegate.
> >
> > I removed use of the browser, as you suggested, and modified the
> > necessary delegates.
> >
> > http://codereview.chromium.org/2941001/show
> >
>

[Ben Goodger (Google), 2010-07-21 17:15:08]

Sorry for my latency, I have been really sick and away from work. I am going
to think about this now.

-Ben

On Tue, Jul 20, 2010 at 6:43 PM, <twiz@chromium.org> wrote:

> On 2010/07/15 19:11:21, Ben Goodger wrote:
>
>> Cool. I have been out sick this week. I will make sure I get to this
>> on Monday morning.
>>
>
> Hi Ben,
>
> Any chance you'll have a moment to look at this change?  I'm finding that
> people
> are continuing to add use of the API that I've removed.
>
> Thanks,
>
> Jeff
>
>
>  -Ben
>>
>
>  On Thu, Jul 15, 2010 at 12:01 PM,  <mailto:twiz@chromium.org> wrote:
>> >
>> > http://codereview.chromium.org/2941001/diff/1/9
>> > File chrome/browser/dom_ui/history_ui.cc (right):
>> >
>> > http://codereview.chromium.org/2941001/diff/1/9#newcode241
>> > chrome/browser/dom_ui/history_ui.cc:241:
>> > browser->OpenClearBrowsingDataDialog();
>> > On 2010/07/08 19:18:20, Ben Goodger wrote:
>> >>
>> >> Brett and I do not want DOMUI to depend on Browser directly either. I
>> >
>> > will
>> >>
>> >> discuss with him and come up with a suggestion for this.
>> >
>> > Have you had any discussions? &nbsp;I can look into modifying the
>> history
>>
>> > service routines to remove the direct usage of the browser here.
>> >
>> > http://codereview.chromium.org/2941001/diff/1/12
>> > File chrome/browser/tab_contents/tab_contents.cc (right):
>> >
>> > http://codereview.chromium.org/2941001/diff/1/12#newcode534
>> > chrome/browser/tab_contents/tab_contents.cc:534: true))
>> > On 2010/07/08 19:18:20, Ben Goodger wrote:
>> >>
>> >> I believe this misses the point of the request I was making... nothing
>> >
>> > in
>> >>
>> >> TabContents land should know anything about Browsers.
>> >
>> > I removed the use of browser here, and everywhere else in this file.
>> &nbsp;I
>>
>> > added a new method to TabStripModelDelegate to determine if large icons
>> > are permitted.
>> >
>> > http://codereview.chromium.org/2941001/diff/1/14
>> > File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):
>> >
>> > http://codereview.chromium.org/2941001/diff/1/14#newcode286
>> > chrome/browser/views/tabs/browser_tab_strip_controller.cc:286: if
>> > (browser && browser->OpenAppsPanelAsNewTab())
>> > On 2010/07/08 19:18:20, Ben Goodger wrote:
>> >>
>> >> I don't like this object being directly coupled to the Browser. This
>> >
>> > should be
>> >>
>> >> implemented in the TabStripModel, and wired through to the Browser via
>> >> TabStripModelDelegate.
>> >
>> > I removed use of the browser, as you suggested, and modified the
>> > necessary delegates.
>> >
>> > http://codereview.chromium.org/2941001/show
>> >
>>
>
>
>
>
> http://codereview.chromium.org/2941001/show
>

[Ben Goodger (Google), 2010-07-21 17:17:20]

http://codereview.chromium.org/2941001/diff/1/9
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/2941001/diff/1/9#newcode241
chrome/browser/dom_ui/history_ui.cc:241: browser->OpenClearBrowsingDataDialog();
Let's just leave this as you have it for now. Please add a comment like:

// TODO(beng): This is an improper direct dependency on Browser. Route this
through some sort of delegate.

I need to come up with a general case solution for DOMUIs.

[Ben Goodger (Google), 2010-07-21 17:33:36]

http://codereview.chromium.org/2941001/diff/62001/63014
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/2941001/diff/62001/63014#newcode179
chrome/browser/extensions/extension_dom_ui.cc:179: return
BrowserList::FindBrowserWithProfile(DOMUI::GetProfile());
please add comments in all these DOMUIs noting that what they're doing is wrong.

make them TODO(beng):

http://codereview.chromium.org/2941001/diff/62001/63019
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/2941001/diff/62001/63019#newcode249
chrome/browser/tabs/tab_strip_model.h:249: virtual bool LargeIconsPermitted() =
0;
const

http://codereview.chromium.org/2941001/diff/62001/63021
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/62001/63021#newcode287
chrome/browser/views/tabs/browser_tab_strip_controller.cc:287: if
(model_->delegate()->OpenAppsPanelAsNewTab())
I think it's OK for this file to know about Browser directly. This is the
coordinator between the world of Browsers and the world of TabStrips. This way
you can avoid adding these delegate methods.

[Jeff Timanus, 2010-07-21 21:40:29]

http://codereview.chromium.org/2941001/diff/1/9
File chrome/browser/dom_ui/history_ui.cc (right):

http://codereview.chromium.org/2941001/diff/1/9#newcode241
chrome/browser/dom_ui/history_ui.cc:241: browser->OpenClearBrowsingDataDialog();
On 2010/07/21 17:17:20, Ben Goodger wrote:
> Let's just leave this as you have it for now. Please add a comment like:
> 
> // TODO(beng): This is an improper direct dependency on Browser. Route this
> through some sort of delegate.
> 
> I need to come up with a general case solution for DOMUIs.

Done, and in history2_ui.cc

http://codereview.chromium.org/2941001/diff/62001/63014
File chrome/browser/extensions/extension_dom_ui.cc (right):

http://codereview.chromium.org/2941001/diff/62001/63014#newcode179
chrome/browser/extensions/extension_dom_ui.cc:179: return
BrowserList::FindBrowserWithProfile(DOMUI::GetProfile());
On 2010/07/21 17:33:36, Ben Goodger wrote:
> please add comments in all these DOMUIs noting that what they're doing is
wrong.
> 
> make them TODO(beng):

Done.

http://codereview.chromium.org/2941001/diff/62001/63019
File chrome/browser/tabs/tab_strip_model.h (right):

http://codereview.chromium.org/2941001/diff/62001/63019#newcode249
chrome/browser/tabs/tab_strip_model.h:249: virtual bool LargeIconsPermitted() =
0;
On 2010/07/21 17:33:36, Ben Goodger wrote:
> const

Done.

http://codereview.chromium.org/2941001/diff/62001/63021
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/62001/63021#newcode287
chrome/browser/views/tabs/browser_tab_strip_controller.cc:287: if
(model_->delegate()->OpenAppsPanelAsNewTab())
On 2010/07/21 17:33:36, Ben Goodger wrote:
> I think it's OK for this file to know about Browser directly. This is the
> coordinator between the world of Browsers and the world of TabStrips. This way
> you can avoid adding these delegate methods.

Done.

[Jeff Timanus, 2010-07-26 18:28:57]

Any more changes that you recommend?  Things look good to you, Ben?

Thanks,

Jeff

[Ben Goodger (Google), 2010-07-27 21:54:20]

http://codereview.chromium.org/2941001/diff/44003/53063
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/44003/53063#newcode273
chrome/browser/views/tabs/browser_tab_strip_controller.cc:273: Browser* browser
= BrowserList::FindBrowserWithProfile(model_->profile());
You shouldn't use this enumerator function. Just have this object be created
with a Browser*.

[Jeff Timanus, 2010-07-28 19:22:05]

http://codereview.chromium.org/2941001/diff/44003/53063
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/44003/53063#newcode273
chrome/browser/views/tabs/browser_tab_strip_controller.cc:273: Browser* browser
= BrowserList::FindBrowserWithProfile(model_->profile());
On 2010/07/27 21:54:20, Ben Goodger wrote:
> You shouldn't use this enumerator function. Just have this object be created
> with a Browser*.

Done.

[Jeff Timanus, 2010-07-30 18:47:08]

Any further comments?  Good to go?

[Ben Goodger (Google), 2010-08-02 15:14:11]

Yes LGTM.

http://codereview.chromium.org/2941001/diff/75014/54042
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/75014/54042#newcode10
chrome/browser/views/tabs/browser_tab_strip_controller.cc:10: #include
"chrome/browser/browser_list.h"
nix this include you don't need it anymore.

[Jeff Timanus, 2010-08-02 16:07:26]

http://codereview.chromium.org/2941001/diff/75014/54042
File chrome/browser/views/tabs/browser_tab_strip_controller.cc (right):

http://codereview.chromium.org/2941001/diff/75014/54042#newcode10
chrome/browser/views/tabs/browser_tab_strip_controller.cc:10: #include
"chrome/browser/browser_list.h"
On 2010/08/02 15:14:11, Ben Goodger wrote:
> nix this include you don't need it anymore.

Done.

[Aaron Boodman, 2010-10-26 19:36:29]

This change broke a feature of chrome extensions. The change to
ExtensionDOMUI::GetBrowser() was not correct.

ExtensionFunctionDispatcher::Delegate::GetBrowser() is not supposed to return
any old browser, it is supposed to return the browser containing the code
executing the extension function.

[Aaron Boodman, 2010-10-26 19:36:29]

This change broke a feature of chrome extensions. The change to
ExtensionDOMUI::GetBrowser() was not correct.

ExtensionFunctionDispatcher::Delegate::GetBrowser() is not supposed to return
any old browser, it is supposed to return the browser containing the code
executing the extension function.

[Jeff Timanus, 2010-10-26 21:36:51]

On 2010/10/26 19:36:29, Aaron Boodman wrote:
> This change broke a feature of chrome extensions. The change to
> ExtensionDOMUI::GetBrowser() was not correct.
> 
> ExtensionFunctionDispatcher::Delegate::GetBrowser() is not supposed to return
> any old browser, it is supposed to return the browser containing the code
> executing the extension function.

Would a suitable fix be using the TabContentsIterator (browser_list.h) utility
class to iterate through all of the browsers until we find one that matches the
TabContents of the ExtensionDOMUI?

[Aaron Boodman, 2010-10-26 21:40:56]

On Tue, Oct 26, 2010 at 2:36 PM,  <twiz@chromium.org> wrote:
> On 2010/10/26 19:36:29, Aaron Boodman wrote:
>>
>> This change broke a feature of chrome extensions. The change to
>> ExtensionDOMUI::GetBrowser() was not correct.
>
>> ExtensionFunctionDispatcher::Delegate::GetBrowser() is not supposed to
>> return
>> any old browser, it is supposed to return the browser containing the code
>> executing the extension function.
>
> Would a suitable fix be using the TabContentsIterator (browser_list.h)
> utility
> class to iterate through all of the browsers until we find one that matches
> the
> TabContents of the ExtensionDOMUI?

Is there a reference to the original complaint? I'd like to better
understand what the issue was with the original code.

- a

[Jeff Timanus, 2010-10-26 21:50:43]

On 2010/10/26 21:40:56, Aaron Boodman wrote:
> On Tue, Oct 26, 2010 at 2:36 PM,  <mailto:twiz@chromium.org> wrote:
> > On 2010/10/26 19:36:29, Aaron Boodman wrote:
> >>
> >> This change broke a feature of chrome extensions. The change to
> >> ExtensionDOMUI::GetBrowser() was not correct.
> >
> >> ExtensionFunctionDispatcher::Delegate::GetBrowser() is not supposed to
> >> return
> >> any old browser, it is supposed to return the browser containing the code
> >> executing the extension function.
> >
> > Would a suitable fix be using the TabContentsIterator (browser_list.h)
> > utility
> > class to iterate through all of the browsers until we find one that matches
> > the
> > TabContents of the ExtensionDOMUI?
> 
> Is there a reference to the original complaint? I'd like to better
> understand what the issue was with the original code.

I removed the TabContentsDelegate::GetBrowser method because it had introduced
an abstraction layering violation.  

See the comments in this review for background: 
http://codereview.chromium.org/434046/show

Jeff
> 
> - a
>