Remove any platform specific code from the chrome/browser/website_settings.* .

Remove any platform specific code from the chrome/browser/website_settings.* .
Let the WebsiteSettingsPopupGtk own the WebsiteSettings object.

Move all none platform specific website settings code to chrome/browser/ui/website_settings/.

BUG=113688
TEST=WebsiteSettingsTest*


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

[markusheintz_, 2012-05-31 17:22:27]

@tfarina: As follow up on your comments on CL
http://codereview.chromium.org/10456017/: I moved the "Show" method from the
"WebsiteSettings" class to "WebsiteSettingsPopupGtk". If this is what you had in
mind, then I'll change this for the CL 10456017 too. And get this CL reviewed.

[tfarina, 2012-05-31 17:34:19]

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
chrome/browser/ui/gtk/browser_window_gtk.cc:1142:
WebsiteSettingsPopupGtk::Show(GetNativeHandle(),
This works for me. But it isn't what I suggested.

What I have in mind is:

WebsiteSettings::Show(...);

and in website_settings_popup_gtk.cc (at the end of the file):

void WebsiteSettings::Show(...) {
  new WebsiteSettingsPopupGtk(...);
}

and in chrome/browser/website_settings.h

#if defined(TOOLKIT_GTK)
static void Show(...); // the gtk declaration, which unfortunately differ from
views declaration because BubbleDelegate requires an anchor_view not a
gfx::NativeWindow :/
#elif defined(TOOLKIT_VIEWS)
static void Show(...);
#endif

That way you can open this popup from other places outside of
chrome/browser/ui/gtk.

If can take a look at how speech and intents do this in chrome/browser/speech,
and chrome/browser/ui/intents/ and various browser::ShowFooDialog() calls.

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:132:
scoped_ptr<WebsiteSettings> presenter_;
Is this change from a weak pointer to a managed one necessary? I don't
understand the ownership of this code too well to judge it necessary or not yet.

[tfarina, 2012-05-31 18:56:58]

On 2012/05/31 17:34:19, tfarina wrote:
> If can take a look at how speech and intents do this in 
Ops, sorry. s/If/You/

[markusheintz_, 2012-05-31 19:08:19]

Thanks a lot for the clarification and the examples.

I used your comments on the other CL to do a refactoring.

Currently the WebsiteSettings class is responsible for the lifecycle management
of the popup and owns the WebsiteSettingsPopup*.

This CL reverses this. Now the WebsiteSettingsPopup* owns the WebsiteSettings
class and controls the lifecycle of the popup. That's why I need to change the
weak ptr to a  scoped_ptr.

This change allows me to reduce the lines of code since I don't have to notify
the WebsiteSettings class that the popup was closed.
And I don't need a single ifdef.

I don't need to call the popup from out side chrome/browser/ui/{gtk, views}  as
the code that calls the "Show" method already lives in the
chrome/browser/ui/{gtk, views}.

If this still works for you then I would like to go into this direction. 

On 2012/05/31 17:34:19, tfarina wrote:
>
https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
> File chrome/browser/ui/gtk/browser_window_gtk.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
> chrome/browser/ui/gtk/browser_window_gtk.cc:1142:
> WebsiteSettingsPopupGtk::Show(GetNativeHandle(),
> This works for me. But it isn't what I suggested.
> 
> What I have in mind is:
> 
> WebsiteSettings::Show(...);
> 
> and in website_settings_popup_gtk.cc (at the end of the file):
> 
> void WebsiteSettings::Show(...) {
>   new WebsiteSettingsPopupGtk(...);
> }
> 
> and in chrome/browser/website_settings.h
> 
> #if defined(TOOLKIT_GTK)
> static void Show(...); // the gtk declaration, which unfortunately differ from
> views declaration because BubbleDelegate requires an anchor_view not a
> gfx::NativeWindow :/
> #elif defined(TOOLKIT_VIEWS)
> static void Show(...);
> #endif
> 
> That way you can open this popup from other places outside of
> chrome/browser/ui/gtk.
> 
> If can take a look at how speech and intents do this in chrome/browser/speech,
> and chrome/browser/ui/intents/ and various browser::ShowFooDialog() calls.
> 
>
https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
> File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):
> 
>
https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
> chrome/browser/ui/gtk/website_settings_popup_gtk.h:132:
> scoped_ptr<WebsiteSettings> presenter_;
> Is this change from a weak pointer to a managed one necessary? I don't
> understand the ownership of this code too well to judge it necessary or not
yet.

[tfarina, 2012-05-31 19:23:00]

I have commented inline.

Take in mind that Ben might disagree with me, so do whatever he wants. ;)

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:31: class
WebsiteSettingsPopupGtk : public WebsiteSettingsUI,
I have another suggestion, feel free to ignore though.

I'd rename WebsiteSettingsUI to WebsiteSettingsPopup. I think UI is too
ambiguous to be used here.

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
File chrome/browser/ui/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
chrome/browser/ui/website_settings_ui.h:35: class WebsiteSettingsUI {
This is close to WebIntentPicker. You may have the "static void Show()" function
here. Just another suggestion to keep in mind, but to desconsider in your own.

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
chrome/browser/ui/website_settings_ui.h:100: class CookieInfoList : public
std::vector<WebsiteSettingsUI::CookieInfo> {
unrelated: this is very uncommon. I think it's the same as:

typedef std::vector<..> CookieInfoList;

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/webs...
File chrome/browser/website_settings.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/webs...
chrome/browser/website_settings.h:34: class WebsiteSettings : public
TabSpecificContentSettings::SiteDataObserver {
I have another suggestion that you could consider, but also feel free to ignore
it.

I'd move this file and the unittest to chrome/browser/ui/website_settings/ along
with website_settings_ui.h and add an OWNERS file with you on it.

[tfarina, 2012-05-31 19:27:23]

On 2012/05/31 19:08:19, markusheintz_ wrote:
> Thanks a lot for the clarification and the examples.
> 
> I used your comments on the other CL to do a refactoring.
> 
> Currently the WebsiteSettings class is responsible for the lifecycle
management
> of the popup and owns the WebsiteSettingsPopup*.
> 
> This CL reverses this. Now the WebsiteSettingsPopup* owns the WebsiteSettings
> class and controls the lifecycle of the popup.
I'm not sure that is what we want to do, someone more familiar with this
ownership model should comment. Should View controls the lifetime of the
Controller in the MVC sense? Not sure :/ 

> This change allows me to reduce the lines of code since I don't have to notify
> the WebsiteSettings class that the popup was closed.
> And I don't need a single ifdef.
> 
> I don't need to call the popup from out side chrome/browser/ui/{gtk, views} 
as
> the code that calls the "Show" method already lives in the
> chrome/browser/ui/{gtk, views}.
> 
> If this still works for you then I would like to go into this direction. 
Yes, this works for me as long as we don't include chrome/browser/ui/gtk and
views/ outside of it nor call anything from there ;)

[markusheintz_, 2012-05-31 20:07:29]

@Ben: The WebsiteSettingUI currently follows the Model View Presenter pattern
where the presenter (WebsiteSettings) is the owner of the UI
(WebsiteSettingsPopupGtk) and controls the lifecycle. 

I have experimented with the idea of reversing the ownership and let the UI own
the presenter. The result is this CL.

What is your opinion on this experiment? While it is a bit odd that the
Presenter owns the UI it would allow me to get rid of the ifdefs in my code.
(see discussion with Thiago above).


On 2012/05/31 19:27:23, tfarina wrote:
> On 2012/05/31 19:08:19, markusheintz_ wrote:
> > Thanks a lot for the clarification and the examples.
> > 
> > I used your comments on the other CL to do a refactoring.
> > 
> > Currently the WebsiteSettings class is responsible for the lifecycle
> management
> > of the popup and owns the WebsiteSettingsPopup*.
> > 
> > This CL reverses this. Now the WebsiteSettingsPopup* owns the
WebsiteSettings
> > class and controls the lifecycle of the popup.
> I'm not sure that is what we want to do, someone more familiar with this
> ownership model should comment. Should View controls the lifetime of the
> Controller in the MVC sense? Not sure :/ 
> 
> > This change allows me to reduce the lines of code since I don't have to
notify
> > the WebsiteSettings class that the popup was closed.
> > And I don't need a single ifdef.
> > 
> > I don't need to call the popup from out side chrome/browser/ui/{gtk, views} 
> as
> > the code that calls the "Show" method already lives in the
> > chrome/browser/ui/{gtk, views}.
> > 
> > If this still works for you then I would like to go into this direction. 
> Yes, this works for me as long as we don't include chrome/browser/ui/gtk and
> views/ outside of it nor call anything from there ;)

[Ben Goodger (Google), 2012-05-31 20:51:51]

The approach seems fine.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/browser_window_gtk.cc:1142:
WebsiteSettingsPopupGtk::Show(GetNativeHandle(),
nit: outdent

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:46:
WebsiteSettingsPopupGtk(gfx::NativeWindow parent,
can your ctor be private then?

[tfarina, 2012-05-31 23:58:08]

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:14: #include
"googleurl/src/gurl.h"
nit: please, forward declare this instead.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:17: class BubbleGtk;
remove this forward declaration.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:30: // displayed in a popup
that is positioned relative the an anchor element.
nit: relative to an anchor element? can this comment be improved?

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:52: // WebsiteSettingsUI
implementations.
I bet you can also move these overrides and the BubbleDelegateGtk one to the
private section.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:132:
scoped_ptr<WebsiteSettings> presenter_;
include base/memory/scoped_ptr.h for this.

[markusheintz_, 2012-06-01 10:06:06]

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:31: class
WebsiteSettingsPopupGtk : public WebsiteSettingsUI,
On 2012/05/31 19:23:01, tfarina wrote:
> I have another suggestion, feel free to ignore though.
> 
> I'd rename WebsiteSettingsUI to WebsiteSettingsPopup. I think UI is too
> ambiguous to be used here.

The reason why I named it UI instead of Popup is that the implementation must
not necessarily be a Popup.

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:132:
scoped_ptr<WebsiteSettings> presenter_;
On 2012/05/31 17:34:19, tfarina wrote:
> Is this change from a weak pointer to a managed one necessary? I don't
> understand the ownership of this code too well to judge it necessary or not
yet.

Yes it is since the UI owns the presenter_.

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
File chrome/browser/ui/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
chrome/browser/ui/website_settings_ui.h:100: class CookieInfoList : public
std::vector<WebsiteSettingsUI::CookieInfo> {
On 2012/05/31 19:23:01, tfarina wrote:
> unrelated: this is very uncommon. I think it's the same as:
> 
> typedef std::vector<..> CookieInfoList;

I copied this from the cookie monster. This is the same way how the CookieList
is defined.

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/webs...
File chrome/browser/website_settings.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/webs...
chrome/browser/website_settings.h:34: class WebsiteSettings : public
TabSpecificContentSettings::SiteDataObserver {
On 2012/05/31 19:23:01, tfarina wrote:
> I have another suggestion that you could consider, but also feel free to
ignore
> it.
> 
> I'd move this file and the unittest to chrome/browser/ui/website_settings/
along
> with website_settings_ui.h and add an OWNERS file with you on it.

SGTM. Done.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/browser_window_gtk.cc:1142:
WebsiteSettingsPopupGtk::Show(GetNativeHandle(),
On 2012/05/31 20:51:51, Ben Goodger (Google) wrote:
> nit: outdent

Done.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:14: #include
"googleurl/src/gurl.h"
On 2012/05/31 23:58:08, tfarina wrote:
> nit: please, forward declare this instead.

Done.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:17: class BubbleGtk;
On 2012/05/31 23:58:08, tfarina wrote:
> remove this forward declaration.

Done.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:30: // displayed in a popup
that is positioned relative the an anchor element.
On 2012/05/31 23:58:08, tfarina wrote:
> nit: relative to an anchor element? can this comment be improved?

I guess this part of the comment is not really necessary. Are you fine with
deleting it?

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:46:
WebsiteSettingsPopupGtk(gfx::NativeWindow parent,
On 2012/05/31 20:51:51, Ben Goodger (Google) wrote:
> can your ctor be private then?

Done.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:52: // WebsiteSettingsUI
implementations.
On 2012/05/31 23:58:08, tfarina wrote:
> I bet you can also move these overrides and the BubbleDelegateGtk one to the
> private section.

The WebsiteSettingsUI methods are the public API used by the WebsiteSettings
class (presenter).

The BubbleClosing method is the callback for the BubbeGTK to notify the Popup
that it is closing.

That's why all of them need to be public.

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:132:
scoped_ptr<WebsiteSettings> presenter_;
On 2012/05/31 23:58:08, tfarina wrote:
> include base/memory/scoped_ptr.h for this.

Done.

[tfarina, 2012-06-01 21:20:26]

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/9001/chrome/browser/ui/g...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:30: // displayed in a popup
that is positioned relative the an anchor element.
On 2012/06/01 10:06:06, markusheintz_ wrote:
> On 2012/05/31 23:58:08, tfarina wrote:
> > nit: relative to an anchor element? can this comment be improved?
> 
> I guess this part of the comment is not really necessary. Are you fine with
> deleting it?
Yes, much cleaner now. Thanks.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/browser_window_gtk.cc:1143: GetNativeHandle(),
nit: like I said in the other comment, you could save some vertical lines,
writting the parameters until you hit the 80 chars limit. ;)

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:7: #include
"base/basictypes.h"
include this for DISALLOW_ macro and compiler_specific for OVERRIDE in the
header file, and remove them from here.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:32: #include
"googleurl/src/gurl.h"
sort this.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:75: void
WebsiteSettingsPopupGtk::Show(gfx::NativeWindow parent,
the order of the implementation of this function is also fine, but I tend to put
the at the end of the file.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:80: new
WebsiteSettingsPopupGtk(parent,
since this is your code and I'm not the OWNER, this style is fine by me. But my
preference, and Peter's one (pkasting) is to write it like this:

new WebsiteSettingsPopupGtk(parent, profile, tab_contents_wrapper, url, ssl);

to save as much as possible vertical lines.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:131: presenter_.reset(new
WebsiteSettings(
so you will have to do this in views and cocoa. :/

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:146: presenter_.reset(NULL);
you don't need to pass NULL here, scoped_ptr::reset has it as default parameter
(C* p = NULL). Yeah, I know this is forbidden by the style guide (I mean, the
use of default argument).

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:540: ContentSetting(value));
nit: while you are, fix the indentation here.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:9: #include
"chrome/browser/ui/website_settings/website_settings_ui.h"
nit: group with the other includes below. This is a header file, I'm pretty sure
we don't use this order in header files.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:17: class BubbleGtk;
nit: forgot to remove?

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:19: namespace content {
please, move this after the forward declarations below.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:23: class GtkThemeService;
group these forward declarations with the Browser one above.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:46: virtual void
SetCookieInfo(const CookieInfoList& cookie_info_list) OVERRIDE;
I'm away from my Linux desktop, but I think I can prove that you can move these
to private section, with a simple test program. ;)
An exercise that I'll do in the weekend.

If I'm not wrong, this might work, but I'll verify on the weekend:

class FooInterface {
 public:
  virtual void Show() = 0;

 protected:
  virtual ~FooInterface();
};

class FooImpl : public FooInterface {
 private:
  virtual void Show() OVERRIDE;
};

FooInterface* interface = new FooImpl();
interface->Show();

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/website_settings/OWNERS (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/website_settings/OWNERS:1: set noparent
There was a thread on chromium-dev about this. I need to check, I'm not sure if
we should or not keep using "set noparent" anymore.

[markusheintz_, 2012-06-02 21:22:32]

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/gtk/browser_window_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/browser_window_gtk.cc:1143: GetNativeHandle(),
On 2012/06/01 21:20:26, tfarina wrote:
> nit: like I said in the other comment, you could save some vertical lines,
> writting the parameters until you hit the 80 chars limit. ;)

I don't have any strong feelings about the style in this case.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:7: #include
"base/basictypes.h"
On 2012/06/01 21:20:26, tfarina wrote:
> include this for DISALLOW_ macro and compiler_specific for OVERRIDE in the
> header file, and remove them from here.

Done.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:32: #include
"googleurl/src/gurl.h"
On 2012/06/01 21:20:26, tfarina wrote:
> sort this.

Done.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:75: void
WebsiteSettingsPopupGtk::Show(gfx::NativeWindow parent,
On 2012/06/01 21:20:26, tfarina wrote:
> the order of the implementation of this function is also fine, but I tend to
put
> the at the end of the file.

I'd prefere to have the same order as in the header file. If you don't mind.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:80: new
WebsiteSettingsPopupGtk(parent,
On 2012/06/01 21:20:26, tfarina wrote:
> since this is your code and I'm not the OWNER, this style is fine by me. But
my
> preference, and Peter's one (pkasting) is to write it like this:
> 
> new WebsiteSettingsPopupGtk(parent, profile, tab_contents_wrapper, url, ssl);
> 
> to save as much as possible vertical lines.

Cool. This fits in a single line. Great.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:131: presenter_.reset(new
WebsiteSettings(
On 2012/06/01 21:20:26, tfarina wrote:
> so you will have to do this in views and cocoa. :/

I instantiated the WebsiteSettings class in the platform specific show method in
the old version. This would have also required to do this for Views and cocoa
too.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:146: presenter_.reset(NULL);
On 2012/06/01 21:20:26, tfarina wrote:
> you don't need to pass NULL here, scoped_ptr::reset has it as default
parameter
> (C* p = NULL). Yeah, I know this is forbidden by the style guide (I mean, the
> use of default argument).

Done.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:540: ContentSetting(value));
On 2012/06/01 21:20:26, tfarina wrote:
> nit: while you are, fix the indentation here.

Done.  Same above.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:9: #include
"chrome/browser/ui/website_settings/website_settings_ui.h"
On 2012/06/01 21:20:26, tfarina wrote:
> nit: group with the other includes below. This is a header file, I'm pretty
sure
> we don't use this order in header files.

Done.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:17: class BubbleGtk;
On 2012/06/01 21:20:26, tfarina wrote:
> nit: forgot to remove?
uups sorry. Removed.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:19: namespace content {
On 2012/06/01 21:20:26, tfarina wrote:
> please, move this after the forward declarations below.

Done.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:23: class GtkThemeService;
On 2012/06/01 21:20:26, tfarina wrote:
> group these forward declarations with the Browser one above.

Done.

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:46: virtual void
SetCookieInfo(const CookieInfoList& cookie_info_list) OVERRIDE;
On 2012/06/01 21:20:26, tfarina wrote:
> I'm away from my Linux desktop, but I think I can prove that you can move
these
> to private section, with a simple test program. ;)
> An exercise that I'll do in the weekend.
> 
> If I'm not wrong, this might work, but I'll verify on the weekend:
> 
> class FooInterface {
>  public:
>   virtual void Show() = 0;
> 
>  protected:
>   virtual ~FooInterface();
> };
> 
> class FooImpl : public FooInterface {
>  private:
>   virtual void Show() OVERRIDE;
> };
> 
> FooInterface* interface = new FooImpl();
> interface->Show();

hm ... why should I make these methods private here? After all they are part of
the public interface. Being a C++ Padawan I don't understand the motivation for
this change (yet).

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/website_settings/OWNERS (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/website_settings/OWNERS:1: set noparent
On 2012/06/01 21:20:26, tfarina wrote:
> There was a thread on chromium-dev about this. I need to check, I'm not sure
if
> we should or not keep using "set noparent" anymore.

I vaguely remember a thread but not the result. I guess I can easily add it
later.

Personally I find the set noparent very usefull since it enforces OWNER LGTM. In
emergency situation people can always commit as TBR.

[tfarina, 2012-06-03 16:09:04]

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/12002/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:46: virtual void
SetCookieInfo(const CookieInfoList& cookie_info_list) OVERRIDE;
On 2012/06/02 21:22:33, markusheintz_ wrote:
> On 2012/06/01 21:20:26, tfarina wrote:
> > I'm away from my Linux desktop, but I think I can prove that you can move
> these
> > to private section, with a simple test program. ;)
> > An exercise that I'll do in the weekend.
> > 
> > If I'm not wrong, this might work, but I'll verify on the weekend:
> > 
> > class FooInterface {
> >  public:
> >   virtual void Show() = 0;
> > 
> >  protected:
> >   virtual ~FooInterface();
> > };
> > 
> > class FooImpl : public FooInterface {
> >  private:
> >   virtual void Show() OVERRIDE;
> > };
> > 
> > FooInterface* interface = new FooImpl();
> > interface->Show();
> 
> hm ... why should I make these methods private here? After all they are part
of
> the public interface. Being a C++ Padawan I don't understand the motivation
for
> this change (yet).
>  
The reason I suggested this is:

- WebsiteSettingsPopupGtk is a delegate for BubbleGtk. So BubbleGtk will call
BubbleClosing through its BubbleDelegateGtk interface, which I showed an example
above.
- Same thing for WebsiteSettingsUI.
- While these overridden methods might be part of WebsiteSettingsPopupGtk public
interface, I see they aren't intended to be called by the consumers of
WebsiteSettingsPopupGtk.

You aren't going to do this, for example:
WebsiteSettingsPopupGtk* popup = new WebsiteSettingsPopupGtk (...);
popup->SetPermissingInfo(...);

But that is just my view, I'm fine with whatever you think it's more
appropriate.

[tfarina, 2012-06-03 16:12:19]

https://chromiumcodereview.appspot.com/10440092/diff/20001/chrome/browser/ui/...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/20001/chrome/browser/ui/...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:7: #include
"base/compiler_specific.h"
nit: remove this, already included in header file.

[tfarina, 2012-06-03 16:22:24]

https://chromiumcodereview.appspot.com/10440092/diff/20001/chrome/browser/ui/...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (right):

https://chromiumcodereview.appspot.com/10440092/diff/20001/chrome/browser/ui/...
chrome/browser/ui/website_settings/website_settings_unittest.cc:112:
website_settings_.reset(NULL);
nit: website_settings_.reset();

https://chromiumcodereview.appspot.com/10440092/diff/20001/chrome/browser/ui/...
chrome/browser/ui/website_settings/website_settings_unittest.cc:145: // The UI
is owned by |web_site_settings_|.
nit: Is this comment still valid?

[Ben Goodger (Google), 2012-06-04 15:24:53]

LGTM modulo thiago's comments.

[markusheintz_, 2012-06-04 16:33:54]

http://codereview.chromium.org/10440092/diff/12002/chrome/browser/ui/gtk/webs...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

http://codereview.chromium.org/10440092/diff/12002/chrome/browser/ui/gtk/webs...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:46: virtual void
SetCookieInfo(const CookieInfoList& cookie_info_list) OVERRIDE;
On 2012/06/03 16:09:04, tfarina wrote:
> On 2012/06/02 21:22:33, markusheintz_ wrote:
> > On 2012/06/01 21:20:26, tfarina wrote:
> > > I'm away from my Linux desktop, but I think I can prove that you can move
> > these
> > > to private section, with a simple test program. ;)
> > > An exercise that I'll do in the weekend.
> > > 
> > > If I'm not wrong, this might work, but I'll verify on the weekend:
> > > 
> > > class FooInterface {
> > >  public:
> > >   virtual void Show() = 0;
> > > 
> > >  protected:
> > >   virtual ~FooInterface();
> > > };
> > > 
> > > class FooImpl : public FooInterface {
> > >  private:
> > >   virtual void Show() OVERRIDE;
> > > };
> > > 
> > > FooInterface* interface = new FooImpl();
> > > interface->Show();
> > 
> > hm ... why should I make these methods private here? After all they are part
> of
> > the public interface. Being a C++ Padawan I don't understand the motivation
> for
> > this change (yet).
> >  
> The reason I suggested this is:
> 
> - WebsiteSettingsPopupGtk is a delegate for BubbleGtk. So BubbleGtk will call
> BubbleClosing through its BubbleDelegateGtk interface, which I showed an
example
> above.
> - Same thing for WebsiteSettingsUI.
> - While these overridden methods might be part of WebsiteSettingsPopupGtk
public
> interface, I see they aren't intended to be called by the consumers of
> WebsiteSettingsPopupGtk.
> 
> You aren't going to do this, for example:
> WebsiteSettingsPopupGtk* popup = new WebsiteSettingsPopupGtk (...);
> popup->SetPermissingInfo(...);
> 
> But that is just my view, I'm fine with whatever you think it's more
> appropriate.

You are right. Thanks a lot for the long description !!!

I made these methods private.

http://codereview.chromium.org/10440092/diff/20001/chrome/browser/ui/gtk/webs...
File chrome/browser/ui/gtk/website_settings_popup_gtk.cc (right):

http://codereview.chromium.org/10440092/diff/20001/chrome/browser/ui/gtk/webs...
chrome/browser/ui/gtk/website_settings_popup_gtk.cc:7: #include
"base/compiler_specific.h"
On 2012/06/03 16:12:19, tfarina wrote:
> nit: remove this, already included in header file.

Done.

http://codereview.chromium.org/10440092/diff/20001/chrome/browser/ui/website_...
File chrome/browser/ui/website_settings/website_settings_unittest.cc (right):

http://codereview.chromium.org/10440092/diff/20001/chrome/browser/ui/website_...
chrome/browser/ui/website_settings/website_settings_unittest.cc:112:
website_settings_.reset(NULL);
On 2012/06/03 16:22:24, tfarina wrote:
> nit: website_settings_.reset();

Done.

http://codereview.chromium.org/10440092/diff/20001/chrome/browser/ui/website_...
chrome/browser/ui/website_settings/website_settings_unittest.cc:145: // The UI
is owned by |web_site_settings_|.
On 2012/06/03 16:22:24, tfarina wrote:
> nit: Is this comment still valid?

Sorry of course not. Removed.

[tfarina, 2012-06-04 20:06:13]

LGTM, with the following nit.

Thanks for the patience! ;)

http://codereview.chromium.org/10440092/diff/18006/chrome/browser/ui/gtk/webs...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

http://codereview.chromium.org/10440092/diff/18006/chrome/browser/ui/gtk/webs...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:13: #include
"chrome/browser/ui/gtk/bubble/bubble_gtk.h"
nit: still missing the include to compiler_specific.h

[markusheintz_, 2012-06-04 22:32:42]

Thanks a lot for the review.

http://codereview.chromium.org/10440092/diff/18006/chrome/browser/ui/gtk/webs...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

http://codereview.chromium.org/10440092/diff/18006/chrome/browser/ui/gtk/webs...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:13: #include
"chrome/browser/ui/gtk/bubble/bubble_gtk.h"
On 2012/06/04 20:06:13, tfarina wrote:
> nit: still missing the include to compiler_specific.h

Done.

[tfarina, 2012-06-04 22:33:50]

http://codereview.chromium.org/10440092/diff/30006/chrome/browser/ui/gtk/webs...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

http://codereview.chromium.org/10440092/diff/30006/chrome/browser/ui/gtk/webs...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:11: #include
"base/compiler_specific.h"
nit: sort this ;)

[commit-bot: I haz the power, 2012-06-04 22:34:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10440092/30006

[markusheintz_, 2012-06-04 22:39:47]

The difference between p and P ;-)

http://codereview.chromium.org/10440092/diff/30006/chrome/browser/ui/gtk/webs...
File chrome/browser/ui/gtk/website_settings_popup_gtk.h (right):

http://codereview.chromium.org/10440092/diff/30006/chrome/browser/ui/gtk/webs...
chrome/browser/ui/gtk/website_settings_popup_gtk.h:11: #include
"base/compiler_specific.h"
On 2012/06/04 22:33:50, tfarina wrote:
> nit: sort this ;)

noooooooo :) this can't be :-)

done.

[commit-bot: I haz the power, 2012-06-04 22:40:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/markusheintz@chromium.org/10440092/17004

[commit-bot: I haz the power, 2012-06-05 00:00:35]

Change committed as 140436

[tfarina, 2012-06-30 19:24:47]

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
File chrome/browser/ui/website_settings_ui.h (right):

https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
chrome/browser/ui/website_settings_ui.h:100: class CookieInfoList : public
std::vector<WebsiteSettingsUI::CookieInfo> {
On 2012/06/01 10:06:06, markusheintz_ wrote:
> On 2012/05/31 19:23:01, tfarina wrote:
> > unrelated: this is very uncommon. I think it's the same as:
> > 
> > typedef std::vector<..> CookieInfoList;
> 
> I copied this from the cookie monster. This is the same way how the CookieList
> is defined.
> 
I just found something about this in Item 7 of Effective STL from Scott Meyers.
And this is not good, because std::vector doesn't have a virtual destructor, so
inheriting from it is a no-no. We should fix this, both here and CookieList.

[markusheintz_, 2012-07-02 12:46:05]

Thanks for pointing this out and filing http://crbug.com/135335.

On 2012/06/30 19:24:47, tfarina wrote:
>
https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
> File chrome/browser/ui/website_settings_ui.h (right):
> 
>
https://chromiumcodereview.appspot.com/10440092/diff/1008/chrome/browser/ui/w...
> chrome/browser/ui/website_settings_ui.h:100: class CookieInfoList : public
> std::vector<WebsiteSettingsUI::CookieInfo> {
> On 2012/06/01 10:06:06, markusheintz_ wrote:
> > On 2012/05/31 19:23:01, tfarina wrote:
> > > unrelated: this is very uncommon. I think it's the same as:
> > > 
> > > typedef std::vector<..> CookieInfoList;
> > 
> > I copied this from the cookie monster. This is the same way how the
CookieList
> > is defined.
> > 
> I just found something about this in Item 7 of Effective STL from Scott
Meyers.
> And this is not good, because std::vector doesn't have a virtual destructor,
so
> inheriting from it is a no-no. We should fix this, both here and CookieList.