Issue 2247073006: Add loadend event when finishing loading image

Created:
4 years, 4 months ago by Peng Xinchao

Modified:
4 years, 3 months ago

Reviewers:
Nate Chapin, Mike West

CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, blink-reviews-w3ctests_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, eae+blinkwatch, gavinp+loader_chromium.org, jam, Nate Chapin, kochi, loading-reviews_chromium.org, rwlbuis, sof, tfarina, tyoshino+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add loadend event when finishing loading image BUG=627706 Signed-off-by: Peng Xinchao <pxinchao@gmail.com>;

Patch Set 1 #

Total comments: 3

Patch Set 2 : Add loadend event when finishing loading image #

Patch Set 3 : fix issue of layoutTest #

Created: 4 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+56 lines, -2 lines)			Patch
M	third_party/WebKit/LayoutTests/TestExpectations	View		1 chunk	+0 lines, -1 line	0 comments	Download
A	third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/invalid-src-expected.txt	View	1 2	1 chunk	+5 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/dom/Document.cpp	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/html/HTMLImageLoader.h	View	1 2	1 chunk	+4 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/html/HTMLImageLoader.cpp	View	1 2	4 chunks	+29 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/loader/ImageLoader.h	View	1 2	1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/loader/ImageLoader.cpp	View	1 2	4 chunks	+13 lines, -1 line	0 comments	Download

Messages

Total messages: 13 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Mike West

The code change looks good, but adding a test to the imported directory is not ...

4 years, 4 months ago (2016-08-18 08:38:39 UTC) #3

Peng Xinchao

On 2016/08/18 08:38:39, Mike West (OOO until 29th) wrote: > The code change looks good, ...

4 years, 4 months ago (2016-08-19 10:00:41 UTC) #4

On 2016/08/18 08:38:39, Mike West (OOO until 29th) wrote:
> The code change looks good, but adding a test to the imported directory is not
> going to work. See below:
> 
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> File
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html
> (right):
> 
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:1:
> <!doctype html>
> Creating a new imported test seems wrong. I'd suggest upstreaming this test to
> WPT instead. The processes for getting new tests committed the W3C repos are
at
> http://testthewebforward.org/docs/submission-process.html (basically, just
open
> a PR against https://github.com/w3c/web-platform-tests/).
> 
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:13:
> img.addEventListener('error', t.step_func(function(){errorevent = true;}));
> Style nit: The function should be split across lines, as you've done in the
next
> test.
> 
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:24:
> img.addEventListener('load', t.step_func(function(){loadevent = true;}));
> Style nit: Ditto.
@Mike, thank you for your reivewed.
In patch 2, i delete my new testcase, and still keep the invalid-str.html.
But i still have a issue .
html/semantics/embedded-content/the-img-element/invalid-src.html still be
timeouts.
I guess that  becasue the img's url is "http://also a broken ur".It will request
url from network.
But the layout test will be timeouts before the img received  error event .
I tried to change from 'img.src = "http://also a broken url";' to 'img.src =
"also a broken url";' or 'img.src = "wrongurl.img"'. 
The testcase is passed.
Could you help me?
Thanks

Peng Xinchao

On 2016/08/19 10:00:41, Peng Xinchao wrote: > On 2016/08/18 08:38:39, Mike West (OOO until 29th) ...

4 years, 4 months ago (2016-08-22 06:35:00 UTC) #5

On 2016/08/19 10:00:41, Peng Xinchao wrote:
> On 2016/08/18 08:38:39, Mike West (OOO until 29th) wrote:
> > The code change looks good, but adding a test to the imported directory is
not
> > going to work. See below:
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> > File
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:1:
> > <!doctype html>
> > Creating a new imported test seems wrong. I'd suggest upstreaming this test
to
> > WPT instead. The processes for getting new tests committed the W3C repos are
> at
> > http://testthewebforward.org/docs/submission-process.html (basically, just
> open
> > a PR against https://github.com/w3c/web-platform-tests/).
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:13:
> > img.addEventListener('error', t.step_func(function(){errorevent = true;}));
> > Style nit: The function should be split across lines, as you've done in the
> next
> > test.
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:24:
> > img.addEventListener('load', t.step_func(function(){loadevent = true;}));
> > Style nit: Ditto.
> @Mike, thank you for your reivewed.
> In patch 2, i delete my new testcase, and still keep the invalid-str.html.
> But i still have a issue .
> html/semantics/embedded-content/the-img-element/invalid-src.html still be
> timeouts.
> I guess that  becasue the img's url is "http://also a broken ur".It will
request
> url from network.
> But the layout test will be timeouts before the img received  error event .
> I tried to change from 'img.src = "http://also a broken url";' to 'img.src =
> "also a broken url";' or 'img.src = "wrongurl.img"'. 
> The testcase is passed.
> Could you help me?
> Thanks

@Mike, i have fixed it. pleae review agian. thank you!

Peng Xinchao

On 2016/08/19 10:00:41, Peng Xinchao wrote: > On 2016/08/18 08:38:39, Mike West (OOO until 29th) ...

4 years, 4 months ago (2016-08-22 06:35:00 UTC) #6

On 2016/08/19 10:00:41, Peng Xinchao wrote:
> On 2016/08/18 08:38:39, Mike West (OOO until 29th) wrote:
> > The code change looks good, but adding a test to the imported directory is
not
> > going to work. See below:
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> > File
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html
> > (right):
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:1:
> > <!doctype html>
> > Creating a new imported test seems wrong. I'd suggest upstreaming this test
to
> > WPT instead. The processes for getting new tests committed the W3C repos are
> at
> > http://testthewebforward.org/docs/submission-process.html (basically, just
> open
> > a PR against https://github.com/w3c/web-platform-tests/).
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:13:
> > img.addEventListener('error', t.step_func(function(){errorevent = true;}));
> > Style nit: The function should be split across lines, as you've done in the
> next
> > test.
> > 
> >
>
https://codereview.chromium.org/2247073006/diff/1/third_party/WebKit/LayoutTe...
> >
>
third_party/WebKit/LayoutTests/imported/wpt/html/semantics/embedded-content/the-img-element/img.loadend.html:24:
> > img.addEventListener('load', t.step_func(function(){loadevent = true;}));
> > Style nit: Ditto.
> @Mike, thank you for your reivewed.
> In patch 2, i delete my new testcase, and still keep the invalid-str.html.
> But i still have a issue .
> html/semantics/embedded-content/the-img-element/invalid-src.html still be
> timeouts.
> I guess that  becasue the img's url is "http://also a broken ur".It will
request
> url from network.
> But the layout test will be timeouts before the img received  error event .
> I tried to change from 'img.src = "http://also a broken url";' to 'img.src =
> "also a broken url";' or 'img.src = "wrongurl.img"'. 
> The testcase is passed.
> Could you help me?
> Thanks

@Mike, i have fixed it. pleae review agian. thank you!

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2247073006/40001

4 years, 3 months ago (2016-08-31 01:19:18 UTC) #8

commit-bot: I haz the power

No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...

4 years, 3 months ago (2016-08-31 01:19:21 UTC) #10

Peng Xinchao

On 2016/08/31 01:19:21, commit-bot: I haz the power wrote: > No L-G-T-M from a valid ...

4 years, 3 months ago (2016-09-06 02:45:18 UTC) #11

@japhet , Could you help me to review it ?

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages