Change the semantics of ImageLoader::has_pending_load_event_

Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.

Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".
This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697, 719759
Review-Url: https://codereview.chromium.org/2859093003
Cr-Commit-Position: refs/heads/master@{#473684}
Committed: https://chromium.googlesource.com/chromium/src/+/2e6f72c80e0106f348fd9437536edc2e9f3b7acc

[hiroshige, 2017-05-05 00:45:53]

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

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

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-05-05 00:47:13]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

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

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

BUG=

This CL replaces
- |has_pending_load_event_| that is true after image loading is started until
load event is fired
to
- |has_pending_load_event_| that is true only while load event is scheduled
(i.e. after ImageNotifyFinished) and
- |image_| which is true from when image loading is started until
ImageNotifyFinished().

To do this systematically, I
1. Replaces reads of |has_pending_load_event_| with |has_pending_load_event_ ||
image_|, and do conditional constant propagation to simplify the code.
2. Checks all writes to |has_pending_load_event_| or |image_| to check whether
|has_pending_load_event_ || image_| is equal to the previous
|has_pending_load_event_|.
==========

[hiroshige, 2017-05-05 01:11:03]

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

[commit-bot: I haz the power, 2017-05-05 01:11:30]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-05-05 01:22:39]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

BUG=

This CL replaces
- |has_pending_load_event_| that is true after image loading is started until
load event is fired
to
- |has_pending_load_event_| that is true only while load event is scheduled
(i.e. after ImageNotifyFinished) and
- |image_| which is true from when image loading is started until
ImageNotifyFinished().

To do this systematically, I
1. Replaces reads of |has_pending_load_event_| with |has_pending_load_event_ ||
image_|, and do conditional constant propagation to simplify the code.
2. Checks all writes to |has_pending_load_event_| or |image_| to check whether
|has_pending_load_event_ || image_| is equal to the previous
|has_pending_load_event_|.
==========

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3).
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by .

This is preparation for that replaces has_pending_load_event_ with a
reference to a task that corresponds to the pending load event.

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

[hiroshige, 2017-05-05 01:23:50]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3).
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by .

This is preparation for that replaces has_pending_load_event_ with a
reference to a task that corresponds to the pending load event.

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

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3).
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

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

[hiroshige, 2017-05-05 01:25:41]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3).
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

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

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3).
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697
==========

[commit-bot: I haz the power, 2017-05-05 03:07:40]

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

[commit-bot: I haz the power, 2017-05-05 03:07:41]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-05-05 19:58:31]

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (left):

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:523:
CHECK(has_pending_load_event_);
Previous: We assert has_pending_load_event_ == true.

Now: This means
     (image_ && !image_complete_) || has_pending_load_event_ == true,
     at the beginning of ImageNotifyFinished().
     image_ is non-null (as ImageNotifyFinished() is called),
     image_complete_ is false (asserted at Line 502)
     therefore this assert always holds and I removed this CHECK().
     (actually has_pending_load_event_ is expected to be false)

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right):

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:166:
RESOURCE_LOADING_DVLOG(1) << "new ImageLoader " << this;
Previous: has_pending_load_event_ == false.

Now: (image_ && !image_complete_) || has_pending_load_event_ == false:
  has_pending_load_event_ == false
  image_complete_ == true
  image_ == null

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:174: << ";
m_hasPendingLoadEvent=" << has_pending_load_event_
I left logging of has_pending_load_event_ as is. ditto below.

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:206: if
(has_pending_load_event_) {
Here we don't check "(image_ && !image_complete_) || has_pending_load_event_"
because, if has_pending_load_event_ == false,
Lines 207--208 should be no-op.

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:220: }
Previous: has_pending_load_event_ == false.

Now: (image_ && !image_complete_) || has_pending_load_event_ == false:
  has_pending_load_event_ == false
  image_complete_ == true

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:345: if
(has_pending_load_event_) {
Ditto as Line 206.

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:364: 
Previous: has_pending_load_event_ == true iff new_image is non-null.

Now: if new_image is non-null,
  (image_ && !image_complete_) || has_pending_load_event_ == true:
    has_pending_load_event_ == false
    image_complete_ == false
    image_ != null

  otherwise,
  (image_ && !image_complete_) || has_pending_load_event_ == false:
    has_pending_load_event_ == false
    image_complete_ == true
    image == null

https://codereview.chromium.org/2859093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:522:
has_pending_load_event_ = false;
Because |image_complete_| is true here,
"(image_ && !image_complete_) || has_pending_load_event_"
is equivalent to has_pending_load_event_, so we just set it to false.

[hiroshige, 2017-05-05 20:04:20]

hiroshige@chromium.org changed reviewers:
+ japhet@chromium.org, kinuko@chromium.org, kouhei@chromium.org,
yhirano@chromium.org

[hiroshige, 2017-05-05 20:04:21]

PTAL.

[kinuko, 2017-05-08 05:36:46]

lgtm

https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.h (right):

https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.h:190: bool
has_pending_load_event_ : 1;
Could we have a short comment for this field here too?

[kouhei (in TOK), 2017-05-08 13:18:40]

https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.h (right):

https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.h:99: return (image_ &&
!image_complete_) || has_pending_load_event_ ||
:) This totally makes sense!

https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.h:190: bool
has_pending_load_event_ : 1;
On 2017/05/08 05:36:46, kinuko wrote:
> Could we have a short comment for this field here too?

+1
I think you basically need to say that we are shortly going to replace this in
https://codereview.chromium.org/2863763003/

[kouhei (in TOK), 2017-05-08 13:19:48]

lgtm of course

[hiroshige, 2017-05-08 19:04:07]

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

[hiroshige, 2017-05-08 19:05:02]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3).
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697
==========

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697
==========

[hiroshige, 2017-05-08 19:05:19]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.
Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".

This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697
==========

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.

Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".
This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697
==========

[hiroshige, 2017-05-08 19:05:26]

https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/ImageLoader.h (right):

https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/ImageLoader.h:190: bool
has_pending_load_event_ : 1;
On 2017/05/08 13:18:40, kouhei (on transit) wrote:
> On 2017/05/08 05:36:46, kinuko wrote:
> > Could we have a short comment for this field here too?
> 
> +1
> I think you basically need to say that we are shortly going to replace this in
> https://codereview.chromium.org/2863763003/

Done.

[commit-bot: I haz the power, 2017-05-08 19:05:44]

Dry run: 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-08 20:12:36]

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

[commit-bot: I haz the power, 2017-05-08 20:12:37]

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

[hiroshige, 2017-05-09 00:11:49]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.

Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".
This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697
==========

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.

Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".
This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697, 719759
==========

[hiroshige, 2017-05-09 00:21:15]

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

[commit-bot: I haz the power, 2017-05-09 00:26:57]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-05-09 00:34:42]

On 2017/05/08 19:05:26, hiroshige wrote:
>
https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/loader/ImageLoader.h (right):
> 
>
https://codereview.chromium.org/2859093003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/loader/ImageLoader.h:190: bool
> has_pending_load_event_ : 1;
> On 2017/05/08 13:18:40, kouhei (on transit) wrote:
> > On 2017/05/08 05:36:46, kinuko wrote:
> > > Could we have a short comment for this field here too?
> > 
> > +1
> > I think you basically need to say that we are shortly going to replace this
in
> > https://codereview.chromium.org/2863763003/
> 
> Done.

Oh, I found this CL causes HasPendingActivity() to be true for ImageDocument
while it is loading (previously it was false).
Perhaps this doesn't cause a problem? (I'll check lsan bots later)

To keep the original behavior, HasPendingActivity() should be:
(image_ && !image_complete_ && !loading_image_document_) ||
has_pending_load_event_ || has_pending_error_event_ || pending_task_;

[commit-bot: I haz the power, 2017-05-09 02:17:23]

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

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

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

[hiroshige, 2017-05-10 16:56:47]

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

[commit-bot: I haz the power, 2017-05-10 16:58:35]

Dry run: 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-10 19:43:33]

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

[commit-bot: I haz the power, 2017-05-10 19:43:34]

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

[hiroshige, 2017-05-10 22:51:25]

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

[commit-bot: I haz the power, 2017-05-10 22:52:35]

Dry run: 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-11 01:30:43]

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

[commit-bot: I haz the power, 2017-05-11 01:30:44]

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

[hiroshige, 2017-05-11 16:06:52]

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

[commit-bot: I haz the power, 2017-05-11 16:07:52]

Dry run: 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-11 18:08:15]

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

[commit-bot: I haz the power, 2017-05-11 18:08:16]

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

[hiroshige, 2017-05-11 18:21:54]

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

[commit-bot: I haz the power, 2017-05-11 18:22:52]

Dry run: 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-11 21:40:57]

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

[commit-bot: I haz the power, 2017-05-11 21:40:58]

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

[hiroshige, 2017-05-11 22:55:02]

> To keep the original behavior, HasPendingActivity() should
> be:
> (image_ && !image_complete_ && !loading_image_document_) ||
> has_pending_load_event_ || has_pending_error_event_ ||
> pending_task_;
I made this change to keep the current behavior anyway, and
introduced HasPendingEvent().

[kinuko, 2017-05-12 08:28:24]

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

https://codereview.chromium.org/2859093003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:625:
has_pending_load_event_ || has_pending_error_event_;
This condition is getting slightly complex, maybe worth adding some comment?

'True if it has pending tasks for load/error event, or regular image load
(!loading_image_document_) is in progress (image_ && !image_complete_)'

[yhirano, 2017-05-12 10:49:50]

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

https://codereview.chromium.org/2859093003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:217: //
|has_pending_load_event_| is always false and |image_complete_| is
Is this comment stale?

[hiroshige, 2017-05-19 20:56:35]

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

https://codereview.chromium.org/2859093003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:217: //
|has_pending_load_event_| is always false and |image_complete_| is
On 2017/05/12 10:49:50, yhirano wrote:
> Is this comment stale?

Done.

https://codereview.chromium.org/2859093003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/ImageLoader.cpp:625:
has_pending_load_event_ || has_pending_error_event_;
On 2017/05/12 08:28:24, kinuko wrote:
> This condition is getting slightly complex, maybe worth adding some comment?
> 
> 'True if it has pending tasks for load/error event, or regular image load
> (!loading_image_document_) is in progress (image_ && !image_complete_)'
> 

Done.

[hiroshige, 2017-05-19 20:56:39]

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

[commit-bot: I haz the power, 2017-05-19 20:57:37]

Dry run: 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-19 23:05:43]

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

[commit-bot: I haz the power, 2017-05-19 23:05:44]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2017-05-22 17:53:05]

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

[commit-bot: I haz the power, 2017-05-22 17:53:36]

Dry run: CQ is trying da patch.

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

[hiroshige, 2017-05-22 19:18:39]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2017-05-22 19:18:45]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2017-05-22 19:18:45]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2859093003/#ps240001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-22 19:19:27]

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-22 20:49:30]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495480725467020,
"parent_rev": "54bafaab23c53c4e2261acd661a837530685dfef", "commit_rev":
"2e6f72c80e0106f348fd9437536edc2e9f3b7acc"}

[commit-bot: I haz the power, 2017-05-22 20:49:43]

Description was changed from

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.

Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".
This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697, 719759
==========

to

==========
Change the semantics of ImageLoader::has_pending_load_event_

ImageLoader works like:
(1) Image loading is started (AddObserver()),
(2) Image loading is finished and the image element's load event is
    scheduled (ImageNotifyFinished()), and then
(3) The load event is fired.

Previously, has_pending_load_event_ was true from (1) to (3).
This CL makes has_pending_load_event_ true only from (2) to (3), i.e.
only while there is a pending task on EventSender.

Because "image_ && !image_complete_" is true from (1) to (2),
The previous "has_pending_load_event_" is equivalent to the current
"(image_ && !image_complete_) || has_pending_load_event_".
This CL, however, leaves most of "has_pending_load_event_" usage
unchanged, because in such cases they are equivalent, given the
assumptions around image_ and image_complete_, some of which are
confirmed by [1].

This is preparation for [2] that replaces has_pending_load_event_ with
a reference to a task that corresponds to the pending load event.

[1] https://codereview.chromium.org/2859383002/
[2] https://codereview.chromium.org/2863763003/

BUG=624697, 719759

Review-Url: https://codereview.chromium.org/2859093003
Cr-Commit-Position: refs/heads/master@{#473684}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e6f72c80e0106f348fd9437536e...
==========

[commit-bot: I haz the power, 2017-05-22 20:49:46]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/2e6f72c80e0106f348fd9437536e...