Mark ResourceClient/ImageResourceObserver finished just before notifying

Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG=633696
Committed: https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918
Cr-Commit-Position: refs/heads/master@{#414423}

[hiroshige, 2016-08-03 10:10:21]

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

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

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, 2016-08-03 11:01:29]

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

[commit-bot: I haz the power, 2016-08-03 11:01:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hiroshige, 2016-08-03 19:35:17]

Description was changed from

==========
Mark ResourceClient/ImageResourceObserver finished just before notifying

BUG=633696
==========

to

==========
Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG=633696
==========

[hiroshige, 2016-08-03 19:35:17]

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

[hiroshige, 2016-08-03 19:36:51]

PTAL. Probably this should be merged into M-52 (stable), but not so sure this CL
is mergeable.

[hiroshige, 2016-08-03 19:45:07]

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

[commit-bot: I haz the power, 2016-08-03 19:45:39]

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, 2016-08-03 22:02:31]

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

[commit-bot: I haz the power, 2016-08-03 22:02:32]

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

[hiroshige, 2016-08-04 05:59:24]

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

[commit-bot: I haz the power, 2016-08-04 05:59:42]

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, 2016-08-04 07:27:43]

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

[commit-bot: I haz the power, 2016-08-04 07:27:43]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-08-04 15:33:39]

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

[hiroshige, 2016-08-04 15:33:40]

+japhet@, could you also take a look as a core/fetch OWNER?

The issue is regression since M-52 (current stable), so perhaps I have to merge
this to M-52.

[Nate Chapin, 2016-08-09 00:05:00]

LGTM. I'm ok with this getting merged to M-52, but I find this enum value
difficult to reason about, and I'd ideally like to find a solution that doesn't
require it. Would it be reasonable to add a TODO to investigate it further?

[yhirano, 2016-08-09 09:35:59]

Is it possible / desirable to
 1. Keep the checkNotify interface
 2. Mark clients as finished in checkNotify
 3. Create a non-virtual function Resource::checkNotifyWithoutMarkingAsFinished
 4. Insert some custom logic in  ImageResource::onePartInMultipartReceived

?

That way other Resource subclasses won't need to worry about the case where
checkNotify doesn't mark clients as finished.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html
(right):

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:11:
var url  = '../cache/resources/etag-200.php?' + Math.floor(100000000 *
Math.random());
const

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:13:
xhr1 = new XMLHttpRequest();
const

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:16:
shouldBeEqualToNumber("xhr1.status", 200);
indent

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:18:
xhr2 = new XMLHttpRequest();
const

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html
(right):

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:11:
var url  = '../cache/resources/etag.php?' + Math.floor(100000000 *
Math.random());
const

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:13:
xhr1 = new XMLHttpRequest();
const

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:16:
shouldBeEqualToNumber("xhr1.status", 200);
indent

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:18:
xhr2 = new XMLHttpRequest();
const

[hiroshige, 2016-08-09 10:23:55]

> but I find this enum value
> difficult to reason about, and I'd ideally like to find a solution that
doesn't
> require it. Would it be reasonable to add a TODO to investigate it further?
This is related to whether we should call second notifyFinished() when multipart
image loading is cancelled after the first part ends.
Currently, when multipart image loading is cancelled after the first part ends,
notifyFinished() is called with errorOccurred() == true and the image is
replaced with the broken image marker.
If we can omit it (i.e. we can keep the last image before cancellation), we can
omit the enum and always mark clients finished in checkNotify().
(Also we have to modify layout tests like http/tests/multipart/stop-loading.html
that checks whether the loading is cancelled by checking whether the image
becomes broken, but I have a WIP CL for that)

I feel we can omit it, but not so sure we can merge it to stable.

On 2016/08/09 09:35:59, yhirano wrote:
> Is it possible / desirable to
>  1. Keep the checkNotify interface
>  2. Mark clients as finished in checkNotify
>  3. Create a non-virtual function
Resource::checkNotifyWithoutMarkingAsFinished
>  4. Insert some custom logic in  ImageResource::onePartInMultipartReceived
IIUC,
- checkNotifyWithoutMarkingAsFinished() would be almost the same as the current
(before this CL) ImageResource::checkNotify(), and
- probably checkNotifyWithoutMarkingAsFinished() has duplicated/similar code to
checkNotify() in this CL,
right?

[hiroshige, 2016-08-10 06:03:08]

Reflected yhirano@'s comments.

Patch Set 7 keeps checkNotify() interface and introduces
notifyClients/ObserversInternal() in order to keep non-Image Resource subclasses
unchanged.

Also I turned the newly added tests to testharness-based as presubmit warning
says js-test is deprecated.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html
(right):

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:11:
var url  = '../cache/resources/etag-200.php?' + Math.floor(100000000 *
Math.random());
On 2016/08/09 09:35:58, yhirano wrote:
> const

Done.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:13:
xhr1 = new XMLHttpRequest();
On 2016/08/09 09:35:58, yhirano wrote:
> const

Done.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:16:
shouldBeEqualToNumber("xhr1.status", 200);
On 2016/08/09 09:35:58, yhirano wrote:
> indent

Done.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:18:
xhr2 = new XMLHttpRequest();
On 2016/08/09 09:35:58, yhirano wrote:
> const

Done.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html
(right):

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:11:
var url  = '../cache/resources/etag.php?' + Math.floor(100000000 *
Math.random());
On 2016/08/09 09:35:58, yhirano wrote:
> const

Done.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:13:
xhr1 = new XMLHttpRequest();
On 2016/08/09 09:35:58, yhirano wrote:
> const

Done.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:16:
shouldBeEqualToNumber("xhr1.status", 200);
On 2016/08/09 09:35:58, yhirano wrote:
> indent

Done.

https://codereview.chromium.org/2210473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-successful-revalidation.html:18:
xhr2 = new XMLHttpRequest();
On 2016/08/09 09:35:58, yhirano wrote:
> const

Done.

[yhirano, 2016-08-10 06:13:50]

lgtm

https://codereview.chromium.org/2210473002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html
(right):

https://codereview.chromium.org/2210473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:7:
const url  = 'resources/etag-200.php?' + Math.floor(100000000 * Math.random());
nit optional; If you want to wrap lines in 80 cols (I think yo do because of
L23-L24), this line should be fixed.

[hiroshige, 2016-08-10 13:54:14]

japhet@, could you take a look again?
(Changes after Patch Set 5: https://codereview.chromium.org/2210473002/#msg21)

[hiroshige, 2016-08-11 12:32:41]

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

[commit-bot: I haz the power, 2016-08-11 12:32:50]

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, 2016-08-11 14:18:20]

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

[commit-bot: I haz the power, 2016-08-11 14:18:21]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-08-23 10:35:07]

https://codereview.chromium.org/2210473002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html
(right):

https://codereview.chromium.org/2210473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/cache/xmlhttprequest-onload-event-for-failed-revalidation.html:7:
const url  = 'resources/etag-200.php?' + Math.floor(100000000 * Math.random());
On 2016/08/10 06:13:50, yhirano wrote:
> nit optional; If you want to wrap lines in 80 cols (I think yo do because of
> L23-L24), this line should be fixed.

Done.

[hiroshige, 2016-08-25 12:22:51]

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

[commit-bot: I haz the power, 2016-08-25 12:23:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hiroshige, 2016-08-25 12:28:22]

The CQ bit was unchecked by hiroshige@chromium.org

[hiroshige, 2016-08-25 12:28:28]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2016-08-25 12:28:29]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2210473002/#ps140001
(title: "Wrap by 80 cols and Rebase.")

[commit-bot: I haz the power, 2016-08-25 12:28:58]

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, 2016-08-25 13:56:53]

Description was changed from

==========
Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG=633696
==========

to

==========
Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG=633696
==========

[commit-bot: I haz the power, 2016-08-25 13:56:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-25 13:58:53]

Description was changed from

==========
Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG=633696
==========

to

==========
Mark ResourceClient/ImageResourceObserver finished just before notifying

Previously, all clients (except for awaiting ones) and observers are marked
finished just after checkNotify() is called.
However, it is possible that we have clients in |m_client| newly added during
ResourceClient::notifyFinished(), and such new clients shouldn't be marked
finished without calling notifyFinished().

This CL marks a client/observer just before notifyFinished()/
imageNotifyFinished() is called for the client/observer, and removes
markClientsAndObserversFinished() methods.

This CL also introduces MarkFinishedOption for checkNotify() for multipart
images: In multipart images we call notifyFinished()/imageNotifyFinished()
without marking clients/observers finished when the first part ends.

BUG=633696

Committed: https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918
Cr-Commit-Position: refs/heads/master@{#414423}
==========

[commit-bot: I haz the power, 2016-08-25 13:58:54]

Patchset 8 (id:??) landed as
https://crrev.com/62bb97b2652c495598942879e66bd7f1a5dbb918
Cr-Commit-Position: refs/heads/master@{#414423}