Consider empty url case when loading image

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/+/691a0f3c2d30008438fc155c91d99fde3f57d10a

[Sunghoon Kim, 2017-04-25 23:23:05]

shoon.kim@lge.com changed reviewers:
+ japhet@chromium.org, tyoshino@chromium.org

[Sunghoon Kim, 2017-04-25 23:23:06]

Could you please to review this CL?
I cannot write TC because if there is base url by loading file it cannot be
reproduced.

[Nate Chapin, 2017-04-25 23:30:36]

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.

[Sunghoon Kim, 2017-04-25 23:32:16]

Thank you for your comment.
I'll try.

[Sunghoon Kim, 2017-04-26 05:58:46]

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.

[Nate Chapin, 2017-05-02 22:58:03]

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.

[Sunghoon Kim, 2017-05-08 02:19:50]

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.

[Sunghoon Kim, 2017-05-16 07:29:06]

Ping! PTAL

[dmazzoni, 2017-05-16 21:08:05]

dmazzoni@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dmazzoni, 2017-05-16 21:08:06]

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?

[dmazzoni, 2017-05-16 21:10:43]

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.

[Sunghoon Kim, 2017-05-17 05:05:40]

I did change on test code which checks LayoutText and it has alt text.
PTAL.
Thanks.

[Nate Chapin, 2017-05-17 17:53:50]

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.

[Sunghoon Kim, 2017-05-18 00:08:19]

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.

[Nate Chapin, 2017-05-23 23:37:54]

ok, I'll live with that. At least we're walking only a small subset of the
layout tree. LGTM.

[Sunghoon Kim, 2017-05-23 23:40:29]

The CQ bit was checked by shoon.kim@lge.com

[commit-bot: I haz the power, 2017-05-23 23:43:55]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 00:35:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 00:35:17]

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_...)

[Sunghoon Kim, 2017-05-24 01:17:28]

The CQ bit was checked by shoon.kim@lge.com

[commit-bot: I haz the power, 2017-05-24 01:18:20]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 04:59:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495588648952920,
"parent_rev": "bdaa44e465ae9645d6012a6b08971a7f364698a0", "commit_rev":
"691a0f3c2d30008438fc155c91d99fde3f57d10a"}

[commit-bot: I haz the power, 2017-05-24 05:00:00]

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...
==========

[commit-bot: I haz the power, 2017-05-24 05:00:02]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/691a0f3c2d30008438fc155c91d9...