[Not committed] Make image load completion async and remove EventSender from ImageLoader

Make image load completion async and remove EventSender from ImageLoader

New attempt -> https://codereview.chromium.org/2863763003/

Previously, ImageLoader::notifyFinished() is the point of image completion:
i.e. after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), by adding an
IncrementLoadEventDelayCount that blocks document load event between
ImageLoader::notifyFinished() and ImageLoader::finish().

This CL
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes (catched by newly added loading/onload/ tests) are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170

[hiroshige, 2016-03-25 22:34:31]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 22:34:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/1

[commit-bot: I haz the power, 2016-03-25 22:50:34]

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

[commit-bot: I haz the power, 2016-03-25 22:50:35]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[hiroshige, 2016-03-25 23:12:57]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-25 23:13:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/20001

[commit-bot: I haz the power, 2016-03-25 23:33:50]

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

[commit-bot: I haz the power, 2016-03-25 23:33:51]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[hiroshige, 2016-03-26 00:42:37]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-26 00:42:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/40001

[commit-bot: I haz the power, 2016-03-26 00:58:23]

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

[commit-bot: I haz the power, 2016-03-26 00:58:24]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[hiroshige, 2016-03-28 21:28:23]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-28 21:28:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/60001

[hiroshige, 2016-03-28 21:30:20]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-28 21:30:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/80001

[commit-bot: I haz the power, 2016-03-28 21:33:12]

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

[commit-bot: I haz the power, 2016-03-28 21:33:13]

Dry run: Try jobs failed on following builders:
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[hiroshige, 2016-03-28 21:45:32]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-28 21:45:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/100001

[hiroshige, 2016-03-28 21:46:15]

Description was changed from

==========
async finish

BUG=
==========

to

==========
Make Images' onload event async

BUG=
==========

[commit-bot: I haz the power, 2016-03-28 22:37:06]

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

[commit-bot: I haz the power, 2016-03-28 22:37:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2016-03-28 22:45:04]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-28 22:45:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/120001

[commit-bot: I haz the power, 2016-03-28 23:48:12]

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

[commit-bot: I haz the power, 2016-03-28 23:48:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[hiroshige, 2016-03-29 00:59:02]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 00:59:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/140001

[hiroshige, 2016-03-29 01:14:23]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 01:14:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/160001

[hiroshige, 2016-03-29 01:16:53]

Description was changed from

==========
Make Images' onload event async

BUG=
==========

to

==========
Make Image onload event async

BUG=
==========

[commit-bot: I haz the power, 2016-03-29 02:23:06]

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

[commit-bot: I haz the power, 2016-03-29 02:23:08]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[hiroshige, 2016-03-29 03:23:07]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 03:23:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/200001

[hiroshige, 2016-03-29 03:33:11]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 03:33:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/220001

[commit-bot: I haz the power, 2016-03-29 04:35:52]

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

[commit-bot: I haz the power, 2016-03-29 04:35:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2016-03-29 17:21:09]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 17:21:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/240001

[hiroshige, 2016-03-29 18:28:26]

Description was changed from

==========
Make Image onload event async

BUG=
==========

to

==========
Make image load completion async and remove EventSender from ImageLoader

Previously, ImageLoader::notifyFinished() is the point of image completion:
after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), in order to:
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170
==========

[hiroshige, 2016-03-29 18:28:26]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org

[hiroshige, 2016-03-29 18:31:05]

Description was changed from

==========
Make image load completion async and remove EventSender from ImageLoader

Previously, ImageLoader::notifyFinished() is the point of image completion:
after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), in order to:
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170
==========

to

==========
Make image load completion async and remove EventSender from ImageLoader

Previously, ImageLoader::notifyFinished() is the point of image completion:
i.e. after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), by adding an
IncrementLoadEventDelayCount that blocks document load event between
ImageLoader::notifyFinished() and ImageLoader::finish().

This CL
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170
==========

[hiroshige, 2016-03-29 18:32:35]

Nate, what do you think of this?

[hiroshige, 2016-03-29 18:33:35]

Description was changed from

==========
Make image load completion async and remove EventSender from ImageLoader

Previously, ImageLoader::notifyFinished() is the point of image completion:
i.e. after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), by adding an
IncrementLoadEventDelayCount that blocks document load event between
ImageLoader::notifyFinished() and ImageLoader::finish().

This CL
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170
==========

to

==========
Make image load completion async and remove EventSender from ImageLoader

Previously, ImageLoader::notifyFinished() is the point of image completion:
i.e. after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), by adding an
IncrementLoadEventDelayCount that blocks document load event between
ImageLoader::notifyFinished() and ImageLoader::finish().

This CL
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes (catched by newly added loading/onload/ tests) are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170
==========

[commit-bot: I haz the power, 2016-03-29 18:35:20]

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

[commit-bot: I haz the power, 2016-03-29 18:35:21]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[Nate Chapin, 2016-03-29 19:04:13]

This seems reasonable.

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:172: if
(m_loadDelayCounterForError) {
Why does this have an if() wrapper, but cancelPendingFinish() doesn't?

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:466: void
ImageLoader::scheduleFinish()
scheduleFinish() is only called once. Inline it in notifyFinished()?

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:475: ImageResource*
cachedImage = m_image.get();
cachedImage->imageResource? CachedImage is what ImageResource was called until
about 2013, and it's confusing to have a new variable with that name.

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:476: ASSERT(cachedImage);
The theory is that the calls to cancelPendingFinish() handle all cases where
m_image might be cleared or modified while waiting for the task to happen?

[hiroshige, 2016-03-29 19:20:48]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 19:20:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/260001

[hiroshige, 2016-03-29 19:43:41]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 19:44:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/280001

[commit-bot: I haz the power, 2016-03-29 20:56:41]

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

[commit-bot: I haz the power, 2016-03-29 20:56:42]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[hiroshige, 2016-03-29 21:41:50]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 21:42:20]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/300001

[hiroshige, 2016-03-29 21:49:53]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 21:50:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/320001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/320001

[hiroshige, 2016-03-29 21:51:59]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-29 21:52:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/340001

[commit-bot: I haz the power, 2016-03-29 23:02:36]

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

[commit-bot: I haz the power, 2016-03-29 23:02:37]

Dry run: Try jobs failed on following builders:
  linux_blink_oilpan_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_blink_oil...)

[hiroshige, 2016-04-04 01:23:49]

Patchset #19 (id:360001) has been deleted

[hiroshige, 2016-04-04 02:19:48]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 02:20:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/380001

[commit-bot: I haz the power, 2016-04-04 04:37:36]

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

[commit-bot: I haz the power, 2016-04-04 04:37:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hiroshige, 2016-04-04 05:15:55]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 05:16:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/380001

[hiroshige, 2016-04-04 05:19:15]

The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-04 05:19:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1833303002/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1833303002/400001

[commit-bot: I haz the power, 2016-04-04 06:53:52]

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

[commit-bot: I haz the power, 2016-04-04 06:53:54]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-05-18 22:13:15]

Closing this old issue, as a new attempt
https://codereview.chromium.org/2863763003/ is being made.

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:172: if
(m_loadDelayCounterForError) {
On 2016/03/29 19:04:13, Nate Chapin wrote:
> Why does this have an if() wrapper, but cancelPendingFinish() doesn't?

|m_loadDelayCounterForError| is non-null only between scheduleErrorEvent() and
its actual dispatching (i.e. while |m_dispatchErrorEventTask| is on flight), so
we can put Line 173-174 into the if statement. I think we can also remove the if
condition at Line 172 safely.
This is inherited e.g. from L224-227 on the left.

As for cancelPendingFinish(), if we place corresponding if statements, then it
would be:
if (m_loadDelayCounterForFinish) {
    // We have to always cancel in-flight async finish() calls, because
    // we always post |m_finishTask| in notifyFinished(), regardless
    // of |m_hasPendingLoadEvent|.
    // |m_loadDelayCounterForFinish| is true only if |m_finishTask| is
    // in-flight.
    m_finishTask->cancel();
    m_loadDelayCounterForFinish.clear();
}
if (m_hasPendingLoadEvent) {
    // This if statement is inherited from e.g. Line 220 on the left.
    // There was |loadEventSender().cancelEvent(this);| on the left, but
    // we no longer need to cancel in-flight load event, because load event
    // is now dispatched synchronously in finish().
    // I removed this if-statement because it doesn't look meaningful now.
    m_hasPendingLoadEvent = false;
}

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:466: void
ImageLoader::scheduleFinish()
On 2016/03/29 19:04:13, Nate Chapin wrote:
> scheduleFinish() is only called once. Inline it in notifyFinished()?

Done.

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:475: ImageResource*
cachedImage = m_image.get();
On 2016/03/29 19:04:13, Nate Chapin wrote:
> cachedImage->imageResource? CachedImage is what ImageResource was called until
> about 2013, and it's confusing to have a new variable with that name.

Done.

https://codereview.chromium.org/1833303002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:476: ASSERT(cachedImage);
On 2016/03/29 19:04:13, Nate Chapin wrote:
> The theory is that the calls to cancelPendingFinish() handle all cases where
> m_image might be cleared or modified while waiting for the task to happen?

Yes. I inserted cancelPendingFinish() at Line 412 to make this sure.

I thought ImageLoader can/should also assume |m_image| non-null here, because
- HTMLImageLoader::notifyFinished() in the current code base assumes |m_image|
to be non-null (no null checks)
- ImageLoader::notifyFinished() contains |ASSERT(resource == m_image.get());|,
which implies |m_image| is non-null.

(ImageLoader::notifyFinished() contains null checks for |m_image| though. Can we
remove those null checks?)

[hiroshige, 2017-05-18 22:13:37]

Description was changed from

==========
Make image load completion async and remove EventSender from ImageLoader

Previously, ImageLoader::notifyFinished() is the point of image completion:
i.e. after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), by adding an
IncrementLoadEventDelayCount that blocks document load event between
ImageLoader::notifyFinished() and ImageLoader::finish().

This CL
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes (catched by newly added loading/onload/ tests) are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170
==========

to

==========
Make image load completion async and remove EventSender from ImageLoader

New attempt -> https://codereview.chromium.org/2863763003/

Previously, ImageLoader::notifyFinished() is the point of image completion:
i.e. after that point,
- |img.complete| is true (and thus we can draw the image on a canvas),
- <img>'s load event can be dispatched, and
- the image loading no longer blocks document's load event.

This CL moves the point to ImageLoader::finish(), which is called
asynchronously from ImageLoader::notifyFinished(), by adding an
IncrementLoadEventDelayCount that blocks document load event between
ImageLoader::notifyFinished() and ImageLoader::finish().

This CL
- Fix Issue 382170. The problem was that |img.complete| becomes true before
  font (data URL) subresource loading of a SVG completes.
  A subsequent CL [1] will start font subresource loading in
  SVGImage::dataChanged(), which is called before ImageLoader::notifyFinished()
  in successful loading, and thus asynchronous data URL loading is completed
  before ImageLoader::finish() and thus before |img.complete| becomes true.
  [1] https://codereview.chromium.org/1832333003/
- Remove EventSender singletons in ImageLoader.
  loadEventSender()/errorEventSender() are singletons shared among all
  ResourceFetchers, and they cause causality between image/document's load
  event timing between different ResourceFetcher.
  This CL replaces EventSenders with CancellableTasks per ImageLoader.

Observable behavior changes (catched by newly added loading/onload/ tests) are:
- Whether |img.complete| becomes true synchronously when setting |img.src|:
    For cached images: Previous Chromium: Yes / Firefox: Yes / This CL: No
      (e.g. images with the same URL is loaded multiple times in a page)
    For data URLs: Previous Chromium: Yes / Firefox: No / This CL: No
  This change causes changes in the layout tests in this CL.
- Whether starting loading a new image in another image's load event blocks
  document's load event that would be dispatched otherwise:
    Previous Chromium: No / Firefox: Yes / This CL: Yes

BUG=382170
==========