Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2355453002:
[NoStatePrefetch] Support only GET and HEAD (Closed)
Description[NoStatePrefetch] Support only GET and HEAD
Unlike prerendering, NoStatePrefetch only supports GET and HEAD.
Committed: https://crrev.com/56b3870186e8774c5b1015a41ed2f453476857c7
Cr-Commit-Position: refs/heads/master@{#420291}
Patch Set 1 : format #
Total comments: 8
Patch Set 2 : Review comments #
Total comments: 6
Patch Set 3 : review comments #
Messages
Total messages: 32 (16 generated)
droger@chromium.org changed reviewers: + mattcary@chromium.org, pasko@chromium.org
The CQ bit was checked by droger@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...
Patchset #1 (id:1) has been deleted
lgtm Nice catch. Where did this come up?
On 2016/09/19 15:08:45, mattcary wrote: > Nice catch. Where did this come up? The design doc mentions it, and I figured nobody implemented it yet.
droger@chromium.org changed reviewers: + mmenke@chromium.org
+mmenke as OWNER
The CQ bit was checked by droger@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.
Quick comment, not a review. I'll look at this later today. https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:124: if (!prerender_contents->IsValidHttpMethod(method)) { Would it make more sense just to cancel the individual request, not the entire prerender, when this fails for prefetch prerenders? For "real" prerenders, if we cancel a request, the entire prerender is unusable. For prefetch prerenders, however, the whole point is the resources we fetch, so there doesn't seem to be a reason to cancel the prerender other resources, unless this is the main frame request, right?
https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:124: if (!prerender_contents->IsValidHttpMethod(method)) { On 2016/09/20 13:31:18, mmenke wrote: > Would it make more sense just to cancel the individual request, not the entire > prerender, when this fails for prefetch prerenders? For "real" prerenders, if > we cancel a request, the entire prerender is unusable. For prefetch prerenders, > however, the whole point is the resources we fetch, so there doesn't seem to be > a reason to cancel the prerender other resources, unless this is the main frame > request, right? I think you are right indeed, we could cancel the individual request instead of the whole prefetch. To do this, it would probably be enough to set cancel to true, but only call >Destroy() if the resource_type is "main frame". I will play with this.
https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:241: for (size_t i = 0; i < arraysize(kValidHttpMethods); ++i) { optional nit: for both the loops, can just do: for (const auto& : kValidHttpMethods) https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:242: if (method.compare(kValidHttpMethods[i]) == 0) Know this is what the old code did, but maybe we should just be using == instead of compare? It's potentially faster, since compare has to care about the difference of < vs >, while == does not. Same for the loop below.
The CQ bit was checked by droger@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/2355453002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:241: for (size_t i = 0; i < arraysize(kValidHttpMethods); ++i) { On 2016/09/20 15:25:32, mmenke wrote: > optional nit: for both the loops, can just do: for (const auto& : > kValidHttpMethods) Done. https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:242: if (method.compare(kValidHttpMethods[i]) == 0) On 2016/09/20 15:25:32, mmenke wrote: > Know this is what the old code did, but maybe we should just be using == instead > of compare? It's potentially faster, since compare has to care about the > difference of < vs >, while == does not. Same for the loop below. TIL According to this: http://stackoverflow.com/questions/9158894/differences-between-c-string-and-c... == is implemented using compare() in our case, so the performance should be the same. However == is really what we want conceptually. Also note that if both strings are std::strings, == can be faster since it compares first the size of the strings before iterating them. This does not apply to us here, because we're comparing a C-string to a std::string. Done. https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:124: if (!prerender_contents->IsValidHttpMethod(method)) { On 2016/09/20 13:31:18, mmenke wrote: > Would it make more sense just to cancel the individual request, not the entire > prerender, when this fails for prefetch prerenders? For "real" prerenders, if > we cancel a request, the entire prerender is unusable. For prefetch prerenders, > however, the whole point is the resources we fetch, so there doesn't seem to be > a reason to cancel the prerender other resources, unless this is the main frame > request, right? Done, although the scenario where a prefetch subresource has an invalid method cannot actually happen currently (since these requests are created by the HTML preload scanner).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2355453002/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:124: if (!prerender_contents->IsValidHttpMethod(method)) { On 2016/09/21 09:57:48, droger wrote: > On 2016/09/20 13:31:18, mmenke wrote: > > Would it make more sense just to cancel the individual request, not the entire > > prerender, when this fails for prefetch prerenders? For "real" prerenders, if > > we cancel a request, the entire prerender is unusable. For prefetch > prerenders, > > however, the whole point is the resources we fetch, so there doesn't seem to > be > > a reason to cancel the prerender other resources, unless this is the main > frame > > request, right? > > Done, although the scenario where a prefetch subresource has an invalid method > cannot actually happen currently (since these requests are created by the HTML > preload scanner). Good point, though I suppose extensions and Chrome components can do all sorts of weird things. May make sense just to remove this class entirely, once we get rid of prerenders. https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:126: // whole prefetch unless this is the main frame. Think this would be clearer if it said what we're doing here, not what we're not doing here. i.e., something like: "If this is a full prerender, cancel the prerender in response to invalid requests. For prefetches, cancel invalid requests but keep the prerender going, unless it's the main frame that's invalid." I'm also fine with removing the main frame check, if you want - if it's an invalid URL, we'll just pass the ERR_ABORTED on to the renderer, anyways, when we cancel the request, so not like anything particularly exciting will happen if we don't cancel the prerender. Up to you, I'm good, either way.
Oh, and I don't think this particular CL has a need for any integration tests that weren't already needed... But have you thought about integrations for NoStatePrefetch, if we don't have any already? Make sure the right resources are requested, saved to the cache, and then can be pulled from the cache?
lgtm, thank you! only a few minor, non-mandatory nits mmenke: I agree, the integration tests would be nice, mattcary has a few browsertests almost ready (which I have not looked at yet), let's see whether those can serve the purpose. https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:238: // method has been canonicalized to upper case at this point so we can just nit: s/method/|method|/ https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:249: for (const char* const valid_method : kValidHttpMethodsForPrerendering) { nit: do we really need to be so explicit here about const? perhaps auto* valid_method?
For browser tests, see cls 2304953002 and the follow-on 2350813003 and 2359713002. Comments welcome.
Thanks for the reviews. https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_contents.cc (right): https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:238: // method has been canonicalized to upper case at this point so we can just On 2016/09/21 17:59:53, pasko wrote: > nit: s/method/|method|/ Done. https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_contents.cc:249: for (const char* const valid_method : kValidHttpMethodsForPrerendering) { On 2016/09/21 17:59:53, pasko wrote: > nit: do we really need to be so explicit here about const? > > perhaps auto* valid_method? Done. https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_resource_throttle.cc (right): https://codereview.chromium.org/2355453002/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_resource_throttle.cc:126: // whole prefetch unless this is the main frame. On 2016/09/21 16:56:20, mmenke wrote: > Think this would be clearer if it said what we're doing here, not what we're not > doing here. > > i.e., something like: "If this is a full prerender, cancel the prerender in > response to invalid requests. For prefetches, cancel invalid requests but keep > the prerender going, unless it's the main frame that's invalid." > Done. > I'm also fine with removing the main frame check, if you want - if it's an > invalid URL, we'll just pass the ERR_ABORTED on to the renderer, anyways, when > we cancel the request, so not like anything particularly exciting will happen if > we don't cancel the prerender. Up to you, I'm good, either way.
The CQ bit was checked by droger@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mattcary@chromium.org, pasko@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2355453002/#ps60001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [NoStatePrefetch] Support only GET and HEAD Unlike prerendering, NoStatePrefetch only supports GET and HEAD. ========== to ========== [NoStatePrefetch] Support only GET and HEAD Unlike prerendering, NoStatePrefetch only supports GET and HEAD. Committed: https://crrev.com/56b3870186e8774c5b1015a41ed2f453476857c7 Cr-Commit-Position: refs/heads/master@{#420291} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/56b3870186e8774c5b1015a41ed2f453476857c7 Cr-Commit-Position: refs/heads/master@{#420291} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
