Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
485023003:
Use range request for CORS access check on Android. (Closed)
|
Created:
6 years, 4 months ago by DaleCurtis Modified:
5 years, 5 months ago CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionUse range request for CORS access check on Android.
Avoids unnecessarily downloading data which will be unused.
BUG=400788
TEST=none
Committed: https://crrev.com/364fed76f11a1e81e89b7de0686723ec6ad8736d
Cr-Commit-Position: refs/heads/master@{#291944}
Patch Set 1 #Patch Set 2 : Use range request. #
Total comments: 4
Patch Set 3 : Comments. #Messages
Total messages: 21 (0 generated)
I've been looking through this code recently, saw the bug, fix seems relatively easy... Haven't been able to test yet, do you have any recommendations besides the bots?
lgtm. I don't have any better ways to test this either
On 2014/08/23 00:30:20, qinmin wrote: > lgtm. I don't have any better ways to test this either When testing videos from the Cast perspective, we found some popular sites that don't respect HEAD requests. See http://code.google.com/p/chromium/issues/detail?id=367790 for an example.
Yeah, the same thought occurred to me over the weekend. It looks like you resolved this with a 1-byte range request, which in the worst case ends up falling back to a normal request that's cancelled.
dalecurtis@chromium.org changed reviewers: + acolwell@chromium.org
Switched to a range request. +acolwell to double check my usage here.
https://codereview.chromium.org/485023003/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_info_loader.cc (right): https://codereview.chromium.org/485023003/diff/20001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:57: request.addHTTPHeaderField("Range", "bytes=0-1"); nit: This is a 2 byte range request. Please update the comment. https://codereview.chromium.org/485023003/diff/20001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:132: if (response.httpStatusCode() == kHttpOK) { Range requests result in a 206 response so you'll need to update the code to handle that.
Thanks for review. https://codereview.chromium.org/485023003/diff/20001/content/renderer/media/a... File content/renderer/media/android/media_info_loader.cc (right): https://codereview.chromium.org/485023003/diff/20001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:57: request.addHTTPHeaderField("Range", "bytes=0-1"); On 2014/08/25 19:40:08, acolwell wrote: > nit: This is a 2 byte range request. Please update the comment. Done. https://codereview.chromium.org/485023003/diff/20001/content/renderer/media/a... content/renderer/media/android/media_info_loader.cc:132: if (response.httpStatusCode() == kHttpOK) { On 2014/08/25 19:40:08, acolwell wrote: > Range requests result in a 206 response so you'll need to update the code to > handle that. Done.
lgtm
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/485023003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/485023003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by dalecurtis@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/485023003/40001
Message was sent while issue was closed.
Committed patchset #3 (40001) as 831c98e9650ed897908869ab46d399d5ac15337d
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/364fed76f11a1e81e89b7de0686723ec6ad8736d Cr-Commit-Position: refs/heads/master@{#291944} |
