Remove dependency of chrome in WebRequestPermissions.

Move kWebstoreAppId constant to//extensions.


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

[Xi Han, 2014-08-27 21:41:51]

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

[Fady Samuel, 2014-08-27 21:49:18]

https://codereview.chromium.org/515563003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/web_request/web_request_permissions.cc
(right):

https://codereview.chromium.org/515563003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:24:
extensions::WebRequestPermissionsDelegate* GetWebRequestPermissionsDelegate() {
This is VERY bad. You're creating a new delegate every time you call
GetWebRequestPermissionsDelegate().

I don't think you need a delegate here at all. Could you simply hang this
directly off ExtensionsAPIClient? That's already a singleton.

https://codereview.chromium.org/515563003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:69:
GetWebRequestPermissionsDelegate()->IsUpdateURL(url));
Indent off.

https://codereview.chromium.org/515563003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/web_request/web_request_permissions.cc:100:
GetWebRequestPermissionsDelegate()->ContainsWebStoreAppId(
Indent off.

[Xi Han, 2014-08-28 17:47:17]

Patchset #3 (id:40001) has been deleted

[Xi Han, 2014-08-28 17:48:23]

I have removed all the unnecessary delegates, PTAL. Thanks!

[Fady Samuel, 2014-08-28 17:53:36]

LGTM from a technical perspective, but I'm not a huge fan of the naming but I
don't feel strongly enough to block this CL either. Please see how Ken or James
feel.

[Xi Han, 2014-08-28 17:55:53]

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

[Xi Han, 2014-08-28 17:55:53]

+rockot@chromium.org

[Ken Rockot(use gerrit already), 2014-08-29 07:00:23]

https://codereview.chromium.org/515563003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right):

https://codereview.chromium.org/515563003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/chrome_extensions_api_client.cc:97: bool
ChromeExtensionsAPIClient::MustNotBeModifiedURL(const GURL& url) {
Please wait on this. I need to dig up a patch I have in a local branch which
also refactored these extension_urls. The end result is that you won't need this
method at all on the API client, because the extension_urls functions are going
to move to //extensions.

So I would say, for this CL, just leave this code in its original place with the
dependency remaining on chrome. I will clean it up ASAP with the patch I already
have in progress.

https://codereview.chromium.org/515563003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/chrome_extensions_api_client.cc:107: return
extension_misc::kWebStoreAppId;
I think it's OK to just move this constant to //extensions/common/constants.h/cc
for now. No need to add the client interface!

[Xi Han, 2014-08-29 14:39:24]

https://codereview.chromium.org/515563003/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right):

https://codereview.chromium.org/515563003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/chrome_extensions_api_client.cc:97: bool
ChromeExtensionsAPIClient::MustNotBeModifiedURL(const GURL& url) {
On 2014/08/29 07:00:22, Ken Rockot wrote:
> Please wait on this. I need to dig up a patch I have in a local branch which
> also refactored these extension_urls. The end result is that you won't need
this
> method at all on the API client, because the extension_urls functions are
going
> to move to //extensions.
> 
> So I would say, for this CL, just leave this code in its original place with
the
> dependency remaining on chrome. I will clean it up ASAP with the patch I
already
> have in progress.

Cool, I will revert these changes and wait for your cl landed:)

https://codereview.chromium.org/515563003/diff/60001/chrome/browser/extension...
chrome/browser/extensions/api/chrome_extensions_api_client.cc:107: return
extension_misc::kWebStoreAppId;
On 2014/08/29 07:00:22, Ken Rockot wrote:
> I think it's OK to just move this constant to
//extensions/common/constants.h/cc
> for now. No need to add the client interface!

Done.

[Ken Rockot(use gerrit already), 2014-08-29 16:16:45]

LGTM but please update the description so that it makes more sense out of
context. I think it's fine to say simply "Move kWebstoreAppId constant to
//extensions."

[Xi Han, 2014-08-29 16:21:29]

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

[Xi Han, 2014-08-29 16:21:29]

sky@chromium.org: Please review changes in :
-chrome/browser/sync/
-chrome/browser/ui/

Thanks!

[Fady Samuel, 2014-08-29 19:39:47]

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_app_sorting.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_app_sorting.cc:12: #include
"chrome/common/extensions/extension_constants.h"
Is this still necessary?

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_app_sorting_unittest.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_app_sorting_unittest.cc:10: #include
"chrome/common/extensions/extension_constants.h"
Is this still necessary?

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:23:
#include "chrome/common/extensions/extension_constants.h"
Is this still necessary?

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_install_ui_browsertest.cc:20: #include
"chrome/common/extensions/extension_constants.h"
Is this still necessary?

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:19: #include
"chrome/common/extensions/extension_constants.h"
Do you need this?

[Xi Han, 2014-08-29 19:52:54]

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_app_sorting.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_app_sorting.cc:12: #include
"chrome/common/extensions/extension_constants.h"
On 2014/08/29 19:39:46, Fady Samuel wrote:
> Is this still necessary?

Yes, this one is needed.

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_app_sorting_unittest.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_app_sorting_unittest.cc:10: #include
"chrome/common/extensions/extension_constants.h"
On 2014/08/29 19:39:46, Fady Samuel wrote:
> Is this still necessary?

Removed.

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:23:
#include "chrome/common/extensions/extension_constants.h"
On 2014/08/29 19:39:46, Fady Samuel wrote:
> Is this still necessary?

Removed.

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/extension_install_ui_browsertest.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/extension...
chrome/browser/extensions/extension_install_ui_browsertest.cc:20: #include
"chrome/common/extensions/extension_constants.h"
On 2014/08/29 19:39:46, Fady Samuel wrote:
> Is this still necessary?

Removed.

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/515563003/diff/80001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:19: #include
"chrome/common/extensions/extension_constants.h"
On 2014/08/29 19:39:47, Fady Samuel wrote:
> Do you need this?

Removed.

[sky, 2014-09-02 16:42:08]

LGTM

[Xi Han, 2014-09-02 17:41:41]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-02 17:43:51]

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

[Xi Han, 2014-09-02 19:49:36]

Patchset #6 (id:120001) has been deleted

[Xi Han, 2014-09-02 20:23:07]

Patchset #6 (id:140001) has been deleted

[Xi Han, 2014-09-02 20:45:40]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-02 20:46:29]

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

[commit-bot: I haz the power, 2014-09-02 21:59:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[commit-bot: I haz the power, 2014-09-02 22:52:44]

Committed patchset #6 (id:160001) as 731006fde0ebfb5007e8e6eb3d229443ba963e30

[commit-bot: I haz the power, 2014-09-10 03:21:35]

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