Making webRequest.addEventListener internal

Making webRequest.addEventListener internal

Changes required to hide addEventListener,
moving it from webRequest to webRequestInternal.

BUG=115467
TEST=loading extensions from the ticket 115467 and observing the behaviour

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

[vabr (Chromium), 2012-05-14 18:10:34]

Hi Dominic,

Please take a look.


Note:
Pre-submit checks failed, complaining that:
"This change modifies the extension docs but the generated docs have not been
updated properly. See chrome/common/extensions/docs/README.txt for more info.
 - Docs out of sync with chrome/common/extensions/api/web_request.json changes.
 - Docs out of sync with chrome/common/extensions/api/web_request_internal.json
changes."

But there is no change to be made:
 * change in web_request.json is removing not documented part of API, and
 * the new web_request_internal.json only contains an internal, thus not
documented, API.

Let me please know, what you think.

Cheers,

Vaclav

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:882: if
(apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
Is this too ad-hoc?

[battre, 2012-05-14 18:44:53]

Awesome! Congratulations.

I have left a couple of comments.

It happens to me all the time that the presubmit scripts complain about expired
documentation. This is what I do:

make DumpRenderTree
chrome/common/extensions/docs/build/build.py

If the warning messages remain, I blame the documentation system on it.
Best regards,
Dominic

On 2012/05/14 18:10:34, vabr (Chromium) wrote:
> Hi Dominic,
> 
> Please take a look.
> 
> 
> Note:
> Pre-submit checks failed, complaining that:
> "This change modifies the extension docs but the generated docs have not been
> updated properly. See chrome/common/extensions/docs/README.txt for more info.
>  - Docs out of sync with chrome/common/extensions/api/web_request.json
changes.
>  - Docs out of sync with
chrome/common/extensions/api/web_request_internal.json
> changes."
> 
> But there is no change to be made:
>  * change in web_request.json is removing not documented part of API, and
>  * the new web_request_internal.json only contains an internal, thus not
> documented, API.
> 
> Let me please know, what you think.
> 
> Cheers,
> 
> Vaclav
> 
>
http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
> File chrome/common/extensions/extension_permission_set.cc (right):
> 
>
http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
> chrome/common/extensions/extension_permission_set.cc:882: if
> (apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
> Is this too ad-hoc?

[vabr (Chromium), 2012-05-14 21:07:51]

Hi Dominic,

Are you sure you left comments? I only see the one I made...

> make DumpRenderTree
> chrome/common/extensions/docs/build/build.py

I did this and it did not help, because the document build script did not make
any changes (because no documented API was changed). I will retry to be sure.

Once we finish our round of review comments, I will invite the extensions OWNERS
for an additional review.

Thanks,

Vaclav

[battre, 2012-05-15 09:23:38]

http://codereview.chromium.org/10310028/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.h:391:
DECLARE_EXTENSION_FUNCTION_NAME("webRequest.eventHandled");
I think it would make sense to cover this function in the CL as well.

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/a...
File chrome/common/extensions/api/web_request_internal.json (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/a...
chrome/common/extensions/api/web_request_internal.json:12: "nodoc": true,
You should move this "nodoc": true one level up. Otherwise our build script for
documentation (chrome/common/extensions/docs/build/build.py) would create an
empty page for this API.

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:882: if
(apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
On 2012/05/14 18:10:35, vabr (Chromium) wrote:
> Is this too ad-hoc?

I think this is fine, but somebody from the extensions team should judge.

http://codereview.chromium.org/10310028/diff/11001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_request_custom_bindings.js
(right):

http://codereview.chromium.org/10310028/diff/11001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:168:
chromeHidden.registerCustomHook('webRequestInternal', function(api) {
I think this should go into web_request_internal_custom_bindings.js for
consistency reasons.

[vabr (Chromium), 2012-05-15 11:57:51]

Hi Dominic,

PTAL.

I tried again building the API documentation, with this result:
 * The build script (c/c/e/docs/build/build.py) said: "Output files match
existing files. No changes made."
 * The webRequest.html was not changed, because both removed functions have
already been undocumented.
 * An empty (up to HTML headers and a title) webRequestInternal.html file was
generated. I did not add it to the CL, because we clearly do not want to have
webRequestInternal documented.
 * As a consequence, the pre-submit checks warned about the out-dated
documentation for webRequest and webRequestInternal. I ignored that warning.

Thanks,

Vaclav

http://codereview.chromium.org/10310028/diff/11001/chrome/browser/extensions/...
File chrome/browser/extensions/api/web_request/web_request_api.h (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/browser/extensions/...
chrome/browser/extensions/api/web_request/web_request_api.h:391:
DECLARE_EXTENSION_FUNCTION_NAME("webRequest.eventHandled");
On 2012/05/15 09:23:38, battre wrote:
> I think it would make sense to cover this function in the CL as well.

Done.

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/a...
File chrome/common/extensions/api/web_request_internal.json (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/a...
chrome/common/extensions/api/web_request_internal.json:12: "nodoc": true,
On 2012/05/15 09:23:38, battre wrote:
> You should move this "nodoc": true one level up. Otherwise our build script
for
> documentation (chrome/common/extensions/docs/build/build.py) would create an
> empty page for this API.

Done.
Still, would not internal:true for the whole namespace mean that the
documentation does not generated?

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:882: if
(apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
On 2012/05/15 09:23:38, battre wrote:
> On 2012/05/14 18:10:35, vabr (Chromium) wrote:
> > Is this too ad-hoc?
> 
> I think this is fine, but somebody from the extensions team should judge.

OK, I'll wait until further reviewers are roped in. My suggestion of a less
ad-hoc solution would be to do this automatically based on some dependency
stored in JSON.

http://codereview.chromium.org/10310028/diff/11001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/web_request_custom_bindings.js
(right):

http://codereview.chromium.org/10310028/diff/11001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/web_request_custom_bindings.js:168:
chromeHidden.registerCustomHook('webRequestInternal', function(api) {
On 2012/05/15 09:23:38, battre wrote:
> I think this should go into web_request_internal_custom_bindings.js for
> consistency reasons.

Done.

[vabr (Chromium), 2012-05-16 11:08:18]

Hi Dominic,

As per personal communication, I corrected the placement of "nodoc" in
webRequestInternal.json.

Now the documentation build script does not bother to produce
webRequestInternal.html, good! Still, the pre-submit checks complain about
out-dated webRequest*.html, but that is OK as explained earlier.

Once you are satisfied, I will ask Matt for review.

Cheers,

Vaclav

[battre, 2012-05-16 11:14:03]

LGTM

[vabr (Chromium), 2012-05-16 11:22:42]

Hi Matt,

I have hidden the internal part of the webRequest API from extension code.
Dominic seems to be satisfied with the change. Could you please take a look, as
an owner of this part of code?

Thanks,

Vaclav

[Matt Perry, 2012-05-16 23:39:20]

lgtm

[Matt Perry, 2012-05-16 23:47:00]

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:882: if
(apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
On 2012/05/15 11:57:52, vabr (Chromium) wrote:
> On 2012/05/15 09:23:38, battre wrote:
> > On 2012/05/14 18:10:35, vabr (Chromium) wrote:
> > > Is this too ad-hoc?
> > 
> > I think this is fine, but somebody from the extensions team should judge.
> 
> OK, I'll wait until further reviewers are roped in. My suggestion of a less
> ad-hoc solution would be to do this automatically based on some dependency
> stored in JSON.

Wait, this is a good point. Why do we need this at all? Isn't the API
automatically exposed to the chromeHidden.internalAPIs? What does the
"declarative" (now "events") API do?

[battre, 2012-05-17 08:56:19]

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:882: if
(apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
On 2012/05/16 23:47:00, Matt Perry wrote:
> On 2012/05/15 11:57:52, vabr (Chromium) wrote:
> > On 2012/05/15 09:23:38, battre wrote:
> > > On 2012/05/14 18:10:35, vabr (Chromium) wrote:
> > > > Is this too ad-hoc?
> > > 
> > > I think this is fine, but somebody from the extensions team should judge.
> > 
> > OK, I'll wait until further reviewers are roped in. My suggestion of a less
> > ad-hoc solution would be to do this automatically based on some dependency
> > stored in JSON.
> 
> Wait, this is a good point. Why do we need this at all? Isn't the API
> automatically exposed to the chromeHidden.internalAPIs? What does the
> "declarative" (now "events") API do?

It is added to kNonPermissionModuleNames[] in extension_permission_set.cc. Given
that lazy loading of .json files is already implemented or should be
implemented, I don't know whether we want to add webRequestInternal to all
extensions, even those not requesting webRequest.

[Matt Perry, 2012-05-17 18:44:45]

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:882: if
(apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
On 2012/05/17 08:56:19, battre wrote:
> On 2012/05/16 23:47:00, Matt Perry wrote:
> > On 2012/05/15 11:57:52, vabr (Chromium) wrote:
> > > On 2012/05/15 09:23:38, battre wrote:
> > > > On 2012/05/14 18:10:35, vabr (Chromium) wrote:
> > > > > Is this too ad-hoc?
> > > > 
> > > > I think this is fine, but somebody from the extensions team should
judge.
> > > 
> > > OK, I'll wait until further reviewers are roped in. My suggestion of a
less
> > > ad-hoc solution would be to do this automatically based on some dependency
> > > stored in JSON.
> > 
> > Wait, this is a good point. Why do we need this at all? Isn't the API
> > automatically exposed to the chromeHidden.internalAPIs? What does the
> > "declarative" (now "events") API do?
> 
> It is added to kNonPermissionModuleNames[] in extension_permission_set.cc.
Given
> that lazy loading of .json files is already implemented or should be
> implemented, I don't know whether we want to add webRequestInternal to all
> extensions, even those not requesting webRequest.

It's best if we only load webRequestInternal for extensions requesting
webRequest. If this code is the way to ensure that, let's keep it.

[vabr (Chromium), 2012-05-17 21:23:37]

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
File chrome/common/extensions/extension_permission_set.cc (right):

http://codereview.chromium.org/10310028/diff/11001/chrome/common/extensions/e...
chrome/common/extensions/extension_permission_set.cc:882: if
(apis_.find(ExtensionAPIPermission::kWebRequest) != apis_.end())
> It's best if we only load webRequestInternal for extensions requesting
> webRequest. If this code is the way to ensure that, let's keep it.

Yes, I believe that this code does "add webRequestInternal permission if there
is already a webRequest permission". If this mechanism is to be used for other
APIs we might consider marking this kind of dependency in the JSON files instead
of hard-coding it here.

Given your last comment, Matt, I guess it is OK for me to add this to CQ. If you
object, please let me know, I will wait at least one more day (possibly longer).

[vabr (Chromium), 2012-05-17 21:26:05]

Added one more comment, but did not manage to send a mail when publishing it.
Sorry, next time I'll be better. :)

[Matt Perry, 2012-05-17 21:38:26]

Yep, LGTM.

[commit-bot: I haz the power, 2012-05-18 14:35:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10310028/10005

[commit-bot: I haz the power, 2012-05-18 15:14:07]

Try job failure for 10310028-10005 (retry) on mac_rel for step "unit_tests".
It's a second try, previously, step "unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[vabr (Chromium), 2012-05-18 17:44:35]

On 2012/05/18 15:14:07, I haz the power (commit-bot) wrote:
> Try job failure for 10310028-10005 (retry) on mac_rel for step "unit_tests".
> It's a second try, previously, step "unit_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

This has an easy fix: I need to add skipping testing the representation strings
for kWebRequestInternal in the ExtensionPermissionsTest.PermissionMessages
unit-test, because kWebRequestInternal is private.
However, I will have to deliver the patch on Monday, from the git branch on my
work computer. When I did the fix on my laptop, the patch was re-based, and it
was not clear for review purposes which differences are mine and which are
against the previous diff set.

[vabr (Chromium), 2012-05-21 08:10:02]

Hi Dominic and Matt,

The ExtensionPermissionsTest.PermissionMessages unit-test needed to be fixed --
WebRequestInternal does not need an explanation string because it is private.

PTAL,

Vaclav

[battre, 2012-05-21 08:33:14]

lgtm

[commit-bot: I haz the power, 2012-05-22 18:43:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10310028/24001

[commit-bot: I haz the power, 2012-05-23 02:12:42]

Try job failure for 10310028-24001 (retry) (retry) (previous was lost) on
win_rel for step "sync_unit_tests".
It's a second try, previously, step "sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-05-23 06:01:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10310028/24001

[commit-bot: I haz the power, 2012-05-23 12:55:15]

Try job failure for 10310028-24001 (retry) (retry) on win_rel for step
"sync_unit_tests".
It's a second try, previously, step "sync_unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[vabr (Chromium), 2012-05-23 13:56:47]

On 2012/05/23 12:55:15, I haz the power (commit-bot) wrote:
> Try job failure for 10310028-24001 (retry) (retry) on win_rel for step
> "sync_unit_tests".
> It's a second try, previously, step "sync_unit_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

I will retry now, as CL 10388250 has been landed.

[commit-bot: I haz the power, 2012-05-23 13:57:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vabr@chromium.org/10310028/24001

[commit-bot: I haz the power, 2012-05-23 15:50:33]

Change committed as 138481