Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2797993007:
Show image placeholders on dimension decode error (Closed)
DescriptionShow image placeholders on dimension decode error
When client LoFi previews feature is on, and if image dimensions cannot
be decoded from the range response, show a placeholder generated from
the dimensions decoded from the full image response.
BUG=692808
Review-Url: https://codereview.chromium.org/2797993007
Cr-Commit-Position: refs/heads/master@{#482154}
Committed: https://chromium.googlesource.com/chromium/src/+/0182d6f08535bda5f8465ce991f720a8f21b28fc
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments #Patch Set 3 : tryjob #
Total comments: 2
Patch Set 4 : fix comment #
Messages
Total messages: 43 (27 generated)
rajendrant@chromium.org changed reviewers: + sclittle@chromium.org
ptal
https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:493: WebURLRequest::PreviewsState previewsState = nit: maybe name this "previewsStateBeforeReload" or "oldPreviewsState" or something to differentiate it from the current previews state of the request? https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:501: if (previewsState & WebURLRequest::ClientLoFiOn) { Chrome should still show the full image if the user manually reloaded the image, e.g. by clicking "Show Original" on the previews infobar. I think |policy| is |kReloadAlways| in that case, so ImageResource could just check that here. https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:505: } Could you add tests for the new behavior you've added? https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/ImageResource.h (right): https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.h:163: // Show placeholder, and do not reload. nit: could you clarify in the comment here that the original image will still be loaded and shown if the image is explicitly reloaded, e.g. when reloadIfLoFiOrPlaceholderImage is called with kReloadAlways?
https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:493: WebURLRequest::PreviewsState previewsState = On 2017/04/06 21:58:06, sclittle wrote: > nit: maybe name this "previewsStateBeforeReload" or "oldPreviewsState" or > something to differentiate it from the current previews state of the request? Renamed as old_previews_state as per new naming convention. https://codereview.chromium.org/2797993007/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:501: if (previewsState & WebURLRequest::ClientLoFiOn) { On 2017/04/06 21:58:06, sclittle wrote: > Chrome should still show the full image if the user manually reloaded the image, > e.g. by clicking "Show Original" on the previews infobar. > > I think |policy| is |kReloadAlways| in that case, so ImageResource could just > check that here. Thanks. Added this check.
Sorry, I must've not seen this one. LGTM!
rajendrant@google.com changed reviewers: + hiroshige@chromium.org, rajendrant@google.com
hiroshige: ptal
hiroshige: friendly ping. ptal
hiroshige: friendly ping. ptal
Sorry for delay, lgtm.
The CQ bit was checked by rajendrant@google.com
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: 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-...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rajendrant@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 #3 (id:40001) has been deleted
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_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 rajendrant@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 rajendrant@google.com
The CQ bit was checked by rajendrant@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2797993007/#ps60001 (title: "tryjob")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ojan@chromium.org changed reviewers: + ojan@chromium.org
lgtm https://codereview.chromium.org/2797993007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2797993007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:247: // EXPECT_TRUE(content->GetImage()->IsBitmapImage()); We don't typically check in commented out code. Did you intend to comment this out?
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
also, please let reviewers know if they don't need to look anymore. I had this open in a tab and was looking it over and didn't realize ojan already reviewed until i refreshed. lgtm with comment addressed. https://codereview.chromium.org/2797993007/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2797993007/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:247: // EXPECT_TRUE(content->GetImage()->IsBitmapImage()); On 2017/06/24 00:10:34, ojan wrote: > We don't typically check in commented out code. Did you intend to comment this > out? +1, please uncomment this or delete it.
The CQ bit was checked by rajendrant@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.
The CQ bit was checked by rajendrant@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org, sclittle@chromium.org, dcheng@chromium.org, ojan@chromium.org Link to the patchset: https://codereview.chromium.org/2797993007/#ps80001 (title: "fix comment")
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": 80001, "attempt_start_ts": 1498324683019070,
"parent_rev": "0a3ecc6715b7ad1c10bc6b4384d0bad5c8e0d165", "commit_rev":
"71a6b44896a5366977b2f3323cc96627f3ab0adf"}
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498324683019070,
"parent_rev": "180095eb1bca7df1cdcb02547340499c2ee3af6e", "commit_rev":
"0182d6f08535bda5f8465ce991f720a8f21b28fc"}
Message was sent while issue was closed.
Description was changed from ========== Show image placeholders on dimension decode error When client LoFi previews feature is on, and if image dimensions cannot be decoded from the range response, show a placeholder generated from the dimensions decoded from the full image response. BUG=692808 ========== to ========== Show image placeholders on dimension decode error When client LoFi previews feature is on, and if image dimensions cannot be decoded from the range response, show a placeholder generated from the dimensions decoded from the full image response. BUG=692808 Review-Url: https://codereview.chromium.org/2797993007 Cr-Commit-Position: refs/heads/master@{#482154} Committed: https://chromium.googlesource.com/chromium/src/+/0182d6f08535bda5f8465ce991f7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/0182d6f08535bda5f8465ce991f7... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
