Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16520007:
Serialize <input type="image"> images (Closed)
|
Created:
7 years, 6 months ago by Tiger Modified:
7 years, 4 months ago Reviewers:
abarth-chromium CC:
blink-reviews, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionSerialize <input type="image"> images
Add support for serializing images specified in an input tag with
type image. This images is extracted in the PageSerializer and added
as a SerializedResource.
R=
BUG=248867
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155598
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use toHTMLInputElement and avoid cast #Patch Set 3 : Added test framework and a test #
Total comments: 4
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : Removed pre-commited image #Patch Set 7 : Rebased #Patch Set 8 : Revert image deletion #Patch Set 9 : Add non-existing resources as error URLs #Patch Set 10 : Rebase #
Messages
Total messages: 33 (0 generated)
Would you be willing to write a test? https://codereview.chromium.org/16520007/diff/1/Source/core/page/PageSerializ... File Source/core/page/PageSerializer.cpp (right): https://codereview.chromium.org/16520007/diff/1/Source/core/page/PageSerializ... Source/core/page/PageSerializer.cpp:229: HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(element); static_cast<HTMLInputElement*> -> toHTMLInputElement https://codereview.chromium.org/16520007/diff/1/Source/core/page/PageSerializ... Source/core/page/PageSerializer.cpp:232: CachedImage* cachedImage = static_cast<RenderImage*>(inputElement->renderer())->cachedImage(); Is this static_cast always safe?
On 2013/06/12 18:41:11, abarth wrote: > Would you be willing to write a test? > Sure! I can't find any existing PageSerializer test, but perhaps you can give me some pointers on how to get started with this?
On 2013/06/13 10:25:48, Gustav Tiger wrote: > Sure! I can't find any existing PageSerializer test, but perhaps you can give me > some pointers on how to get started with this? I did a git log --follow on the file you're changing and noticed that it was introduced in this change: https://chromium.googlesource.com/chromium/blink/+/d08488f5fa0c5e8462768661ec... That change added a number of tests that are still in the same place: https://chromium.googlesource.com/chromium/blink/+/master/Source/WebKit/chrom... Hopefully we can test this change using those tests as a model.
On 2013/06/13 19:57:20, abarth wrote: > On 2013/06/13 10:25:48, Gustav Tiger wrote: > > Sure! I can't find any existing PageSerializer test, but perhaps you can give > me > > some pointers on how to get started with this? > > I did a git log --follow on the file you're changing and noticed that it was > introduced in this change: > > https://chromium.googlesource.com/chromium/blink/+/d08488f5fa0c5e8462768661ec... > > That change added a number of tests that are still in the same place: > > https://chromium.googlesource.com/chromium/blink/+/master/Source/WebKit/chrom... > > Hopefully we can test this change using those tests as a model. If possible it might be a good idea to test the serialization and the generation separately. I added a new PageSerializerTest.cpp for testing the PageSerializer directly. As for the test itself it is quite small. Some comments regarding the test itself: - Interestingly the non-existing-button.png test works as expected here, but if you run it in Chromium it will serialize the 'broken image' image as the non-existing-button.png which seems to be the wrong thing to do. I'll look at fixing this in a later patch. - The data URIs doesn't work as expected either. It will serialize the data which of course results in the image data occurring not once in the resulting data, but three times (or thrice). I'll look at fixing this in a later patch as well.
LGTM Below are just some style nits about your test. Thanks! https://codereview.chromium.org/16520007/diff/9001/Source/WebKit/chromium/tes... File Source/WebKit/chromium/tests/PageSerializerTest.cpp (right): https://codereview.chromium.org/16520007/diff/9001/Source/WebKit/chromium/tes... Source/WebKit/chromium/tests/PageSerializerTest.cpp:44: There's no need for all these blank lines. You can just put the includes in one big list a alphabetize them. The one exception to that is the config.h header, which goes at the top. If there's a primary header (e.g., "Foo.h" for "Foo.cpp"), that goes right after the config.h header. Then there's a single blank line. Then there's all the rest of the headers in an alphabetized list. https://codereview.chromium.org/16520007/diff/9001/Source/WebKit/chromium/tes... Source/WebKit/chromium/tests/PageSerializerTest.cpp:73: class PageSerializeTest : public testing::Test { PageSerializeTest -> PageSerializerTest https://codereview.chromium.org/16520007/diff/9001/Source/WebKit/chromium/tes... Source/WebKit/chromium/tests/PageSerializerTest.cpp:75: PageSerializeTest() : m_folder(WebString::fromUTF8("pageserializer/")), Please add a line break between the () and the : Also, the comma should be vertically aligned under the : rather than at the end of the previous line. https://codereview.chromium.org/16520007/diff/9001/Source/WebKit/chromium/tes... Source/WebKit/chromium/tests/PageSerializerTest.cpp:98: m_webViewImpl->close(); Please set m_webViewImpl to 0 after closing it.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiger@opera.com/16520007/14001
Can't process patch for file Source/WebKit/chromium/tests/data/pageserializer/input-image/button.png. Binary file support is temporarilly disabled due to a bug. Please commit blindly the binary files first then commit the source change as a separate CL. Sorry for the annoyance.
@Gustav: Are you a committer? If not, I can land this CL manually for you.
On 2013/06/18 18:44:31, abarth wrote: > @Gustav: Are you a committer? If not, I can land this CL manually for you. @abarth: No, not a committer. Would be great if you could take care of it abarth, thanks.
I had trouble running this test on my Mac: [----------] 1 test from PageSerializerTest [ RUN ] PageSerializerTest.InputImage [13367:519:0623/140314:1450294456491578:FATAL:platform_support_mac.mm(161)] Failed to locate resources. The applicaiton is not bundled. [13367:519:0623/140314:1450294456491578:FATAL:platform_support_mac.mm(161)] Failed to locate resources. The applicaiton is not bundled. Trace/BPT trap: 5
I landed the binary file for you in https://codereview.chromium.org/17584002 You can try landing the rest of your CL via the commit-queue, but I think you're going to run into that error on Mac.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiger@opera.com/16520007/19001
Can't process patch for file Source/WebKit/chromium/tests/data/pageserializer/input-image/button.png. Binary file support is temporarilly disabled due to a bug. Please commit blindly the binary files first then commit the source change as a separate CL. Sorry for the annoyance.
On 2013/06/24 19:32:36, I haz the power (commit-bot) wrote: > Can't process patch for file > Source/WebKit/chromium/tests/data/pageserializer/input-image/button.png. > Binary file support is temporarilly disabled due to a bug. Please commit blindly > the binary files first then commit the source change as a separate CL. Sorry for > the annoyance. Do I really need to create a separate CL, or can I publish another patch set?
On 2013/06/23 21:09:33, abarth wrote: > I landed the binary file for you in https://codereview.chromium.org/17584002 > > You can try landing the rest of your CL via the commit-queue, but I think you're > going to run into that error on Mac. Thanks. That error is a bit strange.. not sure how these changes would make that error appear. I'll try to run it by the commit bots.
On 2013/06/24 19:33:25, Tiger wrote: > Do I really need to create a separate CL, or can I publish another patch set? Re-using this CL is fine.
On 2013/06/24 19:34:44, Tiger wrote: > Thanks. That error is a bit strange.. not sure how these changes would make that > error appear. I'll try to run it by the commit bots. My config might just have been screwed up.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiger@opera.com/16520007/35001
Retried try job too often on linux_layout_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
On 2013/07/03 14:40:08, I haz the power (commit-bot) wrote: > Retried try job too often on linux_layout_rel for step(s) webkit_unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo... Obviously that didn't work since the button.png image wasn't in the build. Rebased and added the non-existing resources using registerMockedErrorURL. @abarth, does this still LGTM?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiger@opera.com/16520007/54001
Retried try job too often on linux_layout_rel for step(s) webkit_python_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiger@opera.com/16520007/54001
Retried try job too often on mac_layout_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
On 2013/07/09 18:32:15, I haz the power (commit-bot) wrote: > Retried try job too often on mac_layout_rel for step(s) webkit_unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... That "FATAL:platform_support_mac.mm(161) - Failed to locate resources. The application is not bundled." error did occur here as well. I think this is because it tries to read in the broken image data which apparently is not included or at least not working on the mac builds. I actually think this fails on other platforms as well, but does so silently. It would be nice to be able to test what happens to non-existing images in the unit tests -- but right now I'm not sure how to fix this. @abarth, do you have any ideas?
On 2013/07/10 09:39:39, Tiger wrote: > On 2013/07/09 18:32:15, I haz the power (commit-bot) wrote: > > Retried try job too often on mac_layout_rel for step(s) webkit_unit_tests > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout... > > > That "FATAL:platform_support_mac.mm(161) - Failed to locate resources. The > application is not bundled." error did occur here as well. I think this is > because it tries to read in the broken image data which apparently is not > included or at least not working on the mac builds. I actually think this fails > on other platforms as well, but does so silently. It would be nice to be able to > test what happens to non-existing images in the unit tests -- but right now I'm > not sure how to fix this. @abarth, do you have any ideas? Correction: doesn't seem to fail on linux, and probably doesn't on windows either.
Dunno. You'll need to debug locally.
On 2013/07/10 20:08:58, abarth wrote: > Dunno. You'll need to debug locally. Potential fix here: https://codereview.chromium.org/18429012/
On 2013/07/11 15:14:10, Tiger wrote: > On 2013/07/10 20:08:58, abarth wrote: > > Dunno. You'll need to debug locally. > > Potential fix here: https://codereview.chromium.org/18429012/ Fix committed. Rebased this review. Retrying.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tiger@opera.com/16520007/82001
Message was sent while issue was closed.
Change committed as 155598 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
