|
|
Created:
3 years, 7 months ago by Sunghoon Kim Modified:
3 years, 6 months ago CC:
chromium-reviews, blink-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionConsider empty url case when loading image
If using loadData() in Android WebView or inspector manually,
request url for image whose url is specified in relative url becomes empty.
Currently, it checks only that requested url is null.
Additionally, It should check it is empty.
Also in empty case, it should show broken image and alt text.
BUG=714818
Review-Url: https://codereview.chromium.org/2839633003
Cr-Commit-Position: refs/heads/master@{#474167}
Committed: https://chromium.googlesource.com/chromium/src/+/691a0f3c2d30008438fc155c91d99fde3f57d10a
Patch Set 1 #Patch Set 2 : added unit test #
Total comments: 1
Patch Set 3 : rebased #
Total comments: 2
Patch Set 4 : Consider empty url case when loading image #
Total comments: 2
Patch Set 5 : Consider empty url case when loading image #
Messages
Total messages: 24 (7 generated)
shoon.kim@lge.com changed reviewers: + japhet@chromium.org, tyoshino@chromium.org
Could you please to review this CL? I cannot write TC because if there is base url by loading file it cannot be reproduced.
On 2017/04/25 23:23:06, Sunghoon Kim wrote: > Could you please to review this CL? > I cannot write TC because if there is base url by loading file it cannot be > reproduced. We really ought to have a test for this if possible, and I think it shouldn't be too difficult. Based on the bug and patch description, I see 2 possibilities: * You may be able to write a layout test that triggers this case via inspector interaction. * If this can be triggered via loadData(), you should be able to write a unit test (probably in WebFrameTest.cpp), which can call WebLocalFrame::loadData() directly. My guess is the unit test would be easier to write.
Thank you for your comment. I'll try.
I added a unit test in WebFrameTest (AltTextOnAboutBlankPage) as you recommended. TC uses DumpLayoutTreeAsText() and check it contains alt text "foo". Without this CL, it fails. Review again, please.
Sorry for the delay! https://codereview.chromium.org/2839633003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2839633003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:12021: std::string text = WebFrameContentDumper::DumpLayoutTreeAsText( String text = ToWebLocalFrameImpl(web_view->MainFrame())->GetFrame()->GetDocument()->innerText(); EXPECT_EQ(String("foo"), text); ...or similar would be more canonical.
Thank you for your comment. I tried to get the inner text, but i got empty text. I used like belows * ToWebLocalFrameImpl(web_view->MainFrame())->GetFrame()->GetDocument()->innerText(); * frame->ExecuteCommand(WebString::FromUTF8("SelectAll")); WebString selection_text = frame->SelectionAsText(); When I use console in inspector and try to get the inner text with command "document.documentElement.innerText" with page which has broken image and alt text, it returns "" (empty string). The code in WebFrameTest is the only thing that i found. Could you recommend more smart manner? Thanks.
Ping! PTAL
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/2839633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2839633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:12069: std::string text = WebFrameContentDumper::DumpLayoutTreeAsText( I think this is not a good fit for a unit test that lives in Source/web/tests, because my understanding is that those tests are supposed to use public apis, and DumpLayoutTreeAsText is kind of a big hammer. Options: 1. Just make it a layout test - that doesn't seem unreasonable to me 2. Write a unit test inside core/ so you can access the LayoutText directly and assert - maybe use core/layout/ImageQualityControllerTest.cpp as a guide?
https://codereview.chromium.org/2839633003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2839633003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:12069: std::string text = WebFrameContentDumper::DumpLayoutTreeAsText( Maybe I'm wrong - if accessing a LayoutObject is fine from this directory, then I'd suggest finding the Element using GetElementById(), then getting its layout object, then asserting that it has a LayoutText child with the alt text.
I did change on test code which checks LayoutText and it has alt text. PTAL. Thanks.
Almost there! Just one last test optimization... https://codereview.chromium.org/2839633003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2839633003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:12163: "<img src='foo' alt='foo alt' width='200' height='200'>"; Add id='i' to this... https://codereview.chromium.org/2839633003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:12171: for (LayoutObject* obj = root; obj; obj = obj->NextInPreOrder()) { If I understood dmazzoni's comment correctly, if you add the id to the img tag, you should be able to do: LayoutText* layout_text = ToLayoutText(frame->GetFrame()->GetDocument()->GetElementById("i")->GetLayoutObject()->SlowFirstChild()); ..which would avoid the loop.
Thanks for your comment. I modified test code using getElementById(), but could not remove the loop because the first child of image object is not LayoutText. Please, take a look.
ok, I'll live with that. At least we're walking only a small subset of the layout tree. LGTM.
The CQ bit was checked by shoon.kim@lge.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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by shoon.kim@lge.com
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": 1495588648952920, "parent_rev": "bdaa44e465ae9645d6012a6b08971a7f364698a0", "commit_rev": "691a0f3c2d30008438fc155c91d99fde3f57d10a"}
Message was sent while issue was closed.
Description was changed from ========== Consider empty url case when loading image If using loadData() in Android WebView or inspector manually, request url for image whose url is specified in relative url becomes empty. Currently, it checks only that requested url is null. Additionally, It should check it is empty. Also in empty case, it should show broken image and alt text. BUG=714818 ========== to ========== Consider empty url case when loading image If using loadData() in Android WebView or inspector manually, request url for image whose url is specified in relative url becomes empty. Currently, it checks only that requested url is null. Additionally, It should check it is empty. Also in empty case, it should show broken image and alt text. BUG=714818 Review-Url: https://codereview.chromium.org/2839633003 Cr-Commit-Position: refs/heads/master@{#474167} Committed: https://chromium.googlesource.com/chromium/src/+/691a0f3c2d30008438fc155c91d9... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/691a0f3c2d30008438fc155c91d9... |