Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2700553002:
Introduce WebRequestResourceType. (Closed)
|
Created:
3 years, 10 months ago by pkalinnikov Modified:
3 years, 10 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIntroduce 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
Messages
Total messages: 68 (45 generated)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add extended ResourceType, supporting WebSocket. BUG=129353 ========== to ========== Add extended ResourceType, supporting WebSocket. So far, creating a WebSocket ResourceTypeExt is forbidden. This will be changed in a follow-up CL. BUG=129353 ==========
pkalinnikov@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
pkalinnikov@chromium.org changed reviewers: + tyoshino@chromium.org
rdevlin.cronin@: Please take a look. tyoshino@: This is the implementation of your proposal, feel free to review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/15 17:54:27, pkalinnikov wrote: > rdevlin.cronin@: Please take a look. > > tyoshino@: This is the implementation of your proposal, feel free to review. Is there a reason we can't just add RESOURCE_TYPE_WEB_SOCKET?
On 2017/02/15 20:13:24, Devlin wrote: > On 2017/02/15 17:54:27, pkalinnikov wrote: > > rdevlin.cronin@: Please take a look. > > > > tyoshino@: This is the implementation of your proposal, feel free to review. > > Is there a reason we can't just add RESOURCE_TYPE_WEB_SOCKET? As discussed offline, there are complications with adding a content::RESOURCE_TYPE_WEB_SOCKET. That said, this approach still seems rather convoluted to me. Given we already need a separation of ResourceType and WebRequestResourceType (ResourceTypeExt), would it make sense to just have a WebRequestResourceType enum, which contains equivalents for all content::ResourceTypes + web socket? To me, having a strict enum we can match against seems much simpler than having to consider an enum + bool, especially when the bool is deterministic with the resource type (i.e., is_websocket is never true for any resource type other than LAST, and is never false for web socket requests). WDYT?
> As discussed offline, there are complications with adding a > content::RESOURCE_TYPE_WEB_SOCKET. That said, this approach still seems rather > convoluted to me. Given we already need a separation of ResourceType and > WebRequestResourceType (ResourceTypeExt), would it make sense to just have a > WebRequestResourceType enum, which contains equivalents for all > content::ResourceTypes + web socket? To me, having a strict enum we can match > against seems much simpler than having to consider an enum + bool, especially > when the bool is deterministic with the resource type (i.e., is_websocket is > never true for any resource type other than LAST, and is never false for web > socket requests). > > WDYT? To me this seems a good alternative. I will shortly replace the ResourceTypeExt structure with the proposed WebRequestResourceType enum. Thanks!
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add extended ResourceType, supporting WebSocket. So far, creating a WebSocket ResourceTypeExt is forbidden. This will be changed in a follow-up CL. BUG=129353 ========== to ========== Add extended ResourceType, supporting WebSocket. The WebRequestResourceType is extends content::ResourceType with a WebSocket request. So far, this new type is not used, but this will be changed in a follow-up CL. BUG=129353 ==========
Done. Please take another look. Thank you.
Description was changed from ========== Add extended ResourceType, supporting WebSocket. The WebRequestResourceType is extends content::ResourceType with a WebSocket request. So far, this new type is not used, but this will be changed in a follow-up CL. BUG=129353 ========== to ========== Add extended ResourceType, supporting WebSocket. The WebRequestResourceType extends content::ResourceType with a WebSocket request. So far, this new type is not used, but this will be changed in a follow-up CL. BUG=129353 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Nice! I think this is much more readable; thanks! https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:95: WebRequestResourceType::LAST_TYPE, // Represents "other". if these aren't included in UMA anywhere, it could make sense to rename this to be OTHER (since we're no longer strictly bound by content::ResourceType definitions). https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.h (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.h:341: // Stores an |extension::WebRequestResourceType| representation in |types| if nit: s/extension::/extensions:: https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:36: WEBSOCKET, nit: Why WEBSOCKET instead of WEB_SOCKET? (If this is more consistent with another place, then it's fine but strikes me as a little odd) https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:45: inline WebRequestResourceType ToWebRequestResourceType( When we begin intercepting WebSocket requests, this in and of itself won't be able to determine the resource type. I wonder if it makes sense to have this take a net::URLRequest* instead, so we can determine based on both type and url? Something like: WebRequestResourceType ToWebRequestResourceType( net::URLRequest* request) { if (request->url().SchemeIsWSOrWSS()) return WebRequestResourceType::WEBSOCKET; content::ResourceRequestInfo* info = content::ResourceRequestInfo::ForRequest(request); if (!info) // Not sure we need this... return WebRequestResourceType::LAST_TYPE; content::ResourceType content_resource_type = info->GetResourceType(); return type == content::RESOURCE_TYPE_LAST_TYPE ? WebRequestResourceType::LAST_TYPE : static_cast<WebRequestResourceType>(type); } WDYT? Otherwise, it seems like we'll need special handling before we call ToWebRequestResourceType(), and I'd prefer to centralize the conversion logic in one place.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... 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: > if these aren't included in UMA anywhere, it could make sense to rename this to > be OTHER (since we're no longer strictly bound by content::ResourceType > definitions). Done. https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.h (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.h:341: // Stores an |extension::WebRequestResourceType| representation in |types| if On 2017/02/16 15:13:05, Devlin wrote: > nit: s/extension::/extensions:: Done. https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:36: WEBSOCKET, On 2017/02/16 15:13:05, Devlin wrote: > nit: Why WEBSOCKET instead of WEB_SOCKET? (If this is more consistent with > another place, then it's fine but strikes me as a little odd) Changed to WEB_SOCKET. https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:45: inline WebRequestResourceType ToWebRequestResourceType( On 2017/02/16 15:13:05, Devlin wrote: > When we begin intercepting WebSocket requests, this in and of itself won't be > able to determine the resource type. I wonder if it makes sense to have this > take a net::URLRequest* instead, so we can determine based on both type and url? > > Something like: > WebRequestResourceType ToWebRequestResourceType( > net::URLRequest* request) { > if (request->url().SchemeIsWSOrWSS()) > return WebRequestResourceType::WEBSOCKET; > content::ResourceRequestInfo* info = > content::ResourceRequestInfo::ForRequest(request); > if (!info) // Not sure we need this... > return WebRequestResourceType::LAST_TYPE; > content::ResourceType content_resource_type = info->GetResourceType(); > return type == content::RESOURCE_TYPE_LAST_TYPE > ? WebRequestResourceType::LAST_TYPE > : static_cast<WebRequestResourceType>(type); > } > > WDYT? Otherwise, it seems like we'll need special handling before we call > ToWebRequestResourceType(), and I'd prefer to centralize the conversion logic in > one place. I had been thinking about this as well, but I had 2 concerns: - A enum header knows about URLRequest (probably it's okay) - ResourceRequestInfo is already retrieved in the call site, so getting it in ToWebRequestResourceType adds a bit of overhead. I could make a function that takes ResourceRequestInfo as a parameter, though. I am reluctant to remove the original ToWebRequestResourceType function, because it still seems handy when there is no URLRequest nearby (e.g., in IsRelevantResourceType function). Let's add both functions and see in the follow-up CL if it's possible to get rid of the original.
https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:56: WebRequestResourceType ToWebRequestResourceType(const net::URLRequest* request); Actually, GetWebRequestResourceType fits this function better, because it's not a conversion unlike in the above function. WDYT?
https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc:187: ToWebRequestResourceType(info->GetResourceType())) != This seems like a place where we should be using the other variant of this function so that we can account for WebSockets (and since we fetch the ResourceRequestInfo). That said, it looks like |info| can be null here (assuming the check wasn't erroneous), which makes it a little more painful. I think the simplest way to do this would be to add a WebRequestResourceType::INVALID that can be returned when it's not a web socket request there's no associated ResourceRequestInfo. Then, |types_| here should never include that, and we could just make this whole function return (request_data.stage & GetStages()) != 0 && std::find(types_.begin(), types_.end(), GetWebRequestResourceType(request_data.request)) != types_.end(); https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1527: resource_type = ToWebRequestResourceType(info->GetResourceType()); here, too, looks like this type should be able to include WebSockets (and so use the version that takes the request). https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.h:20: #include "extensions/browser/api/web_request/web_request_resource_type.h" nit: with C++11, we can forward-declare enum classes. Does that work here and let us move this include to the .cc file? https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.cc:20: : WebRequestResourceType::OTHER; and then return INVALID if info is null rather than OTHER https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:42: OTHER, (see other comment, but I wonder if we should put an INVALID here) https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:56: WebRequestResourceType ToWebRequestResourceType(const net::URLRequest* request); On 2017/02/16 15:55:38, pkalinnikov wrote: > Actually, GetWebRequestResourceType fits this function better, because it's not > a conversion unlike in the above function. WDYT? I was going to suggest something like WebRequestResourceTypeForRequest or something, but GetWebRequestResourceType is also good (I'd be fine with either). I agree that To* doesn't fit quite as well.
lgtm https://codereview.chromium.org/2700553002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:43: "content::ResourceType has been modified."); The comment for ResourceType is saying: // Used in histograms; explicitly assign each type and do not re-use old values. Strictly speaking, this doesn't guarantee that new value is never added between existing ones. Then, this static_assert cannot capture such a change. Ideally, I'd like to have an assert that doesn't depend on this assumption. But, comparing RESOURCE_TYPE_LAST_TYPE with the first element we add here (currently WebRequestResourceType::WEBSOCKET) still doesn't work. Hmm, I don't come up with any simple solution that can capture such a change. But, given that this can at least capture next addition of new ResourceType and there's no gap in ResourceType at this point, it's too much to worry about that now. So, ok. https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1264: : "other"; Not a change proposal. Let me explain my understanding. Please review it to check if it's correct. Maybe we have linear look up here not to depend on the assumption that the integer values assigned to content::ResourceType starts from 0 and don't leap. It's a bit unfortunate from performance perspective if we need this even after we introduce the extensions directory's own enum. Because of that, I was about to propose changing kResourceTypeStrings to list the string for all enum items including ones correspond to "other" and was wondering if the negative impact to ParseResourceType() by increasing the number of elements to scan would be bigger. But it looks ToWebRequestResourceType() is more performance sensitive and we shouldn't introduce any look up to it, right? WebRequestEventDetails() has both ResourceTypeToString() and ToWebRequestResourceType(). WebRequestConditionAttributeResourceType::IsFulfilled() and ExtensionWebRequestEventRouter::GetMatchingListeners() have only ToWebRequestResourceType() and are performance sensitive. So, we shouldn't make them slow. Correct?
On 2017/02/16 16:13:59, tyoshino wrote: > It's a bit unfortunate from performance perspective if we need this even after > we introduce the extensions directory's own enum. Because of that, I was about > to propose changing kResourceTypeStrings to list the string for all enum items > including ones correspond to "other" and then introduce logic to ToWebRequestResourceType() that maps from ResourceType to WebRequestResourceType.
https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... 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: > On 2017/02/16 15:13:05, Devlin wrote: > > if these aren't included in UMA anywhere, it could make sense to rename this > to > > be OTHER (since we're no longer strictly bound by content::ResourceType > > definitions). > > Done. sounds good
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:42: OTHER, On 2017/02/16 16:08:26, Devlin wrote: > (see other comment, but I wonder if we should put an INVALID here) Oh. Using OTHER here would make kResourceTypeValues[] look more consistent with kResourceTypeStrings[] as Devlin suggested, but it's a bit weird that SUB_RESOURCE, PREFETCH and CSP_REPORT listed here will be converted by ResourceTypeToString() into "other" which corresponds to OTHER also listed here. Hmm, on the other hand, e.g. ExtensionWebRequestEventRouter::GetMatchingListenersImpl() looks up the result of ToWebRequestResourceType() from RequestFilter::types but it's set by ParseResourceType() which never pushes any of these 3 types into the types argument. So, these 3 types are handled not as OTHER stuffs. Maybe this inconsistency should be cleaned up if not intentional.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Guys, please review the whole CL once again. There are couple of improvements: - The 'media' type seems to be ignored (was that intentional?), so I added it to the type list. - Some of request types (SUB_RESOURCE, PREFETCH, CSP_RESPORT) were ignored. I made them consistently OTHER. This means that both the string representation and the enum value are consistent. - Now the WebRequestResourceType is independent from content::ResourceType mostly. - ToWebRequestResourceType function catches both additions and deletions from content::ResourceType with a compilation error. - WebRequestResourceTypeToString is now O(1). https://codereview.chromium.org/2700553002/diff/60001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/60001/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:43: "content::ResourceType has been modified."); On 2017/02/16 16:13:58, tyoshino wrote: > The comment for ResourceType is saying: > > // Used in histograms; explicitly assign each type and do not re-use old values. > > Strictly speaking, this doesn't guarantee that new value is never added between > existing ones. Then, this static_assert cannot capture such a change. > > Ideally, I'd like to have an assert that doesn't depend on this assumption. But, > comparing RESOURCE_TYPE_LAST_TYPE with the first element we add here (currently > WebRequestResourceType::WEBSOCKET) still doesn't work. Hmm, I don't come up with > any simple solution that can capture such a change. > > But, given that this can at least capture next addition of new ResourceType and > there's no gap in ResourceType at this point, it's too much to worry about that > now. > > So, ok. Ack. This is approximately what I thought here as well :) After my last changes this is not a problem any more. https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.cc (right): https://codereview.chromium.org/2700553002/diff/80001/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.cc:1264: : "other"; On 2017/02/16 16:13:58, tyoshino wrote: > Not a change proposal. Let me explain my understanding. Please review it to > check if it's correct. > > Maybe we have linear look up here not to depend on the assumption that the > integer values assigned to content::ResourceType starts from 0 and don't leap. > > It's a bit unfortunate from performance perspective if we need this even after > we introduce the extensions directory's own enum. Because of that, I was about > to propose changing kResourceTypeStrings to list the string for all enum items > including ones correspond to "other" and was wondering if the negative impact to > ParseResourceType() by increasing the number of elements to scan would be > bigger. > > But it looks ToWebRequestResourceType() is more performance sensitive and we > shouldn't introduce any look up to it, right? > > WebRequestEventDetails() has both ResourceTypeToString() and > ToWebRequestResourceType(). > WebRequestConditionAttributeResourceType::IsFulfilled() and > ExtensionWebRequestEventRouter::GetMatchingListeners() have only > ToWebRequestResourceType() and are performance sensitive. So, we shouldn't make > them slow. > > Correct? Please see the new change. Looks like it addressed this concern as well. https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.cc:187: ToWebRequestResourceType(info->GetResourceType())) != On 2017/02/16 16:08:26, Devlin wrote: > This seems like a place where we should be using the other variant of this > function so that we can account for WebSockets (and since we fetch the > ResourceRequestInfo). > > That said, it looks like |info| can be null here (assuming the check wasn't > erroneous), which makes it a little more painful. > > I think the simplest way to do this would be to add a > WebRequestResourceType::INVALID that can be returned when it's not a web socket > request there's no associated ResourceRequestInfo. Then, |types_| here should > never include that, and we could just make this whole function > > return (request_data.stage & GetStages()) != 0 && > std::find(types_.begin(), types_.end(), > GetWebRequestResourceType(request_data.request)) != > types_.end(); SGTM. How about UNDEFINED? https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_api.cc:1527: resource_type = ToWebRequestResourceType(info->GetResourceType()); On 2017/02/16 16:08:26, Devlin wrote: > here, too, looks like this type should be able to include WebSockets (and so use > the version that takes the request). Yes, I have already done it in the follow-up CL (not uploaded yet). My motivation to not include such code to this CL was to avoid using WEB_SOCKET type until it gets full support. But after the second thought, WebSocket requests can't get through anyway because of permissions. So yeah, let me go ahead and use the new function here. https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_api_helpers.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_api_helpers.h:20: #include "extensions/browser/api/web_request/web_request_resource_type.h" On 2017/02/16 16:08:26, Devlin wrote: > nit: with C++11, we can forward-declare enum classes. Does that work here and > let us move this include to the .cc file? Not relevant any more. https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.cc:20: : WebRequestResourceType::OTHER; On 2017/02/16 16:08:26, Devlin wrote: > and then return INVALID if info is null rather than OTHER Done. https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... File extensions/browser/api/web_request/web_request_resource_type.h (right): https://codereview.chromium.org/2700553002/diff/70010/extensions/browser/api/... extensions/browser/api/web_request/web_request_resource_type.h:42: OTHER, On 2017/02/16 17:07:37, tyoshino wrote: > On 2017/02/16 16:08:26, Devlin wrote: > > (see other comment, but I wonder if we should put an INVALID here) > > Oh. Using OTHER here would make kResourceTypeValues[] look more consistent with > kResourceTypeStrings[] as Devlin suggested, but it's a bit weird that > SUB_RESOURCE, PREFETCH and CSP_REPORT listed here will be converted by > ResourceTypeToString() into "other" which corresponds to OTHER also listed here. > > Hmm, on the other hand, e.g. > ExtensionWebRequestEventRouter::GetMatchingListenersImpl() looks up the result > of ToWebRequestResourceType() from RequestFilter::types but it's set by > ParseResourceType() which never pushes any of these 3 types into the types > argument. So, these 3 types are handled not as OTHER stuffs. > > Maybe this inconsistency should be cleaned up if not intentional. I addressed the inconsistency. Please see the global comment.
Some questions and self-nits. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h:108: const std::vector<WebRequestResourceType> types_; We could make this vector a bitmask easily, but this is out of scope of this CL. Will leave a TODO. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/web_request/web_request_event_details.h (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_event_details.h:8: #include <stdint.h> Remove this. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:45: case content::RESOURCE_TYPE_SUB_RESOURCE: Devlin, Takeshi: Do you feel this (and PREFETCH, CSP_REPORT) should be UNDEFINED instead, as in the pre-CL implementation? https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:68: case content::RESOURCE_TYPE_LAST_TYPE: Should LAST_TYPE be UNDEFINED or OTHER? https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:87: DCHECK_LE(index, kResourceTypeStringsLength); Oups, DCHECK_LT.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Important comment. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1516: auto resource_type = GetWebRequestResourceType(request); A have looked at the failing test, and apparently the reason is that this line gets resource_type == UNDEFINED, but the matching condition is OTHER. The test hangs and waits for events, but no events are reported. I think that we should eliminate UNDEFINED, and make only OTHER type. I also think that the matching condition in declarative WebRequest API should be the same as here, i.e., the WebRequestConditionAttributeResourceType::IsFulfilled method should also receive OTHER type for such requests. WDYT?
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/declarative_webrequest/webrequest_condition_attribute.h:108: const std::vector<WebRequestResourceType> types_; On 2017/02/16 19:43:38, pkalinnikov wrote: > We could make this vector a bitmask easily, but this is out of scope of this CL. > Will leave a TODO. Done. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/web_request/web_request_api.cc (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_api.cc:1516: auto resource_type = GetWebRequestResourceType(request); On 2017/02/16 21:01:05, pkalinnikov wrote: > A have looked at the failing test, and apparently the reason is that this line > gets resource_type == UNDEFINED, but the matching condition is OTHER. The test > hangs and waits for events, but no events are reported. > > I think that we should eliminate UNDEFINED, and make only OTHER type. I also > think that the matching condition in declarative WebRequest API should be the > same as here, i.e., the WebRequestConditionAttributeResourceType::IsFulfilled > method should also receive OTHER type for such requests. > > WDYT? Done. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/web_request/web_request_event_details.h (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_event_details.h:8: #include <stdint.h> On 2017/02/16 19:43:38, pkalinnikov wrote: > Remove this. Done. https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/110001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:87: DCHECK_LE(index, kResourceTypeStringsLength); On 2017/02/16 19:43:38, pkalinnikov wrote: > Oups, DCHECK_LT. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Cool, I think this lgtm as long as the bots are happy https://codereview.chromium.org/2700553002/diff/130001/extensions/browser/api... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/130001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:93: for (size_t i = 0; i < kResourceTypeStringsLength; ++i) nit: braces around multi-line loop bodies
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add extended ResourceType, supporting WebSocket. The WebRequestResourceType extends content::ResourceType with a WebSocket request. So far, this new type is not used, but this will be changed in a follow-up CL. BUG=129353 ========== to ========== 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. 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 a MEDIA/'media' type to WebRequest API. BUG=129353 ==========
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... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/130001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:93: for (size_t i = 0; i < kResourceTypeStringsLength; ++i) On 2017/02/16 23:33:21, Devlin wrote: > nit: braces around multi-line loop bodies Done.
lgtm. it's very cleaner now!! https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:57: return WebRequestResourceType::OTHER; how about explaining this change (the 3 types are now mapped to OTHER) in the CL description?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== 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. 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 a MEDIA/'media' type to WebRequest API. BUG=129353 ========== to ========== 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 ==========
https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api... File extensions/browser/api/web_request/web_request_resource_type.cc (right): https://codereview.chromium.org/2700553002/diff/150001/extensions/browser/api... extensions/browser/api/web_request/web_request_resource_type.cc:57: return WebRequestResourceType::OTHER; On 2017/02/17 10:39:58, tyoshino wrote: > how about explaining this change (the 3 types are now mapped to OTHER) in the CL > description? Done.
The CQ bit was checked by pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2700553002/#ps150001 (title: "Make compiler happy; make type<->string mapping self-checking.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 150001, "attempt_start_ts": 1487332470824930,
"parent_rev": "d7519ff3a7e074f03a8d481c1f63fe3f54431682", "commit_rev":
"a7a2ee3f307090e4f5dd8848ce498c939993b3db"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/a7a2ee3f307090e4f5dd8848ce49... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:150001) as https://chromium.googlesource.com/chromium/src/+/a7a2ee3f307090e4f5dd8848ce49... |
