Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2874073003:
Add layout tests for ImageLoader::UpdateFromElement(kUpdateForcedReload) (Closed)
DescriptionAdd layout tests for ImageLoader::UpdateFromElement(kUpdateForcedReload)
HTMLImageElement::ForceReload() and
ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by
[1] but are not covered by any tests.
This CL adds layout tests for this code path.
This is also confirming the correctness of [2] and [3].
[1] https://codereview.chromium.org/1112513005/
[2] https://codereview.chromium.org/2877583002/
[3] https://codereview.chromium.org/2859383002/
BUG=485295, 624697, 719759
Review-Url: https://codereview.chromium.org/2874073003
Cr-Commit-Position: refs/heads/master@{#471401}
Committed: https://chromium.googlesource.com/chromium/src/+/f33a3ba6de05eaed0115636c6dd552b9f8c80333
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Comment #
Total comments: 8
Patch Set 4 : Reflect comments (test, Internals) #Patch Set 5 : fix #
Total comments: 9
Patch Set 6 : Reflect comments #Patch Set 7 : comment #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 33 (23 generated)
Description was changed from ========== Add layout tests for HTMLImageElement::ForceReload() BUG= ========== to ========== Add layout tests for HTMLImageElement::ForceReload() HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. [1] https://codereview.chromium.org/1112513005 BUG=485295 ==========
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 ========== Add layout tests for HTMLImageElement::ForceReload() HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. [1] https://codereview.chromium.org/1112513005 BUG=485295 ========== to ========== Add layout tests for HTMLImageElement::ForceReload() HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. This is also confirming the correctness of [2] and [3]. [1] https://codereview.chromium.org/1112513005/ [2] https://codereview.chromium.org/2877583002/ [3] https://codereview.chromium.org/2859383002/ BUG=485295, 624697, 719759 ==========
hiroshige@chromium.org changed reviewers: + kouhei@chromium.org, megjablon@chromium.org, yhirano@chromium.org
Description was changed from ========== Add layout tests for HTMLImageElement::ForceReload() HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. This is also confirming the correctness of [2] and [3]. [1] https://codereview.chromium.org/1112513005/ [2] https://codereview.chromium.org/2877583002/ [3] https://codereview.chromium.org/2859383002/ BUG=485295, 624697, 719759 ========== to ========== Add layout tests for ImageLoader::UpdateFromElement(kUpdateForcedReload) HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. This is also confirming the correctness of [2] and [3]. [1] https://codereview.chromium.org/1112513005/ [2] https://codereview.chromium.org/2877583002/ [3] https://codereview.chromium.org/2859383002/ BUG=485295, 624697, 719759 ==========
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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
PTAL.
https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html:15: // We need an URL that returns different non-SVG images for each reload. fix todo? https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:2786: DCHECK(isHTMLImageElement(*element)); replace DCHECKs w/ if (element && isHTMLImageElement(*element))
https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html:15: // We need an URL that returns different non-SVG images for each reload. On 2017/05/11 16:50:52, kouhei (on transit) wrote: > fix todo? Hmm. One way to do this is to have .cgi page (or .php or something) to count the number of requests (by writing to a file on temp dir) and switch some predefined imaged based on the request count. This might be not super-clean (especially multiple tests accessing to the .cgi are running at the same time) though, but the similar thing is already done in some other tests (e.g. third_party/WebKit/LayoutTests/http/tests/resources/network-simulator.php). https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:2786: DCHECK(isHTMLImageElement(*element)); On 2017/05/11 16:50:52, kouhei (on transit) wrote: > replace DCHECKs w/ if (element && isHTMLImageElement(*element)) Because this is Internals, and for debugging/testing purpose I expect crashing by DCHECK() is better than silently skipping non-HTMLImageElement.
https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html:15: // We need an URL that returns different non-SVG images for each reload. On 2017/05/11 17:11:44, hiroshige wrote: > On 2017/05/11 16:50:52, kouhei (on transit) wrote: > > fix todo? > > Hmm. One way to do this is to have .cgi page (or .php or something) to count the > number of requests (by writing to a file on temp dir) and switch some predefined > imaged based on the request count. > This might be not super-clean (especially multiple tests accessing to the .cgi > are running at the same time) though, but the similar thing is already done in > some other tests (e.g. > third_party/WebKit/LayoutTests/http/tests/resources/network-simulator.php). I see. I don't see a super clean way to do this. May be omit the test? https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:2786: DCHECK(isHTMLImageElement(*element)); On 2017/05/11 17:11:44, hiroshige wrote: > On 2017/05/11 16:50:52, kouhei (on transit) wrote: > > replace DCHECKs w/ if (element && isHTMLImageElement(*element)) > > Because this is Internals, and for debugging/testing purpose I expect crashing > by DCHECK() is better than silently skipping non-HTMLImageElement. Other Intenals API throws an exception or silently fails on invalid arg, so I expect forceImageReload to follow it too.
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...
https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html:15: // We need an URL that returns different non-SVG images for each reload. On 2017/05/11 17:30:14, kouhei (on transit) wrote: > On 2017/05/11 17:11:44, hiroshige wrote: > > On 2017/05/11 16:50:52, kouhei (on transit) wrote: > > > fix todo? > > > > Hmm. One way to do this is to have .cgi page (or .php or something) to count > the > > number of requests (by writing to a file on temp dir) and switch some > predefined > > imaged based on the request count. > > This might be not super-clean (especially multiple tests accessing to the .cgi > > are running at the same time) though, but the similar thing is already done in > > some other tests (e.g. > > third_party/WebKit/LayoutTests/http/tests/resources/network-simulator.php). > I see. I don't see a super clean way to do this. May be omit the test? This test is necessary to confirm there's no crashes. (This test executes a code path in ImageLoader that no other tests execute) Updated the tests in Patch Set 5. I made the php script to count the number of request separately for different id parameter to avoid race conditions between different tests. WDYT? https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2874073003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/testing/Internals.cpp:2786: DCHECK(isHTMLImageElement(*element)); On 2017/05/11 17:30:14, kouhei (on transit) wrote: > On 2017/05/11 17:11:44, hiroshige wrote: > > On 2017/05/11 16:50:52, kouhei (on transit) wrote: > > > replace DCHECKs w/ if (element && isHTMLImageElement(*element)) > > > > Because this is Internals, and for debugging/testing purpose I expect crashing > > by DCHECK() is better than silently skipping non-HTMLImageElement. > > Other Intenals API throws an exception or silently fails on invalid arg, so I > expect forceImageReload to follow it too. Sounds good. Done.
lgtm https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php (right): https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php:6: if (!sys_get_temp_dir()) { I'm not sure if we are guaranteed to get same tempdir from sys_get_temp_dir(), but all other tests seems to rely on this...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php (right): https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php:2: # Returns a cacheable image randomly. Isn't this comment (and filename) confusing? The tests expect that they get different images on reload reliably. https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html (right): https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html:18: +img.onerror https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload.html (right): https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload.html:15: +img.onerror https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload.html:25: <img id="img" src="../cache/resources/random-cached-image.php?id=1"> [optional] I think using the test name as id is safer because it's harder to misuse.
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
https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php (right): https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/cache/resources/random-cached-image.php:2: # Returns a cacheable image randomly. On 2017/05/12 10:30:43, yhirano wrote: > Isn't this comment (and filename) confusing? The tests expect that they get > different images on reload reliably. > Maybe. I chose the name because this is used similarly to other random-cached* PHPs. Updated the comment to describe the current situation, but left the name as-is. Probably we might want to refactor how to write such tests, but not in this CL. https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html (right): https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload-image-document.html:18: On 2017/05/12 10:30:43, yhirano wrote: > +img.onerror Done. https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/images/force-reload.html (right): https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload.html:15: On 2017/05/12 10:30:43, yhirano wrote: > +img.onerror Done. https://codereview.chromium.org/2874073003/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/images/force-reload.html:25: <img id="img" src="../cache/resources/random-cached-image.php?id=1"> On 2017/05/12 10:30:43, yhirano wrote: > [optional] I think using the test name as id is safer because it's harder to > misuse. Done.
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, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2874073003/#ps120001 (title: "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": 120001, "attempt_start_ts": 1494609366763250,
"parent_rev": "039db889ebfc27567cf5c699231f9d32ba27dbfa", "commit_rev":
"f33a3ba6de05eaed0115636c6dd552b9f8c80333"}
Message was sent while issue was closed.
Description was changed from ========== Add layout tests for ImageLoader::UpdateFromElement(kUpdateForcedReload) HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. This is also confirming the correctness of [2] and [3]. [1] https://codereview.chromium.org/1112513005/ [2] https://codereview.chromium.org/2877583002/ [3] https://codereview.chromium.org/2859383002/ BUG=485295, 624697, 719759 ========== to ========== Add layout tests for ImageLoader::UpdateFromElement(kUpdateForcedReload) HTMLImageElement::ForceReload() and ImageLoader::UpdateFromElement(kUpdateForcedReload) were introduced by [1] but are not covered by any tests. This CL adds layout tests for this code path. This is also confirming the correctness of [2] and [3]. [1] https://codereview.chromium.org/1112513005/ [2] https://codereview.chromium.org/2877583002/ [3] https://codereview.chromium.org/2859383002/ BUG=485295, 624697, 719759 Review-Url: https://codereview.chromium.org/2874073003 Cr-Commit-Position: refs/heads/master@{#471401} Committed: https://chromium.googlesource.com/chromium/src/+/f33a3ba6de05eaed0115636c6dd5... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f33a3ba6de05eaed0115636c6dd5... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
