Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161073002:
Adds RequestContextType information to the NavigationHandle. (Closed)
|
Created:
4 years, 5 months ago by carlosk Modified:
4 years, 4 months ago CC:
Mike West, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, loading-reviews_chromium.org, mmenke, nasko+codewatch_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds RequestContextType information to the NavigationHandle.
This is a spin off from the change to partially move mixed content
checks to the browser process [1]. With the move the information about the
RequestContextType of the navigtation load needs to be available in
browser and this change makes that happen.
[1] https://codereview.chromium.org/1905033002/
BUG=1905033002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/489d9e29eea7297eab0bf3b8000f1be0096b4262
Cr-Commit-Position: refs/heads/master@{#407472}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressing reviewer comments. #Patch Set 3 : Added tests. #Patch Set 4 : Rebase. #
Total comments: 17
Patch Set 5 : Addressed clamy@'s and nasko@'s comments. #
Total comments: 8
Patch Set 6 : Addressed clamy@'s latest comments. #
Total comments: 4
Patch Set 7 : Comments. Better ones. #
Total comments: 3
Patch Set 8 : Excluded test failing with PlzNavigate enabled #Patch Set 9 : Rebase. #Messages
Total messages: 52 (30 generated)
Description was changed from ========== Adds RequestContextType information to the NavigationHandle. This is a spin off from the change to partially move mixed content checks to the browser process [1]. With the move the information about the RequestContextType of the navigtation load needs to be available in browser and this change makes that happen. [1] https://codereview.chromium.org/1905033002/ BUG=1905033002 ========== to ========== Adds RequestContextType information to the NavigationHandle. This is a spin off from the change to partially move mixed content checks to the browser process [1]. With the move the information about the RequestContextType of the navigtation load needs to be available in browser and this change makes that happen. [1] https://codereview.chromium.org/1905033002/ BUG=1905033002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by carlosk@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...
carlosk@chromium.org changed reviewers: + clamy@chromium.org
clamy@: PTAL. nasko@, mkwst@: FYI.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
carlosk@chromium.org changed reviewers: + nasko@chromium.org
nasko@ PTAL. To keep this moving I'm making nasko@ a reviewer too as clamy@ is OOO today.
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:75: pending_nav_entry_id_(pending_nav_entry_id) { Can you please initialize `fetch_request_context_type_`, perhaps to `REQUEST_CONTEXT_TYPE_UNSPECIFIED `? https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:167: return fetch_request_context_type_; You never verify this value in a test; I know it's trivial, but all the more reason to add the trivial test. :) https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:216: // FrameLoader.cpp. Why not do this now? If we can set this field correctly, that seems better than hard-coding it into something arbitrary. CSP does look at differences in navigation type when enforcing `form-action`, for instance; let's give it correct values when doing so. https://codereview.chromium.org/2161073002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:2262: // FrameLoader.cpp. Ditto.
Thanks! https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.cc:75: pending_nav_entry_id_(pending_nav_entry_id) { On 2016/07/20 09:52:46, Mike West wrote: > Can you please initialize `fetch_request_context_type_`, perhaps to > `REQUEST_CONTEXT_TYPE_UNSPECIFIED `? Done. I missed that. https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:167: return fetch_request_context_type_; On 2016/07/20 09:52:46, Mike West wrote: > You never verify this value in a test; I know it's trivial, but all the more > reason to add the trivial test. :) Yes, but it will be verified once the main mixed content CL lands by all mixed content related layout tests. The only reason this is a separate CL is to simplify that one. Is that enough? https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:216: // FrameLoader.cpp. On 2016/07/20 09:52:46, Mike West wrote: > Why not do this now? If we can set this field correctly, that seems better than > hard-coding it into something arbitrary. CSP does look at differences in > navigation type when enforcing `form-action`, for instance; let's give it > correct values when doing so. To save time (as I disappear in less than a week) and because this code is PlzNavigate only and so not active IRL yet. The other TODOs are there for the same reasons. https://codereview.chromium.org/2161073002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:2262: // FrameLoader.cpp. On 2016/07/20 09:52:46, Mike West wrote: > Ditto. Ditto too.
https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:167: return fetch_request_context_type_; On 2016/07/20 at 10:12:02, carlosk wrote: > Yes, but it will be verified once the main mixed content CL lands by all mixed content related layout tests. The only reason this is a separate CL is to simplify that one. Is that enough? I'd prefer to see a unit test land along with the patch that introduces the method under test. https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:216: // FrameLoader.cpp. On 2016/07/20 at 10:12:02, carlosk wrote: > On 2016/07/20 09:52:46, Mike West wrote: > > Why not do this now? If we can set this field correctly, that seems better than > > hard-coding it into something arbitrary. CSP does look at differences in > > navigation type when enforcing `form-action`, for instance; let's give it > > correct values when doing so. > > To save time (as I disappear in less than a week) and because this code is PlzNavigate only and so not active IRL yet. The other TODOs are there for the same reasons. Perhaps `TODO(clamy)`, then? I worry that things like this will get dropped.
The CQ bit was checked by carlosk@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...
Thanks. Gosh! It's hard to wrap my head around these browser tests (aka took me a long time to write something that won't freeze midway through)! https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_handle_impl.h:167: return fetch_request_context_type_; On 2016/07/20 11:56:36, Mike West wrote: > On 2016/07/20 at 10:12:02, carlosk wrote: > > Yes, but it will be verified once the main mixed content CL lands by all mixed > content related layout tests. The only reason this is a separate CL is to > simplify that one. Is that enough? > > I'd prefer to see a unit test land along with the patch that introduces the > method under test. Done. https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2161073002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_request.cc:216: // FrameLoader.cpp. On 2016/07/20 11:56:36, Mike West wrote: > On 2016/07/20 at 10:12:02, carlosk wrote: > > On 2016/07/20 09:52:46, Mike West wrote: > > > Why not do this now? If we can set this field correctly, that seems better > than > > > hard-coding it into something arbitrary. CSP does look at differences in > > > navigation type when enforcing `form-action`, for instance; let's give it > > > correct values when doing so. > > > > To save time (as I disappear in less than a week) and because this code is > PlzNavigate only and so not active IRL yet. The other TODOs are there for the > same reasons. > > Perhaps `TODO(clamy)`, then? I worry that things like this will get dropped. Done. But it's unlikely that will be forgotten as there are other cases of TODO(carlosk) that will have to be addressed for PlzNavigate to be ready.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, thanks for taking another pass.
Thanks! A few comments. https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:166: RequestContextType fetch_request_context_type() const { Add a DCHECK that the state is at least WILL_SEND_REQUEST (probably need to move this to the .cc file). https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:684: "a.com", "/cross_site_iframe_factory.html?a(b(c))")); I'm not sure I see the point of testing frame c here. Also, I would find it more interesting to test various kind of navigations, to get different values for REQUEST_CONTEXT_TYPE. https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:222: REQUEST_CONTEXT_TYPE_LOCATION, The RequestContextType is passed to the browser in BeginNavigationParams. Please use it and remove the TODO. https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... content/browser/loader/DEPS:119: "+content/public/common/request_context_type.h", This should be included in the part below the #TODO (it's used to instantiate the NavigationResourceThrottle which is itself below the TODO). https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2262: // FrameLoader.cpp. You have the type in BeginNavigationParams in the info that is passed from the UI thread.
https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:318: RequestContextType fetch_request_context_type_; I'm a bit puzzled by the "fetch_" part of the name. Isn't it just the type of the request context? Fetch to me invokes a lot more thinking of the ServiceWorker/web API named "fetch", so this is confusing. https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:159: CHECK_NE(REQUEST_CONTEXT_TYPE_UNSPECIFIED, Why would it be unspecified? At the start of the request, shouldn't we know what it is? https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:684: "a.com", "/cross_site_iframe_factory.html?a(b(c))")); On 2016/07/21 11:30:21, clamy wrote: > I'm not sure I see the point of testing frame c here. Also, I would find it more > interesting to test various kind of navigations, to get different values for > REQUEST_CONTEXT_TYPE. Actually, testing a frame that is not direct child of the main frame is a good way to catch bugs that assume frame->parent is the top and we've seem some of those :). Ditto on the different context types though. The context is hardcoded a few places and this test won't test much if it only checks for that one value.
The CQ bit was checked by carlosk@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...
Thanks! https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:166: RequestContextType fetch_request_context_type() const { On 2016/07/21 11:30:21, clamy wrote: > Add a DCHECK that the state is at least WILL_SEND_REQUEST (probably need to move > this to the .cc file). Done. https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:318: RequestContextType fetch_request_context_type_; On 2016/07/21 22:04:35, nasko wrote: > I'm a bit puzzled by the "fetch_" part of the name. Isn't it just the type of > the request context? Fetch to me invokes a lot more thinking of the > ServiceWorker/web API named "fetch", so this is confusing. I was keeping it consistent with the naming in ResourceRequest where this value is obtained from. But indeed this information was included there to serve ServiceWorker code. I renamed to request_context_type_ as it matches the PlzNavigate version in BeginNavigationParams (that clamy@ brought to my attention). https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:159: CHECK_NE(REQUEST_CONTEXT_TYPE_UNSPECIFIED, On 2016/07/21 22:04:35, nasko wrote: > Why would it be unspecified? At the start of the request, shouldn't we know what > it is? The only thing we know here is that it should have been set and not to UNSPECIFIED. The next CHECKs below confirm the value won't change as the navigation progresses. Given these guarantees the actual test can simply check a single point in time in the navigation for the expected value to have to whole flow tested. https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:684: "a.com", "/cross_site_iframe_factory.html?a(b(c))")); On 2016/07/21 22:04:35, nasko wrote: > On 2016/07/21 11:30:21, clamy wrote: > > I'm not sure I see the point of testing frame c here. Also, I would find it > more > > interesting to test various kind of navigations, to get different values for > > REQUEST_CONTEXT_TYPE. > > Actually, testing a frame that is not direct child of the main frame is a good > way to catch bugs that assume frame->parent is the top and we've seem some of > those :). > > Ditto on the different context types though. The context is hardcoded a few > places and this test won't test much if it only checks for that one value. Oh well... I had removed the testing of the extra node and now reverted back to having it. I hope you both agree? :) I'm a bit puzzled about when do different values apply. For instance, I don't understand why we don't get *_FRAME or *_IFRAME in these cases, but only *_LOCATION. I did some codesearch-ing and found no C++ reference of those frame related values (nor of their Blink counterparts in WebURLRequest::RequestContext) that are used for *setting* any values (except for tests). I added tests for HYPERLINK and FORM. I tried and failed adding tests for IMAGE and DOWNLOAD. For the former I ran a browser initiated navigation to an image file but the type was still LOCATION. For the latter I was unable to get the download part to work; I think I'd need to mix the harnesses from both this test and download_browsertest.cc and that is too much trouble for this IMO. Most other values should be for sub resources and not used in navigations (as seems to be the case for IMAGE). https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:222: REQUEST_CONTEXT_TYPE_LOCATION, On 2016/07/21 11:30:21, clamy wrote: > The RequestContextType is passed to the browser in BeginNavigationParams. Please > use it and remove the TODO. Done. I have no idea how I missed that... https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... File content/browser/loader/DEPS (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... content/browser/loader/DEPS:119: "+content/public/common/request_context_type.h", On 2016/07/21 11:30:21, clamy wrote: > This should be included in the part below the #TODO (it's used to instantiate > the NavigationResourceThrottle which is itself below the TODO). Done. https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2262: // FrameLoader.cpp. On 2016/07/21 11:30:21, clamy wrote: > You have the type in BeginNavigationParams in the info that is passed from the > UI thread. Done.
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_...)
Thanks! A few more comments, and this should be good. https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:684: "a.com", "/cross_site_iframe_factory.html?a(b(c))")); On 2016/07/22 12:51:26, carlosk wrote: > On 2016/07/21 22:04:35, nasko wrote: > > On 2016/07/21 11:30:21, clamy wrote: > > > I'm not sure I see the point of testing frame c here. Also, I would find it > > more > > > interesting to test various kind of navigations, to get different values for > > > REQUEST_CONTEXT_TYPE. > > > > Actually, testing a frame that is not direct child of the main frame is a good > > way to catch bugs that assume frame->parent is the top and we've seem some of > > those :). > > > > Ditto on the different context types though. The context is hardcoded a few > > places and this test won't test much if it only checks for that one value. > > Oh well... I had removed the testing of the extra node and now reverted back to > having it. I hope you both agree? :) > > I'm a bit puzzled about when do different values apply. For instance, I don't > understand why we don't get *_FRAME or *_IFRAME in these cases, but only > *_LOCATION. I did some codesearch-ing and found no C++ reference of those frame > related values (nor of their Blink counterparts in > WebURLRequest::RequestContext) that are used for *setting* any values (except > for tests). > > I added tests for HYPERLINK and FORM. I tried and failed adding tests for IMAGE > and DOWNLOAD. For the former I ran a browser initiated navigation to an image > file but the type was still LOCATION. For the latter I was unable to get the > download part to work; I think I'd need to mix the harnesses from both this test > and download_browsertest.cc and that is too much trouble for this IMO. > > Most other values should be for sub resources and not used in navigations (as > seems to be the case for IMAGE). I think it's normal that a navigation to an image is marked as _LOCATION (most important art is that it's a navigation). I agree that _DOWNLOAD is a bit hard to get there. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:107: CHECK_GE(state_, WILL_SEND_REQUEST); DCHECK. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:121: RequestContextType GetFetchRequestContextType() const; s/GetFetchRequestContextType/GetRequestContextType to match variable name. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:751: // Starts and checks the main frame navigation. I don't think this part adds much. Let's just navigate to it and focus on the HYPERLINK part below. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:796: // Starts and checks the main frame navigation. Same as the test above, I'd rather we focus on checking the FORM part, to make the test clearer.
The CQ bit was checked by carlosk@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...
Thanks. https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:684: "a.com", "/cross_site_iframe_factory.html?a(b(c))")); On 2016/07/22 13:51:31, clamy wrote: > On 2016/07/22 12:51:26, carlosk wrote: > > On 2016/07/21 22:04:35, nasko wrote: > > > On 2016/07/21 11:30:21, clamy wrote: > > > > I'm not sure I see the point of testing frame c here. Also, I would find > it > > > more > > > > interesting to test various kind of navigations, to get different values > for > > > > REQUEST_CONTEXT_TYPE. > > > > > > Actually, testing a frame that is not direct child of the main frame is a > good > > > way to catch bugs that assume frame->parent is the top and we've seem some > of > > > those :). > > > > > > Ditto on the different context types though. The context is hardcoded a few > > > places and this test won't test much if it only checks for that one value. > > > > Oh well... I had removed the testing of the extra node and now reverted back > to > > having it. I hope you both agree? :) > > > > I'm a bit puzzled about when do different values apply. For instance, I don't > > understand why we don't get *_FRAME or *_IFRAME in these cases, but only > > *_LOCATION. I did some codesearch-ing and found no C++ reference of those > frame > > related values (nor of their Blink counterparts in > > WebURLRequest::RequestContext) that are used for *setting* any values (except > > for tests). > > > > I added tests for HYPERLINK and FORM. I tried and failed adding tests for > IMAGE > > and DOWNLOAD. For the former I ran a browser initiated navigation to an image > > file but the type was still LOCATION. For the latter I was unable to get the > > download part to work; I think I'd need to mix the harnesses from both this > test > > and download_browsertest.cc and that is too much trouble for this IMO. > > > > Most other values should be for sub resources and not used in navigations (as > > seems to be the case for IMAGE). > > I think it's normal that a navigation to an image is marked as _LOCATION (most > important art is that it's a navigation). I agree that _DOWNLOAD is a bit hard > to get there. Acknowledged. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.cc:107: CHECK_GE(state_, WILL_SEND_REQUEST); On 2016/07/22 13:51:31, clamy wrote: > DCHECK. Done. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl.h:121: RequestContextType GetFetchRequestContextType() const; On 2016/07/22 13:51:31, clamy wrote: > s/GetFetchRequestContextType/GetRequestContextType to match variable name. Done. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:751: // Starts and checks the main frame navigation. On 2016/07/22 13:51:31, clamy wrote: > I don't think this part adds much. Let's just navigate to it and focus on the > HYPERLINK part below. Done. I was in doubt about this and went with the other option. But simplifying SGTM. https://codereview.chromium.org/2161073002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_handle_impl_browsertest.cc:796: // Starts and checks the main frame navigation. On 2016/07/22 13:51:31, clamy wrote: > Same as the test above, I'd rather we focus on checking the FORM part, to make > the test clearer. Done.
Thanks! Lgtm. https://codereview.chromium.org/2161073002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:748: // Executes the main frame navigation. nit: // Navigate to a page with a link. ? https://codereview.chromium.org/2161073002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:784: // Executes the main frame navigation. nit: // Navigate to a page with a form. ?
There is one thing that needs to change, but can be done as part of your bigger CL or a small rename CL. I'll send this one to the CQ now. https://codereview.chromium.org/2161073002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:121: RequestContextType GetRequestContextType() const; This is just an accessor, so it should be hacker_case.
The CQ bit was checked by nasko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, clamy@chromium.org Link to the patchset: https://codereview.chromium.org/2161073002/#ps120001 (title: "Comments. Better ones.")
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
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_...)
The CQ bit was checked by carlosk@chromium.org
The CQ bit was unchecked by carlosk@chromium.org
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 carlosk@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The failed test is due to an error in PlzNavigate: we are firing navigation observer events for a javascript URL when we shouldn't. I added the test to our exclusion filter and will work on a fix for that. I also forgot to answer as Done a couple of previous comments. https://codereview.chromium.org/2161073002/diff/100001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl_browsertest.cc (right): https://codereview.chromium.org/2161073002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:748: // Executes the main frame navigation. On 2016/07/22 16:09:45, clamy wrote: > nit: // Navigate to a page with a link. ? Done. https://codereview.chromium.org/2161073002/diff/100001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl_browsertest.cc:784: // Executes the main frame navigation. On 2016/07/22 16:09:45, clamy wrote: > nit: // Navigate to a page with a form. ? Done. https://codereview.chromium.org/2161073002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:121: RequestContextType GetRequestContextType() const; On 2016/07/22 23:36:25, nasko wrote: > This is just an accessor, so it should be hacker_case. It used to be the case before but as I added the DCHECK clamy@ asked for I moved the implementation to the .CC file and renamed it to the CamelCase. It is still a simple function but can a hacker_case function implementation live on the .CC file?
The CQ bit was checked by carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2161073002/#ps160001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2161073002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigation_handle_impl.h (right): https://codereview.chromium.org/2161073002/diff/120001/content/browser/frame_... content/browser/frame_host/navigation_handle_impl.h:121: RequestContextType GetRequestContextType() const; On 2016/07/25 13:06:10, carlosk wrote: > On 2016/07/22 23:36:25, nasko wrote: > > This is just an accessor, so it should be hacker_case. > > It used to be the case before but as I added the DCHECK clamy@ asked for I moved > the implementation to the .CC file and renamed it to the CamelCase. > > It is still a simple function but can a hacker_case function implementation live > on the .CC file? DCHECK isn't really adding complexity and in release build is noop. Example: https://cs.chromium.org/chromium/src/content/renderer/render_widget.h?sq=pack.... content$ git grep DCHECK *.h | grep -v "//" | wc -l 111
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Adds RequestContextType information to the NavigationHandle. This is a spin off from the change to partially move mixed content checks to the browser process [1]. With the move the information about the RequestContextType of the navigtation load needs to be available in browser and this change makes that happen. [1] https://codereview.chromium.org/1905033002/ BUG=1905033002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Adds RequestContextType information to the NavigationHandle. This is a spin off from the change to partially move mixed content checks to the browser process [1]. With the move the information about the RequestContextType of the navigtation load needs to be available in browser and this change makes that happen. [1] https://codereview.chromium.org/1905033002/ BUG=1905033002 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/489d9e29eea7297eab0bf3b8000f1be0096b4262 Cr-Commit-Position: refs/heads/master@{#407472} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/489d9e29eea7297eab0bf3b8000f1be0096b4262 Cr-Commit-Position: refs/heads/master@{#407472} |
