status_icons: Factor out status icon observer into its own header file.

status_icons: Factor out status icon observer into its own header file.

R=atwilson@chromium.org
TBR=pkasting@chromium.org

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

[Andrew T Wilson (Slow), 2012-05-22 00:53:56]

LGTM.

One question - what's the impetus for this change? Is there a style rule that
says that having a nested observer interface is illegal (it has to live in its
own file?)

BTW, nobody uses the onclick mechanism for status icons as far as I can tell, so
we could potentially remove it. I don't have a strong opinion either way.

http://codereview.chromium.org/10408039/diff/6001/chrome/browser/status_icons...
File chrome/browser/status_icons/status_icon_observer.h (right):

http://codereview.chromium.org/10408039/diff/6001/chrome/browser/status_icons...
chrome/browser/status_icons/status_icon_observer.h:14: // right clicks on the
icon to display the context menu, OnClicked will not
nit: OnClicked->OnStatusIconClicked()

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

On 2012/05/22 00:53:56, Andrew T Wilson wrote:
> One question - what's the impetus for this change?
The reason is that it allow us to forward declare StatusIconObserver instead of
having to include status_icon.h. Also it doesn't pollute the header files with a
bunch of unnecessary includes. sky, ben and jhawkins has been advocating this
throughout.

And also jam:
http://codereview.chromium.org/8726023/diff/1/chrome/browser/importer/externa...

I have done this for many listeners/observers/delegates in ui/views/ and we have
been pretty consistent in this way under ui/ directory. A good example of
following this pattern is ui/app_list.

> Is there a style rule that
> says that having a nested observer interface is illegal (it has to live in its
> own file?)
> 
The style guide doesn't say it's illegal, but I think the preference is to have
observers/listeners/delegates into its own header files.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Nested_Classes

http://codereview.chromium.org/10408039/diff/6001/chrome/browser/status_icons...
File chrome/browser/status_icons/status_icon_observer.h (right):

http://codereview.chromium.org/10408039/diff/6001/chrome/browser/status_icons...
chrome/browser/status_icons/status_icon_observer.h:14: // right clicks on the
icon to display the context menu, OnClicked will not
On 2012/05/22 00:53:56, Andrew T Wilson wrote:
> nit: OnClicked->OnStatusIconClicked()

Done.

[tfarina, 2012-05-23 15:01:06]

ash/ and content/ are also good examples of this practice.

On Wed, May 23, 2012 at 11:58 AM,  <tfarina@chromium.org> wrote:
> On 2012/05/22 00:53:56, Andrew T Wilson wrote:
>>
>> One question - what's the impetus for this change?
>
> The reason is that it allow us to forward declare StatusIconObserver instead
> of
> having to include status_icon.h. Also it doesn't pollute the header files
> with a
> bunch of unnecessary includes. sky, ben and jhawkins has been advocating
> this
> throughout.
>
> And also jam:
>
http://codereview.chromium.org/8726023/diff/1/chrome/browser/importer/externa...
>
> I have done this for many listeners/observers/delegates in ui/views/ and we
> have
> been pretty consistent in this way under ui/ directory. A good example of
> following this pattern is ui/app_list.
>
>
>> Is there a style rule that
>> says that having a nested observer interface is illegal (it has to live in
>> its
>> own file?)
>
>
> The style guide doesn't say it's illegal, but I think the preference is to
> have
> observers/listeners/delegates into its own header files.
>
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Nested_Classes
>
>
>
>
http://codereview.chromium.org/10408039/diff/6001/chrome/browser/status_icons...
> File chrome/browser/status_icons/status_icon_observer.h (right):
>
>
http://codereview.chromium.org/10408039/diff/6001/chrome/browser/status_icons...
> chrome/browser/status_icons/status_icon_observer.h:14: // right clicks
> on the icon to display the context menu, OnClicked will not
> On 2012/05/22 00:53:56, Andrew T Wilson wrote:
>>
>> nit: OnClicked->OnStatusIconClicked()
>
>
> Done.
>
> http://codereview.chromium.org/10408039/



-- 
Thiago