Removal of TabContentsDelegate::GetBrowser() interface method

Clone of issue 2941001.  See initial review below.

http://codereview.chromium.org/2941001

Initial submit broke the chromeos build.  This patch includes the original change, and fixes to compile ChromeOs properly.

BUG=None
TEST=None



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

[Jeff Timanus, 2010-08-02 19:45:08]

Ben,

my initial submit broke the ChromeOS build.  Resubmitting with the appropriate
fixes.

Zel, you may want to review the changes I made to filebrowse_ui.cc.  Note that I
did double check, and constructing a new Browser object does publish that
instance to the browser list, so FindBrowserWithProfile should return the new
Browser, if applicable.

Jeff

[Jeff Timanus, 2010-08-05 19:09:21]

On 2010/08/02 19:45:08, Jeff Timanus wrote:
> Ben,
> 
> my initial submit broke the ChromeOS build.  Resubmitting with the appropriate
> fixes.
> 
> Zel, you may want to review the changes I made to filebrowse_ui.cc.  Note that
I
> did double check, and constructing a new Browser object does publish that
> instance to the browser list, so FindBrowserWithProfile should return the new
> Browser, if applicable.

I just verified that the following line, in Browser::AddTabWithURL, will
activate the window, so FindBrowserWithProfile will return the appropriate
browser instance.

b->window()->Show();

> 
> Jeff

[Ben Goodger (Google), 2010-08-05 19:14:28]

Can you describe what changed outside of the ChromeOS bits?

-Ben

On Mon, Aug 2, 2010 at 12:45 PM, <twiz@chromium.org> wrote:

> Reviewers: Ben Goodger, zel,
>
> Message:
> Ben,
>
> my initial submit broke the ChromeOS build.  Resubmitting with the
> appropriate
> fixes.
>
> Zel, you may want to review the changes I made to filebrowse_ui.cc.  Note
> that I
> did double check, and constructing a new Browser object does publish that
> instance to the browser list, so FindBrowserWithProfile should return the
> new
> Browser, if applicable.
>
> Jeff
>
> Description:
> Clone of issue 2941001.  See initial review below.
>
> http://codereview.chromium.org/2941001
>
> Initial submit broke the chromeos build.  This patch includes the original
> change, and fixes to compile ChromeOs properly.
>
> BUG=None
> TEST=None
>
>
>
> Please review this at http://codereview.chromium.org/2819086/show
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>  M     chrome/browser/autofill/autofill_cc_infobar_delegate.h
>  M     chrome/browser/autofill/autofill_cc_infobar_delegate.cc
>  M     chrome/browser/autofill/autofill_manager.h
>  M     chrome/browser/browser.h
>  M     chrome/browser/browser.cc
>  M     chrome/browser/browser_browsertest.cc
>  M     chrome/browser/browser_init.cc
>  M     chrome/browser/chromeos/dom_ui/internet_options_handler.cc
>  M     chrome/browser/cocoa/tab_strip_controller_unittest.mm
>  M     chrome/browser/dom_ui/dom_ui.h
>  M     chrome/browser/dom_ui/dom_ui.cc
>  M     chrome/browser/dom_ui/filebrowse_ui.cc
>  M     chrome/browser/dom_ui/history2_ui.cc
>  M     chrome/browser/dom_ui/history_ui.cc
>  M     chrome/browser/extensions/extension_dom_ui.cc
>  M     chrome/browser/external_tab_container_win.h
>  M     chrome/browser/tab_contents/tab_contents.cc
>  M     chrome/browser/tab_contents/tab_contents_delegate.h
>  M     chrome/browser/tab_contents/tab_contents_delegate.cc
>  M     chrome/browser/tabs/tab_strip_model.h
>  M     chrome/browser/tabs/tab_strip_model_unittest.cc
>  M     chrome/browser/views/frame/browser_view.cc
>  M     chrome/browser/views/tabs/browser_tab_strip_controller.h
>  M     chrome/browser/views/tabs/browser_tab_strip_controller.cc
>
>
>

[Jeff Timanus, 2010-08-05 19:20:47]

On 2010/08/05 19:14:28, Ben Goodger wrote:
> Can you describe what changed outside of the ChromeOS bits?

Nothing changed from the original CL outside of the corrections I had to make in
filebrowse_ui.cc, and internet_options_handler.cc.

Jeff

> 
> -Ben
> 
> On Mon, Aug 2, 2010 at 12:45 PM, <mailto:twiz@chromium.org> wrote:
> 
> > Reviewers: Ben Goodger, zel,
> >
> > Message:
> > Ben,
> >
> > my initial submit broke the ChromeOS build.  Resubmitting with the
> > appropriate
> > fixes.
> >
> > Zel, you may want to review the changes I made to filebrowse_ui.cc.  Note
> > that I
> > did double check, and constructing a new Browser object does publish that
> > instance to the browser list, so FindBrowserWithProfile should return the
> > new
> > Browser, if applicable.
> >
> > Jeff
> >
> > Description:
> > Clone of issue 2941001.  See initial review below.
> >
> > http://codereview.chromium.org/2941001
> >
> > Initial submit broke the chromeos build.  This patch includes the original
> > change, and fixes to compile ChromeOs properly.
> >
> > BUG=None
> > TEST=None
> >
> >
> >
> > Please review this at http://codereview.chromium.org/2819086/show
> >
> > SVN Base: svn://svn.chromium.org/chrome/trunk/src/
> >
> > Affected files:
> >  M     chrome/browser/autofill/autofill_cc_infobar_delegate.h
> >  M     chrome/browser/autofill/autofill_cc_infobar_delegate.cc
> >  M     chrome/browser/autofill/autofill_manager.h
> >  M     chrome/browser/browser.h
> >  M     chrome/browser/browser.cc
> >  M     chrome/browser/browser_browsertest.cc
> >  M     chrome/browser/browser_init.cc
> >  M     chrome/browser/chromeos/dom_ui/internet_options_handler.cc
> >  M     chrome/browser/cocoa/tab_strip_controller_unittest.mm
> >  M     chrome/browser/dom_ui/dom_ui.h
> >  M     chrome/browser/dom_ui/dom_ui.cc
> >  M     chrome/browser/dom_ui/filebrowse_ui.cc
> >  M     chrome/browser/dom_ui/history2_ui.cc
> >  M     chrome/browser/dom_ui/history_ui.cc
> >  M     chrome/browser/extensions/extension_dom_ui.cc
> >  M     chrome/browser/external_tab_container_win.h
> >  M     chrome/browser/tab_contents/tab_contents.cc
> >  M     chrome/browser/tab_contents/tab_contents_delegate.h
> >  M     chrome/browser/tab_contents/tab_contents_delegate.cc
> >  M     chrome/browser/tabs/tab_strip_model.h
> >  M     chrome/browser/tabs/tab_strip_model_unittest.cc
> >  M     chrome/browser/views/frame/browser_view.cc
> >  M     chrome/browser/views/tabs/browser_tab_strip_controller.h
> >  M     chrome/browser/views/tabs/browser_tab_strip_controller.cc
> >
> >
> >
>

[Ben Goodger (Google), 2010-08-05 19:21:47]

Oh in that case Zel/dhg are better reviewers than I.

-Ben

On Thu, Aug 5, 2010 at 12:20 PM, <twiz@chromium.org> wrote:

> On 2010/08/05 19:14:28, Ben Goodger wrote:
>
>> Can you describe what changed outside of the ChromeOS bits?
>>
>
> Nothing changed from the original CL outside of the corrections I had to
> make in
> filebrowse_ui.cc, and internet_options_handler.cc.
>
> Jeff
>
>
>  -Ben
>>
>
>  On Mon, Aug 2, 2010 at 12:45 PM, <mailto:twiz@chromium.org> wrote:
>>
>
>  > Reviewers: Ben Goodger, zel,
>> >
>> > Message:
>> > Ben,
>> >
>> > my initial submit broke the ChromeOS build.  Resubmitting with the
>> > appropriate
>> > fixes.
>> >
>> > Zel, you may want to review the changes I made to filebrowse_ui.cc.
>>  Note
>> > that I
>> > did double check, and constructing a new Browser object does publish
>> that
>> > instance to the browser list, so FindBrowserWithProfile should return
>> the
>> > new
>> > Browser, if applicable.
>> >
>> > Jeff
>> >
>> > Description:
>> > Clone of issue 2941001.  See initial review below.
>> >
>> > http://codereview.chromium.org/2941001
>> >
>> > Initial submit broke the chromeos build.  This patch includes the
>> original
>> > change, and fixes to compile ChromeOs properly.
>> >
>> > BUG=None
>> > TEST=None
>> >
>> >
>> >
>> > Please review this at http://codereview.chromium.org/2819086/show
>> >
>> > SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>> >
>> > Affected files:
>> >  M     chrome/browser/autofill/autofill_cc_infobar_delegate.h
>> >  M     chrome/browser/autofill/autofill_cc_infobar_delegate.cc
>> >  M     chrome/browser/autofill/autofill_manager.h
>> >  M     chrome/browser/browser.h
>> >  M     chrome/browser/browser.cc
>> >  M     chrome/browser/browser_browsertest.cc
>> >  M     chrome/browser/browser_init.cc
>> >  M     chrome/browser/chromeos/dom_ui/internet_options_handler.cc
>> >  M     chrome/browser/cocoa/tab_strip_controller_unittest.mm
>> >  M     chrome/browser/dom_ui/dom_ui.h
>> >  M     chrome/browser/dom_ui/dom_ui.cc
>> >  M     chrome/browser/dom_ui/filebrowse_ui.cc
>> >  M     chrome/browser/dom_ui/history2_ui.cc
>> >  M     chrome/browser/dom_ui/history_ui.cc
>> >  M     chrome/browser/extensions/extension_dom_ui.cc
>> >  M     chrome/browser/external_tab_container_win.h
>> >  M     chrome/browser/tab_contents/tab_contents.cc
>> >  M     chrome/browser/tab_contents/tab_contents_delegate.h
>> >  M     chrome/browser/tab_contents/tab_contents_delegate.cc
>> >  M     chrome/browser/tabs/tab_strip_model.h
>> >  M     chrome/browser/tabs/tab_strip_model_unittest.cc
>> >  M     chrome/browser/views/frame/browser_view.cc
>> >  M     chrome/browser/views/tabs/browser_tab_strip_controller.h
>> >  M     chrome/browser/views/tabs/browser_tab_strip_controller.cc
>> >
>> >
>> >
>>
>
>
>
>
> http://codereview.chromium.org/2819086/show
>

[dhg, 2010-08-05 19:30:36]

http://codereview.chromium.org/2819086/diff/1/13
File chrome/browser/dom_ui/filebrowse_ui.cc (right):

http://codereview.chromium.org/2819086/diff/1/13#newcode759
chrome/browser/dom_ui/filebrowse_ui.cc:759: browser =
BrowserList::FindBrowserWithProfile(contents->profile());
What if more than one browser was created with that profile?

[Jeff Timanus, 2010-08-05 19:38:34]

On 2010/08/05 19:30:36, dhg wrote:
> http://codereview.chromium.org/2819086/diff/1/13
> File chrome/browser/dom_ui/filebrowse_ui.cc (right):
> 
> http://codereview.chromium.org/2819086/diff/1/13#newcode759
> chrome/browser/dom_ui/filebrowse_ui.cc:759: browser =
> BrowserList::FindBrowserWithProfile(contents->profile());
> What if more than one browser was created with that profile?

FindBrowserWithProfile first searches the last active set of browsers with the
given profile, so even if there are multiple browsers with the given profile,
the last active one (which includes the potentially created browser) will be
returned.

A potential problem is that Browser::CreateForPopup does not activate the
browser, so it may not be the instance returned by FindBrowserWithProfile. 
However, it seems that AddTabWithURL explicitly checks if the browser is a popup
and subsequently creates and activates a new browser, which will be returned
correctly by the find routine.

The code is a little complex, but I believe it will work.

[dhg, 2010-08-05 19:50:25]

This sounds way more complex, and a little too much is left to "oh it
should work."  Its screaming a race condition.

On Thu, Aug 5, 2010 at 12:38 PM,  <twiz@chromium.org> wrote:
> On 2010/08/05 19:30:36, dhg wrote:
>>
>> http://codereview.chromium.org/2819086/diff/1/13
>> File chrome/browser/dom_ui/filebrowse_ui.cc (right):
>
>> http://codereview.chromium.org/2819086/diff/1/13#newcode759
>> chrome/browser/dom_ui/filebrowse_ui.cc:759: browser =
>> BrowserList::FindBrowserWithProfile(contents->profile());
>> What if more than one browser was created with that profile?
>
> FindBrowserWithProfile first searches the last active set of browsers with
> the
> given profile, so even if there are multiple browsers with the given
> profile,
> the last active one (which includes the potentially created browser) will be
> returned.
>
> A potential problem is that Browser::CreateForPopup does not activate the
> browser, so it may not be the instance returned by FindBrowserWithProfile.
> However, it seems that AddTabWithURL explicitly checks if the browser is a
> popup
> and subsequently creates and activates a new browser, which will be returned
> correctly by the find routine.
>
> The code is a little complex, but I believe it will work.
>
> http://codereview.chromium.org/2819086/show
>

[Jeff Timanus, 2010-08-05 19:56:40]

On 2010/08/05 19:50:25, dhg wrote:
> This sounds way more complex, and a little too much is left to "oh it
> should work."  Its screaming a race condition.

Yes, I was concerned about race conditions too - especially wrt activating the
browser.

I'll have a second look at the code to see if there is an easier approach.

> 
> On Thu, Aug 5, 2010 at 12:38 PM,  <mailto:twiz@chromium.org> wrote:
> > On 2010/08/05 19:30:36, dhg wrote:
> >>
> >> http://codereview.chromium.org/2819086/diff/1/13
> >> File chrome/browser/dom_ui/filebrowse_ui.cc (right):
> >
> >> http://codereview.chromium.org/2819086/diff/1/13#newcode759
> >> chrome/browser/dom_ui/filebrowse_ui.cc:759: browser =
> >> BrowserList::FindBrowserWithProfile(contents->profile());
> >> What if more than one browser was created with that profile?
> >
> > FindBrowserWithProfile first searches the last active set of browsers with
> > the
> > given profile, so even if there are multiple browsers with the given
> > profile,
> > the last active one (which includes the potentially created browser) will be
> > returned.
> >
> > A potential problem is that Browser::CreateForPopup does not activate the
> > browser, so it may not be the instance returned by FindBrowserWithProfile.
> > However, it seems that AddTabWithURL explicitly checks if the browser is a
> > popup
> > and subsequently creates and activates a new browser, which will be returned
> > correctly by the find routine.
> >
> > The code is a little complex, but I believe it will work.
> >
> > http://codereview.chromium.org/2819086/show
> >
>

[Jeff Timanus, 2010-08-10 18:02:53]

On 2010/08/05 19:50:25, dhg wrote:
> This sounds way more complex, and a little too much is left to "oh it
> should work."  Its screaming a race condition.

I took a moment to change the Browser::AddTabWithUrl method so that it now
conditionally returns a pointer to the Browser instance that was actually used
to add the tab.  I also changed the usage of the API to correct possible errors
similar to the original code in filebrowse_ui.cc.

See files: 
chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc
chrome/browser/extensions/extension_install_ui.cc
chrome/browser/extensions/extension_tabs_module.cc
chrome/browser/importer/importer.cc
chrome/browser/sessions/session_restore.cc
chrome/browser/translate/translate_manager.cc
chrome/browser/views/app_launcher.cc

Also, see the updated tests here:
chrome\browser\autocomplete\autocomplete_browsertest.cc
chrome\browser\ssl\ssl_browser_tests.cc
chrome\browser\find_bar_host_browsertest.cc
chrome\browser\sessions\session_restore_browsertest.cc
chrome\browser\task_manager_browsertest.cc
chrome\test\in_process_browser_test.cc

> 
> On Thu, Aug 5, 2010 at 12:38 PM,  <mailto:twiz@chromium.org> wrote:
> > On 2010/08/05 19:30:36, dhg wrote:
> >>
> >> http://codereview.chromium.org/2819086/diff/1/13
> >> File chrome/browser/dom_ui/filebrowse_ui.cc (right):
> >
> >> http://codereview.chromium.org/2819086/diff/1/13#newcode759
> >> chrome/browser/dom_ui/filebrowse_ui.cc:759: browser =
> >> BrowserList::FindBrowserWithProfile(contents->profile());
> >> What if more than one browser was created with that profile?
> >
> > FindBrowserWithProfile first searches the last active set of browsers with
> > the
> > given profile, so even if there are multiple browsers with the given
> > profile,
> > the last active one (which includes the potentially created browser) will be
> > returned.
> >
> > A potential problem is that Browser::CreateForPopup does not activate the
> > browser, so it may not be the instance returned by FindBrowserWithProfile.
> > However, it seems that AddTabWithURL explicitly checks if the browser is a
> > popup
> > and subsequently creates and activates a new browser, which will be returned
> > correctly by the find routine.
> >
> > The code is a little complex, but I believe it will work.
> >
> > http://codereview.chromium.org/2819086/show
> >
>

[dhg, 2010-08-10 19:11:22]

nice. that feels cleaner to me :) great job.

On Tue, Aug 10, 2010 at 11:02 AM,  <twiz@chromium.org> wrote:
> On 2010/08/05 19:50:25, dhg wrote:
>>
>> This sounds way more complex, and a little too much is left to "oh it
>> should work."  Its screaming a race condition.
>
> I took a moment to change the Browser::AddTabWithUrl method so that it now
> conditionally returns a pointer to the Browser instance that was actually
> used
> to add the tab.  I also changed the usage of the API to correct possible
> errors
> similar to the original code in filebrowse_ui.cc.
>
> See files:
> chrome/browser/dom_ui/html_dialog_tab_contents_delegate.cc
> chrome/browser/extensions/extension_install_ui.cc
> chrome/browser/extensions/extension_tabs_module.cc
> chrome/browser/importer/importer.cc
> chrome/browser/sessions/session_restore.cc
> chrome/browser/translate/translate_manager.cc
> chrome/browser/views/app_launcher.cc
>
> Also, see the updated tests here:
> chrome\browser\autocomplete\autocomplete_browsertest.cc
> chrome\browser\ssl\ssl_browser_tests.cc
> chrome\browser\find_bar_host_browsertest.cc
> chrome\browser\sessions\session_restore_browsertest.cc
> chrome\browser\task_manager_browsertest.cc
> chrome\test\in_process_browser_test.cc
>
>
>> On Thu, Aug 5, 2010 at 12:38 PM,  <mailto:twiz@chromium.org> wrote:
>> > On 2010/08/05 19:30:36, dhg wrote:
>> >>
>> >> http://codereview.chromium.org/2819086/diff/1/13
>> >> File chrome/browser/dom_ui/filebrowse_ui.cc (right):
>> >
>> >> http://codereview.chromium.org/2819086/diff/1/13#newcode759
>> >> chrome/browser/dom_ui/filebrowse_ui.cc:759: browser =
>> >> BrowserList::FindBrowserWithProfile(contents->profile());
>> >> What if more than one browser was created with that profile?
>> >
>> > FindBrowserWithProfile first searches the last active set of browsers
>> > with
>> > the
>> > given profile, so even if there are multiple browsers with the given
>> > profile,
>> > the last active one (which includes the potentially created browser)
>> > will be
>> > returned.
>> >
>> > A potential problem is that Browser::CreateForPopup does not activate
>> > the
>> > browser, so it may not be the instance returned by
>> > FindBrowserWithProfile.
>> > However, it seems that AddTabWithURL explicitly checks if the browser is
>> > a
>> > popup
>> > and subsequently creates and activates a new browser, which will be
>> > returned
>> > correctly by the find routine.
>> >
>> > The code is a little complex, but I believe it will work.
>> >
>> > http://codereview.chromium.org/2819086/show
>> >
>
>
>
>
> http://codereview.chromium.org/2819086/show
>

[Ben Goodger (Google), 2010-09-29 21:27:25]

On 2010/08/05 19:20:47, Jeff Timanus wrote:
> On 2010/08/05 19:14:28, Ben Goodger wrote:
> > Can you describe what changed outside of the ChromeOS bits?
> 
> Nothing changed from the original CL outside of the corrections I had to make
in
> filebrowse_ui.cc, and internet_options_handler.cc.

Not true. I am now discovering you changed the API for AddTabWithURL in an
awkward and pervasive way (returning Browser as an out parameter when every
client in non-ChromeOS code already has a valid Browser), and changed a huge
amount of the codebase to reflect this.

I would have appreciated a more accurate response here, given the amount of
email I get it's hard for me to evaluate everything and I rely on people to tell
me what I should be looking at.

I am going to correct the API problems this time. I may roll some or all of this
changelist back.

-Ben

[Jeff Timanus, 2010-09-29 21:56:29]

On 2010/09/29 21:27:25, Ben Goodger wrote:
> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> > > Can you describe what changed outside of the ChromeOS bits?
> > 
> > Nothing changed from the original CL outside of the corrections I had to
make
> in
> > filebrowse_ui.cc, and internet_options_handler.cc.
> 
> Not true. I am now discovering you changed the API for AddTabWithURL in an
> awkward and pervasive way (returning Browser as an out parameter when every
> client in non-ChromeOS code already has a valid Browser), and changed a huge
> amount of the codebase to reflect this.

Hi Ben,

I'm sorry that this slipped through, and has become an issue.  Do note that the
changes you are referring to happened after you had signed off on the review
process, and during the back-forth with dhg.

I added that callback because my first attempt at a fix (making use of the
BrowserList::FindBrowserWithProfile function) was deemed as too fragile for
callers of the various AddTabWithUrl routines.  See the patch history of
filebrowse_ui.cc.

What is problematic with having these routines return a browser instance via a
parameter?  It is possible that a new browser was created to service them, and
that browser is not directly accessible via just the newly constructed
tab-contents.

Let me know if I can help.

Jeff

> 
> I would have appreciated a more accurate response here, given the amount of
> email I get it's hard for me to evaluate everything and I rely on people to
tell
> me what I should be looking at.
> 
> I am going to correct the API problems this time. I may roll some or all of
this
> changelist back.
> 
> -Ben

[Ben Goodger (Google), 2010-09-29 22:02:33]

For the record, I signed off on a much more limited version of this change
that seemed to primarily affect the app/extension related functions on
Browser, not AddTabWithURL which is a more central API.

The trouble is that none of the code in Chrome-land ever needed this before,
and it's not immediately clear why they would given that they always obtain
a valid browser prior to calling this function. So it's a layer of
complexity that is required only for a specific use case (that I have not
discovered yet, I will determine what that is once I get further into the
build/test process).

I will sort the rest of this out myself.

-Ben

On Wed, Sep 29, 2010 at 2:56 PM, <twiz@chromium.org> wrote:

> On 2010/09/29 21:27:25, Ben Goodger wrote:
>
>> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>> > > Can you describe what changed outside of the ChromeOS bits?
>> >
>> > Nothing changed from the original CL outside of the corrections I had to
>>
> make
>
>> in
>> > filebrowse_ui.cc, and internet_options_handler.cc.
>>
>
>  Not true. I am now discovering you changed the API for AddTabWithURL in an
>> awkward and pervasive way (returning Browser as an out parameter when
>> every
>> client in non-ChromeOS code already has a valid Browser), and changed a
>> huge
>> amount of the codebase to reflect this.
>>
>
> Hi Ben,
>
> I'm sorry that this slipped through, and has become an issue.  Do note that
> the
> changes you are referring to happened after you had signed off on the
> review
> process, and during the back-forth with dhg.
>
> I added that callback because my first attempt at a fix (making use of the
> BrowserList::FindBrowserWithProfile function) was deemed as too fragile for
> callers of the various AddTabWithUrl routines.  See the patch history of
> filebrowse_ui.cc.
>
> What is problematic with having these routines return a browser instance
> via a
> parameter?  It is possible that a new browser was created to service them,
> and
> that browser is not directly accessible via just the newly constructed
> tab-contents.
>
> Let me know if I can help.
>
> Jeff
>
>
>
>  I would have appreciated a more accurate response here, given the amount
>> of
>> email I get it's hard for me to evaluate everything and I rely on people
>> to
>>
> tell
>
>> me what I should be looking at.
>>
>
>  I am going to correct the API problems this time. I may roll some or all
>> of
>>
> this
>
>> changelist back.
>>
>
>  -Ben
>>
>
>
>
> http://codereview.chromium.org/2819086/show
>

[dhg, 2010-09-29 23:17:27]

ben,

You say that all the code in non-chromeos already has this context, but what
about chromeos... doesn't that matter?


On 2010/09/29 22:02:33, Ben Goodger wrote:
> For the record, I signed off on a much more limited version of this change
> that seemed to primarily affect the app/extension related functions on
> Browser, not AddTabWithURL which is a more central API.
> 
> The trouble is that none of the code in Chrome-land ever needed this before,
> and it's not immediately clear why they would given that they always obtain
> a valid browser prior to calling this function. So it's a layer of
> complexity that is required only for a specific use case (that I have not
> discovered yet, I will determine what that is once I get further into the
> build/test process).
> 
> I will sort the rest of this out myself.
> 
> -Ben
> 
> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org> wrote:
> 
> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >
> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >> > > Can you describe what changed outside of the ChromeOS bits?
> >> >
> >> > Nothing changed from the original CL outside of the corrections I had to
> >>
> > make
> >
> >> in
> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >>
> >
> >  Not true. I am now discovering you changed the API for AddTabWithURL in an
> >> awkward and pervasive way (returning Browser as an out parameter when
> >> every
> >> client in non-ChromeOS code already has a valid Browser), and changed a
> >> huge
> >> amount of the codebase to reflect this.
> >>
> >
> > Hi Ben,
> >
> > I'm sorry that this slipped through, and has become an issue.  Do note that
> > the
> > changes you are referring to happened after you had signed off on the
> > review
> > process, and during the back-forth with dhg.
> >
> > I added that callback because my first attempt at a fix (making use of the
> > BrowserList::FindBrowserWithProfile function) was deemed as too fragile for
> > callers of the various AddTabWithUrl routines.  See the patch history of
> > filebrowse_ui.cc.
> >
> > What is problematic with having these routines return a browser instance
> > via a
> > parameter?  It is possible that a new browser was created to service them,
> > and
> > that browser is not directly accessible via just the newly constructed
> > tab-contents.
> >
> > Let me know if I can help.
> >
> > Jeff
> >
> >
> >
> >  I would have appreciated a more accurate response here, given the amount
> >> of
> >> email I get it's hard for me to evaluate everything and I rely on people
> >> to
> >>
> > tell
> >
> >> me what I should be looking at.
> >>
> >
> >  I am going to correct the API problems this time. I may roll some or all
> >> of
> >>
> > this
> >
> >> changelist back.
> >>
> >
> >  -Ben
> >>
> >
> >
> >
> > http://codereview.chromium.org/2819086/show
> >
>

[Ben Goodger (Google), 2010-09-30 00:01:20]

What is ChromeOS code doing such that it requires this change to the API?

-Ben

On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:

> ben,
>
> You say that all the code in non-chromeos already has this context, but
> what
> about chromeos... doesn't that matter?
>
>
>
> On 2010/09/29 22:02:33, Ben Goodger wrote:
>
>> For the record, I signed off on a much more limited version of this change
>> that seemed to primarily affect the app/extension related functions on
>> Browser, not AddTabWithURL which is a more central API.
>>
>
>  The trouble is that none of the code in Chrome-land ever needed this
>> before,
>> and it's not immediately clear why they would given that they always
>> obtain
>> a valid browser prior to calling this function. So it's a layer of
>> complexity that is required only for a specific use case (that I have not
>> discovered yet, I will determine what that is once I get further into the
>> build/test process).
>>
>
>  I will sort the rest of this out myself.
>>
>
>  -Ben
>>
>
>  On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org> wrote:
>>
>
>  > On 2010/09/29 21:27:25, Ben Goodger wrote:
>> >
>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>> >> > > Can you describe what changed outside of the ChromeOS bits?
>> >> >
>> >> > Nothing changed from the original CL outside of the corrections I had
>> to
>> >>
>> > make
>> >
>> >> in
>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
>> >>
>> >
>> >  Not true. I am now discovering you changed the API for AddTabWithURL in
>> an
>> >> awkward and pervasive way (returning Browser as an out parameter when
>> >> every
>> >> client in non-ChromeOS code already has a valid Browser), and changed a
>> >> huge
>> >> amount of the codebase to reflect this.
>> >>
>> >
>> > Hi Ben,
>> >
>> > I'm sorry that this slipped through, and has become an issue.  Do note
>> that
>> > the
>> > changes you are referring to happened after you had signed off on the
>> > review
>> > process, and during the back-forth with dhg.
>> >
>> > I added that callback because my first attempt at a fix (making use of
>> the
>> > BrowserList::FindBrowserWithProfile function) was deemed as too fragile
>> for
>> > callers of the various AddTabWithUrl routines.  See the patch history of
>> > filebrowse_ui.cc.
>> >
>> > What is problematic with having these routines return a browser instance
>> > via a
>> > parameter?  It is possible that a new browser was created to service
>> them,
>> > and
>> > that browser is not directly accessible via just the newly constructed
>> > tab-contents.
>> >
>> > Let me know if I can help.
>> >
>> > Jeff
>> >
>> >
>> >
>> >  I would have appreciated a more accurate response here, given the
>> amount
>> >> of
>> >> email I get it's hard for me to evaluate everything and I rely on
>> people
>> >> to
>> >>
>> > tell
>> >
>> >> me what I should be looking at.
>> >>
>> >
>> >  I am going to correct the API problems this time. I may roll some or
>> all
>> >> of
>> >>
>> > this
>> >
>> >> changelist back.
>> >>
>> >
>> >  -Ben
>> >>
>> >
>> >
>> >
>> > http://codereview.chromium.org/2819086/show
>> >
>>
>
>
>
>
> http://codereview.chromium.org/2819086/show
>

[dhg, 2010-09-30 00:10:05]

There was a discussion further back, but when it creates the tab, it
needs to know the browser associated with that tab.  (so it can
subscribe to events, etc) Since add tab with url can actually open up
in a different browser, this can cause a problem.

On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> What is ChromeOS code doing such that it requires this change to the API?
> -Ben
>
> On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
>>
>> ben,
>>
>> You say that all the code in non-chromeos already has this context, but
>> what
>> about chromeos... doesn't that matter?
>>
>>
>> On 2010/09/29 22:02:33, Ben Goodger wrote:
>>>
>>> For the record, I signed off on a much more limited version of this
>>> change
>>> that seemed to primarily affect the app/extension related functions on
>>> Browser, not AddTabWithURL which is a more central API.
>>
>>> The trouble is that none of the code in Chrome-land ever needed this
>>> before,
>>> and it's not immediately clear why they would given that they always
>>> obtain
>>> a valid browser prior to calling this function. So it's a layer of
>>> complexity that is required only for a specific use case (that I have not
>>> discovered yet, I will determine what that is once I get further into the
>>> build/test process).
>>
>>> I will sort the rest of this out myself.
>>
>>> -Ben
>>
>>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org> wrote:
>>
>>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
>>> >
>>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>>> >> > > Can you describe what changed outside of the ChromeOS bits?
>>> >> >
>>> >> > Nothing changed from the original CL outside of the corrections I
>>> >> > had to
>>> >>
>>> > make
>>> >
>>> >> in
>>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
>>> >>
>>> >
>>> >  Not true. I am now discovering you changed the API for AddTabWithURL
>>> > in an
>>> >> awkward and pervasive way (returning Browser as an out parameter when
>>> >> every
>>> >> client in non-ChromeOS code already has a valid Browser), and changed
>>> >> a
>>> >> huge
>>> >> amount of the codebase to reflect this.
>>> >>
>>> >
>>> > Hi Ben,
>>> >
>>> > I'm sorry that this slipped through, and has become an issue.  Do note
>>> > that
>>> > the
>>> > changes you are referring to happened after you had signed off on the
>>> > review
>>> > process, and during the back-forth with dhg.
>>> >
>>> > I added that callback because my first attempt at a fix (making use of
>>> > the
>>> > BrowserList::FindBrowserWithProfile function) was deemed as too fragile
>>> > for
>>> > callers of the various AddTabWithUrl routines.  See the patch history
>>> > of
>>> > filebrowse_ui.cc.
>>> >
>>> > What is problematic with having these routines return a browser
>>> > instance
>>> > via a
>>> > parameter?  It is possible that a new browser was created to service
>>> > them,
>>> > and
>>> > that browser is not directly accessible via just the newly constructed
>>> > tab-contents.
>>> >
>>> > Let me know if I can help.
>>> >
>>> > Jeff
>>> >
>>> >
>>> >
>>> >  I would have appreciated a more accurate response here, given the
>>> > amount
>>> >> of
>>> >> email I get it's hard for me to evaluate everything and I rely on
>>> >> people
>>> >> to
>>> >>
>>> > tell
>>> >
>>> >> me what I should be looking at.
>>> >>
>>> >
>>> >  I am going to correct the API problems this time. I may roll some or
>>> > all
>>> >> of
>>> >>
>>> > this
>>> >
>>> >> changelist back.
>>> >>
>>> >
>>> >  -Ben
>>> >>
>>> >
>>> >
>>> >
>>> > http://codereview.chromium.org/2819086/show
>>> >
>>
>>
>>
>>
>> http://codereview.chromium.org/2819086/show
>
>

[Ben Goodger (Google), 2010-09-30 00:15:29]

The conventional pattern is:

1. Look for an existing browser that matches what you need using the browser
list.
2. If you can't find one, or if you always want a new window, create a new
one.
3. Call AddTabWithURL on the new one.

I know AddTabWithURL can create a Browser internally but this is not an
aspect of this function that I enjoy, considering the pattern is so often:

Browser* b = locate_browser_somehow;
b->AddTabWithURL(..., &b) (may create a new browser and call Show() on its
window)
b->window()->Show(); // Show called twice, potentially.

-Ben

On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org> wrote:

> There was a discussion further back, but when it creates the tab, it
> needs to know the browser associated with that tab.  (so it can
> subscribe to events, etc) Since add tab with url can actually open up
> in a different browser, this can cause a problem.
>
> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > What is ChromeOS code doing such that it requires this change to the API?
> > -Ben
> >
> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
> >>
> >> ben,
> >>
> >> You say that all the code in non-chromeos already has this context, but
> >> what
> >> about chromeos... doesn't that matter?
> >>
> >>
> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
> >>>
> >>> For the record, I signed off on a much more limited version of this
> >>> change
> >>> that seemed to primarily affect the app/extension related functions on
> >>> Browser, not AddTabWithURL which is a more central API.
> >>
> >>> The trouble is that none of the code in Chrome-land ever needed this
> >>> before,
> >>> and it's not immediately clear why they would given that they always
> >>> obtain
> >>> a valid browser prior to calling this function. So it's a layer of
> >>> complexity that is required only for a specific use case (that I have
> not
> >>> discovered yet, I will determine what that is once I get further into
> the
> >>> build/test process).
> >>
> >>> I will sort the rest of this out myself.
> >>
> >>> -Ben
> >>
> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org> wrote:
> >>
> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >>> >
> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >>> >> > > Can you describe what changed outside of the ChromeOS bits?
> >>> >> >
> >>> >> > Nothing changed from the original CL outside of the corrections I
> >>> >> > had to
> >>> >>
> >>> > make
> >>> >
> >>> >> in
> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >>> >>
> >>> >
> >>> >  Not true. I am now discovering you changed the API for AddTabWithURL
> >>> > in an
> >>> >> awkward and pervasive way (returning Browser as an out parameter
> when
> >>> >> every
> >>> >> client in non-ChromeOS code already has a valid Browser), and
> changed
> >>> >> a
> >>> >> huge
> >>> >> amount of the codebase to reflect this.
> >>> >>
> >>> >
> >>> > Hi Ben,
> >>> >
> >>> > I'm sorry that this slipped through, and has become an issue.  Do
> note
> >>> > that
> >>> > the
> >>> > changes you are referring to happened after you had signed off on the
> >>> > review
> >>> > process, and during the back-forth with dhg.
> >>> >
> >>> > I added that callback because my first attempt at a fix (making use
> of
> >>> > the
> >>> > BrowserList::FindBrowserWithProfile function) was deemed as too
> fragile
> >>> > for
> >>> > callers of the various AddTabWithUrl routines.  See the patch history
> >>> > of
> >>> > filebrowse_ui.cc.
> >>> >
> >>> > What is problematic with having these routines return a browser
> >>> > instance
> >>> > via a
> >>> > parameter?  It is possible that a new browser was created to service
> >>> > them,
> >>> > and
> >>> > that browser is not directly accessible via just the newly
> constructed
> >>> > tab-contents.
> >>> >
> >>> > Let me know if I can help.
> >>> >
> >>> > Jeff
> >>> >
> >>> >
> >>> >
> >>> >  I would have appreciated a more accurate response here, given the
> >>> > amount
> >>> >> of
> >>> >> email I get it's hard for me to evaluate everything and I rely on
> >>> >> people
> >>> >> to
> >>> >>
> >>> > tell
> >>> >
> >>> >> me what I should be looking at.
> >>> >>
> >>> >
> >>> >  I am going to correct the API problems this time. I may roll some or
> >>> > all
> >>> >> of
> >>> >>
> >>> > this
> >>> >
> >>> >> changelist back.
> >>> >>
> >>> >
> >>> >  -Ben
> >>> >>
> >>> >
> >>> >
> >>> >
> >>> > http://codereview.chromium.org/2819086/show
> >>> >
> >>
> >>
> >>
> >>
> >> http://codereview.chromium.org/2819086/show
> >
> >
>

[dhg, 2010-09-30 00:19:35]

and since we have to keep around the browser (for various download
reasons, etc), we need to get back the browser, otherwise we may have
a copy of a browser that has gone away, and thus we would crash.  If
add tab with url was guarenteed to open in the same browser it was
told to, then there would be no problem.

On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> The conventional pattern is:
> 1. Look for an existing browser that matches what you need using the browser
> list.
> 2. If you can't find one, or if you always want a new window, create a new
> one.
> 3. Call AddTabWithURL on the new one.
> I know AddTabWithURL can create a Browser internally but this is not an
> aspect of this function that I enjoy, considering the pattern is so often:
> Browser* b = locate_browser_somehow;
> b->AddTabWithURL(..., &b) (may create a new browser and call Show() on its
> window)
> b->window()->Show(); // Show called twice, potentially.
> -Ben
>
> On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org> wrote:
>>
>> There was a discussion further back, but when it creates the tab, it
>> needs to know the browser associated with that tab.  (so it can
>> subscribe to events, etc) Since add tab with url can actually open up
>> in a different browser, this can cause a problem.
>>
>> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google) <ben@chromium.org>
>> wrote:
>> > What is ChromeOS code doing such that it requires this change to the
>> > API?
>> > -Ben
>> >
>> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
>> >>
>> >> ben,
>> >>
>> >> You say that all the code in non-chromeos already has this context, but
>> >> what
>> >> about chromeos... doesn't that matter?
>> >>
>> >>
>> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
>> >>>
>> >>> For the record, I signed off on a much more limited version of this
>> >>> change
>> >>> that seemed to primarily affect the app/extension related functions on
>> >>> Browser, not AddTabWithURL which is a more central API.
>> >>
>> >>> The trouble is that none of the code in Chrome-land ever needed this
>> >>> before,
>> >>> and it's not immediately clear why they would given that they always
>> >>> obtain
>> >>> a valid browser prior to calling this function. So it's a layer of
>> >>> complexity that is required only for a specific use case (that I have
>> >>> not
>> >>> discovered yet, I will determine what that is once I get further into
>> >>> the
>> >>> build/test process).
>> >>
>> >>> I will sort the rest of this out myself.
>> >>
>> >>> -Ben
>> >>
>> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org> wrote:
>> >>
>> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
>> >>> >
>> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>> >>> >> > > Can you describe what changed outside of the ChromeOS bits?
>> >>> >> >
>> >>> >> > Nothing changed from the original CL outside of the corrections I
>> >>> >> > had to
>> >>> >>
>> >>> > make
>> >>> >
>> >>> >> in
>> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
>> >>> >>
>> >>> >
>> >>> >  Not true. I am now discovering you changed the API for
>> >>> > AddTabWithURL
>> >>> > in an
>> >>> >> awkward and pervasive way (returning Browser as an out parameter
>> >>> >> when
>> >>> >> every
>> >>> >> client in non-ChromeOS code already has a valid Browser), and
>> >>> >> changed
>> >>> >> a
>> >>> >> huge
>> >>> >> amount of the codebase to reflect this.
>> >>> >>
>> >>> >
>> >>> > Hi Ben,
>> >>> >
>> >>> > I'm sorry that this slipped through, and has become an issue.  Do
>> >>> > note
>> >>> > that
>> >>> > the
>> >>> > changes you are referring to happened after you had signed off on
>> >>> > the
>> >>> > review
>> >>> > process, and during the back-forth with dhg.
>> >>> >
>> >>> > I added that callback because my first attempt at a fix (making use
>> >>> > of
>> >>> > the
>> >>> > BrowserList::FindBrowserWithProfile function) was deemed as too
>> >>> > fragile
>> >>> > for
>> >>> > callers of the various AddTabWithUrl routines.  See the patch
>> >>> > history
>> >>> > of
>> >>> > filebrowse_ui.cc.
>> >>> >
>> >>> > What is problematic with having these routines return a browser
>> >>> > instance
>> >>> > via a
>> >>> > parameter?  It is possible that a new browser was created to service
>> >>> > them,
>> >>> > and
>> >>> > that browser is not directly accessible via just the newly
>> >>> > constructed
>> >>> > tab-contents.
>> >>> >
>> >>> > Let me know if I can help.
>> >>> >
>> >>> > Jeff
>> >>> >
>> >>> >
>> >>> >
>> >>> >  I would have appreciated a more accurate response here, given the
>> >>> > amount
>> >>> >> of
>> >>> >> email I get it's hard for me to evaluate everything and I rely on
>> >>> >> people
>> >>> >> to
>> >>> >>
>> >>> > tell
>> >>> >
>> >>> >> me what I should be looking at.
>> >>> >>
>> >>> >
>> >>> >  I am going to correct the API problems this time. I may roll some
>> >>> > or
>> >>> > all
>> >>> >> of
>> >>> >>
>> >>> > this
>> >>> >
>> >>> >> changelist back.
>> >>> >>
>> >>> >
>> >>> >  -Ben
>> >>> >>
>> >>> >
>> >>> >
>> >>> >
>> >>> > http://codereview.chromium.org/2819086/show
>> >>> >
>> >>
>> >>
>> >>
>> >>
>> >> http://codereview.chromium.org/2819086/show
>> >
>> >
>
>

[Ben Goodger (Google), 2010-09-30 00:21:16]

I don't understand how this follows from the pattern I described. If you ask
the BrowserList for a matching browser you will always scan available
non-deleted Browsers, so you can never crash.

-Ben

On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org> wrote:

> and since we have to keep around the browser (for various download
> reasons, etc), we need to get back the browser, otherwise we may have
> a copy of a browser that has gone away, and thus we would crash.  If
> add tab with url was guarenteed to open in the same browser it was
> told to, then there would be no problem.
>
> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > The conventional pattern is:
> > 1. Look for an existing browser that matches what you need using the
> browser
> > list.
> > 2. If you can't find one, or if you always want a new window, create a
> new
> > one.
> > 3. Call AddTabWithURL on the new one.
> > I know AddTabWithURL can create a Browser internally but this is not an
> > aspect of this function that I enjoy, considering the pattern is so
> often:
> > Browser* b = locate_browser_somehow;
> > b->AddTabWithURL(..., &b) (may create a new browser and call Show() on
> its
> > window)
> > b->window()->Show(); // Show called twice, potentially.
> > -Ben
> >
> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org> wrote:
> >>
> >> There was a discussion further back, but when it creates the tab, it
> >> needs to know the browser associated with that tab.  (so it can
> >> subscribe to events, etc) Since add tab with url can actually open up
> >> in a different browser, this can cause a problem.
> >>
> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google) <ben@chromium.org
> >
> >> wrote:
> >> > What is ChromeOS code doing such that it requires this change to the
> >> > API?
> >> > -Ben
> >> >
> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
> >> >>
> >> >> ben,
> >> >>
> >> >> You say that all the code in non-chromeos already has this context,
> but
> >> >> what
> >> >> about chromeos... doesn't that matter?
> >> >>
> >> >>
> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
> >> >>>
> >> >>> For the record, I signed off on a much more limited version of this
> >> >>> change
> >> >>> that seemed to primarily affect the app/extension related functions
> on
> >> >>> Browser, not AddTabWithURL which is a more central API.
> >> >>
> >> >>> The trouble is that none of the code in Chrome-land ever needed this
> >> >>> before,
> >> >>> and it's not immediately clear why they would given that they always
> >> >>> obtain
> >> >>> a valid browser prior to calling this function. So it's a layer of
> >> >>> complexity that is required only for a specific use case (that I
> have
> >> >>> not
> >> >>> discovered yet, I will determine what that is once I get further
> into
> >> >>> the
> >> >>> build/test process).
> >> >>
> >> >>> I will sort the rest of this out myself.
> >> >>
> >> >>> -Ben
> >> >>
> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org> wrote:
> >> >>
> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >> >>> >
> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >> >>> >> > > Can you describe what changed outside of the ChromeOS bits?
> >> >>> >> >
> >> >>> >> > Nothing changed from the original CL outside of the corrections
> I
> >> >>> >> > had to
> >> >>> >>
> >> >>> > make
> >> >>> >
> >> >>> >> in
> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >> >>> >>
> >> >>> >
> >> >>> >  Not true. I am now discovering you changed the API for
> >> >>> > AddTabWithURL
> >> >>> > in an
> >> >>> >> awkward and pervasive way (returning Browser as an out parameter
> >> >>> >> when
> >> >>> >> every
> >> >>> >> client in non-ChromeOS code already has a valid Browser), and
> >> >>> >> changed
> >> >>> >> a
> >> >>> >> huge
> >> >>> >> amount of the codebase to reflect this.
> >> >>> >>
> >> >>> >
> >> >>> > Hi Ben,
> >> >>> >
> >> >>> > I'm sorry that this slipped through, and has become an issue.  Do
> >> >>> > note
> >> >>> > that
> >> >>> > the
> >> >>> > changes you are referring to happened after you had signed off on
> >> >>> > the
> >> >>> > review
> >> >>> > process, and during the back-forth with dhg.
> >> >>> >
> >> >>> > I added that callback because my first attempt at a fix (making
> use
> >> >>> > of
> >> >>> > the
> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed as too
> >> >>> > fragile
> >> >>> > for
> >> >>> > callers of the various AddTabWithUrl routines.  See the patch
> >> >>> > history
> >> >>> > of
> >> >>> > filebrowse_ui.cc.
> >> >>> >
> >> >>> > What is problematic with having these routines return a browser
> >> >>> > instance
> >> >>> > via a
> >> >>> > parameter?  It is possible that a new browser was created to
> service
> >> >>> > them,
> >> >>> > and
> >> >>> > that browser is not directly accessible via just the newly
> >> >>> > constructed
> >> >>> > tab-contents.
> >> >>> >
> >> >>> > Let me know if I can help.
> >> >>> >
> >> >>> > Jeff
> >> >>> >
> >> >>> >
> >> >>> >
> >> >>> >  I would have appreciated a more accurate response here, given the
> >> >>> > amount
> >> >>> >> of
> >> >>> >> email I get it's hard for me to evaluate everything and I rely on
> >> >>> >> people
> >> >>> >> to
> >> >>> >>
> >> >>> > tell
> >> >>> >
> >> >>> >> me what I should be looking at.
> >> >>> >>
> >> >>> >
> >> >>> >  I am going to correct the API problems this time. I may roll some
> >> >>> > or
> >> >>> > all
> >> >>> >> of
> >> >>> >>
> >> >>> > this
> >> >>> >
> >> >>> >> changelist back.
> >> >>> >>
> >> >>> >
> >> >>> >  -Ben
> >> >>> >>
> >> >>> >
> >> >>> >
> >> >>> >
> >> >>> > http://codereview.chromium.org/2819086/show
> >> >>> >
> >> >>
> >> >>
> >> >>
> >> >>
> >> >> http://codereview.chromium.org/2819086/show
> >> >
> >> >
> >
> >
>

[dhg, 2010-09-30 00:22:56]

But then how do I know if i have the SAME filebrowser as the one that
the user is acting on.  Since there can be more than one filebrowser.

On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> I don't understand how this follows from the pattern I described. If you ask
> the BrowserList for a matching browser you will always scan available
> non-deleted Browsers, so you can never crash.
> -Ben
>
> On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org> wrote:
>>
>> and since we have to keep around the browser (for various download
>> reasons, etc), we need to get back the browser, otherwise we may have
>> a copy of a browser that has gone away, and thus we would crash.  If
>> add tab with url was guarenteed to open in the same browser it was
>> told to, then there would be no problem.
>>
>> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google) <ben@chromium.org>
>> wrote:
>> > The conventional pattern is:
>> > 1. Look for an existing browser that matches what you need using the
>> > browser
>> > list.
>> > 2. If you can't find one, or if you always want a new window, create a
>> > new
>> > one.
>> > 3. Call AddTabWithURL on the new one.
>> > I know AddTabWithURL can create a Browser internally but this is not an
>> > aspect of this function that I enjoy, considering the pattern is so
>> > often:
>> > Browser* b = locate_browser_somehow;
>> > b->AddTabWithURL(..., &b) (may create a new browser and call Show() on
>> > its
>> > window)
>> > b->window()->Show(); // Show called twice, potentially.
>> > -Ben
>> >
>> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org> wrote:
>> >>
>> >> There was a discussion further back, but when it creates the tab, it
>> >> needs to know the browser associated with that tab.  (so it can
>> >> subscribe to events, etc) Since add tab with url can actually open up
>> >> in a different browser, this can cause a problem.
>> >>
>> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
>> >> <ben@chromium.org>
>> >> wrote:
>> >> > What is ChromeOS code doing such that it requires this change to the
>> >> > API?
>> >> > -Ben
>> >> >
>> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
>> >> >>
>> >> >> ben,
>> >> >>
>> >> >> You say that all the code in non-chromeos already has this context,
>> >> >> but
>> >> >> what
>> >> >> about chromeos... doesn't that matter?
>> >> >>
>> >> >>
>> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
>> >> >>>
>> >> >>> For the record, I signed off on a much more limited version of this
>> >> >>> change
>> >> >>> that seemed to primarily affect the app/extension related functions
>> >> >>> on
>> >> >>> Browser, not AddTabWithURL which is a more central API.
>> >> >>
>> >> >>> The trouble is that none of the code in Chrome-land ever needed
>> >> >>> this
>> >> >>> before,
>> >> >>> and it's not immediately clear why they would given that they
>> >> >>> always
>> >> >>> obtain
>> >> >>> a valid browser prior to calling this function. So it's a layer of
>> >> >>> complexity that is required only for a specific use case (that I
>> >> >>> have
>> >> >>> not
>> >> >>> discovered yet, I will determine what that is once I get further
>> >> >>> into
>> >> >>> the
>> >> >>> build/test process).
>> >> >>
>> >> >>> I will sort the rest of this out myself.
>> >> >>
>> >> >>> -Ben
>> >> >>
>> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org> wrote:
>> >> >>
>> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
>> >> >>> >
>> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>> >> >>> >> > > Can you describe what changed outside of the ChromeOS bits?
>> >> >>> >> >
>> >> >>> >> > Nothing changed from the original CL outside of the
>> >> >>> >> > corrections I
>> >> >>> >> > had to
>> >> >>> >>
>> >> >>> > make
>> >> >>> >
>> >> >>> >> in
>> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
>> >> >>> >>
>> >> >>> >
>> >> >>> >  Not true. I am now discovering you changed the API for
>> >> >>> > AddTabWithURL
>> >> >>> > in an
>> >> >>> >> awkward and pervasive way (returning Browser as an out parameter
>> >> >>> >> when
>> >> >>> >> every
>> >> >>> >> client in non-ChromeOS code already has a valid Browser), and
>> >> >>> >> changed
>> >> >>> >> a
>> >> >>> >> huge
>> >> >>> >> amount of the codebase to reflect this.
>> >> >>> >>
>> >> >>> >
>> >> >>> > Hi Ben,
>> >> >>> >
>> >> >>> > I'm sorry that this slipped through, and has become an issue.  Do
>> >> >>> > note
>> >> >>> > that
>> >> >>> > the
>> >> >>> > changes you are referring to happened after you had signed off on
>> >> >>> > the
>> >> >>> > review
>> >> >>> > process, and during the back-forth with dhg.
>> >> >>> >
>> >> >>> > I added that callback because my first attempt at a fix (making
>> >> >>> > use
>> >> >>> > of
>> >> >>> > the
>> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed as too
>> >> >>> > fragile
>> >> >>> > for
>> >> >>> > callers of the various AddTabWithUrl routines.  See the patch
>> >> >>> > history
>> >> >>> > of
>> >> >>> > filebrowse_ui.cc.
>> >> >>> >
>> >> >>> > What is problematic with having these routines return a browser
>> >> >>> > instance
>> >> >>> > via a
>> >> >>> > parameter?  It is possible that a new browser was created to
>> >> >>> > service
>> >> >>> > them,
>> >> >>> > and
>> >> >>> > that browser is not directly accessible via just the newly
>> >> >>> > constructed
>> >> >>> > tab-contents.
>> >> >>> >
>> >> >>> > Let me know if I can help.
>> >> >>> >
>> >> >>> > Jeff
>> >> >>> >
>> >> >>> >
>> >> >>> >
>> >> >>> >  I would have appreciated a more accurate response here, given
>> >> >>> > the
>> >> >>> > amount
>> >> >>> >> of
>> >> >>> >> email I get it's hard for me to evaluate everything and I rely
>> >> >>> >> on
>> >> >>> >> people
>> >> >>> >> to
>> >> >>> >>
>> >> >>> > tell
>> >> >>> >
>> >> >>> >> me what I should be looking at.
>> >> >>> >>
>> >> >>> >
>> >> >>> >  I am going to correct the API problems this time. I may roll
>> >> >>> > some
>> >> >>> > or
>> >> >>> > all
>> >> >>> >> of
>> >> >>> >>
>> >> >>> > this
>> >> >>> >
>> >> >>> >> changelist back.
>> >> >>> >>
>> >> >>> >
>> >> >>> >  -Ben
>> >> >>> >>
>> >> >>> >
>> >> >>> >
>> >> >>> >
>> >> >>> > http://codereview.chromium.org/2819086/show
>> >> >>> >
>> >> >>
>> >> >>
>> >> >>
>> >> >>
>> >> >> http://codereview.chromium.org/2819086/show
>> >> >
>> >> >
>> >
>> >
>
>

[Ben Goodger (Google), 2010-09-30 00:25:11]

BrowserList always returns the most recently active item.

-Ben

On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org> wrote:

> But then how do I know if i have the SAME filebrowser as the one that
> the user is acting on.  Since there can be more than one filebrowser.
>
> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > I don't understand how this follows from the pattern I described. If you
> ask
> > the BrowserList for a matching browser you will always scan available
> > non-deleted Browsers, so you can never crash.
> > -Ben
> >
> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org> wrote:
> >>
> >> and since we have to keep around the browser (for various download
> >> reasons, etc), we need to get back the browser, otherwise we may have
> >> a copy of a browser that has gone away, and thus we would crash.  If
> >> add tab with url was guarenteed to open in the same browser it was
> >> told to, then there would be no problem.
> >>
> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google) <ben@chromium.org
> >
> >> wrote:
> >> > The conventional pattern is:
> >> > 1. Look for an existing browser that matches what you need using the
> >> > browser
> >> > list.
> >> > 2. If you can't find one, or if you always want a new window, create a
> >> > new
> >> > one.
> >> > 3. Call AddTabWithURL on the new one.
> >> > I know AddTabWithURL can create a Browser internally but this is not
> an
> >> > aspect of this function that I enjoy, considering the pattern is so
> >> > often:
> >> > Browser* b = locate_browser_somehow;
> >> > b->AddTabWithURL(..., &b) (may create a new browser and call Show() on
> >> > its
> >> > window)
> >> > b->window()->Show(); // Show called twice, potentially.
> >> > -Ben
> >> >
> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org>
> wrote:
> >> >>
> >> >> There was a discussion further back, but when it creates the tab, it
> >> >> needs to know the browser associated with that tab.  (so it can
> >> >> subscribe to events, etc) Since add tab with url can actually open up
> >> >> in a different browser, this can cause a problem.
> >> >>
> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
> >> >> <ben@chromium.org>
> >> >> wrote:
> >> >> > What is ChromeOS code doing such that it requires this change to
> the
> >> >> > API?
> >> >> > -Ben
> >> >> >
> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
> >> >> >>
> >> >> >> ben,
> >> >> >>
> >> >> >> You say that all the code in non-chromeos already has this
> context,
> >> >> >> but
> >> >> >> what
> >> >> >> about chromeos... doesn't that matter?
> >> >> >>
> >> >> >>
> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
> >> >> >>>
> >> >> >>> For the record, I signed off on a much more limited version of
> this
> >> >> >>> change
> >> >> >>> that seemed to primarily affect the app/extension related
> functions
> >> >> >>> on
> >> >> >>> Browser, not AddTabWithURL which is a more central API.
> >> >> >>
> >> >> >>> The trouble is that none of the code in Chrome-land ever needed
> >> >> >>> this
> >> >> >>> before,
> >> >> >>> and it's not immediately clear why they would given that they
> >> >> >>> always
> >> >> >>> obtain
> >> >> >>> a valid browser prior to calling this function. So it's a layer
> of
> >> >> >>> complexity that is required only for a specific use case (that I
> >> >> >>> have
> >> >> >>> not
> >> >> >>> discovered yet, I will determine what that is once I get further
> >> >> >>> into
> >> >> >>> the
> >> >> >>> build/test process).
> >> >> >>
> >> >> >>> I will sort the rest of this out myself.
> >> >> >>
> >> >> >>> -Ben
> >> >> >>
> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org>
> wrote:
> >> >> >>
> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >> >> >>> >
> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >> >> >>> >> > > Can you describe what changed outside of the ChromeOS
> bits?
> >> >> >>> >> >
> >> >> >>> >> > Nothing changed from the original CL outside of the
> >> >> >>> >> > corrections I
> >> >> >>> >> > had to
> >> >> >>> >>
> >> >> >>> > make
> >> >> >>> >
> >> >> >>> >> in
> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >> >> >>> >>
> >> >> >>> >
> >> >> >>> >  Not true. I am now discovering you changed the API for
> >> >> >>> > AddTabWithURL
> >> >> >>> > in an
> >> >> >>> >> awkward and pervasive way (returning Browser as an out
> parameter
> >> >> >>> >> when
> >> >> >>> >> every
> >> >> >>> >> client in non-ChromeOS code already has a valid Browser), and
> >> >> >>> >> changed
> >> >> >>> >> a
> >> >> >>> >> huge
> >> >> >>> >> amount of the codebase to reflect this.
> >> >> >>> >>
> >> >> >>> >
> >> >> >>> > Hi Ben,
> >> >> >>> >
> >> >> >>> > I'm sorry that this slipped through, and has become an issue.
>  Do
> >> >> >>> > note
> >> >> >>> > that
> >> >> >>> > the
> >> >> >>> > changes you are referring to happened after you had signed off
> on
> >> >> >>> > the
> >> >> >>> > review
> >> >> >>> > process, and during the back-forth with dhg.
> >> >> >>> >
> >> >> >>> > I added that callback because my first attempt at a fix (making
> >> >> >>> > use
> >> >> >>> > of
> >> >> >>> > the
> >> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed as too
> >> >> >>> > fragile
> >> >> >>> > for
> >> >> >>> > callers of the various AddTabWithUrl routines.  See the patch
> >> >> >>> > history
> >> >> >>> > of
> >> >> >>> > filebrowse_ui.cc.
> >> >> >>> >
> >> >> >>> > What is problematic with having these routines return a browser
> >> >> >>> > instance
> >> >> >>> > via a
> >> >> >>> > parameter?  It is possible that a new browser was created to
> >> >> >>> > service
> >> >> >>> > them,
> >> >> >>> > and
> >> >> >>> > that browser is not directly accessible via just the newly
> >> >> >>> > constructed
> >> >> >>> > tab-contents.
> >> >> >>> >
> >> >> >>> > Let me know if I can help.
> >> >> >>> >
> >> >> >>> > Jeff
> >> >> >>> >
> >> >> >>> >
> >> >> >>> >
> >> >> >>> >  I would have appreciated a more accurate response here, given
> >> >> >>> > the
> >> >> >>> > amount
> >> >> >>> >> of
> >> >> >>> >> email I get it's hard for me to evaluate everything and I rely
> >> >> >>> >> on
> >> >> >>> >> people
> >> >> >>> >> to
> >> >> >>> >>
> >> >> >>> > tell
> >> >> >>> >
> >> >> >>> >> me what I should be looking at.
> >> >> >>> >>
> >> >> >>> >
> >> >> >>> >  I am going to correct the API problems this time. I may roll
> >> >> >>> > some
> >> >> >>> > or
> >> >> >>> > all
> >> >> >>> >> of
> >> >> >>> >>
> >> >> >>> > this
> >> >> >>> >
> >> >> >>> >> changelist back.
> >> >> >>> >>
> >> >> >>> >
> >> >> >>> >  -Ben
> >> >> >>> >>
> >> >> >>> >
> >> >> >>> >
> >> >> >>> >
> >> >> >>> > http://codereview.chromium.org/2819086/show
> >> >> >>> >
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >>
> >> >> >> http://codereview.chromium.org/2819086/show
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>

[dhg, 2010-09-30 00:26:31]

Is that the right one?

On Wed, Sep 29, 2010 at 5:25 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> BrowserList always returns the most recently active item.
> -Ben
>
> On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org> wrote:
>>
>> But then how do I know if i have the SAME filebrowser as the one that
>> the user is acting on.  Since there can be more than one filebrowser.
>>
>> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google) <ben@chromium.org>
>> wrote:
>> > I don't understand how this follows from the pattern I described. If you
>> > ask
>> > the BrowserList for a matching browser you will always scan available
>> > non-deleted Browsers, so you can never crash.
>> > -Ben
>> >
>> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org> wrote:
>> >>
>> >> and since we have to keep around the browser (for various download
>> >> reasons, etc), we need to get back the browser, otherwise we may have
>> >> a copy of a browser that has gone away, and thus we would crash.  If
>> >> add tab with url was guarenteed to open in the same browser it was
>> >> told to, then there would be no problem.
>> >>
>> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google)
>> >> <ben@chromium.org>
>> >> wrote:
>> >> > The conventional pattern is:
>> >> > 1. Look for an existing browser that matches what you need using the
>> >> > browser
>> >> > list.
>> >> > 2. If you can't find one, or if you always want a new window, create
>> >> > a
>> >> > new
>> >> > one.
>> >> > 3. Call AddTabWithURL on the new one.
>> >> > I know AddTabWithURL can create a Browser internally but this is not
>> >> > an
>> >> > aspect of this function that I enjoy, considering the pattern is so
>> >> > often:
>> >> > Browser* b = locate_browser_somehow;
>> >> > b->AddTabWithURL(..., &b) (may create a new browser and call Show()
>> >> > on
>> >> > its
>> >> > window)
>> >> > b->window()->Show(); // Show called twice, potentially.
>> >> > -Ben
>> >> >
>> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> There was a discussion further back, but when it creates the tab, it
>> >> >> needs to know the browser associated with that tab.  (so it can
>> >> >> subscribe to events, etc) Since add tab with url can actually open
>> >> >> up
>> >> >> in a different browser, this can cause a problem.
>> >> >>
>> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
>> >> >> <ben@chromium.org>
>> >> >> wrote:
>> >> >> > What is ChromeOS code doing such that it requires this change to
>> >> >> > the
>> >> >> > API?
>> >> >> > -Ben
>> >> >> >
>> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
>> >> >> >>
>> >> >> >> ben,
>> >> >> >>
>> >> >> >> You say that all the code in non-chromeos already has this
>> >> >> >> context,
>> >> >> >> but
>> >> >> >> what
>> >> >> >> about chromeos... doesn't that matter?
>> >> >> >>
>> >> >> >>
>> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
>> >> >> >>>
>> >> >> >>> For the record, I signed off on a much more limited version of
>> >> >> >>> this
>> >> >> >>> change
>> >> >> >>> that seemed to primarily affect the app/extension related
>> >> >> >>> functions
>> >> >> >>> on
>> >> >> >>> Browser, not AddTabWithURL which is a more central API.
>> >> >> >>
>> >> >> >>> The trouble is that none of the code in Chrome-land ever needed
>> >> >> >>> this
>> >> >> >>> before,
>> >> >> >>> and it's not immediately clear why they would given that they
>> >> >> >>> always
>> >> >> >>> obtain
>> >> >> >>> a valid browser prior to calling this function. So it's a layer
>> >> >> >>> of
>> >> >> >>> complexity that is required only for a specific use case (that I
>> >> >> >>> have
>> >> >> >>> not
>> >> >> >>> discovered yet, I will determine what that is once I get further
>> >> >> >>> into
>> >> >> >>> the
>> >> >> >>> build/test process).
>> >> >> >>
>> >> >> >>> I will sort the rest of this out myself.
>> >> >> >>
>> >> >> >>> -Ben
>> >> >> >>
>> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org>
>> >> >> >>> wrote:
>> >> >> >>
>> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
>> >> >> >>> >
>> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>> >> >> >>> >> > > Can you describe what changed outside of the ChromeOS
>> >> >> >>> >> > > bits?
>> >> >> >>> >> >
>> >> >> >>> >> > Nothing changed from the original CL outside of the
>> >> >> >>> >> > corrections I
>> >> >> >>> >> > had to
>> >> >> >>> >>
>> >> >> >>> > make
>> >> >> >>> >
>> >> >> >>> >> in
>> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
>> >> >> >>> >>
>> >> >> >>> >
>> >> >> >>> >  Not true. I am now discovering you changed the API for
>> >> >> >>> > AddTabWithURL
>> >> >> >>> > in an
>> >> >> >>> >> awkward and pervasive way (returning Browser as an out
>> >> >> >>> >> parameter
>> >> >> >>> >> when
>> >> >> >>> >> every
>> >> >> >>> >> client in non-ChromeOS code already has a valid Browser), and
>> >> >> >>> >> changed
>> >> >> >>> >> a
>> >> >> >>> >> huge
>> >> >> >>> >> amount of the codebase to reflect this.
>> >> >> >>> >>
>> >> >> >>> >
>> >> >> >>> > Hi Ben,
>> >> >> >>> >
>> >> >> >>> > I'm sorry that this slipped through, and has become an issue.
>> >> >> >>> >  Do
>> >> >> >>> > note
>> >> >> >>> > that
>> >> >> >>> > the
>> >> >> >>> > changes you are referring to happened after you had signed off
>> >> >> >>> > on
>> >> >> >>> > the
>> >> >> >>> > review
>> >> >> >>> > process, and during the back-forth with dhg.
>> >> >> >>> >
>> >> >> >>> > I added that callback because my first attempt at a fix
>> >> >> >>> > (making
>> >> >> >>> > use
>> >> >> >>> > of
>> >> >> >>> > the
>> >> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed as
>> >> >> >>> > too
>> >> >> >>> > fragile
>> >> >> >>> > for
>> >> >> >>> > callers of the various AddTabWithUrl routines.  See the patch
>> >> >> >>> > history
>> >> >> >>> > of
>> >> >> >>> > filebrowse_ui.cc.
>> >> >> >>> >
>> >> >> >>> > What is problematic with having these routines return a
>> >> >> >>> > browser
>> >> >> >>> > instance
>> >> >> >>> > via a
>> >> >> >>> > parameter?  It is possible that a new browser was created to
>> >> >> >>> > service
>> >> >> >>> > them,
>> >> >> >>> > and
>> >> >> >>> > that browser is not directly accessible via just the newly
>> >> >> >>> > constructed
>> >> >> >>> > tab-contents.
>> >> >> >>> >
>> >> >> >>> > Let me know if I can help.
>> >> >> >>> >
>> >> >> >>> > Jeff
>> >> >> >>> >
>> >> >> >>> >
>> >> >> >>> >
>> >> >> >>> >  I would have appreciated a more accurate response here, given
>> >> >> >>> > the
>> >> >> >>> > amount
>> >> >> >>> >> of
>> >> >> >>> >> email I get it's hard for me to evaluate everything and I
>> >> >> >>> >> rely
>> >> >> >>> >> on
>> >> >> >>> >> people
>> >> >> >>> >> to
>> >> >> >>> >>
>> >> >> >>> > tell
>> >> >> >>> >
>> >> >> >>> >> me what I should be looking at.
>> >> >> >>> >>
>> >> >> >>> >
>> >> >> >>> >  I am going to correct the API problems this time. I may roll
>> >> >> >>> > some
>> >> >> >>> > or
>> >> >> >>> > all
>> >> >> >>> >> of
>> >> >> >>> >>
>> >> >> >>> > this
>> >> >> >>> >
>> >> >> >>> >> changelist back.
>> >> >> >>> >>
>> >> >> >>> >
>> >> >> >>> >  -Ben
>> >> >> >>> >>
>> >> >> >>> >
>> >> >> >>> >
>> >> >> >>> >
>> >> >> >>> > http://codereview.chromium.org/2819086/show
>> >> >> >>> >
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >>
>> >> >> >> http://codereview.chromium.org/2819086/show
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>

[twiz, 2010-09-30 01:02:29]

On Wed, Sep 29, 2010 at 8:26 PM, David Garcia <dhg@chromium.org> wrote:

> Is that the right one?
>

That was the exact reasoning during the review that caused me to add the
extra parameter.  It was thought that we couldn't guarantee that using the
browser lists would always return the correct browser.  My dept of knowledge
wrt the browser list api was such that I deferred to the more concrete, but
perhaps more invasive solution that is presently in the code.

Jeff


>
> On Wed, Sep 29, 2010 at 5:25 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > BrowserList always returns the most recently active item.
> > -Ben
> >
> > On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org> wrote:
> >>
> >> But then how do I know if i have the SAME filebrowser as the one that
> >> the user is acting on.  Since there can be more than one filebrowser.
> >>
> >> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google) <ben@chromium.org
> >
> >> wrote:
> >> > I don't understand how this follows from the pattern I described. If
> you
> >> > ask
> >> > the BrowserList for a matching browser you will always scan available
> >> > non-deleted Browsers, so you can never crash.
> >> > -Ben
> >> >
> >> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org>
> wrote:
> >> >>
> >> >> and since we have to keep around the browser (for various download
> >> >> reasons, etc), we need to get back the browser, otherwise we may have
> >> >> a copy of a browser that has gone away, and thus we would crash.  If
> >> >> add tab with url was guarenteed to open in the same browser it was
> >> >> told to, then there would be no problem.
> >> >>
> >> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google)
> >> >> <ben@chromium.org>
> >> >> wrote:
> >> >> > The conventional pattern is:
> >> >> > 1. Look for an existing browser that matches what you need using
> the
> >> >> > browser
> >> >> > list.
> >> >> > 2. If you can't find one, or if you always want a new window,
> create
> >> >> > a
> >> >> > new
> >> >> > one.
> >> >> > 3. Call AddTabWithURL on the new one.
> >> >> > I know AddTabWithURL can create a Browser internally but this is
> not
> >> >> > an
> >> >> > aspect of this function that I enjoy, considering the pattern is so
> >> >> > often:
> >> >> > Browser* b = locate_browser_somehow;
> >> >> > b->AddTabWithURL(..., &b) (may create a new browser and call Show()
> >> >> > on
> >> >> > its
> >> >> > window)
> >> >> > b->window()->Show(); // Show called twice, potentially.
> >> >> > -Ben
> >> >> >
> >> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org>
> >> >> > wrote:
> >> >> >>
> >> >> >> There was a discussion further back, but when it creates the tab,
> it
> >> >> >> needs to know the browser associated with that tab.  (so it can
> >> >> >> subscribe to events, etc) Since add tab with url can actually open
> >> >> >> up
> >> >> >> in a different browser, this can cause a problem.
> >> >> >>
> >> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
> >> >> >> <ben@chromium.org>
> >> >> >> wrote:
> >> >> >> > What is ChromeOS code doing such that it requires this change to
> >> >> >> > the
> >> >> >> > API?
> >> >> >> > -Ben
> >> >> >> >
> >> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
> >> >> >> >>
> >> >> >> >> ben,
> >> >> >> >>
> >> >> >> >> You say that all the code in non-chromeos already has this
> >> >> >> >> context,
> >> >> >> >> but
> >> >> >> >> what
> >> >> >> >> about chromeos... doesn't that matter?
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
> >> >> >> >>>
> >> >> >> >>> For the record, I signed off on a much more limited version of
> >> >> >> >>> this
> >> >> >> >>> change
> >> >> >> >>> that seemed to primarily affect the app/extension related
> >> >> >> >>> functions
> >> >> >> >>> on
> >> >> >> >>> Browser, not AddTabWithURL which is a more central API.
> >> >> >> >>
> >> >> >> >>> The trouble is that none of the code in Chrome-land ever
> needed
> >> >> >> >>> this
> >> >> >> >>> before,
> >> >> >> >>> and it's not immediately clear why they would given that they
> >> >> >> >>> always
> >> >> >> >>> obtain
> >> >> >> >>> a valid browser prior to calling this function. So it's a
> layer
> >> >> >> >>> of
> >> >> >> >>> complexity that is required only for a specific use case (that
> I
> >> >> >> >>> have
> >> >> >> >>> not
> >> >> >> >>> discovered yet, I will determine what that is once I get
> further
> >> >> >> >>> into
> >> >> >> >>> the
> >> >> >> >>> build/test process).
> >> >> >> >>
> >> >> >> >>> I will sort the rest of this out myself.
> >> >> >> >>
> >> >> >> >>> -Ben
> >> >> >> >>
> >> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org>
> >> >> >> >>> wrote:
> >> >> >> >>
> >> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >> >> >> >>> >
> >> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >> >> >> >>> >> > > Can you describe what changed outside of the ChromeOS
> >> >> >> >>> >> > > bits?
> >> >> >> >>> >> >
> >> >> >> >>> >> > Nothing changed from the original CL outside of the
> >> >> >> >>> >> > corrections I
> >> >> >> >>> >> > had to
> >> >> >> >>> >>
> >> >> >> >>> > make
> >> >> >> >>> >
> >> >> >> >>> >> in
> >> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >  Not true. I am now discovering you changed the API for
> >> >> >> >>> > AddTabWithURL
> >> >> >> >>> > in an
> >> >> >> >>> >> awkward and pervasive way (returning Browser as an out
> >> >> >> >>> >> parameter
> >> >> >> >>> >> when
> >> >> >> >>> >> every
> >> >> >> >>> >> client in non-ChromeOS code already has a valid Browser),
> and
> >> >> >> >>> >> changed
> >> >> >> >>> >> a
> >> >> >> >>> >> huge
> >> >> >> >>> >> amount of the codebase to reflect this.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> > Hi Ben,
> >> >> >> >>> >
> >> >> >> >>> > I'm sorry that this slipped through, and has become an
> issue.
> >> >> >> >>> >  Do
> >> >> >> >>> > note
> >> >> >> >>> > that
> >> >> >> >>> > the
> >> >> >> >>> > changes you are referring to happened after you had signed
> off
> >> >> >> >>> > on
> >> >> >> >>> > the
> >> >> >> >>> > review
> >> >> >> >>> > process, and during the back-forth with dhg.
> >> >> >> >>> >
> >> >> >> >>> > I added that callback because my first attempt at a fix
> >> >> >> >>> > (making
> >> >> >> >>> > use
> >> >> >> >>> > of
> >> >> >> >>> > the
> >> >> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed as
> >> >> >> >>> > too
> >> >> >> >>> > fragile
> >> >> >> >>> > for
> >> >> >> >>> > callers of the various AddTabWithUrl routines.  See the
> patch
> >> >> >> >>> > history
> >> >> >> >>> > of
> >> >> >> >>> > filebrowse_ui.cc.
> >> >> >> >>> >
> >> >> >> >>> > What is problematic with having these routines return a
> >> >> >> >>> > browser
> >> >> >> >>> > instance
> >> >> >> >>> > via a
> >> >> >> >>> > parameter?  It is possible that a new browser was created to
> >> >> >> >>> > service
> >> >> >> >>> > them,
> >> >> >> >>> > and
> >> >> >> >>> > that browser is not directly accessible via just the newly
> >> >> >> >>> > constructed
> >> >> >> >>> > tab-contents.
> >> >> >> >>> >
> >> >> >> >>> > Let me know if I can help.
> >> >> >> >>> >
> >> >> >> >>> > Jeff
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> >  I would have appreciated a more accurate response here,
> given
> >> >> >> >>> > the
> >> >> >> >>> > amount
> >> >> >> >>> >> of
> >> >> >> >>> >> email I get it's hard for me to evaluate everything and I
> >> >> >> >>> >> rely
> >> >> >> >>> >> on
> >> >> >> >>> >> people
> >> >> >> >>> >> to
> >> >> >> >>> >>
> >> >> >> >>> > tell
> >> >> >> >>> >
> >> >> >> >>> >> me what I should be looking at.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >  I am going to correct the API problems this time. I may
> roll
> >> >> >> >>> > some
> >> >> >> >>> > or
> >> >> >> >>> > all
> >> >> >> >>> >> of
> >> >> >> >>> >>
> >> >> >> >>> > this
> >> >> >> >>> >
> >> >> >> >>> >> changelist back.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >  -Ben
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> > http://codereview.chromium.org/2819086/show
> >> >> >> >>> >
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> http://codereview.chromium.org/2819086/show
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>



-- 
Software Developer
Google Canada
(514) 670-8756

[Ben Goodger (Google), 2010-09-30 01:16:53]

If that's the one you're clicking on then yes. MRU is based on window
activation.

If you don't have a pointer to the current browser using BrowserList API
with qualifiers designed to find that type will find it for you.

-Ben

On Wed, Sep 29, 2010 at 5:26 PM, David Garcia <dhg@chromium.org> wrote:

> Is that the right one?
>
> On Wed, Sep 29, 2010 at 5:25 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > BrowserList always returns the most recently active item.
> > -Ben
> >
> > On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org> wrote:
> >>
> >> But then how do I know if i have the SAME filebrowser as the one that
> >> the user is acting on.  Since there can be more than one filebrowser.
> >>
> >> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google) <ben@chromium.org
> >
> >> wrote:
> >> > I don't understand how this follows from the pattern I described. If
> you
> >> > ask
> >> > the BrowserList for a matching browser you will always scan available
> >> > non-deleted Browsers, so you can never crash.
> >> > -Ben
> >> >
> >> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org>
> wrote:
> >> >>
> >> >> and since we have to keep around the browser (for various download
> >> >> reasons, etc), we need to get back the browser, otherwise we may have
> >> >> a copy of a browser that has gone away, and thus we would crash.  If
> >> >> add tab with url was guarenteed to open in the same browser it was
> >> >> told to, then there would be no problem.
> >> >>
> >> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google)
> >> >> <ben@chromium.org>
> >> >> wrote:
> >> >> > The conventional pattern is:
> >> >> > 1. Look for an existing browser that matches what you need using
> the
> >> >> > browser
> >> >> > list.
> >> >> > 2. If you can't find one, or if you always want a new window,
> create
> >> >> > a
> >> >> > new
> >> >> > one.
> >> >> > 3. Call AddTabWithURL on the new one.
> >> >> > I know AddTabWithURL can create a Browser internally but this is
> not
> >> >> > an
> >> >> > aspect of this function that I enjoy, considering the pattern is so
> >> >> > often:
> >> >> > Browser* b = locate_browser_somehow;
> >> >> > b->AddTabWithURL(..., &b) (may create a new browser and call Show()
> >> >> > on
> >> >> > its
> >> >> > window)
> >> >> > b->window()->Show(); // Show called twice, potentially.
> >> >> > -Ben
> >> >> >
> >> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org>
> >> >> > wrote:
> >> >> >>
> >> >> >> There was a discussion further back, but when it creates the tab,
> it
> >> >> >> needs to know the browser associated with that tab.  (so it can
> >> >> >> subscribe to events, etc) Since add tab with url can actually open
> >> >> >> up
> >> >> >> in a different browser, this can cause a problem.
> >> >> >>
> >> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
> >> >> >> <ben@chromium.org>
> >> >> >> wrote:
> >> >> >> > What is ChromeOS code doing such that it requires this change to
> >> >> >> > the
> >> >> >> > API?
> >> >> >> > -Ben
> >> >> >> >
> >> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
> >> >> >> >>
> >> >> >> >> ben,
> >> >> >> >>
> >> >> >> >> You say that all the code in non-chromeos already has this
> >> >> >> >> context,
> >> >> >> >> but
> >> >> >> >> what
> >> >> >> >> about chromeos... doesn't that matter?
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
> >> >> >> >>>
> >> >> >> >>> For the record, I signed off on a much more limited version of
> >> >> >> >>> this
> >> >> >> >>> change
> >> >> >> >>> that seemed to primarily affect the app/extension related
> >> >> >> >>> functions
> >> >> >> >>> on
> >> >> >> >>> Browser, not AddTabWithURL which is a more central API.
> >> >> >> >>
> >> >> >> >>> The trouble is that none of the code in Chrome-land ever
> needed
> >> >> >> >>> this
> >> >> >> >>> before,
> >> >> >> >>> and it's not immediately clear why they would given that they
> >> >> >> >>> always
> >> >> >> >>> obtain
> >> >> >> >>> a valid browser prior to calling this function. So it's a
> layer
> >> >> >> >>> of
> >> >> >> >>> complexity that is required only for a specific use case (that
> I
> >> >> >> >>> have
> >> >> >> >>> not
> >> >> >> >>> discovered yet, I will determine what that is once I get
> further
> >> >> >> >>> into
> >> >> >> >>> the
> >> >> >> >>> build/test process).
> >> >> >> >>
> >> >> >> >>> I will sort the rest of this out myself.
> >> >> >> >>
> >> >> >> >>> -Ben
> >> >> >> >>
> >> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org>
> >> >> >> >>> wrote:
> >> >> >> >>
> >> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >> >> >> >>> >
> >> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >> >> >> >>> >> > > Can you describe what changed outside of the ChromeOS
> >> >> >> >>> >> > > bits?
> >> >> >> >>> >> >
> >> >> >> >>> >> > Nothing changed from the original CL outside of the
> >> >> >> >>> >> > corrections I
> >> >> >> >>> >> > had to
> >> >> >> >>> >>
> >> >> >> >>> > make
> >> >> >> >>> >
> >> >> >> >>> >> in
> >> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >  Not true. I am now discovering you changed the API for
> >> >> >> >>> > AddTabWithURL
> >> >> >> >>> > in an
> >> >> >> >>> >> awkward and pervasive way (returning Browser as an out
> >> >> >> >>> >> parameter
> >> >> >> >>> >> when
> >> >> >> >>> >> every
> >> >> >> >>> >> client in non-ChromeOS code already has a valid Browser),
> and
> >> >> >> >>> >> changed
> >> >> >> >>> >> a
> >> >> >> >>> >> huge
> >> >> >> >>> >> amount of the codebase to reflect this.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> > Hi Ben,
> >> >> >> >>> >
> >> >> >> >>> > I'm sorry that this slipped through, and has become an
> issue.
> >> >> >> >>> >  Do
> >> >> >> >>> > note
> >> >> >> >>> > that
> >> >> >> >>> > the
> >> >> >> >>> > changes you are referring to happened after you had signed
> off
> >> >> >> >>> > on
> >> >> >> >>> > the
> >> >> >> >>> > review
> >> >> >> >>> > process, and during the back-forth with dhg.
> >> >> >> >>> >
> >> >> >> >>> > I added that callback because my first attempt at a fix
> >> >> >> >>> > (making
> >> >> >> >>> > use
> >> >> >> >>> > of
> >> >> >> >>> > the
> >> >> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed as
> >> >> >> >>> > too
> >> >> >> >>> > fragile
> >> >> >> >>> > for
> >> >> >> >>> > callers of the various AddTabWithUrl routines.  See the
> patch
> >> >> >> >>> > history
> >> >> >> >>> > of
> >> >> >> >>> > filebrowse_ui.cc.
> >> >> >> >>> >
> >> >> >> >>> > What is problematic with having these routines return a
> >> >> >> >>> > browser
> >> >> >> >>> > instance
> >> >> >> >>> > via a
> >> >> >> >>> > parameter?  It is possible that a new browser was created to
> >> >> >> >>> > service
> >> >> >> >>> > them,
> >> >> >> >>> > and
> >> >> >> >>> > that browser is not directly accessible via just the newly
> >> >> >> >>> > constructed
> >> >> >> >>> > tab-contents.
> >> >> >> >>> >
> >> >> >> >>> > Let me know if I can help.
> >> >> >> >>> >
> >> >> >> >>> > Jeff
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> >  I would have appreciated a more accurate response here,
> given
> >> >> >> >>> > the
> >> >> >> >>> > amount
> >> >> >> >>> >> of
> >> >> >> >>> >> email I get it's hard for me to evaluate everything and I
> >> >> >> >>> >> rely
> >> >> >> >>> >> on
> >> >> >> >>> >> people
> >> >> >> >>> >> to
> >> >> >> >>> >>
> >> >> >> >>> > tell
> >> >> >> >>> >
> >> >> >> >>> >> me what I should be looking at.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >  I am going to correct the API problems this time. I may
> roll
> >> >> >> >>> > some
> >> >> >> >>> > or
> >> >> >> >>> > all
> >> >> >> >>> >> of
> >> >> >> >>> >>
> >> >> >> >>> > this
> >> >> >> >>> >
> >> >> >> >>> >> changelist back.
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >  -Ben
> >> >> >> >>> >>
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> >
> >> >> >> >>> > http://codereview.chromium.org/2819086/show
> >> >> >> >>> >
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>
> >> >> >> >> http://codereview.chromium.org/2819086/show
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>

[dhg, 2010-09-30 15:38:46]

you can't guarantee that.  Since its not necessarily based on user
interaction (popups happen due to notifications, connectivity,
downloads, file accesses, etc), we need some way to make sure.   This
came up in the review.

Can you explain the reasoning behind not having this particular way of
getting the browser? i don't see a compelling reason.

On Wed, Sep 29, 2010 at 6:16 PM, Ben Goodger (Google) <ben@chromium.org> wrote:
> If that's the one you're clicking on then yes. MRU is based on window
> activation.
> If you don't have a pointer to the current browser using BrowserList API
> with qualifiers designed to find that type will find it for you.
> -Ben
>
> On Wed, Sep 29, 2010 at 5:26 PM, David Garcia <dhg@chromium.org> wrote:
>>
>> Is that the right one?
>>
>> On Wed, Sep 29, 2010 at 5:25 PM, Ben Goodger (Google) <ben@chromium.org>
>> wrote:
>> > BrowserList always returns the most recently active item.
>> > -Ben
>> >
>> > On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org> wrote:
>> >>
>> >> But then how do I know if i have the SAME filebrowser as the one that
>> >> the user is acting on.  Since there can be more than one filebrowser.
>> >>
>> >> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google)
>> >> <ben@chromium.org>
>> >> wrote:
>> >> > I don't understand how this follows from the pattern I described. If
>> >> > you
>> >> > ask
>> >> > the BrowserList for a matching browser you will always scan available
>> >> > non-deleted Browsers, so you can never crash.
>> >> > -Ben
>> >> >
>> >> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> and since we have to keep around the browser (for various download
>> >> >> reasons, etc), we need to get back the browser, otherwise we may
>> >> >> have
>> >> >> a copy of a browser that has gone away, and thus we would crash.  If
>> >> >> add tab with url was guarenteed to open in the same browser it was
>> >> >> told to, then there would be no problem.
>> >> >>
>> >> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google)
>> >> >> <ben@chromium.org>
>> >> >> wrote:
>> >> >> > The conventional pattern is:
>> >> >> > 1. Look for an existing browser that matches what you need using
>> >> >> > the
>> >> >> > browser
>> >> >> > list.
>> >> >> > 2. If you can't find one, or if you always want a new window,
>> >> >> > create
>> >> >> > a
>> >> >> > new
>> >> >> > one.
>> >> >> > 3. Call AddTabWithURL on the new one.
>> >> >> > I know AddTabWithURL can create a Browser internally but this is
>> >> >> > not
>> >> >> > an
>> >> >> > aspect of this function that I enjoy, considering the pattern is
>> >> >> > so
>> >> >> > often:
>> >> >> > Browser* b = locate_browser_somehow;
>> >> >> > b->AddTabWithURL(..., &b) (may create a new browser and call
>> >> >> > Show()
>> >> >> > on
>> >> >> > its
>> >> >> > window)
>> >> >> > b->window()->Show(); // Show called twice, potentially.
>> >> >> > -Ben
>> >> >> >
>> >> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> There was a discussion further back, but when it creates the tab,
>> >> >> >> it
>> >> >> >> needs to know the browser associated with that tab.  (so it can
>> >> >> >> subscribe to events, etc) Since add tab with url can actually
>> >> >> >> open
>> >> >> >> up
>> >> >> >> in a different browser, this can cause a problem.
>> >> >> >>
>> >> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
>> >> >> >> <ben@chromium.org>
>> >> >> >> wrote:
>> >> >> >> > What is ChromeOS code doing such that it requires this change
>> >> >> >> > to
>> >> >> >> > the
>> >> >> >> > API?
>> >> >> >> > -Ben
>> >> >> >> >
>> >> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
>> >> >> >> >>
>> >> >> >> >> ben,
>> >> >> >> >>
>> >> >> >> >> You say that all the code in non-chromeos already has this
>> >> >> >> >> context,
>> >> >> >> >> but
>> >> >> >> >> what
>> >> >> >> >> about chromeos... doesn't that matter?
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
>> >> >> >> >>>
>> >> >> >> >>> For the record, I signed off on a much more limited version
>> >> >> >> >>> of
>> >> >> >> >>> this
>> >> >> >> >>> change
>> >> >> >> >>> that seemed to primarily affect the app/extension related
>> >> >> >> >>> functions
>> >> >> >> >>> on
>> >> >> >> >>> Browser, not AddTabWithURL which is a more central API.
>> >> >> >> >>
>> >> >> >> >>> The trouble is that none of the code in Chrome-land ever
>> >> >> >> >>> needed
>> >> >> >> >>> this
>> >> >> >> >>> before,
>> >> >> >> >>> and it's not immediately clear why they would given that they
>> >> >> >> >>> always
>> >> >> >> >>> obtain
>> >> >> >> >>> a valid browser prior to calling this function. So it's a
>> >> >> >> >>> layer
>> >> >> >> >>> of
>> >> >> >> >>> complexity that is required only for a specific use case
>> >> >> >> >>> (that I
>> >> >> >> >>> have
>> >> >> >> >>> not
>> >> >> >> >>> discovered yet, I will determine what that is once I get
>> >> >> >> >>> further
>> >> >> >> >>> into
>> >> >> >> >>> the
>> >> >> >> >>> build/test process).
>> >> >> >> >>
>> >> >> >> >>> I will sort the rest of this out myself.
>> >> >> >> >>
>> >> >> >> >>> -Ben
>> >> >> >> >>
>> >> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org>
>> >> >> >> >>> wrote:
>> >> >> >> >>
>> >> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
>> >> >> >> >>> >
>> >> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>> >> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>> >> >> >> >>> >> > > Can you describe what changed outside of the ChromeOS
>> >> >> >> >>> >> > > bits?
>> >> >> >> >>> >> >
>> >> >> >> >>> >> > Nothing changed from the original CL outside of the
>> >> >> >> >>> >> > corrections I
>> >> >> >> >>> >> > had to
>> >> >> >> >>> >>
>> >> >> >> >>> > make
>> >> >> >> >>> >
>> >> >> >> >>> >> in
>> >> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
>> >> >> >> >>> >>
>> >> >> >> >>> >
>> >> >> >> >>> >  Not true. I am now discovering you changed the API for
>> >> >> >> >>> > AddTabWithURL
>> >> >> >> >>> > in an
>> >> >> >> >>> >> awkward and pervasive way (returning Browser as an out
>> >> >> >> >>> >> parameter
>> >> >> >> >>> >> when
>> >> >> >> >>> >> every
>> >> >> >> >>> >> client in non-ChromeOS code already has a valid Browser),
>> >> >> >> >>> >> and
>> >> >> >> >>> >> changed
>> >> >> >> >>> >> a
>> >> >> >> >>> >> huge
>> >> >> >> >>> >> amount of the codebase to reflect this.
>> >> >> >> >>> >>
>> >> >> >> >>> >
>> >> >> >> >>> > Hi Ben,
>> >> >> >> >>> >
>> >> >> >> >>> > I'm sorry that this slipped through, and has become an
>> >> >> >> >>> > issue.
>> >> >> >> >>> >  Do
>> >> >> >> >>> > note
>> >> >> >> >>> > that
>> >> >> >> >>> > the
>> >> >> >> >>> > changes you are referring to happened after you had signed
>> >> >> >> >>> > off
>> >> >> >> >>> > on
>> >> >> >> >>> > the
>> >> >> >> >>> > review
>> >> >> >> >>> > process, and during the back-forth with dhg.
>> >> >> >> >>> >
>> >> >> >> >>> > I added that callback because my first attempt at a fix
>> >> >> >> >>> > (making
>> >> >> >> >>> > use
>> >> >> >> >>> > of
>> >> >> >> >>> > the
>> >> >> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed as
>> >> >> >> >>> > too
>> >> >> >> >>> > fragile
>> >> >> >> >>> > for
>> >> >> >> >>> > callers of the various AddTabWithUrl routines.  See the
>> >> >> >> >>> > patch
>> >> >> >> >>> > history
>> >> >> >> >>> > of
>> >> >> >> >>> > filebrowse_ui.cc.
>> >> >> >> >>> >
>> >> >> >> >>> > What is problematic with having these routines return a
>> >> >> >> >>> > browser
>> >> >> >> >>> > instance
>> >> >> >> >>> > via a
>> >> >> >> >>> > parameter?  It is possible that a new browser was created
>> >> >> >> >>> > to
>> >> >> >> >>> > service
>> >> >> >> >>> > them,
>> >> >> >> >>> > and
>> >> >> >> >>> > that browser is not directly accessible via just the newly
>> >> >> >> >>> > constructed
>> >> >> >> >>> > tab-contents.
>> >> >> >> >>> >
>> >> >> >> >>> > Let me know if I can help.
>> >> >> >> >>> >
>> >> >> >> >>> > Jeff
>> >> >> >> >>> >
>> >> >> >> >>> >
>> >> >> >> >>> >
>> >> >> >> >>> >  I would have appreciated a more accurate response here,
>> >> >> >> >>> > given
>> >> >> >> >>> > the
>> >> >> >> >>> > amount
>> >> >> >> >>> >> of
>> >> >> >> >>> >> email I get it's hard for me to evaluate everything and I
>> >> >> >> >>> >> rely
>> >> >> >> >>> >> on
>> >> >> >> >>> >> people
>> >> >> >> >>> >> to
>> >> >> >> >>> >>
>> >> >> >> >>> > tell
>> >> >> >> >>> >
>> >> >> >> >>> >> me what I should be looking at.
>> >> >> >> >>> >>
>> >> >> >> >>> >
>> >> >> >> >>> >  I am going to correct the API problems this time. I may
>> >> >> >> >>> > roll
>> >> >> >> >>> > some
>> >> >> >> >>> > or
>> >> >> >> >>> > all
>> >> >> >> >>> >> of
>> >> >> >> >>> >>
>> >> >> >> >>> > this
>> >> >> >> >>> >
>> >> >> >> >>> >> changelist back.
>> >> >> >> >>> >>
>> >> >> >> >>> >
>> >> >> >> >>> >  -Ben
>> >> >> >> >>> >>
>> >> >> >> >>> >
>> >> >> >> >>> >
>> >> >> >> >>> >
>> >> >> >> >>> > http://codereview.chromium.org/2819086/show
>> >> >> >> >>> >
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >>
>> >> >> >> >> http://codereview.chromium.org/2819086/show
>> >> >> >> >
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>

[Ben Goodger (Google), 2010-09-30 15:45:38]

Your click to the window that caused this interaction activated the window
which updated the browser list prior to the event being processed that
sought it out. This has been a solved problem in Chrome code now for over 3
years.

-Ben

On Thu, Sep 30, 2010 at 8:38 AM, David Garcia <dhg@chromium.org> wrote:

> you can't guarantee that.  Since its not necessarily based on user
> interaction (popups happen due to notifications, connectivity,
> downloads, file accesses, etc), we need some way to make sure.   This
> came up in the review.
>
> Can you explain the reasoning behind not having this particular way of
> getting the browser? i don't see a compelling reason.
>
> On Wed, Sep 29, 2010 at 6:16 PM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > If that's the one you're clicking on then yes. MRU is based on window
> > activation.
> > If you don't have a pointer to the current browser using BrowserList API
> > with qualifiers designed to find that type will find it for you.
> > -Ben
> >
> > On Wed, Sep 29, 2010 at 5:26 PM, David Garcia <dhg@chromium.org> wrote:
> >>
> >> Is that the right one?
> >>
> >> On Wed, Sep 29, 2010 at 5:25 PM, Ben Goodger (Google) <ben@chromium.org
> >
> >> wrote:
> >> > BrowserList always returns the most recently active item.
> >> > -Ben
> >> >
> >> > On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org>
> wrote:
> >> >>
> >> >> But then how do I know if i have the SAME filebrowser as the one that
> >> >> the user is acting on.  Since there can be more than one filebrowser.
> >> >>
> >> >> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google)
> >> >> <ben@chromium.org>
> >> >> wrote:
> >> >> > I don't understand how this follows from the pattern I described.
> If
> >> >> > you
> >> >> > ask
> >> >> > the BrowserList for a matching browser you will always scan
> available
> >> >> > non-deleted Browsers, so you can never crash.
> >> >> > -Ben
> >> >> >
> >> >> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org>
> >> >> > wrote:
> >> >> >>
> >> >> >> and since we have to keep around the browser (for various download
> >> >> >> reasons, etc), we need to get back the browser, otherwise we may
> >> >> >> have
> >> >> >> a copy of a browser that has gone away, and thus we would crash.
>  If
> >> >> >> add tab with url was guarenteed to open in the same browser it was
> >> >> >> told to, then there would be no problem.
> >> >> >>
> >> >> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google)
> >> >> >> <ben@chromium.org>
> >> >> >> wrote:
> >> >> >> > The conventional pattern is:
> >> >> >> > 1. Look for an existing browser that matches what you need using
> >> >> >> > the
> >> >> >> > browser
> >> >> >> > list.
> >> >> >> > 2. If you can't find one, or if you always want a new window,
> >> >> >> > create
> >> >> >> > a
> >> >> >> > new
> >> >> >> > one.
> >> >> >> > 3. Call AddTabWithURL on the new one.
> >> >> >> > I know AddTabWithURL can create a Browser internally but this is
> >> >> >> > not
> >> >> >> > an
> >> >> >> > aspect of this function that I enjoy, considering the pattern is
> >> >> >> > so
> >> >> >> > often:
> >> >> >> > Browser* b = locate_browser_somehow;
> >> >> >> > b->AddTabWithURL(..., &b) (may create a new browser and call
> >> >> >> > Show()
> >> >> >> > on
> >> >> >> > its
> >> >> >> > window)
> >> >> >> > b->window()->Show(); // Show called twice, potentially.
> >> >> >> > -Ben
> >> >> >> >
> >> >> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia <dhg@chromium.org
> >
> >> >> >> > wrote:
> >> >> >> >>
> >> >> >> >> There was a discussion further back, but when it creates the
> tab,
> >> >> >> >> it
> >> >> >> >> needs to know the browser associated with that tab.  (so it can
> >> >> >> >> subscribe to events, etc) Since add tab with url can actually
> >> >> >> >> open
> >> >> >> >> up
> >> >> >> >> in a different browser, this can cause a problem.
> >> >> >> >>
> >> >> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
> >> >> >> >> <ben@chromium.org>
> >> >> >> >> wrote:
> >> >> >> >> > What is ChromeOS code doing such that it requires this change
> >> >> >> >> > to
> >> >> >> >> > the
> >> >> >> >> > API?
> >> >> >> >> > -Ben
> >> >> >> >> >
> >> >> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
> >> >> >> >> >>
> >> >> >> >> >> ben,
> >> >> >> >> >>
> >> >> >> >> >> You say that all the code in non-chromeos already has this
> >> >> >> >> >> context,
> >> >> >> >> >> but
> >> >> >> >> >> what
> >> >> >> >> >> about chromeos... doesn't that matter?
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
> >> >> >> >> >>>
> >> >> >> >> >>> For the record, I signed off on a much more limited version
> >> >> >> >> >>> of
> >> >> >> >> >>> this
> >> >> >> >> >>> change
> >> >> >> >> >>> that seemed to primarily affect the app/extension related
> >> >> >> >> >>> functions
> >> >> >> >> >>> on
> >> >> >> >> >>> Browser, not AddTabWithURL which is a more central API.
> >> >> >> >> >>
> >> >> >> >> >>> The trouble is that none of the code in Chrome-land ever
> >> >> >> >> >>> needed
> >> >> >> >> >>> this
> >> >> >> >> >>> before,
> >> >> >> >> >>> and it's not immediately clear why they would given that
> they
> >> >> >> >> >>> always
> >> >> >> >> >>> obtain
> >> >> >> >> >>> a valid browser prior to calling this function. So it's a
> >> >> >> >> >>> layer
> >> >> >> >> >>> of
> >> >> >> >> >>> complexity that is required only for a specific use case
> >> >> >> >> >>> (that I
> >> >> >> >> >>> have
> >> >> >> >> >>> not
> >> >> >> >> >>> discovered yet, I will determine what that is once I get
> >> >> >> >> >>> further
> >> >> >> >> >>> into
> >> >> >> >> >>> the
> >> >> >> >> >>> build/test process).
> >> >> >> >> >>
> >> >> >> >> >>> I will sort the rest of this out myself.
> >> >> >> >> >>
> >> >> >> >> >>> -Ben
> >> >> >> >> >>
> >> >> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM, <mailto:twiz@chromium.org
> >
> >> >> >> >> >>> wrote:
> >> >> >> >> >>
> >> >> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >> >> >> >> >>> >
> >> >> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >> >> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >> >> >> >> >>> >> > > Can you describe what changed outside of the
> ChromeOS
> >> >> >> >> >>> >> > > bits?
> >> >> >> >> >>> >> >
> >> >> >> >> >>> >> > Nothing changed from the original CL outside of the
> >> >> >> >> >>> >> > corrections I
> >> >> >> >> >>> >> > had to
> >> >> >> >> >>> >>
> >> >> >> >> >>> > make
> >> >> >> >> >>> >
> >> >> >> >> >>> >> in
> >> >> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >> >> >> >> >>> >>
> >> >> >> >> >>> >
> >> >> >> >> >>> >  Not true. I am now discovering you changed the API for
> >> >> >> >> >>> > AddTabWithURL
> >> >> >> >> >>> > in an
> >> >> >> >> >>> >> awkward and pervasive way (returning Browser as an out
> >> >> >> >> >>> >> parameter
> >> >> >> >> >>> >> when
> >> >> >> >> >>> >> every
> >> >> >> >> >>> >> client in non-ChromeOS code already has a valid
> Browser),
> >> >> >> >> >>> >> and
> >> >> >> >> >>> >> changed
> >> >> >> >> >>> >> a
> >> >> >> >> >>> >> huge
> >> >> >> >> >>> >> amount of the codebase to reflect this.
> >> >> >> >> >>> >>
> >> >> >> >> >>> >
> >> >> >> >> >>> > Hi Ben,
> >> >> >> >> >>> >
> >> >> >> >> >>> > I'm sorry that this slipped through, and has become an
> >> >> >> >> >>> > issue.
> >> >> >> >> >>> >  Do
> >> >> >> >> >>> > note
> >> >> >> >> >>> > that
> >> >> >> >> >>> > the
> >> >> >> >> >>> > changes you are referring to happened after you had
> signed
> >> >> >> >> >>> > off
> >> >> >> >> >>> > on
> >> >> >> >> >>> > the
> >> >> >> >> >>> > review
> >> >> >> >> >>> > process, and during the back-forth with dhg.
> >> >> >> >> >>> >
> >> >> >> >> >>> > I added that callback because my first attempt at a fix
> >> >> >> >> >>> > (making
> >> >> >> >> >>> > use
> >> >> >> >> >>> > of
> >> >> >> >> >>> > the
> >> >> >> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed
> as
> >> >> >> >> >>> > too
> >> >> >> >> >>> > fragile
> >> >> >> >> >>> > for
> >> >> >> >> >>> > callers of the various AddTabWithUrl routines.  See the
> >> >> >> >> >>> > patch
> >> >> >> >> >>> > history
> >> >> >> >> >>> > of
> >> >> >> >> >>> > filebrowse_ui.cc.
> >> >> >> >> >>> >
> >> >> >> >> >>> > What is problematic with having these routines return a
> >> >> >> >> >>> > browser
> >> >> >> >> >>> > instance
> >> >> >> >> >>> > via a
> >> >> >> >> >>> > parameter?  It is possible that a new browser was created
> >> >> >> >> >>> > to
> >> >> >> >> >>> > service
> >> >> >> >> >>> > them,
> >> >> >> >> >>> > and
> >> >> >> >> >>> > that browser is not directly accessible via just the
> newly
> >> >> >> >> >>> > constructed
> >> >> >> >> >>> > tab-contents.
> >> >> >> >> >>> >
> >> >> >> >> >>> > Let me know if I can help.
> >> >> >> >> >>> >
> >> >> >> >> >>> > Jeff
> >> >> >> >> >>> >
> >> >> >> >> >>> >
> >> >> >> >> >>> >
> >> >> >> >> >>> >  I would have appreciated a more accurate response here,
> >> >> >> >> >>> > given
> >> >> >> >> >>> > the
> >> >> >> >> >>> > amount
> >> >> >> >> >>> >> of
> >> >> >> >> >>> >> email I get it's hard for me to evaluate everything and
> I
> >> >> >> >> >>> >> rely
> >> >> >> >> >>> >> on
> >> >> >> >> >>> >> people
> >> >> >> >> >>> >> to
> >> >> >> >> >>> >>
> >> >> >> >> >>> > tell
> >> >> >> >> >>> >
> >> >> >> >> >>> >> me what I should be looking at.
> >> >> >> >> >>> >>
> >> >> >> >> >>> >
> >> >> >> >> >>> >  I am going to correct the API problems this time. I may
> >> >> >> >> >>> > roll
> >> >> >> >> >>> > some
> >> >> >> >> >>> > or
> >> >> >> >> >>> > all
> >> >> >> >> >>> >> of
> >> >> >> >> >>> >>
> >> >> >> >> >>> > this
> >> >> >> >> >>> >
> >> >> >> >> >>> >> changelist back.
> >> >> >> >> >>> >>
> >> >> >> >> >>> >
> >> >> >> >> >>> >  -Ben
> >> >> >> >> >>> >>
> >> >> >> >> >>> >
> >> >> >> >> >>> >
> >> >> >> >> >>> >
> >> >> >> >> >>> > http://codereview.chromium.org/2819086/show
> >> >> >> >> >>> >
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >>
> >> >> >> >> >> http://codereview.chromium.org/2819086/show
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>

[dhg, 2010-09-30 15:49:52]

wouldn't all this be fixed by fixing the broken api of addtabwithurl.
If i'm calling on a browser, it should open in that browser, and never
create a new browser.

On Thu, Sep 30, 2010 at 8:45 AM, Ben Goodger (Google) <ben@chromium.org> wrote:
> Your click to the window that caused this interaction activated the window
> which updated the browser list prior to the event being processed that
> sought it out. This has been a solved problem in Chrome code now for over 3
> years.
> -Ben
>
> On Thu, Sep 30, 2010 at 8:38 AM, David Garcia <dhg@chromium.org> wrote:
>>
>> you can't guarantee that.  Since its not necessarily based on user
>> interaction (popups happen due to notifications, connectivity,
>> downloads, file accesses, etc), we need some way to make sure.   This
>> came up in the review.
>>
>> Can you explain the reasoning behind not having this particular way of
>> getting the browser? i don't see a compelling reason.
>>
>> On Wed, Sep 29, 2010 at 6:16 PM, Ben Goodger (Google) <ben@chromium.org>
>> wrote:
>> > If that's the one you're clicking on then yes. MRU is based on window
>> > activation.
>> > If you don't have a pointer to the current browser using BrowserList API
>> > with qualifiers designed to find that type will find it for you.
>> > -Ben
>> >
>> > On Wed, Sep 29, 2010 at 5:26 PM, David Garcia <dhg@chromium.org> wrote:
>> >>
>> >> Is that the right one?
>> >>
>> >> On Wed, Sep 29, 2010 at 5:25 PM, Ben Goodger (Google)
>> >> <ben@chromium.org>
>> >> wrote:
>> >> > BrowserList always returns the most recently active item.
>> >> > -Ben
>> >> >
>> >> > On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org>
>> >> > wrote:
>> >> >>
>> >> >> But then how do I know if i have the SAME filebrowser as the one
>> >> >> that
>> >> >> the user is acting on.  Since there can be more than one
>> >> >> filebrowser.
>> >> >>
>> >> >> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google)
>> >> >> <ben@chromium.org>
>> >> >> wrote:
>> >> >> > I don't understand how this follows from the pattern I described.
>> >> >> > If
>> >> >> > you
>> >> >> > ask
>> >> >> > the BrowserList for a matching browser you will always scan
>> >> >> > available
>> >> >> > non-deleted Browsers, so you can never crash.
>> >> >> > -Ben
>> >> >> >
>> >> >> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org>
>> >> >> > wrote:
>> >> >> >>
>> >> >> >> and since we have to keep around the browser (for various
>> >> >> >> download
>> >> >> >> reasons, etc), we need to get back the browser, otherwise we may
>> >> >> >> have
>> >> >> >> a copy of a browser that has gone away, and thus we would crash.
>> >> >> >>  If
>> >> >> >> add tab with url was guarenteed to open in the same browser it
>> >> >> >> was
>> >> >> >> told to, then there would be no problem.
>> >> >> >>
>> >> >> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google)
>> >> >> >> <ben@chromium.org>
>> >> >> >> wrote:
>> >> >> >> > The conventional pattern is:
>> >> >> >> > 1. Look for an existing browser that matches what you need
>> >> >> >> > using
>> >> >> >> > the
>> >> >> >> > browser
>> >> >> >> > list.
>> >> >> >> > 2. If you can't find one, or if you always want a new window,
>> >> >> >> > create
>> >> >> >> > a
>> >> >> >> > new
>> >> >> >> > one.
>> >> >> >> > 3. Call AddTabWithURL on the new one.
>> >> >> >> > I know AddTabWithURL can create a Browser internally but this
>> >> >> >> > is
>> >> >> >> > not
>> >> >> >> > an
>> >> >> >> > aspect of this function that I enjoy, considering the pattern
>> >> >> >> > is
>> >> >> >> > so
>> >> >> >> > often:
>> >> >> >> > Browser* b = locate_browser_somehow;
>> >> >> >> > b->AddTabWithURL(..., &b) (may create a new browser and call
>> >> >> >> > Show()
>> >> >> >> > on
>> >> >> >> > its
>> >> >> >> > window)
>> >> >> >> > b->window()->Show(); // Show called twice, potentially.
>> >> >> >> > -Ben
>> >> >> >> >
>> >> >> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia
>> >> >> >> > <dhg@chromium.org>
>> >> >> >> > wrote:
>> >> >> >> >>
>> >> >> >> >> There was a discussion further back, but when it creates the
>> >> >> >> >> tab,
>> >> >> >> >> it
>> >> >> >> >> needs to know the browser associated with that tab.  (so it
>> >> >> >> >> can
>> >> >> >> >> subscribe to events, etc) Since add tab with url can actually
>> >> >> >> >> open
>> >> >> >> >> up
>> >> >> >> >> in a different browser, this can cause a problem.
>> >> >> >> >>
>> >> >> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
>> >> >> >> >> <ben@chromium.org>
>> >> >> >> >> wrote:
>> >> >> >> >> > What is ChromeOS code doing such that it requires this
>> >> >> >> >> > change
>> >> >> >> >> > to
>> >> >> >> >> > the
>> >> >> >> >> > API?
>> >> >> >> >> > -Ben
>> >> >> >> >> >
>> >> >> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org> wrote:
>> >> >> >> >> >>
>> >> >> >> >> >> ben,
>> >> >> >> >> >>
>> >> >> >> >> >> You say that all the code in non-chromeos already has this
>> >> >> >> >> >> context,
>> >> >> >> >> >> but
>> >> >> >> >> >> what
>> >> >> >> >> >> about chromeos... doesn't that matter?
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
>> >> >> >> >> >>>
>> >> >> >> >> >>> For the record, I signed off on a much more limited
>> >> >> >> >> >>> version
>> >> >> >> >> >>> of
>> >> >> >> >> >>> this
>> >> >> >> >> >>> change
>> >> >> >> >> >>> that seemed to primarily affect the app/extension related
>> >> >> >> >> >>> functions
>> >> >> >> >> >>> on
>> >> >> >> >> >>> Browser, not AddTabWithURL which is a more central API.
>> >> >> >> >> >>
>> >> >> >> >> >>> The trouble is that none of the code in Chrome-land ever
>> >> >> >> >> >>> needed
>> >> >> >> >> >>> this
>> >> >> >> >> >>> before,
>> >> >> >> >> >>> and it's not immediately clear why they would given that
>> >> >> >> >> >>> they
>> >> >> >> >> >>> always
>> >> >> >> >> >>> obtain
>> >> >> >> >> >>> a valid browser prior to calling this function. So it's a
>> >> >> >> >> >>> layer
>> >> >> >> >> >>> of
>> >> >> >> >> >>> complexity that is required only for a specific use case
>> >> >> >> >> >>> (that I
>> >> >> >> >> >>> have
>> >> >> >> >> >>> not
>> >> >> >> >> >>> discovered yet, I will determine what that is once I get
>> >> >> >> >> >>> further
>> >> >> >> >> >>> into
>> >> >> >> >> >>> the
>> >> >> >> >> >>> build/test process).
>> >> >> >> >> >>
>> >> >> >> >> >>> I will sort the rest of this out myself.
>> >> >> >> >> >>
>> >> >> >> >> >>> -Ben
>> >> >> >> >> >>
>> >> >> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM,
>> >> >> >> >> >>> <mailto:twiz@chromium.org>
>> >> >> >> >> >>> wrote:
>> >> >> >> >> >>
>> >> >> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
>> >> >> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
>> >> >> >> >> >>> >> > > Can you describe what changed outside of the
>> >> >> >> >> >>> >> > > ChromeOS
>> >> >> >> >> >>> >> > > bits?
>> >> >> >> >> >>> >> >
>> >> >> >> >> >>> >> > Nothing changed from the original CL outside of the
>> >> >> >> >> >>> >> > corrections I
>> >> >> >> >> >>> >> > had to
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> > make
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >> in
>> >> >> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >  Not true. I am now discovering you changed the API for
>> >> >> >> >> >>> > AddTabWithURL
>> >> >> >> >> >>> > in an
>> >> >> >> >> >>> >> awkward and pervasive way (returning Browser as an out
>> >> >> >> >> >>> >> parameter
>> >> >> >> >> >>> >> when
>> >> >> >> >> >>> >> every
>> >> >> >> >> >>> >> client in non-ChromeOS code already has a valid
>> >> >> >> >> >>> >> Browser),
>> >> >> >> >> >>> >> and
>> >> >> >> >> >>> >> changed
>> >> >> >> >> >>> >> a
>> >> >> >> >> >>> >> huge
>> >> >> >> >> >>> >> amount of the codebase to reflect this.
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > Hi Ben,
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > I'm sorry that this slipped through, and has become an
>> >> >> >> >> >>> > issue.
>> >> >> >> >> >>> >  Do
>> >> >> >> >> >>> > note
>> >> >> >> >> >>> > that
>> >> >> >> >> >>> > the
>> >> >> >> >> >>> > changes you are referring to happened after you had
>> >> >> >> >> >>> > signed
>> >> >> >> >> >>> > off
>> >> >> >> >> >>> > on
>> >> >> >> >> >>> > the
>> >> >> >> >> >>> > review
>> >> >> >> >> >>> > process, and during the back-forth with dhg.
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > I added that callback because my first attempt at a fix
>> >> >> >> >> >>> > (making
>> >> >> >> >> >>> > use
>> >> >> >> >> >>> > of
>> >> >> >> >> >>> > the
>> >> >> >> >> >>> > BrowserList::FindBrowserWithProfile function) was deemed
>> >> >> >> >> >>> > as
>> >> >> >> >> >>> > too
>> >> >> >> >> >>> > fragile
>> >> >> >> >> >>> > for
>> >> >> >> >> >>> > callers of the various AddTabWithUrl routines.  See the
>> >> >> >> >> >>> > patch
>> >> >> >> >> >>> > history
>> >> >> >> >> >>> > of
>> >> >> >> >> >>> > filebrowse_ui.cc.
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > What is problematic with having these routines return a
>> >> >> >> >> >>> > browser
>> >> >> >> >> >>> > instance
>> >> >> >> >> >>> > via a
>> >> >> >> >> >>> > parameter?  It is possible that a new browser was
>> >> >> >> >> >>> > created
>> >> >> >> >> >>> > to
>> >> >> >> >> >>> > service
>> >> >> >> >> >>> > them,
>> >> >> >> >> >>> > and
>> >> >> >> >> >>> > that browser is not directly accessible via just the
>> >> >> >> >> >>> > newly
>> >> >> >> >> >>> > constructed
>> >> >> >> >> >>> > tab-contents.
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > Let me know if I can help.
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > Jeff
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >  I would have appreciated a more accurate response here,
>> >> >> >> >> >>> > given
>> >> >> >> >> >>> > the
>> >> >> >> >> >>> > amount
>> >> >> >> >> >>> >> of
>> >> >> >> >> >>> >> email I get it's hard for me to evaluate everything and
>> >> >> >> >> >>> >> I
>> >> >> >> >> >>> >> rely
>> >> >> >> >> >>> >> on
>> >> >> >> >> >>> >> people
>> >> >> >> >> >>> >> to
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> > tell
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >> me what I should be looking at.
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >  I am going to correct the API problems this time. I may
>> >> >> >> >> >>> > roll
>> >> >> >> >> >>> > some
>> >> >> >> >> >>> > or
>> >> >> >> >> >>> > all
>> >> >> >> >> >>> >> of
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> > this
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >> changelist back.
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >  -Ben
>> >> >> >> >> >>> >>
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >
>> >> >> >> >> >>> >
>> >> >> >> >> >>> > http://codereview.chromium.org/2819086/show
>> >> >> >> >> >>> >
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >>
>> >> >> >> >> >> http://codereview.chromium.org/2819086/show
>> >> >> >> >> >
>> >> >> >> >> >
>> >> >> >> >
>> >> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >
>> >
>
>

[Ben Goodger (Google), 2010-09-30 15:51:29]

That was what I was thinking. I need to audit the call sites though and see
how many of them don't know whether or not the browser they have is capable
of opening tabs.

-Ben

On Thu, Sep 30, 2010 at 8:49 AM, David Garcia <dhg@chromium.org> wrote:

> wouldn't all this be fixed by fixing the broken api of addtabwithurl.
> If i'm calling on a browser, it should open in that browser, and never
> create a new browser.
>
> On Thu, Sep 30, 2010 at 8:45 AM, Ben Goodger (Google) <ben@chromium.org>
> wrote:
> > Your click to the window that caused this interaction activated the
> window
> > which updated the browser list prior to the event being processed that
> > sought it out. This has been a solved problem in Chrome code now for over
> 3
> > years.
> > -Ben
> >
> > On Thu, Sep 30, 2010 at 8:38 AM, David Garcia <dhg@chromium.org> wrote:
> >>
> >> you can't guarantee that.  Since its not necessarily based on user
> >> interaction (popups happen due to notifications, connectivity,
> >> downloads, file accesses, etc), we need some way to make sure.   This
> >> came up in the review.
> >>
> >> Can you explain the reasoning behind not having this particular way of
> >> getting the browser? i don't see a compelling reason.
> >>
> >> On Wed, Sep 29, 2010 at 6:16 PM, Ben Goodger (Google) <ben@chromium.org
> >
> >> wrote:
> >> > If that's the one you're clicking on then yes. MRU is based on window
> >> > activation.
> >> > If you don't have a pointer to the current browser using BrowserList
> API
> >> > with qualifiers designed to find that type will find it for you.
> >> > -Ben
> >> >
> >> > On Wed, Sep 29, 2010 at 5:26 PM, David Garcia <dhg@chromium.org>
> wrote:
> >> >>
> >> >> Is that the right one?
> >> >>
> >> >> On Wed, Sep 29, 2010 at 5:25 PM, Ben Goodger (Google)
> >> >> <ben@chromium.org>
> >> >> wrote:
> >> >> > BrowserList always returns the most recently active item.
> >> >> > -Ben
> >> >> >
> >> >> > On Wed, Sep 29, 2010 at 5:22 PM, David Garcia <dhg@chromium.org>
> >> >> > wrote:
> >> >> >>
> >> >> >> But then how do I know if i have the SAME filebrowser as the one
> >> >> >> that
> >> >> >> the user is acting on.  Since there can be more than one
> >> >> >> filebrowser.
> >> >> >>
> >> >> >> On Wed, Sep 29, 2010 at 5:21 PM, Ben Goodger (Google)
> >> >> >> <ben@chromium.org>
> >> >> >> wrote:
> >> >> >> > I don't understand how this follows from the pattern I
> described.
> >> >> >> > If
> >> >> >> > you
> >> >> >> > ask
> >> >> >> > the BrowserList for a matching browser you will always scan
> >> >> >> > available
> >> >> >> > non-deleted Browsers, so you can never crash.
> >> >> >> > -Ben
> >> >> >> >
> >> >> >> > On Wed, Sep 29, 2010 at 5:19 PM, David Garcia <dhg@chromium.org
> >
> >> >> >> > wrote:
> >> >> >> >>
> >> >> >> >> and since we have to keep around the browser (for various
> >> >> >> >> download
> >> >> >> >> reasons, etc), we need to get back the browser, otherwise we
> may
> >> >> >> >> have
> >> >> >> >> a copy of a browser that has gone away, and thus we would
> crash.
> >> >> >> >>  If
> >> >> >> >> add tab with url was guarenteed to open in the same browser it
> >> >> >> >> was
> >> >> >> >> told to, then there would be no problem.
> >> >> >> >>
> >> >> >> >> On Wed, Sep 29, 2010 at 5:15 PM, Ben Goodger (Google)
> >> >> >> >> <ben@chromium.org>
> >> >> >> >> wrote:
> >> >> >> >> > The conventional pattern is:
> >> >> >> >> > 1. Look for an existing browser that matches what you need
> >> >> >> >> > using
> >> >> >> >> > the
> >> >> >> >> > browser
> >> >> >> >> > list.
> >> >> >> >> > 2. If you can't find one, or if you always want a new window,
> >> >> >> >> > create
> >> >> >> >> > a
> >> >> >> >> > new
> >> >> >> >> > one.
> >> >> >> >> > 3. Call AddTabWithURL on the new one.
> >> >> >> >> > I know AddTabWithURL can create a Browser internally but this
> >> >> >> >> > is
> >> >> >> >> > not
> >> >> >> >> > an
> >> >> >> >> > aspect of this function that I enjoy, considering the pattern
> >> >> >> >> > is
> >> >> >> >> > so
> >> >> >> >> > often:
> >> >> >> >> > Browser* b = locate_browser_somehow;
> >> >> >> >> > b->AddTabWithURL(..., &b) (may create a new browser and call
> >> >> >> >> > Show()
> >> >> >> >> > on
> >> >> >> >> > its
> >> >> >> >> > window)
> >> >> >> >> > b->window()->Show(); // Show called twice, potentially.
> >> >> >> >> > -Ben
> >> >> >> >> >
> >> >> >> >> > On Wed, Sep 29, 2010 at 5:09 PM, David Garcia
> >> >> >> >> > <dhg@chromium.org>
> >> >> >> >> > wrote:
> >> >> >> >> >>
> >> >> >> >> >> There was a discussion further back, but when it creates the
> >> >> >> >> >> tab,
> >> >> >> >> >> it
> >> >> >> >> >> needs to know the browser associated with that tab.  (so it
> >> >> >> >> >> can
> >> >> >> >> >> subscribe to events, etc) Since add tab with url can
> actually
> >> >> >> >> >> open
> >> >> >> >> >> up
> >> >> >> >> >> in a different browser, this can cause a problem.
> >> >> >> >> >>
> >> >> >> >> >> On Wed, Sep 29, 2010 at 5:01 PM, Ben Goodger (Google)
> >> >> >> >> >> <ben@chromium.org>
> >> >> >> >> >> wrote:
> >> >> >> >> >> > What is ChromeOS code doing such that it requires this
> >> >> >> >> >> > change
> >> >> >> >> >> > to
> >> >> >> >> >> > the
> >> >> >> >> >> > API?
> >> >> >> >> >> > -Ben
> >> >> >> >> >> >
> >> >> >> >> >> > On Wed, Sep 29, 2010 at 4:17 PM, <dhg@chromium.org>
> wrote:
> >> >> >> >> >> >>
> >> >> >> >> >> >> ben,
> >> >> >> >> >> >>
> >> >> >> >> >> >> You say that all the code in non-chromeos already has
> this
> >> >> >> >> >> >> context,
> >> >> >> >> >> >> but
> >> >> >> >> >> >> what
> >> >> >> >> >> >> about chromeos... doesn't that matter?
> >> >> >> >> >> >>
> >> >> >> >> >> >>
> >> >> >> >> >> >> On 2010/09/29 22:02:33, Ben Goodger wrote:
> >> >> >> >> >> >>>
> >> >> >> >> >> >>> For the record, I signed off on a much more limited
> >> >> >> >> >> >>> version
> >> >> >> >> >> >>> of
> >> >> >> >> >> >>> this
> >> >> >> >> >> >>> change
> >> >> >> >> >> >>> that seemed to primarily affect the app/extension
> related
> >> >> >> >> >> >>> functions
> >> >> >> >> >> >>> on
> >> >> >> >> >> >>> Browser, not AddTabWithURL which is a more central API.
> >> >> >> >> >> >>
> >> >> >> >> >> >>> The trouble is that none of the code in Chrome-land ever
> >> >> >> >> >> >>> needed
> >> >> >> >> >> >>> this
> >> >> >> >> >> >>> before,
> >> >> >> >> >> >>> and it's not immediately clear why they would given that
> >> >> >> >> >> >>> they
> >> >> >> >> >> >>> always
> >> >> >> >> >> >>> obtain
> >> >> >> >> >> >>> a valid browser prior to calling this function. So it's
> a
> >> >> >> >> >> >>> layer
> >> >> >> >> >> >>> of
> >> >> >> >> >> >>> complexity that is required only for a specific use case
> >> >> >> >> >> >>> (that I
> >> >> >> >> >> >>> have
> >> >> >> >> >> >>> not
> >> >> >> >> >> >>> discovered yet, I will determine what that is once I get
> >> >> >> >> >> >>> further
> >> >> >> >> >> >>> into
> >> >> >> >> >> >>> the
> >> >> >> >> >> >>> build/test process).
> >> >> >> >> >> >>
> >> >> >> >> >> >>> I will sort the rest of this out myself.
> >> >> >> >> >> >>
> >> >> >> >> >> >>> -Ben
> >> >> >> >> >> >>
> >> >> >> >> >> >>> On Wed, Sep 29, 2010 at 2:56 PM,
> >> >> >> >> >> >>> <mailto:twiz@chromium.org>
> >> >> >> >> >> >>> wrote:
> >> >> >> >> >> >>
> >> >> >> >> >> >>> > On 2010/09/29 21:27:25, Ben Goodger wrote:
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >> On 2010/08/05 19:20:47, Jeff Timanus wrote:
> >> >> >> >> >> >>> >> > On 2010/08/05 19:14:28, Ben Goodger wrote:
> >> >> >> >> >> >>> >> > > Can you describe what changed outside of the
> >> >> >> >> >> >>> >> > > ChromeOS
> >> >> >> >> >> >>> >> > > bits?
> >> >> >> >> >> >>> >> >
> >> >> >> >> >> >>> >> > Nothing changed from the original CL outside of the
> >> >> >> >> >> >>> >> > corrections I
> >> >> >> >> >> >>> >> > had to
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> > make
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >> in
> >> >> >> >> >> >>> >> > filebrowse_ui.cc, and internet_options_handler.cc.
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >  Not true. I am now discovering you changed the API
> for
> >> >> >> >> >> >>> > AddTabWithURL
> >> >> >> >> >> >>> > in an
> >> >> >> >> >> >>> >> awkward and pervasive way (returning Browser as an
> out
> >> >> >> >> >> >>> >> parameter
> >> >> >> >> >> >>> >> when
> >> >> >> >> >> >>> >> every
> >> >> >> >> >> >>> >> client in non-ChromeOS code already has a valid
> >> >> >> >> >> >>> >> Browser),
> >> >> >> >> >> >>> >> and
> >> >> >> >> >> >>> >> changed
> >> >> >> >> >> >>> >> a
> >> >> >> >> >> >>> >> huge
> >> >> >> >> >> >>> >> amount of the codebase to reflect this.
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> > Hi Ben,
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> > I'm sorry that this slipped through, and has become an
> >> >> >> >> >> >>> > issue.
> >> >> >> >> >> >>> >  Do
> >> >> >> >> >> >>> > note
> >> >> >> >> >> >>> > that
> >> >> >> >> >> >>> > the
> >> >> >> >> >> >>> > changes you are referring to happened after you had
> >> >> >> >> >> >>> > signed
> >> >> >> >> >> >>> > off
> >> >> >> >> >> >>> > on
> >> >> >> >> >> >>> > the
> >> >> >> >> >> >>> > review
> >> >> >> >> >> >>> > process, and during the back-forth with dhg.
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> > I added that callback because my first attempt at a
> fix
> >> >> >> >> >> >>> > (making
> >> >> >> >> >> >>> > use
> >> >> >> >> >> >>> > of
> >> >> >> >> >> >>> > the
> >> >> >> >> >> >>> > BrowserList::FindBrowserWithProfile function) was
> deemed
> >> >> >> >> >> >>> > as
> >> >> >> >> >> >>> > too
> >> >> >> >> >> >>> > fragile
> >> >> >> >> >> >>> > for
> >> >> >> >> >> >>> > callers of the various AddTabWithUrl routines.  See
> the
> >> >> >> >> >> >>> > patch
> >> >> >> >> >> >>> > history
> >> >> >> >> >> >>> > of
> >> >> >> >> >> >>> > filebrowse_ui.cc.
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> > What is problematic with having these routines return
> a
> >> >> >> >> >> >>> > browser
> >> >> >> >> >> >>> > instance
> >> >> >> >> >> >>> > via a
> >> >> >> >> >> >>> > parameter?  It is possible that a new browser was
> >> >> >> >> >> >>> > created
> >> >> >> >> >> >>> > to
> >> >> >> >> >> >>> > service
> >> >> >> >> >> >>> > them,
> >> >> >> >> >> >>> > and
> >> >> >> >> >> >>> > that browser is not directly accessible via just the
> >> >> >> >> >> >>> > newly
> >> >> >> >> >> >>> > constructed
> >> >> >> >> >> >>> > tab-contents.
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> > Let me know if I can help.
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> > Jeff
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >  I would have appreciated a more accurate response
> here,
> >> >> >> >> >> >>> > given
> >> >> >> >> >> >>> > the
> >> >> >> >> >> >>> > amount
> >> >> >> >> >> >>> >> of
> >> >> >> >> >> >>> >> email I get it's hard for me to evaluate everything
> and
> >> >> >> >> >> >>> >> I
> >> >> >> >> >> >>> >> rely
> >> >> >> >> >> >>> >> on
> >> >> >> >> >> >>> >> people
> >> >> >> >> >> >>> >> to
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> > tell
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >> me what I should be looking at.
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >  I am going to correct the API problems this time. I
> may
> >> >> >> >> >> >>> > roll
> >> >> >> >> >> >>> > some
> >> >> >> >> >> >>> > or
> >> >> >> >> >> >>> > all
> >> >> >> >> >> >>> >> of
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> > this
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >> changelist back.
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >  -Ben
> >> >> >> >> >> >>> >>
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>> > http://codereview.chromium.org/2819086/show
> >> >> >> >> >> >>> >
> >> >> >> >> >> >>
> >> >> >> >> >> >>
> >> >> >> >> >> >>
> >> >> >> >> >> >>
> >> >> >> >> >> >> http://codereview.chromium.org/2819086/show
> >> >> >> >> >> >
> >> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>