Moving notification types which are chrome specific to a new header file chrome_notification_type...

Moving notification types which are chrome specific to a new header file chrome_notification_types.h.
This file lives in chrome\common. The chrome specific notifications start from NOTIFICATION_CONTENT_END
which defines the end of the enum used by content to define notification types. The notificaton_type.h file
in content\common has been renamed to content_notification_types.h

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

[darin (slow to review), 2011-07-07 22:54:55]

http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:21: BROWSER_OPENED =
CHROME_NOTIFICATION_START,
This has the unfortunate side effect of importing all of these names into the
chrome:: namespace.  previously, they were scoped to NotificationType.  it seems
like we should probably employ a naming convention to help.

  chrome::NOTIFICATION_BROWSER_OPENED
  chrome::NOTIFICATION_BROWSER_WINDOW_READY

Then, I would do similar treatment for the ones defined in content/.  For
example, NotificationType::ALL becomes content::NOTIFICATION_ALL.

Another option might be to define a NotificationType struct in both chrome:: and
content:: namespaces, so we'd end up with
chrome::NotificationType::BROWSER_OPENED and content::NotificationType::ALL. 
However, that leaves a bad taste ;-)  It seems like too much.

I apologize as this is probably a fair bit of busy work to implement!

http://codereview.chromium.org/7327007/diff/1444/content/common/notification_...
File content/common/notification_service.h (right):

http://codereview.chromium.org/7327007/diff/1444/content/common/notification_...
content/common/notification_service.h:14: #include <vector>
nit: looks like this include is not needed.

[darin (slow to review), 2011-07-07 22:55:32]

Also, content/common/notification_type.h should probably be changed to
content/common/content_notification_types.h or even
content/common/content_notifications.h :-)  Maybe chrome_notifications.h too!

[Jói, 2011-07-08 15:15:07]

http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:21: BROWSER_OPENED =
CHROME_NOTIFICATION_START,
On 2011/07/07 22:54:56, darin wrote:
> This has the unfortunate side effect of importing all of these names into the
> chrome:: namespace.  previously, they were scoped to NotificationType.  it
seems
> like we should probably employ a naming convention to help.
> 
>   chrome::NOTIFICATION_BROWSER_OPENED
>   chrome::NOTIFICATION_BROWSER_WINDOW_READY
> 
> Then, I would do similar treatment for the ones defined in content/.  For
> example, NotificationType::ALL becomes content::NOTIFICATION_ALL.
> 
> Another option might be to define a NotificationType struct in both chrome::
and
> content:: namespaces, so we'd end up with
> chrome::NotificationType::BROWSER_OPENED and content::NotificationType::ALL. 
> However, that leaves a bad taste ;-)  It seems like too much.
> 
> I apologize as this is probably a fair bit of busy work to implement!

What about a second namespace around the anonymous enum, e.g.

namespace chrome {
namespace notify {

enum {
  ...
}

}
}

Would that be just as good, and perhaps easier to implement?  You'd get e.g.
chrome::notify::BROWSER_OPENED etc.

[darin (slow to review), 2011-07-08 16:20:05]

On Fri, Jul 8, 2011 at 8:15 AM, <joi@chromium.org> wrote:

>
> http://codereview.chromium.**org/7327007/diff/1444/chrome/**
>
common/chrome_notification_**types.h<http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notification_types.h>
> File chrome/common/chrome_**notification_types.h (right):
>
> http://codereview.chromium.**org/7327007/diff/1444/chrome/**
>
common/chrome_notification_**types.h#newcode21<http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notification_types.h#newcode21>
> chrome/common/chrome_**notification_types.h:21: BROWSER_OPENED =
> CHROME_NOTIFICATION_START,
> On 2011/07/07 22:54:56, darin wrote:
>
>> This has the unfortunate side effect of importing all of these names
>>
> into the
>
>> chrome:: namespace.  previously, they were scoped to NotificationType.
>>
>  it seems
>
>> like we should probably employ a naming convention to help.
>>
>
>    chrome::NOTIFICATION_BROWSER_**OPENED
>>   chrome::NOTIFICATION_BROWSER_**WINDOW_READY
>>
>
>  Then, I would do similar treatment for the ones defined in content/.
>>
> For
>
>> example, NotificationType::ALL becomes content::NOTIFICATION_ALL.
>>
>
>  Another option might be to define a NotificationType struct in both
>>
> chrome:: and
>
>> content:: namespaces, so we'd end up with
>> chrome::NotificationType::**BROWSER_OPENED and
>>
> content::NotificationType::**ALL.
>
>> However, that leaves a bad taste ;-)  It seems like too much.
>>
>
>  I apologize as this is probably a fair bit of busy work to implement!
>>
>
> What about a second namespace around the anonymous enum, e.g.
>
> namespace chrome {
> namespace notify {
>
> enum {
>  ...
> }
>
> }
> }
>
> Would that be just as good, and perhaps easier to implement?  You'd get
> e.g. chrome::notify::BROWSER_OPENED etc.


That does seem nice.
-Darin


>
>
>
http://codereview.chromium.**org/7327007/<http://codereview.chromium.org/7327...
>

[iyengar, 2011-07-08 16:41:43]

http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notific...
File chrome/common/chrome_notification_types.h (right):

http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:21: BROWSER_OPENED =
CHROME_NOTIFICATION_START,
On 2011/07/07 22:54:56, darin wrote:
> This has the unfortunate side effect of importing all of these names into the
> chrome:: namespace.  previously, they were scoped to NotificationType.  it
seems
> like we should probably employ a naming convention to help.
> 
>   chrome::NOTIFICATION_BROWSER_OPENED
>   chrome::NOTIFICATION_BROWSER_WINDOW_READY
> 
> Then, I would do similar treatment for the ones defined in content/.  For
> example, NotificationType::ALL becomes content::NOTIFICATION_ALL.
> 
> Another option might be to define a NotificationType struct in both chrome::
and
> content:: namespaces, so we'd end up with
> chrome::NotificationType::BROWSER_OPENED and content::NotificationType::ALL. 
> However, that leaves a bad taste ;-)  It seems like too much.
> 
> I apologize as this is probably a fair bit of busy work to implement!

Done.

http://codereview.chromium.org/7327007/diff/1444/chrome/common/chrome_notific...
chrome/common/chrome_notification_types.h:21: BROWSER_OPENED =
CHROME_NOTIFICATION_START,
On 2011/07/08 15:15:07, Jói wrote:
> On 2011/07/07 22:54:56, darin wrote:
> > This has the unfortunate side effect of importing all of these names into
the
> > chrome:: namespace.  previously, they were scoped to NotificationType.  it
> seems
> > like we should probably employ a naming convention to help.
> > 
> >   chrome::NOTIFICATION_BROWSER_OPENED
> >   chrome::NOTIFICATION_BROWSER_WINDOW_READY
> > 
> > Then, I would do similar treatment for the ones defined in content/.  For
> > example, NotificationType::ALL becomes content::NOTIFICATION_ALL.
> > 
> > Another option might be to define a NotificationType struct in both chrome::
> and
> > content:: namespaces, so we'd end up with
> > chrome::NotificationType::BROWSER_OPENED and content::NotificationType::ALL.

> > However, that leaves a bad taste ;-)  It seems like too much.
> > 
> > I apologize as this is probably a fair bit of busy work to implement!
> 
> What about a second namespace around the anonymous enum, e.g.
> 
> namespace chrome {
> namespace notify {
> 
> enum {
>   ...
> }
> 
> }
> }
> 
> Would that be just as good, and perhaps easier to implement?  You'd get e.g.
> chrome::notify::BROWSER_OPENED etc.
By the time I read this I had already done the donkey work to rename all
notifications to NOTIFICATION_ :)

[iyengar, 2011-07-08 16:43:32]

On 2011/07/07 22:55:32, darin wrote:
> Also, content/common/notification_type.h should probably be changed to
> content/common/content_notification_types.h or even
> content/common/content_notifications.h :-)  Maybe chrome_notifications.h too!

Done

[Jói, 2011-07-08 17:17:47]

Either way seems fine by me, I was mainly suggesting it as a simpler
way to change everything than having to go through and add
NOTIFICATION_ all over the place.

Cheers,
Jói


On Fri, Jul 8, 2011 at 12:43 PM,  <iyengar@google.com> wrote:
> On 2011/07/07 22:55:32, darin wrote:
>>
>> Also, content/common/notification_type.h should probably be changed to
>> content/common/content_notification_types.h or even
>> content/common/content_notifications.h :-)  Maybe chrome_notifications.h
>> too!
>
> Done
>
> http://codereview.chromium.org/7327007/
>

[darin (slow to review), 2011-07-09 04:07:43]

LGTM .. except i suspect you need to compile browsertests and fix bustage.

http://codereview.chromium.org/7327007/diff/7675/chrome/browser/accessibility...
File chrome/browser/accessibility/accessibility_win_browsertest.cc (right):

http://codereview.chromium.org/7327007/diff/7675/chrome/browser/accessibility...
chrome/browser/accessibility/accessibility_win_browsertest.cc:340:
chrome::RENDER_VIEW_HOST_ACCESSIBILITY_TREE_UPDATED);
nit: missing the NOTIFICATION_ prefix?

http://codereview.chromium.org/7327007/diff/7675/content/common/notification_...
File content/common/notification_service.h (right):

http://codereview.chromium.org/7327007/diff/7675/content/common/notification_...
content/common/notification_service.h:14: #include <vector>
nit: this include is not needed?