Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2728643004:
ImageResourceTest: merge successful placeholder/non-placeholder loading (Closed)
DescriptionImageResourceTest: merge successful placeholder/non-placeholder loading
- Introduces testThatIsPlaceholderRequestAndServeResponse() and
testThatIsNotPlaceholderRequestAndServeResponse(), for testing
successful placeholder and non-placeholder loading, and
- Replaces some magic numbers with
|kJpegImageWidth| or |kJpegImageHeight|.
BUG=667641
Review-Url: https://codereview.chromium.org/2728643004
Cr-Commit-Position: refs/heads/master@{#455320}
Committed: https://chromium.googlesource.com/chromium/src/+/e09d3b69602086399cccd6af5d4eb7f8d0f171b3
Patch Set 1 #Patch Set 2 : More refactoring #Patch Set 3 : Rebase. #
Total comments: 3
Patch Set 4 : Fix comment style #Messages
Total messages: 23 (16 generated)
The CQ bit was checked by hiroshige@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 checked by hiroshige@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...
Description was changed from ========== ImageResourceTest: merge code for successfully created placeholder BUG=667641 ========== to ========== ImageResourceTest: merge successful placeholder/non-placeholder loading - Introduces testThatIsPlaceholderRequestAndServeResponse() and testThatIsNotPlaceholderRequestAndServeResponse(), for testing successful placeholder and non-placeholder loading, and - Replaces some magic numbers with |kJpegImageWidth| or |kJpegImageHeight|. BUG=667641 ==========
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, sclittle@chromium.org, yhirano@chromium.org
PTAL.
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 hiroshige@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.
lgtm
LGTM https://codereview.chromium.org/2728643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2728643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:306: // showing a non-placeholder image. nit: could you fix this comment?
https://codereview.chromium.org/2728643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2728643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:306: // showing a non-placeholder image. On 2017/03/07 22:01:26, sclittle wrote: > nit: could you fix this comment? Fixed a little bit. Is this what you meant?
https://codereview.chromium.org/2728643004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2728643004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:306: // showing a non-placeholder image. On 2017/03/07 22:53:15, hiroshige wrote: > On 2017/03/07 22:01:26, sclittle wrote: > > nit: could you fix this comment? > > Fixed a little bit. > Is this what you meant? Yep, thanks!
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2728643004/#ps60001 (title: "Fix comment style")
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": 60001, "attempt_start_ts": 1488929809708680,
"parent_rev": "e72cd03ddb132b80e770939f048f3f80684d2362", "commit_rev":
"e09d3b69602086399cccd6af5d4eb7f8d0f171b3"}
Message was sent while issue was closed.
Description was changed from ========== ImageResourceTest: merge successful placeholder/non-placeholder loading - Introduces testThatIsPlaceholderRequestAndServeResponse() and testThatIsNotPlaceholderRequestAndServeResponse(), for testing successful placeholder and non-placeholder loading, and - Replaces some magic numbers with |kJpegImageWidth| or |kJpegImageHeight|. BUG=667641 ========== to ========== ImageResourceTest: merge successful placeholder/non-placeholder loading - Introduces testThatIsPlaceholderRequestAndServeResponse() and testThatIsNotPlaceholderRequestAndServeResponse(), for testing successful placeholder and non-placeholder loading, and - Replaces some magic numbers with |kJpegImageWidth| or |kJpegImageHeight|. BUG=667641 Review-Url: https://codereview.chromium.org/2728643004 Cr-Commit-Position: refs/heads/master@{#455320} Committed: https://chromium.googlesource.com/chromium/src/+/e09d3b69602086399cccd6af5d4e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e09d3b69602086399cccd6af5d4e... |
