|
|
Chromium Code Reviews|
Created:
3 years, 10 months ago by Yoav Weiss Modified:
3 years, 10 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, kinuko+watch, Nate Chapin Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove type mismatch condition in requestResource
This CL removes the type matching condition in ResourceFetcher::requestResource()
which shouldn't be hit and replaces it with a CHECK to make sure it is actually never
hit.
BUG=690632
Review-Url: https://codereview.chromium.org/2686533005
Cr-Commit-Position: refs/heads/master@{#449547}
Committed: https://chromium.googlesource.com/chromium/src/+/25b17c1a107cbc956487b5a6bb4698b986f6f035
Patch Set 1 #Patch Set 2 : Fix determineRevaliationPolicy #Messages
Total messages: 22 (14 generated)
The CQ bit was checked by yoav@yoav.ws 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 ========== Try to remove type mismatch condition in requestResource This CL tries to remove a condition which shouldn't be hit and replaces it with a CHECK to make sure it is actually never hit. BUG= ========== to ========== Remove type mismatch condition in requestResource This CL removes the type matching condition in ResourceFetcher::requestResource() which shouldn't be hit and replaces it with a CHECK to make sure it is actually never hit. BUG= ==========
yoav@yoav.ws changed reviewers: + csharrison@chromium.org, kinuko@chromium.org
lgtm
Yoav, do you mind attaching a crbug and referencing it in the code? We should have it on our radar to change the CHECK to a DCHECK if no crashes occur.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Looks like the removed check is in fact exercised under several conditions. I'll dig into it and add appropriate documentation
The CQ bit was checked by yoav@yoav.ws 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...
On 2017/02/09 07:39:29, Yoav Weiss wrote: > Looks like the removed check is in fact exercised under several conditions. I'll > dig into it and add appropriate documentation So, I discovered why that condition was in place. Turns out determineRevalidationPolicy was skipping the type check for preloads, so it got added later down requestResource(). Since that makes little sense, I switched the order of checks in determineRevalidationPolicy so that mismatched type preloads will `Reload` rather than `Use`, and removed the condition. Kinuko, Charlie PTAL?
Description was changed from ========== Remove type mismatch condition in requestResource This CL removes the type matching condition in ResourceFetcher::requestResource() which shouldn't be hit and replaces it with a CHECK to make sure it is actually never hit. BUG= ========== to ========== Remove type mismatch condition in requestResource This CL removes the type matching condition in ResourceFetcher::requestResource() which shouldn't be hit and replaces it with a CHECK to make sure it is actually never hit. BUG=690632 ==========
LGTM, assuming CQ is happy
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Cool! still LGTM
The CQ bit was checked by yoav@yoav.ws
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": 20001, "attempt_start_ts": 1486702768826930,
"parent_rev": "769f9a4d6edea0cf066def627d919a65f678fa18", "commit_rev":
"25b17c1a107cbc956487b5a6bb4698b986f6f035"}
Message was sent while issue was closed.
Description was changed from ========== Remove type mismatch condition in requestResource This CL removes the type matching condition in ResourceFetcher::requestResource() which shouldn't be hit and replaces it with a CHECK to make sure it is actually never hit. BUG=690632 ========== to ========== Remove type mismatch condition in requestResource This CL removes the type matching condition in ResourceFetcher::requestResource() which shouldn't be hit and replaces it with a CHECK to make sure it is actually never hit. BUG=690632 Review-Url: https://codereview.chromium.org/2686533005 Cr-Commit-Position: refs/heads/master@{#449547} Committed: https://chromium.googlesource.com/chromium/src/+/25b17c1a107cbc956487b5a6bb46... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/25b17c1a107cbc956487b5a6bb46... |
