Re-factor location bar/toolbar code to get rid of the browser dependency.

Re-factor location bar/toolbar code to get rid of the browser dependency. This CL is needed to allow code reusing by captive portal view, which will show web content using DomView.

BUG=chromium-os:22630
TEST=browser_tests,unit_tests works

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126959
Reverted because of a misprint which was hidden by overloaded method (ExecuteCommand): http://codereview.chromium.org/9703099/


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

[altimofeev, 2012-02-28 14:52:24]

Please take a look.

[sky, 2012-02-28 18:24:24]

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/toolbar/to...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/toolbar/to...
chrome/browser/ui/toolbar/toolbar_model.h:43: explicit ToolbarModel(Browser*
browser);
I don't like having these two modes, it's too error prone and going to break.

If you're going to remove dependencies you need to remove Browser entirely. If
you need to introduce an interface that is used to get things from Browser, do
that.

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/click_handler.cc (right):

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/click_handler.cc:9: #include
"chrome/browser/ui/views/window.h"
sort

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.h:143: Browser*
FindBrowser();
Same comment here as with ToolbarModel.

[altimofeev, 2012-02-29 18:57:29]

PTAL

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/toolbar/to...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/toolbar/to...
chrome/browser/ui/toolbar/toolbar_model.h:43: explicit ToolbarModel(Browser*
browser);
On 2012/02/28 18:24:24, sky wrote:
> I don't like having these two modes, it's too error prone and going to break.
> 
> If you're going to remove dependencies you need to remove Browser entirely. If
> you need to introduce an interface that is used to get things from Browser, do
> that.

Done. See TabContentsWrapperContainer interface.

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/click_handler.cc (right):

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/click_handler.cc:9: #include
"chrome/browser/ui/views/window.h"
On 2012/02/28 18:24:24, sky wrote:
> sort

Done.

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/9013/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.h:143: Browser*
FindBrowser();
On 2012/02/28 18:24:24, sky wrote:
> Same comment here as with ToolbarModel.

Done. I've created interfaces BrowserShowContentRelated, BrowserShowPageInfo
(browser_show_actions.h) which are used by ContentSettingImageView and
ClickHandler. From the other hand, PageActionImageView deeply uses Browser, so
I've delegated creation of PageActionImageView to ToolbarView since I don't need
it.

[sky, 2012-02-29 22:41:10]

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:30: typedef
BrowserShowContentRelated Delegate;
Can we name this class ContentSettingsBubbleModelDelegate?

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/click_handler.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/click_handler.h:25: BrowserShowPageInfo*
delegate);
Can this take LocationBarView::Delegate directly?

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:99: virtual
BrowserShowContentRelated* GetBrowserShowContentRelated() = 0;
Do we really need to expose objects for the methods you care about? Could the
methods you have in  BrowserShowActions be added directly here?

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:110: LocationBarView*
owner, ExtensionAction* action) = 0;
nit: when you wrap, each param on its own line.

[altimofeev, 2012-03-01 10:05:39]

Thank you for your comment. PTAL

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:30: typedef
BrowserShowContentRelated Delegate;
On 2012/02/29 22:41:10, sky wrote:
> Can we name this class ContentSettingsBubbleModelDelegate?

Which one, Delegate or BrowserShowContentRelated?

IMO BrowserShowContentRelated is more general and could be reused by some other
class, not directly related to ContentSettingBubbleModel. Furthermore, it allows
easily switch from "typedef" to inheritance (Delegate : public
BrowserShowContentRelated) if any non-browser functionality will be needed from
the Delegate.

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/click_handler.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/click_handler.h:25: BrowserShowPageInfo*
delegate);
On 2012/02/29 22:41:10, sky wrote:
> Can this take LocationBarView::Delegate directly?

I can expose the delegate. Indeed, it will decrease CL size.
Done.

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:99: virtual
BrowserShowContentRelated* GetBrowserShowContentRelated() = 0;
On 2012/02/29 22:41:10, sky wrote:
> Do we really need to expose objects for the methods you care about? Could the
> methods you have in  BrowserShowActions be added directly here?

They could. But why is it better? BrowserShowAction is an interface for
Browser-like classes. LocationBarView::Delegate implementer seems different for
me.

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:110: LocationBarView*
owner, ExtensionAction* action) = 0;
On 2012/02/29 22:41:10, sky wrote:
> nit: when you wrap, each param on its own line.

Done.

[sky, 2012-03-01 16:34:47]

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:30: typedef
BrowserShowContentRelated Delegate;
On 2012/03/01 10:05:39, altimofeev wrote:
> On 2012/02/29 22:41:10, sky wrote:
> > Can we name this class ContentSettingsBubbleModelDelegate?
> 
> Which one, Delegate or BrowserShowContentRelated?
> 
> IMO BrowserShowContentRelated is more general

Indeed, but focus on the task at hand and not worry about what *might* be needed
at some later date. If we need the flexibility later on, we can add it then.

> and could be reused by some other
> class, not directly related to ContentSettingBubbleModel. Furthermore, it
allows
> easily switch from "typedef" to inheritance (Delegate : public
> BrowserShowContentRelated) if any non-browser functionality will be needed
from
> the Delegate.
>

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:99: virtual
BrowserShowContentRelated* GetBrowserShowContentRelated() = 0;
On 2012/03/01 10:05:39, altimofeev wrote:
> On 2012/02/29 22:41:10, sky wrote:
> > Do we really need to expose objects for the methods you care about? Could
the
> > methods you have in  BrowserShowActions be added directly here?
> 
> They could. But why is it better? BrowserShowAction is an interface for
> Browser-like classes. LocationBarView::Delegate implementer seems different
for
> me.

I believe you are arguing for a class per method approach. I agree that gives
you flexibility, but when you have a single class implementing it all it's
unneeded flexibility that makes the code harder to understand. LocationBarView
already has this Delegate class that is intended for functionality like you want
to add. It should be added here rather than effectively create more delegates.

[altimofeev, 2012-03-01 18:01:07]

PTAL

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:30: typedef
BrowserShowContentRelated Delegate;
On 2012/03/01 16:34:47, sky wrote:
> On 2012/03/01 10:05:39, altimofeev wrote:
> > On 2012/02/29 22:41:10, sky wrote:
> > > Can we name this class ContentSettingsBubbleModelDelegate?
> > 
> > Which one, Delegate or BrowserShowContentRelated?
> > 
> > IMO BrowserShowContentRelated is more general
> 
> Indeed, but focus on the task at hand and not worry about what *might* be
needed
> at some later date. If we need the flexibility later on, we can add it then.
> 
> > and could be reused by some other
> > class, not directly related to ContentSettingBubbleModel. Furthermore, it
> allows
> > easily switch from "typedef" to inheritance (Delegate : public
> > BrowserShowContentRelated) if any non-browser functionality will be needed
> from
> > the Delegate.
> > 
> 

Done.

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/26010/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:99: virtual
BrowserShowContentRelated* GetBrowserShowContentRelated() = 0;
I've gotten rid of "BrowserShow..." classes (one deleted, another renamed). I
left ContentSettingBubbleModelDelegate because ContentSettingBubbleModel is also
used by GTK version, and they pass Browser to it.

[altimofeev, 2012-03-05 10:32:59]

Friendly ping. Also note, that this change blocks my following CL, which is not
refactoring, but P1.

[altimofeev, 2012-03-05 10:36:42]

+John: Owner approval for "chrome/browser/ui/tab_contents/*" part.

[sky, 2012-03-05 16:30:55]

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:31: };
Add a protected virtual destructor to make it clear the model doesn't own the
delegate.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:12: class
NavigationController;
nit: no spaces.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:18: class
TabContentsWrapperContainer {
This name is overly generic. Is there a more specific name you could use?

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:24: virtual int
tab_count() const = 0;
Doesn't use hacker_style for virtual methods. Additionally I don't like having
Browser appear to implement these from an interface. Instead have Browser own
another object that implements this and talks to Browser. There are examples of
this pattern in Browser now.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:36: class
BrowserShowContentRelated;
Can you remove some of these now?

[jam, 2012-03-05 16:41:27]

On 2012/03/05 10:36:42, altimofeev wrote:
> +John: Owner approval for "chrome/browser/ui/tab_contents/*" part.

you don't need me, sky is owner of parent directory

[altimofeev, 2012-03-05 17:13:24]

Thank you for your reply. I have a question, before I start fixing your
comments.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:24: virtual int
tab_count() const = 0;
Will fix hacker_style methods.

Concerning the comment about the interface. Browser already passes corresponding
calls to TabStripModel to implement these methods. Making these methods virtual
doesn't complicate the situation, IMO. Instead it structures the Browser's
methods.

Can you please clarify why your approach is better?

[tfarina, 2012-03-05 17:30:24]

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:20:
TabContentsWrapperContainer() {}
nit: for pure abstract class (i.e, interface) you don't need an empty ctor. Nor
the DISALLOW_COPY... below.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:21: virtual
~TabContentsWrapperContainer() {}
nit: is it possible to make this protected?

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:23: // Count of
TCWs in the container.
nit: // Returns the number of TCWs in this container.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:26: // Index of
the active TCW.
nit: // Returns the index of the active TCW.

[sky, 2012-03-05 18:10:13]

On Mon, Mar 5, 2012 at 9:13 AM,  <altimofeev@chromium.org> wrote:
> Thank you for your reply. I have a question, before I start fixing your
> comments.
>
>
>
>
http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
> File chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h
> (right):
>
>
http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
> chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:24:
> virtual int tab_count() const = 0;
> Will fix hacker_style methods.
>
> Concerning the comment about the interface. Browser already passes
> corresponding calls to TabStripModel to implement these methods. Making
> these methods virtual doesn't complicate the situation, IMO. Instead it
> structures the Browser's methods.

See style guide. It says unix_hacker_style methods are intended for
cheap inlined methods. That isn't the case once you have virtual
methods.

  -Scott

>
> Can you please clarify why your approach is better?
>
> http://codereview.chromium.org/9479008/

[altimofeev, 2012-03-05 19:49:01]

> > Will fix hacker_style methods.
> >
> > Concerning the comment about the interface. Browser already passes
> > corresponding calls to TabStripModel to implement these methods. Making
> > these methods virtual doesn't complicate the situation, IMO. Instead it
> > structures the Browser's methods.
> 
> See style guide. It says unix_hacker_style methods are intended for
> cheap inlined methods. That isn't the case once you have virtual
> methods.
 
Actually, my question wasn't about unix_style methods (I've agreed about them in
the first line of my answer). 
Anyway, what about removing these two methods from the interface? Can I make
Browser to implement that new interface (without unix_style methods)?

[sky, 2012-03-05 22:14:47]

On Mon, Mar 5, 2012 at 11:49 AM,  <altimofeev@chromium.org> wrote:
>> > Will fix hacker_style methods.
>> >
>> > Concerning the comment about the interface. Browser already passes
>> > corresponding calls to TabStripModel to implement these methods. Making
>> > these methods virtual doesn't complicate the situation, IMO. Instead it
>> > structures the Browser's methods.
>
>
>> See style guide. It says unix_hacker_style methods are intended for
>> cheap inlined methods. That isn't the case once you have virtual
>> methods.
>
>
> Actually, my question wasn't about unix_style methods (I've agreed about
> them in
> the first line of my answer).
> Anyway, what about removing these two methods from the interface? Can I make
> Browser to implement that new interface (without unix_style methods)?

No. Seem third comments in this patch from Ben:
http://codereview.chromium.org/6660028/ .

  -Scott

[altimofeev, 2012-03-06 08:34:02]

Why not? Actually, I don't understand how is Ben's comment related to my
situation. In my situation, I'm not introducing any new methods to Browser. Also
I'm not going to remove these methods from "browser.h", because they are used in
hundred of places.

[sky, 2012-03-06 17:12:47]

On Tue, Mar 6, 2012 at 12:34 AM,  <altimofeev@chromium.org> wrote:
> Why not? Actually, I don't understand how is Ben's comment related to my
> situation. In my situation, I'm not introducing any new methods to Browser.
> Also
> I'm not going to remove these methods from "browser.h", because they are
> used in
> hundred of places.

Consider the following. Lets say you have an existing class:

class AddressBook {
 public:
  // Returns the number of entries.
  int count();
  // Returns an address at the specified index:
  Address* GetAddress(int index);
};

But AddressBook ends up providing a ton of functionality and we
instead decide provide a more minimal interface:

class AddressProvider {
 public:
  virtual int GetCount() = 0;
  virtual Address* GetAddress(int index) = 0;
};

We still want AddressBook to provide this functionality, but other
consumers might take an AddressProvider. We could do that by making
AddressBook directly implement the interface:

class AddressBook : public AddressProvider {
 public:
  // AddressProvider overrides:
  virtual int GetCount() OVERRIDE;
  virtual Address* GetAddress(int index) OVERRIDE;
};

Or, we could make AddressBook have an AddressProvider:

class AddressBook {
 public:
  // Returns the number of entries.
  int count();
  // Returns an address at the specified index:
  Address* GetAddress(int index);
 private:
  AddressProviderImpl address_provider_;
};

class AddressProviderImpl : public AddressProvider {
 public:
  AddressProviderImpl(AddressBook* book) : book_(book) {}
  virtual int GetCount() OVERRIDE { return book_->count(); }
  virtual Address* GetAddress(int index) { return book_->GetAddress(index); }
};

The reason for the later is twofold. It does not effect the method
signature of AddressBook in anyway, and it doesn't appear that
AddressBook is providing methods as a result of implementing an
interface.

I believe this is the exact situation you have and the comments from
Ben in the other patch apply to what you're doing. To sum it up, you
should make Browser have a TabContentsWrapperContainer, call it
BrowserTabContentsWrapperContainer. BrowserTabContentsWrapperContainer
takes a browser and forwards all methods to said browser.

  -Scott

[altimofeev, 2012-03-06 18:50:44]

Thanks for the explanations!

Also to not bother again, should I do the same thing for
ContentSettingBubbleModelDelegate?

[sky, 2012-03-06 19:07:32]

yes

On Tue, Mar 6, 2012 at 10:50 AM,  <altimofeev@chromium.org> wrote:
> Thanks for the explanations!
>
> Also to not bother again, should I do the same thing for
> ContentSettingBubbleModelDelegate?
>
> http://codereview.chromium.org/9479008/

[altimofeev, 2012-03-07 21:01:39]

PTAL.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:31: };
On 2012/03/05 16:30:56, sky wrote:
> Add a protected virtual destructor to make it clear the model doesn't own the
> delegate.

Done.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:12: class
NavigationController;
On 2012/03/05 16:30:56, sky wrote:
> nit: no spaces.

Done.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:18: class
TabContentsWrapperContainer {
On 2012/03/05 16:30:56, sky wrote:
> This name is overly generic. Is there a more specific name you could use?

No variant comes to my mind. Do you have a suggestions?

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:20:
TabContentsWrapperContainer() {}
On 2012/03/05 17:30:24, tfarina wrote:
> nit: for pure abstract class (i.e, interface) you don't need an empty ctor.
Nor
> the DISALLOW_COPY... below.

Done.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:21: virtual
~TabContentsWrapperContainer() {}
On 2012/03/05 17:30:24, tfarina wrote:
> nit: is it possible to make this protected?

Why do that?

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:23: // Count of
TCWs in the container.
On 2012/03/05 17:30:24, tfarina wrote:
> nit: // Returns the number of TCWs in this container.

Done.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:26: // Index of
the active TCW.
On 2012/03/05 17:30:24, tfarina wrote:
> nit: // Returns the index of the active TCW.

Done.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/location_bar_view.h:36: class
BrowserShowContentRelated;
On 2012/03/05 16:30:56, sky wrote:
> Can you remove some of these now?

Done.

[sky, 2012-03-07 23:32:38]

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
File chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h (right):

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:18: class
TabContentsWrapperContainer {
On 2012/03/07 21:01:39, altimofeev wrote:
> On 2012/03/05 16:30:56, sky wrote:
> > This name is overly generic. Is there a more specific name you could use?
> 
> No variant comes to my mind. Do you have a suggestions?

It's used by the toolbarmodel, so name it ToolbarModelHost and move it to
ui/toolbar.

http://codereview.chromium.org/9479008/diff/39022/chrome/browser/ui/tab_conte...
chrome/browser/ui/tab_contents/tab_contents_wrapper_container.h:21: virtual
~TabContentsWrapperContainer() {}
On 2012/03/07 21:01:39, altimofeev wrote:
> On 2012/03/05 17:30:24, tfarina wrote:
> > nit: is it possible to make this protected?
> 
> Why do that?

A protected virtual destructor makes it clear that someone else owns the object.
For example, if you have:

class Foo {
  SetDelegate(Delegate* delegate);
};
and you see the delegate has a protected destructor, you know Foo doesn't own
Delegate. Similarly int implementing such an interface you know that you need to
take responsibility for deleting it.

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:26: virtual
~ContentSettingBubbleModelDelegate() {}
Make all the destructors of these classes protected.

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/views/rel...
File chrome/browser/ui/views/reload_button.cc (left):

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/views/rel...
chrome/browser/ui/views/reload_button.cc:113:
browser_->ExecuteCommandWithDisposition(command, disposition);
Doesn't this lose the disposition?

[altimofeev, 2012-03-11 14:53:07]

PTAL

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:26: virtual
~ContentSettingBubbleModelDelegate() {}
On 2012/03/07 23:32:38, sky wrote:
> Make all the destructors of these classes protected.

Done.

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/views/rel...
File chrome/browser/ui/views/reload_button.cc (left):

http://codereview.chromium.org/9479008/diff/64001/chrome/browser/ui/views/rel...
chrome/browser/ui/views/reload_button.cc:113:
browser_->ExecuteCommandWithDisposition(command, disposition);
On 2012/03/07 23:32:38, sky wrote:
> Doesn't this lose the disposition?

Yes it does. Actually, I thought that reload button should only reload the
active tab. But it appeared that there is no other mean to open the same URL in
a new tab (after 'Go' button was illuminated). Also I used to 'Ctrl' + click
behavior, which is anyway overwritten by this button. From the other hand,
middle button click or Command + click (Mac only) will stop working after this
kind of change.

Final decision: I've changed CommandUpdater to be able to pass disposition
explicitly.

[sky, 2012-03-12 15:14:05]

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
File chrome/browser/command_updater.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
chrome/browser/command_updater.h:31: WindowOpenDisposition disposition) = 0;
It doesn't scale to add a new argument to this interface. Could you instead have
the reload button push the window open disposition to the LocationBar and have
reload get the disposition from the locationbar? This is similar to how
OpenCurrentURL works.

[altimofeev, 2012-03-12 15:38:42]

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
File chrome/browser/command_updater.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
chrome/browser/command_updater.h:31: WindowOpenDisposition disposition) = 0;
I'm not agree with you:
1. Having disposition as an argument is logically consistent, because Browser,
which is the only CommmandUpdaterDelegate's successor, always calls
ExecuteCommandWithDisposition.
2. No obvious reason, why this interface will need a new argument.
3. OpenCurrentURL's solution looks like a hack for me: method
GetWindowOpenDisposition doesn't look like related to LocationBar, because
WindowOpenDisposition is event's property, not UI's.
4. Furthermore, OpenCurrentURL hack only confirms, that CommandUpdater interface
should be changed.

What am I missing?

[sky, 2012-03-12 17:08:29]

On Mon, Mar 12, 2012 at 8:38 AM,  <altimofeev@chromium.org> wrote:
>
>
http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
> File chrome/browser/command_updater.h (right):
>
>
http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
> chrome/browser/command_updater.h:31: WindowOpenDisposition disposition)
> = 0;
> I'm not agree with you:
> 1. Having disposition as an argument is logically consistent, because
> Browser, which is the only CommmandUpdaterDelegate's successor, always
> calls ExecuteCommandWithDisposition.

If disposition was used for all commands, I would agree. But it isn't.
This code is used for many commands where a disposition is not
applicable. It is extremely confusing to implement a method when the
arguments some times make sense and some times don't. This is one of
the reasons GURL was not added as an argument.

I see two possibilities. Implement disposition as GURL has been
plumbed through, or provide some way for CommandUpdaterDelegate to get
additional state for a command.

  -Scott

> 2. No obvious reason, why this interface will need a new argument.
> 3. OpenCurrentURL's solution looks like a hack for me: method
> GetWindowOpenDisposition doesn't look like related to LocationBar,
> because WindowOpenDisposition is event's property, not UI's.
> 4. Furthermore, OpenCurrentURL hack only confirms, that CommandUpdater
> interface should be changed.
>
> What am I missing?
>
> http://codereview.chromium.org/9479008/

[altimofeev, 2012-03-12 19:20:44]

Plumbing looks ugly for me because it implies a lot of files changes:
LocatoinBar interface change, introducing interface for ReloadButton (there are
gtk and views realizations), change of all the classes which inherit from these
interfaces. Another caveat is that IDC_RELOAD could be triggered even without
reload button presented (popups etc.)

From the other hand, my implementation only revealed, that CommandUpdater needs
more flexible interface. This CL wasn't aimed to fix it. There is already Ben's
TODO about it.

As for your argument:

> This code is used for many commands where a disposition is not
> applicable. It is extremely confusing to implement a method when the
> arguments some times make sense and some times don't.

Browser already uses similar method. So this is not the fault of this CL.

So what about leaving ExecuteCommandWithDisposition in the interface, but adding
TODO about using more flexible argument passing? For instance, the one which is
used by Notifications.

[sky, 2012-03-12 20:48:17]

On Mon, Mar 12, 2012 at 12:20 PM,  <altimofeev@chromium.org> wrote:
> Plumbing looks ugly for me because it implies a lot of files changes:
> LocatoinBar interface change, introducing interface for ReloadButton (there
> are
> gtk and views realizations), change of all the classes which inherit from
> these
> interfaces. Another caveat is that IDC_RELOAD could be triggered even
> without
> reload button presented (popups etc.)
>
> From the other hand, my implementation only revealed, that CommandUpdater
> needs
> more flexible interface. This CL wasn't aimed to fix it. There is already
> Ben's
> TODO about it.
>
> As for your argument:
>
>
>> This code is used for many commands where a disposition is not
>> applicable. It is extremely confusing to implement a method when the
>> arguments some times make sense and some times don't.
>
>
> Browser already uses similar method. So this is not the fault of this CL.
>
> So what about leaving ExecuteCommandWithDisposition in the interface, but
> adding
> TODO about using more flexible argument passing? For instance, the one which
> is
> used by Notifications.

You've convinced me, SGTM

[sky, 2012-03-12 20:59:19]

Add TODO we discussed, rename ToolbarModelHost to ToolbarModelDelegate and put
ContentSettingsBubbleModelDelegate in its own file then LGTM

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
File chrome/browser/command_updater_unittest.cc (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
chrome/browser/command_updater_unittest.cc:11: virtual void
ExecuteCommandWithDisposition(int id, WindowOpenDisposition) {
OVERRIDE

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:802: int id, WindowOpenDisposition disposition)
OVERRIDE;
nit: once you wrap you shouldn't have multiple params on the same line.

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:24: class
ContentSettingBubbleModelDelegate {
How come you didn't put this in a separate file like you did ToolbarModelHost.
Also, this name (and others) makes me thing you should name ToolbarModelHost
ToolbarModelDelegate.

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.h:78: // ToolbarModel's host.
nit: remove this comment, it doesn't give any more information.

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/views/rel...
File chrome/browser/ui/views/reload_button.cc (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/views/rel...
chrome/browser/ui/views/reload_button.cc:111: if (command_updater_) {
nit: no {}

[altimofeev, 2012-03-13 15:38:05]

PTAL

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
File chrome/browser/command_updater_unittest.cc (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/command_upda...
chrome/browser/command_updater_unittest.cc:11: virtual void
ExecuteCommandWithDisposition(int id, WindowOpenDisposition) {
On 2012/03/12 20:59:19, sky wrote:
> OVERRIDE

Done.

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:802: int id, WindowOpenDisposition disposition)
OVERRIDE;
On 2012/03/12 20:59:19, sky wrote:
> nit: once you wrap you shouldn't have multiple params on the same line.

Done.

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:24: class
ContentSettingBubbleModelDelegate {
On 2012/03/12 20:59:19, sky wrote:
> How come you didn't put this in a separate file like you did ToolbarModelHost.
> Also, this name (and others) makes me thing you should name ToolbarModelHost
> ToolbarModelDelegate.

Done.

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_model.h (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_model.h:78: // ToolbarModel's host.
On 2012/03/12 20:59:19, sky wrote:
> nit: remove this comment, it doesn't give any more information.

Done.

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/views/rel...
File chrome/browser/ui/views/reload_button.cc (right):

http://codereview.chromium.org/9479008/diff/69035/chrome/browser/ui/views/rel...
chrome/browser/ui/views/reload_button.cc:111: if (command_updater_) {
On 2012/03/12 20:59:19, sky wrote:
> nit: no {}

Done.

[sky, 2012-03-13 17:36:04]

LGTM
Thanks!

[commit-bot: I haz the power, 2012-03-13 18:39:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/9479008/81032

[commit-bot: I haz the power, 2012-03-13 18:39:31]

Presubmit check for 9479008-81032 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/sync

Presubmit checks took 3.6s to calculate.

[altimofeev, 2012-03-13 18:44:29]

atwilson@: owner approval for "chrome/browser/sync/sync_globa_error_unittest.cc"
changes

[altimofeev, 2012-03-14 18:37:40]

On 2012/03/13 18:44:29, altimofeev wrote:
> atwilson@: owner approval for
"chrome/browser/sync/sync_globa_error_unittest.cc"
> changes

Friendly ping

[Andrew T Wilson (Slow), 2012-03-15 16:39:55]

browser/sync LGTM

[commit-bot: I haz the power, 2012-03-15 16:46:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/9479008/81032

[commit-bot: I haz the power, 2012-03-15 19:08:15]

Change committed as 126959

[commit-bot: I haz the power, 2012-03-16 07:56:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/altimofeev@chromium.org/9479008/86006

[commit-bot: I haz the power, 2012-03-16 10:05:36]

Change committed as 127145