Chromium Code Reviews
Help | Chromium Project | Sign in
(52)

Issue 2700553002: Introduce WebRequestResourceType. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 month, 1 week ago by pkalinnikov
Modified:
1 month ago
Reviewers:
tyoshino, Devlin
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Introduce WebRequestResourceType. WebRequestResourceType enumerates all resource/request types that WebRequest API cares about. There is a 1-to-1 mapping between WebRequestResourceTypes and their string names. Helper functions are provided to get string representation for each type, and vice versa. Each content::ResourceType maps to a single WebRequestResourceType, while the latter can correspond to multiple content::ResourceTypes, e.g., the OTHER type. Note that this CL also makes 3 content::ResourceTypes (SUB_RESOURCE, PREFETCH, and CSP_REPORT) map to OTHER explicitly (before they were indirectly considered 'other'). Some (so far only 1) types are not covered by content::ResourceType. For example, the WEB_SOCKET type represents a WebSocket request, and corresponds to URLRequests which have a ws/wss scheme. A helper function is provided to compute type from a URLRequest in such a way that it takes both scheme and content::ResourceType (if any) into account. This CL also adds 'media' and 'websocket' types to WebRequest API. BUG=129353 Review-Url: https://codereview.chromium.org/2700553002 Cr-Commit-Position: refs/heads/master@{#451287} Committed: https://chromium.googlesource.com/chromium/src/+/a7a2ee3f307090e4f5dd8848ce498c939993b3db

Patch Set 1 #

Patch Set 2 : Use enum istead of a struct. #

Patch Set 3 : Clean up. #

Patch Set 4 : Fix win build. #

Total comments: 2

Patch Set 5 : Fix fix win build. #

Total comments: 11

Patch Set 6 : Address comments from Devlin. #

Total comments: 13

Patch Set 7 : Address comments; add 'media'; make type mapping consistent. #

Total comments: 10

Patch Set 8 : Eliminate UNDEFINED type; address self-nits. #

Total comments: 2

Patch Set 9 : Make compiler happy; make type<->string mapping self-checking. #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+207 lines, -126 lines) Patch
M extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h View 1 2 3 4 5 6 7 3 chunks +6 lines, -3 lines 0 comments Download
M extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc View 1 2 3 4 5 6 7 6 chunks +10 lines, -11 lines 0 comments Download
M extensions/browser/api/web_request/BUILD.gn View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api.h View 1 2 3 4 5 6 4 chunks +5 lines, -3 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api.cc View 1 2 3 4 5 6 7 9 chunks +9 lines, -11 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api_helpers.h View 1 2 3 4 5 6 2 chunks +0 lines, -15 lines 0 comments Download
M extensions/browser/api/web_request/web_request_api_helpers.cc View 1 2 3 4 5 6 3 chunks +1 line, -78 lines 0 comments Download
M extensions/browser/api/web_request/web_request_event_details.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M extensions/browser/api/web_request/web_request_event_details.cc View 1 2 3 4 5 6 3 chunks +4 lines, -3 lines 0 comments Download
A extensions/browser/api/web_request/web_request_resource_type.h View 1 2 3 4 5 6 7 1 chunk +53 lines, -0 lines 0 comments Download
A extensions/browser/api/web_request/web_request_resource_type.cc View 1 2 3 4 5 6 7 8 1 chunk +115 lines, -0 lines 2 comments Download
M extensions/common/api/web_request.json View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 68 (45 generated)
pkalinnikov
rdevlin.cronin@: Please take a look. tyoshino@: This is the implementation of your proposal, feel free ...
1 month, 1 week ago (2017-02-15 17:54:27 UTC) #6
Devlin
On 2017/02/15 17:54:27, pkalinnikov wrote: > rdevlin.cronin@: Please take a look. > > tyoshino@: This ...
1 month, 1 week ago (2017-02-15 20:13:24 UTC) #9
Devlin
On 2017/02/15 20:13:24, Devlin wrote: > On 2017/02/15 17:54:27, pkalinnikov wrote: > > rdevlin.cronin@: Please ...
1 month, 1 week ago (2017-02-15 22:09:53 UTC) #10
pkalinnikov
> As discussed offline, there are complications with adding a > content::RESOURCE_TYPE_WEB_SOCKET. That said, this ...
1 month, 1 week ago (2017-02-15 23:15:55 UTC) #11
pkalinnikov
Done. Please take another look. Thank you.
1 month, 1 week ago (2017-02-16 11:17:21 UTC) #17
Devlin
Nice! I think this is much more readable; thanks! https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/web_request/web_request_api_helpers.cc File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/web_request/web_request_api_helpers.cc#newcode95 extensions/browser/api/web_request/web_request_api_helpers.cc:95: ...
1 month, 1 week ago (2017-02-16 15:13:06 UTC) #29
pkalinnikov
https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/web_request/web_request_api_helpers.cc File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/web_request/web_request_api_helpers.cc#newcode95 extensions/browser/api/web_request/web_request_api_helpers.cc:95: WebRequestResourceType::LAST_TYPE, // Represents "other". On 2017/02/16 15:13:05, Devlin wrote: ...
1 month ago (2017-02-16 15:52:46 UTC) #32
pkalinnikov
https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/web_request/web_request_resource_type.h File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/web_request/web_request_resource_type.h#newcode56 extensions/browser/api/web_request/web_request_resource_type.h:56: WebRequestResourceType ToWebRequestResourceType(const net::URLRequest* request); Actually, GetWebRequestResourceType fits this function ...
1 month ago (2017-02-16 15:55:38 UTC) #33
Devlin
https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc File extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc#newcode187 extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc:187: ToWebRequestResourceType(info->GetResourceType())) != This seems like a place where we ...
1 month ago (2017-02-16 16:08:26 UTC) #34
tyoshino
lgtm https://codereview.chromium.org/2700553002/diff/60001/extensions/browser/api/web_request/web_request_resource_type.h File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/60001/extensions/browser/api/web_request/web_request_resource_type.h#newcode43 extensions/browser/api/web_request/web_request_resource_type.h:43: "content::ResourceType has been modified."); The comment for ResourceType ...
1 month ago (2017-02-16 16:13:59 UTC) #35
tyoshino
On 2017/02/16 16:13:59, tyoshino wrote: > It's a bit unfortunate from performance perspective if we ...
1 month ago (2017-02-16 16:24:16 UTC) #36
tyoshino
https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/web_request/web_request_api_helpers.cc File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/web_request/web_request_api_helpers.cc#newcode95 extensions/browser/api/web_request/web_request_api_helpers.cc:95: WebRequestResourceType::LAST_TYPE, // Represents "other". On 2017/02/16 15:52:45, pkalinnikov wrote: ...
1 month ago (2017-02-16 16:26:34 UTC) #37
tyoshino
https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/web_request/web_request_resource_type.h File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/web_request/web_request_resource_type.h#newcode42 extensions/browser/api/web_request/web_request_resource_type.h:42: OTHER, On 2017/02/16 16:08:26, Devlin wrote: > (see other ...
1 month ago (2017-02-16 17:07:37 UTC) #40
pkalinnikov
Guys, please review the whole CL once again. There are couple of improvements: - The ...
1 month ago (2017-02-16 19:27:35 UTC) #43
pkalinnikov
Some questions and self-nits. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h File extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h#newcode108 extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h:108: const std::vector<WebRequestResourceType> types_; We could ...
1 month ago (2017-02-16 19:43:39 UTC) #44
pkalinnikov
Important comment. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api/web_request/web_request_api.cc File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api/web_request/web_request_api.cc#newcode1516 extensions/browser/api/web_request/web_request_api.cc:1516: auto resource_type = GetWebRequestResourceType(request); A have looked ...
1 month ago (2017-02-16 21:01:05 UTC) #47
pkalinnikov
https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h File extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h#newcode108 extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h:108: const std::vector<WebRequestResourceType> types_; On 2017/02/16 19:43:38, pkalinnikov wrote: > ...
1 month ago (2017-02-16 21:24:46 UTC) #49
Devlin
Cool, I think this lgtm as long as the bots are happy https://codereview.chromium.org/2700553002/diff/130001/extensions/browser/api/web_request/web_request_resource_type.cc File extensions/browser/api/web_request/web_request_resource_type.cc ...
1 month ago (2017-02-16 23:33:22 UTC) #53
pkalinnikov
Done. + Bonus improvement in web_request_resource_type.cc Takeshi, does this look good to you? https://codereview.chromium.org/2700553002/diff/130001/extensions/browser/api/web_request/web_request_resource_type.cc File ...
1 month ago (2017-02-17 09:51:04 UTC) #57
tyoshino
lgtm. it's very cleaner now!! https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api/web_request/web_request_resource_type.cc File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api/web_request/web_request_resource_type.cc#newcode57 extensions/browser/api/web_request/web_request_resource_type.cc:57: return WebRequestResourceType::OTHER; how about ...
1 month ago (2017-02-17 10:39:58 UTC) #58
pkalinnikov
https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api/web_request/web_request_resource_type.cc File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api/web_request/web_request_resource_type.cc#newcode57 extensions/browser/api/web_request/web_request_resource_type.cc:57: return WebRequestResourceType::OTHER; On 2017/02/17 10:39:58, tyoshino wrote: > how ...
1 month ago (2017-02-17 11:36:27 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2700553002/150001
1 month ago (2017-02-17 11:54:45 UTC) #65
commit-bot: I haz the power
1 month ago (2017-02-17 12:40:34 UTC) #68
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as
https://chromium.googlesource.com/chromium/src/+/a7a2ee3f307090e4f5dd8848ce49...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld d1a128a62