Move declarative_webrequest: action, rules_registry

Move declarative_webrequest: action, rules_registry

This CL focusses on moving the following files:

webrequest_action.h/cc
webrequest_rule_registry.h/cc

from chrome/ to extensions/.

As part of the move, the following files need to move:

web_request_api_helpers.h/cc

and some minor re-factoring occurs (e.g.  moving a static function from web_request_api into web_request_api_helpers.

BUG=352293
TBR=darin@chromium.org

Committed: https://crrev.com/c66af526932c3861ae33bab809679c7c353ebe48
Cr-Commit-Position: refs/heads/master@{#295719}

[wjmaclean, 2014-09-16 15:46:19]

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

[wjmaclean, 2014-09-16 15:46:21]

rockot@ - please look at the extensions/ and chrome/*/extensions/ files

[wjmaclean, 2014-09-16 15:51:52]

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

[wjmaclean, 2014-09-16 15:51:53]

jamescook@chromium.org: Please review changes in 

extensions/browser/DEPS

[James Cook, 2014-09-16 16:06:33]

DEPS LGTM, however one drive-by comment:

I would not move extension_constants.h/cc to extensions/.  We already have
extensions/common/constants.h/cc for that sort of data.  I suggest you move just
the functions and constants you need.

You should talk with Ken about how much of the concept of "web store" should
live in src/extensions vs. src/chrome. In the past we have considered that a
chrome-specific implementation detail.  For example, you might be able to
delegate out the general concept of WebRequestPermissions "should show request".
 But I know Ken was looking at extension update/install, which might have
webstore dependencies, so you should chat with him.

[wjmaclean, 2014-09-16 16:15:07]

On 2014/09/16 16:06:33, James Cook wrote:
> DEPS LGTM, however one drive-by comment:
> 
> I would not move extension_constants.h/cc to extensions/.  We already have
> extensions/common/constants.h/cc for that sort of data.  I suggest you move
just
> the functions and constants you need.
> 
> You should talk with Ken about how much of the concept of "web store" should
> live in src/extensions vs. src/chrome. In the past we have considered that a
> chrome-specific implementation detail.  For example, you might be able to
> delegate out the general concept of WebRequestPermissions "should show
request".
>  But I know Ken was looking at extension update/install, which might have
> webstore dependencies, so you should chat with him.

Thanks James ... I'll do what you suggest w.r.t. the constants first (that will
presumably cut down the size of the CL somewhat).

Then I'll see what Ken wants to do with the WebStore stuff.

[Ken Rockot(use gerrit already), 2014-09-16 16:17:54]

On 2014/09/16 16:15:07, wjmaclean wrote:
> On 2014/09/16 16:06:33, James Cook wrote:
> > DEPS LGTM, however one drive-by comment:
> > 
> > I would not move extension_constants.h/cc to extensions/.  We already have
> > extensions/common/constants.h/cc for that sort of data.  I suggest you move
> just
> > the functions and constants you need.
> > 
> > You should talk with Ken about how much of the concept of "web store" should
> > live in src/extensions vs. src/chrome. In the past we have considered that a
> > chrome-specific implementation detail.  For example, you might be able to
> > delegate out the general concept of WebRequestPermissions "should show
> request".
> >  But I know Ken was looking at extension update/install, which might have
> > webstore dependencies, so you should chat with him.
> 
> Thanks James ... I'll do what you suggest w.r.t. the constants first (that
will
> presumably cut down the size of the CL somewhat).
> 
> Then I'll see what Ken wants to do with the WebStore stuff.

I actually have a patch which moves all of the webstore URL concepts (which are
not actually "constants" at all, anyway) to //extensions
in a reusable way which allows for embedder overrides. I haven't had a chance
to land it yet unfortunately.

Please let me dig it up and see if I can make your life easier for this patch.

[wjmaclean, 2014-09-16 16:57:15]

Sounds great! I'll revise the cl in the meantime to "reset" the
extensions_constants changes, in preparation for your CL.

Does your cl also cover extension_urls::IsBlacklistUpdateUrl(), or should I
move it to extensions/common/constant.h/cc ?

On Tue, Sep 16, 2014 at 12:17 PM, <rockot@chromium.org> wrote:

> On 2014/09/16 16:15:07, wjmaclean wrote:
>
>> On 2014/09/16 16:06:33, James Cook wrote:
>> > DEPS LGTM, however one drive-by comment:
>> >
>> > I would not move extension_constants.h/cc to extensions/.  We already
>> have
>> > extensions/common/constants.h/cc for that sort of data.  I suggest you
>> move
>> just
>> > the functions and constants you need.
>> >
>> > You should talk with Ken about how much of the concept of "web store"
>> should
>> > live in src/extensions vs. src/chrome. In the past we have considered
>> that a
>> > chrome-specific implementation detail.  For example, you might be able
>> to
>> > delegate out the general concept of WebRequestPermissions "should show
>> request".
>> >  But I know Ken was looking at extension update/install, which might
>> have
>> > webstore dependencies, so you should chat with him.
>>
>
>  Thanks James ... I'll do what you suggest w.r.t. the constants first (that
>>
> will
>
>> presumably cut down the size of the CL somewhat).
>>
>
>  Then I'll see what Ken wants to do with the WebStore stuff.
>>
>
> I actually have a patch which moves all of the webstore URL concepts
> (which are
> not actually "constants" at all, anyway) to //extensions
> in a reusable way which allows for embedder overrides. I haven't had a
> chance
> to land it yet unfortunately.
>
> Please let me dig it up and see if I can make your life easier for this
> patch.
>
> https://codereview.chromium.org/566823003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-09-16 16:58:45]

Yes, that will also be covered by my CL

On Tue, Sep 16, 2014 at 9:57 AM, W. James MacLean <wjmaclean@chromium.org>
wrote:

> Sounds great! I'll revise the cl in the meantime to "reset" the
> extensions_constants changes, in preparation for your CL.
>
> Does your cl also cover extension_urls::IsBlacklistUpdateUrl(), or should
> I move it to extensions/common/constant.h/cc ?
>
> On Tue, Sep 16, 2014 at 12:17 PM, <rockot@chromium.org> wrote:
>
>> On 2014/09/16 16:15:07, wjmaclean wrote:
>>
>>> On 2014/09/16 16:06:33, James Cook wrote:
>>> > DEPS LGTM, however one drive-by comment:
>>> >
>>> > I would not move extension_constants.h/cc to extensions/.  We already
>>> have
>>> > extensions/common/constants.h/cc for that sort of data.  I suggest
>>> you move
>>> just
>>> > the functions and constants you need.
>>> >
>>> > You should talk with Ken about how much of the concept of "web store"
>>> should
>>> > live in src/extensions vs. src/chrome. In the past we have considered
>>> that a
>>> > chrome-specific implementation detail.  For example, you might be able
>>> to
>>> > delegate out the general concept of WebRequestPermissions "should show
>>> request".
>>> >  But I know Ken was looking at extension update/install, which might
>>> have
>>> > webstore dependencies, so you should chat with him.
>>>
>>
>>  Thanks James ... I'll do what you suggest w.r.t. the constants first
>>> (that
>>>
>> will
>>
>>> presumably cut down the size of the CL somewhat).
>>>
>>
>>  Then I'll see what Ken wants to do with the WebStore stuff.
>>>
>>
>> I actually have a patch which moves all of the webstore URL concepts
>> (which are
>> not actually "constants" at all, anyway) to //extensions
>> in a reusable way which allows for embedder overrides. I haven't had a
>> chance
>> to land it yet unfortunately.
>>
>> Please let me dig it up and see if I can make your life easier for this
>> patch.
>>
>> https://codereview.chromium.org/566823003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2014-09-16 16:59:22]

On 2014/09/16 16:58:45, Ken Rockot wrote:
> Yes, that will also be covered by my CL
> 
> On Tue, Sep 16, 2014 at 9:57 AM, W. James MacLean
<mailto:wjmaclean@chromium.org>
> wrote:
> 
> > Sounds great! I'll revise the cl in the meantime to "reset" the
> > extensions_constants changes, in preparation for your CL.
> >
> > Does your cl also cover extension_urls::IsBlacklistUpdateUrl(), or should
> > I move it to extensions/common/constant.h/cc ?
> >
> > On Tue, Sep 16, 2014 at 12:17 PM, <mailto:rockot@chromium.org> wrote:
> >
> >> On 2014/09/16 16:15:07, wjmaclean wrote:
> >>
> >>> On 2014/09/16 16:06:33, James Cook wrote:
> >>> > DEPS LGTM, however one drive-by comment:
> >>> >
> >>> > I would not move extension_constants.h/cc to extensions/.  We already
> >>> have
> >>> > extensions/common/constants.h/cc for that sort of data.  I suggest
> >>> you move
> >>> just
> >>> > the functions and constants you need.
> >>> >
> >>> > You should talk with Ken about how much of the concept of "web store"
> >>> should
> >>> > live in src/extensions vs. src/chrome. In the past we have considered
> >>> that a
> >>> > chrome-specific implementation detail.  For example, you might be able
> >>> to
> >>> > delegate out the general concept of WebRequestPermissions "should show
> >>> request".
> >>> >  But I know Ken was looking at extension update/install, which might
> >>> have
> >>> > webstore dependencies, so you should chat with him.
> >>>
> >>
> >>  Thanks James ... I'll do what you suggest w.r.t. the constants first
> >>> (that
> >>>
> >> will
> >>
> >>> presumably cut down the size of the CL somewhat).
> >>>
> >>
> >>  Then I'll see what Ken wants to do with the WebStore stuff.
> >>>
> >>
> >> I actually have a patch which moves all of the webstore URL concepts
> >> (which are
> >> not actually "constants" at all, anyway) to //extensions
> >> in a reusable way which allows for embedder overrides. I haven't had a
> >> chance
> >> to land it yet unfortunately.
> >>
> >> Please let me dig it up and see if I can make your life easier for this
> >> patch.
> >>
> >> https://codereview.chromium.org/566823003/
> >>
> >
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yes, that will also be covered by my CL

[wjmaclean, 2014-09-16 17:30:43]

Revised (and much smaller!) patch that reverts the extensions_constants changes.

This currently violates DEPS, but I uploaded anyways since (1) the DEPS
violation will disappear when we rebase against Ken's CL, and (2) we can get
most of the review work done in parallel in the meantime.

[wjmaclean, 2014-09-16 17:32:10]

wjmaclean@chromium.org changed reviewers:
+ derat@chromium.org

[wjmaclean, 2014-09-16 17:32:11]

derat@chromium.org: Please review changes in 

chrome/browser/chromeos

This is a simple include file move, so let me know if you prefer I just TBR
this.

[Daniel Erat, 2014-09-16 17:42:11]

https://codereview.chromium.org/566823003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/566823003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:23: #include
"chrome/common/extensions/extension_constants.h"
i don't understand why this header got moved down in the list; it's no longer
sorted correctly. is the "For SetHardwareKeyboardLayoutForTesting" comment meant
to apply to this file? the header seems to be used for other things, like
extension_misc::kBrailleImeExtensionId

[wjmaclean, 2014-09-16 17:52:00]

https://codereview.chromium.org/566823003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/input_method/input_method_util.cc (right):

https://codereview.chromium.org/566823003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/input_method/input_method_util.cc:23: #include
"chrome/common/extensions/extension_constants.h"
On 2014/09/16 17:42:10, Daniel Erat wrote:
> i don't understand why this header got moved down in the list; it's no longer
> sorted correctly. is the "For SetHardwareKeyboardLayoutForTesting" comment
meant
> to apply to this file? the header seems to be used for other things, like
> extension_misc::kBrailleImeExtensionId

Sorry, this got out of order when I moved extension_constants.* back, and I had
to bypass hooks when I uploaded this CL so I missed it.

Done.

[Daniel Erat, 2014-09-16 17:53:07]

thanks. don't think you need a review from me now; i don't see any changes to
c/b/chromeos :-)

[wjmaclean, 2014-09-16 17:57:15]

On 2014/09/16 17:53:07, Daniel Erat wrote:
> thanks. don't think you need a review from me now; i don't see any changes to
> c/b/chromeos :-)

Indeed, you did your review so well it became non-existent :-)

[wjmaclean, 2014-09-18 20:47:21]

rockot@ ... PTAL?

[Ken Rockot(use gerrit already), 2014-09-18 20:50:51]

On 2014/09/18 20:47:21, wjmaclean wrote:
> rockot@ ... PTAL?

lgtm!

[wjmaclean, 2014-09-18 21:24:46]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-18 21:27:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/80001

[commit-bot: I haz the power, 2014-09-18 21:45:47]

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

[commit-bot: I haz the power, 2014-09-18 21:45:51]

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

[wjmaclean, 2014-09-19 01:13:27]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-19 01:14:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/80001

[commit-bot: I haz the power, 2014-09-19 01:41:20]

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

[commit-bot: I haz the power, 2014-09-19 01:41:25]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[wjmaclean, 2014-09-19 01:51:32]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-19 01:52:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/80001

[commit-bot: I haz the power, 2014-09-19 02:27:12]

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

[commit-bot: I haz the power, 2014-09-19 02:27:15]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/62852)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[wjmaclean, 2014-09-19 02:44:05]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-19 02:44:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/80001

[commit-bot: I haz the power, 2014-09-19 03:15:57]

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

[commit-bot: I haz the power, 2014-09-19 03:16:00]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/62862)

[wjmaclean, 2014-09-19 11:08:42]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-19 11:09:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/80001

[commit-bot: I haz the power, 2014-09-19 11:14:47]

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

[commit-bot: I haz the power, 2014-09-19 11:14:51]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[wjmaclean, 2014-09-19 12:31:34]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-19 12:32:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/100001

[commit-bot: I haz the power, 2014-09-19 13:08:11]

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

[commit-bot: I haz the power, 2014-09-19 13:08:15]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/63000)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2014-09-19 14:50:40]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-19 14:51:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/100001

[wjmaclean, 2014-09-19 15:28:29]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-09-19 15:28:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/566823003/120001

[commit-bot: I haz the power, 2014-09-19 16:25:57]

Committed patchset #7 (id:120001) as a7751777886a657ccb58f4fc7e9308784c0eb27b

[commit-bot: I haz the power, 2014-09-19 16:26:40]

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