Move ExtensionWarningService and ExtensionsWarningSet to extensions.

Move ExtensionWarningService and ExtensionsWarningSet, which are used by webrequest api, to extensions. 

This cl resolve the dependencies of ExtensionsWarningSet of chrome strings:
- move the declarations of IDS_EXTENSION_WARNINGS_XXX to extensions_strings.grd;
- replace the usage of "IDS_PRODUCT_NAME" by creating GetProductName() function in ExtensionsClient and its subclasses.


BUG=352293
Committed: https://crrev.com/c7e55208cda02430540652a0973071b54a8edb1e
Cr-Commit-Position: refs/heads/master@{#292378}

[Xi Han, 2014-08-25 17:45:58]

Patchset #1 (id:1) has been deleted

[Xi Han, 2014-08-25 17:56:24]

hanxi@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2014-08-25 18:48:31]

lgtm

[Xi Han, 2014-08-25 18:53:18]

hanxi@chromium.org changed reviewers:
+ jamescook@chromium.org

[Xi Han, 2014-08-25 18:53:19]

+ jamescook@chromium.org: Most the changes are made based on our discussions
last time. Could you please take a look at it? Thanks!

[James Cook, 2014-08-25 20:58:12]

A few comments / questions.

https://codereview.chromium.org/503033002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/503033002/diff/20001/chrome/common/extensions...
chrome/common/extensions/chrome_extensions_client.cc:104: return "Chrome";
I don't think you want to do this. You probably want to look up IDS_PRODUCT_NAME
or something similar. For example, the open-source build should have the product
name "Chromium".

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_service.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:21: class NotificationDetails;
nit: forward declare BrowserContext, even though some other header above
probably declares it

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:33: class ExtensionWarningService
: public ExtensionRegistryObserver,
Did you consider renaming this class to just "WarningService", since it could be
referred to as extensions::WarningService?  See battre's comment above. Now
would be a good time to do that, since you already have to touch every file that
includes this header.

I would get Fady's opinion, though.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:63: // Notifies the
ExtensionWarningService of browser_context |profile_id| that
I might rename profile_id to browser_context_id.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_service_unittest.cc (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service_unittest.cc:36: class
ExtensionWarningServiceTest : public ExtensionsTest {
You could just say "typedef ExtensionsTest ExtensionWarningServiceTest;"

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_set.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_set.h:27: class ExtensionWarning {
It would be a little weird to have a class named just "extensions::Warning" but
that might be OK.

extensions::WarningSet works for me.

Again, I'd ask Fady for his opinion. In general I'm in favor of shorter names
when the code lives in src/extensions and the namespace is extensions::

https://codereview.chromium.org/503033002/diff/20001/extensions/common/extens...
File extensions/common/extensions_client.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/common/extens...
extensions/common/extensions_client.h:48: // Returns the application name.
I would provide examples, 'for example, "Chrome" or "Chromium".'

https://codereview.chromium.org/503033002/diff/20001/extensions/extensions_st...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/extensions_st...
extensions/extensions_strings.grd:298: <!-- Global error messgaes for
extensions. Please keep alphabetized. -->
messgaes -> messages

https://codereview.chromium.org/503033002/diff/20001/extensions/shell/common/...
File extensions/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/shell/common/...
extensions/shell/common/shell_extensions_client.cc:108: return "app_shell";
This is fine.

[Fady Samuel, 2014-08-25 21:15:38]

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_service.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:33: class ExtensionWarningService
: public ExtensionRegistryObserver,
On 2014/08/25 20:58:11, James Cook wrote:
> Did you consider renaming this class to just "WarningService", since it could
be
> referred to as extensions::WarningService?  See battre's comment above. Now
> would be a good time to do that, since you already have to touch every file
that
> includes this header.
> 
> I would get Fady's opinion, though.

I agree, it makes sense to do the rename.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:83: content::BrowserContext*
browser_context_;
Does it make sense to make this const?

content::BrowserContext* const browser_context_;

This makes it clear that this member variable will not change in the lifetime of
this object.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_set.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_set.h:27: class ExtensionWarning {
On 2014/08/25 20:58:12, James Cook wrote:
> It would be a little weird to have a class named just "extensions::Warning"
but
> that might be OK.
> 
> extensions::WarningSet works for me.
> 
> Again, I'd ask Fady for his opinion. In general I'm in favor of shorter names
> when the code lives in src/extensions and the namespace is extensions::


+1 to extensions::WarningSet.

[Xi Han, 2014-08-26 17:53:05]

Patchset #2 (id:40001) has been deleted

[Xi Han, 2014-08-26 18:17:55]

Patchset #3 (id:80001) has been deleted

[Xi Han, 2014-08-26 18:21:41]

The following classes are renamed:
-ExtensionWarning will become Warning;
-ExtensionWarningSet will become WarningSet;
-ExtensionWarningService will become WarningService;
-ExtensionWarningServiceTest will become WarningServiceTest.
Please take another look. Thanks!

https://codereview.chromium.org/503033002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/503033002/diff/20001/chrome/common/extensions...
chrome/common/extensions/chrome_extensions_client.cc:104: return "Chrome";
On 2014/08/25 20:58:11, James Cook wrote:
> I don't think you want to do this. You probably want to look up
IDS_PRODUCT_NAME
> or something similar. For example, the open-source build should have the
product
> name "Chromium".

Done.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_service.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:21: class NotificationDetails;
On 2014/08/25 20:58:11, James Cook wrote:
> nit: forward declare BrowserContext, even though some other header above
> probably declares it

Done.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:63: // Notifies the
ExtensionWarningService of browser_context |profile_id| that
On 2014/08/25 20:58:11, James Cook wrote:
> I might rename profile_id to browser_context_id.

Done.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service.h:83: content::BrowserContext*
browser_context_;
On 2014/08/25 21:15:38, Fady Samuel wrote:
> Does it make sense to make this const?
> 
> content::BrowserContext* const browser_context_;
> 
> This makes it clear that this member variable will not change in the lifetime
of
> this object.

Done.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_service_unittest.cc (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_service_unittest.cc:36: class
ExtensionWarningServiceTest : public ExtensionsTest {
On 2014/08/25 20:58:11, James Cook wrote:
> You could just say "typedef ExtensionsTest ExtensionWarningServiceTest;"

Done.

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
File extensions/browser/extension_warning_set.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/browser/exten...
extensions/browser/extension_warning_set.h:27: class ExtensionWarning {
On 2014/08/25 21:15:38, Fady Samuel wrote:
> On 2014/08/25 20:58:12, James Cook wrote:
> > It would be a little weird to have a class named just "extensions::Warning"
> but
> > that might be OK.
> > 
> > extensions::WarningSet works for me.
> > 
> > Again, I'd ask Fady for his opinion. In general I'm in favor of shorter
names
> > when the code lives in src/extensions and the namespace is extensions::
> 
> 
> +1 to extensions::WarningSet.
> 

Since an ExtensionWarningSet is defined at the end of this file, I will rename
in the following way:
-ExtensionWarning will become Warning &
-ExtensionWarningSet will become WarningSet.

https://codereview.chromium.org/503033002/diff/20001/extensions/common/extens...
File extensions/common/extensions_client.h (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/common/extens...
extensions/common/extensions_client.h:48: // Returns the application name.
On 2014/08/25 20:58:12, James Cook wrote:
> I would provide examples, 'for example, "Chrome" or "Chromium".'
> 

Done.

https://codereview.chromium.org/503033002/diff/20001/extensions/extensions_st...
File extensions/extensions_strings.grd (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/extensions_st...
extensions/extensions_strings.grd:298: <!-- Global error messgaes for
extensions. Please keep alphabetized. -->
On 2014/08/25 20:58:12, James Cook wrote:
> messgaes -> messages

Done.

https://codereview.chromium.org/503033002/diff/20001/extensions/shell/common/...
File extensions/shell/common/shell_extensions_client.cc (right):

https://codereview.chromium.org/503033002/diff/20001/extensions/shell/common/...
extensions/shell/common/shell_extensions_client.cc:108: return "app_shell";
On 2014/08/25 20:58:12, James Cook wrote:
> This is fine.

:)

[James Cook, 2014-08-26 22:01:32]

LGTM with (many) nits

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:36: using
extensions::Warning;
nit: I either wouldn't do this, or add "using extensions::WarningSet". It seems
a bit odd to do one but not the other.

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:61: #include
"extensions/browser/warning_set.h"
nit: Should this also include warning_service.h?

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:386:
extension_warning_service_.reset(new WarningService(profile_));
nit: I would rename the variable to just warning_service_

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_warning_badge_service_unittest.cc
(right):

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_warning_badge_service_unittest.cc:12:
#include "extensions/browser/warning_set.h"
nit: #include warning_service.h also

https://codereview.chromium.org/503033002/diff/100001/chrome/common/extension...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/503033002/diff/100001/chrome/common/extension...
chrome/common/extensions/chrome_extensions_client.cc:36: #include
"grit/chromium_strings.h"
nit: I think we're using chrome/grit/chromium_strings.h now

https://codereview.chromium.org/503033002/diff/100001/extensions/browser/warn...
File extensions/browser/warning_service.cc (right):

https://codereview.chromium.org/503033002/diff/100001/extensions/browser/warn...
extensions/browser/warning_service.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
nit: Try uploading this file with "git cl upload --similarity=30" or lower. It
might figure out that this is a rename instead of a new file, which will
preserve the history.

https://codereview.chromium.org/503033002/diff/100001/extensions/browser/warn...
extensions/browser/warning_service.cc:98: extensions::WarningService*
warning_service =
nit: "extensions::" not needed

https://codereview.chromium.org/503033002/diff/100001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/503033002/diff/100001/extensions/extensions.g...
extensions/extensions.gyp:899: 'browser/warning_service_unittest.cc',
alphabetize

[Xi Han, 2014-08-27 13:24:36]

Patchset #2 (id:60001) has been deleted

[Xi Han, 2014-08-27 13:32:55]

hanxi@chromium.org changed reviewers:
+ rockot@chromium.org

[Xi Han, 2014-08-27 13:32:55]

+rockot@chromium.org: Please review all changes. Thanks!

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
(right):

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_api_helpers.cc:36: using
extensions::Warning;
On 2014/08/26 22:01:31, James Cook wrote:
> nit: I either wouldn't do this, or add "using extensions::WarningSet". It
seems
> a bit odd to do one but not the other.

Removed.

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_system_impl.cc (right):

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:61: #include
"extensions/browser/warning_set.h"
On 2014/08/26 22:01:31, James Cook wrote:
> nit: Should this also include warning_service.h?

Added.

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_system_impl.cc:386:
extension_warning_service_.reset(new WarningService(profile_));
On 2014/08/26 22:01:31, James Cook wrote:
> nit: I would rename the variable to just warning_service_

Done.

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/extension_warning_badge_service_unittest.cc
(right):

https://codereview.chromium.org/503033002/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/extension_warning_badge_service_unittest.cc:12:
#include "extensions/browser/warning_set.h"
On 2014/08/26 22:01:31, James Cook wrote:
> nit: #include warning_service.h also

Done.

https://codereview.chromium.org/503033002/diff/100001/chrome/common/extension...
File chrome/common/extensions/chrome_extensions_client.cc (right):

https://codereview.chromium.org/503033002/diff/100001/chrome/common/extension...
chrome/common/extensions/chrome_extensions_client.cc:36: #include
"grit/chromium_strings.h"
On 2014/08/26 22:01:31, James Cook wrote:
> nit: I think we're using chrome/grit/chromium_strings.h now

Great catch, thanks!

https://codereview.chromium.org/503033002/diff/100001/extensions/browser/warn...
File extensions/browser/warning_service.cc (right):

https://codereview.chromium.org/503033002/diff/100001/extensions/browser/warn...
extensions/browser/warning_service.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/08/26 22:01:31, James Cook wrote:
> nit: Try uploading this file with "git cl upload --similarity=30" or lower. It
> might figure out that this is a rename instead of a new file, which will
> preserve the history.

Great suggestion, I will do it for next cl upload.

https://codereview.chromium.org/503033002/diff/100001/extensions/browser/warn...
extensions/browser/warning_service.cc:98: extensions::WarningService*
warning_service =
On 2014/08/26 22:01:31, James Cook wrote:
> nit: "extensions::" not needed

Done.

https://codereview.chromium.org/503033002/diff/100001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/503033002/diff/100001/extensions/extensions.g...
extensions/extensions.gyp:899: 'browser/warning_service_unittest.cc',
On 2014/08/26 22:01:31, James Cook wrote:
> alphabetize

Done.

[Ken Rockot(use gerrit already), 2014-08-27 17:49:12]

lgtm

[Xi Han, 2014-08-27 17:50:30]

hanxi@chromium.org changed reviewers:
+ kalman@chromium.org

[Xi Han, 2014-08-27 17:50:31]

kalman@chromium.org: Please review changes in:
-chrome/browser/ui/webui/extensions/extension_settings_handler.h
-chrome/browser/ui/webui/extensions/extension_settings_handler.cc
Thanks!

[not at google - send to devlin, 2014-08-27 19:23:57]

lgtm

[Xi Han, 2014-08-27 19:42:54]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-27 19:43:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/503033002/120001

[Xi Han, 2014-08-27 19:50:16]

The CQ bit was unchecked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-27 19:50:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[Xi Han, 2014-08-27 20:15:20]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-27 20:16:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/503033002/140001

[Xi Han, 2014-08-27 20:22:34]

The CQ bit was unchecked by hanxi@chromium.org

[Xi Han, 2014-08-27 21:21:26]

Patchset #5 (id:160001) has been deleted

[Xi Han, 2014-08-27 21:33:00]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-27 21:33:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/503033002/200001

[commit-bot: I haz the power, 2014-08-27 22:28:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-27 22:36:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-27 22:36:53]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Xi Han, 2014-08-28 13:04:31]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-28 13:05:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/503033002/220001

[commit-bot: I haz the power, 2014-08-28 14:13:33]

Committed patchset #7 (id:220001) as 35643f423cc34536735fb2ca6412a4c2438a3f82

[commit-bot: I haz the power, 2014-09-10 02:59:26]

Patchset 7 (id:??) landed as
https://crrev.com/c7e55208cda02430540652a0973071b54a8edb1e
Cr-Commit-Position: refs/heads/master@{#292378}