Simplifying finishing a load on Resource

Simplifying finishing a load on Resource

Have a single method to be called for success (Resource::finish) or failure
(Resource::error) that take all the relevant parameters.

BUG=

Committed: https://crrev.com/61e34ff7dd4ac48b8c4275eb3f541ebfb8a50266
Cr-Commit-Position: refs/heads/master@{#392177}

[Nate Chapin, 2016-04-28 18:10:26]

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

[Nate Chapin, 2016-04-28 18:10:27]

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:503: if (isLoading()) {
I'm not sure if this more complicated if/else block is worthwhile, but I'm
inclined to say it is. The benefit is that it removes the last call to
Resource::error() that isn't associated with a ResourceError, allowing us to
assert a non-null ResourceError in Resource::error(), which seems appropriate :)

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:508: notifyObservers();
I'm not 100% all of these are necessary, but it emulates that observable things
error() was doing and keeps the expected results of
ImageResourceTest.ReloadIfLofi the same.

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (left):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:155:
info->setLoadFinishTime(resource->loadFinishTime());
resource->loadFinishTime() is now not set at this point, so use the finishTime
parameter passed to didFinishLoading().

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceLoader.cpp (left):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceLoader.cpp:291: if
(!m_notifiedLoadComplete) {
This block is redundant with cancel()

[hiroshige, 2016-05-02 07:31:23]

This CL also makes Resource::error() to be always associated with a
ResourceError, great!
Please mention it in the CL description.

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:372: clear();
checkNotify() and markClientsAndObserversFinished() are not called if we don't
call error() here.
I'm not sure whether those calles are needed, but making isLoading() false
without notification to Clients/Observers looks strange.

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:503: if (isLoading()) {
On 2016/04/28 18:10:27, Nate Chapin wrote:
> I'm not sure if this more complicated if/else block is worthwhile, but I'm
> inclined to say it is. The benefit is that it removes the last call to
> Resource::error() that isn't associated with a ResourceError, allowing us to
> assert a non-null ResourceError in Resource::error(), which seems appropriate
:)

In this case, Resource::error() is called via ResourceLoader::cancel() and
ResourceError is provided by ResourceLoader, right?

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:508: notifyObservers();
On 2016/04/28 18:10:27, Nate Chapin wrote:
> I'm not 100% all of these are necessary, but it emulates that observable
things
> error() was doing and keeps the expected results of
> ImageResourceTest.ReloadIfLofi the same.

This CL seems to split "something" from error(), and places the "something"
outside error(), e.g. in Lines 506--508 and Line 372 of this file.
It would be nice if we can clearly describe what the "something" is/does, and
split it into a clearly named method (or at least add comments).

My random thoughts:
I feel the sequence of Lines 506--508 is a part of custom Resource state
manipulation (indicated by the |setStatus(NotStarted)| call).
Previously, the error() call is used because the state of Resource after error()
was ready for restarting load (which was not implied by the name of "error()"
though).
At first I thought that the sequence was to "make the Resource to be ready for
start loading again, provided isLoading() is true".
However, the sequence is not executed when starting revalidation, which also
rewinds the Resource's status to NotStarted.

[yhirano, 2016-05-02 09:31:34]

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:348: if (!errorOccurred())
The CL description suggests we can make this branch an assertion, right?

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.h:165: virtual void finish(double
finishTime = 0.0);
I feel a virtual function with default arguments a bit confusing (it's banned in
the google coding guideline[1]). Can you instead add a function |void finish() {
finish(0); }| if it's needed?

1: https://google.github.io/styleguide/cppguide.html#Default_Arguments

[Nate Chapin, 2016-05-02 19:32:25]

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:372: clear();
On 2016/05/02 07:31:23, hiroshige wrote:
> checkNotify() and markClientsAndObserversFinished() are not called if we don't
> call error() here.
> I'm not sure whether those calles are needed, but making isLoading() false
> without notification to Clients/Observers looks strange.

I think this just means that ResourceLoader will keep running and isLoading()
will still return true until the ResourceLoader completes?

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:503: if (isLoading()) {
On 2016/05/02 07:31:23, hiroshige wrote:
> On 2016/04/28 18:10:27, Nate Chapin wrote:
> > I'm not sure if this more complicated if/else block is worthwhile, but I'm
> > inclined to say it is. The benefit is that it removes the last call to
> > Resource::error() that isn't associated with a ResourceError, allowing us to
> > assert a non-null ResourceError in Resource::error(), which seems
appropriate
> :)
> 
> In this case, Resource::error() is called via ResourceLoader::cancel() and
> ResourceError is provided by ResourceLoader, right?

Correct.

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:508: notifyObservers();
On 2016/05/02 07:31:23, hiroshige wrote:
> On 2016/04/28 18:10:27, Nate Chapin wrote:
> > I'm not 100% all of these are necessary, but it emulates that observable
> things
> > error() was doing and keeps the expected results of
> > ImageResourceTest.ReloadIfLofi the same.
> 
> This CL seems to split "something" from error(), and places the "something"
> outside error(), e.g. in Lines 506--508 and Line 372 of this file.
> It would be nice if we can clearly describe what the "something" is/does, and
> split it into a clearly named method (or at least add comments).
> 
> My random thoughts:
> I feel the sequence of Lines 506--508 is a part of custom Resource state
> manipulation (indicated by the |setStatus(NotStarted)| call).
> Previously, the error() call is used because the state of Resource after
error()
> was ready for restarting load (which was not implied by the name of "error()"
> though).
> At first I thought that the sequence was to "make the Resource to be ready for
> start loading again, provided isLoading() is true".
> However, the sequence is not executed when starting revalidation, which also
> rewinds the Resource's status to NotStarted.

Ok, I modified this block slightly and pulled it out in to a helper called
updateImageAndClearBuffer. This logic now matches what we do for multipart
images when a part completes. This means that a lo fi image will still show the
placeholder while the real image is being loaded, but that seems benign. This
requires a small test change.

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:348: if (!errorOccurred())
On 2016/05/02 09:31:34, yhirano (ooo until May 6) wrote:
> The CL description suggests we can make this branch an assertion, right?

I don't think it's guaranteed, because a DecodeError causes errorOccurred() to
return true, but won't call error().

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.h:165: virtual void finish(double
finishTime = 0.0);
On 2016/05/02 09:31:34, yhirano (ooo until May 6) wrote:
> I feel a virtual function with default arguments a bit confusing (it's banned
in
> the google coding guideline[1]). Can you instead add a function |void finish()
{
> finish(0); }| if it's needed?
> 
> 1: https://google.github.io/styleguide/cppguide.html#Default_Arguments

Sounds good. It might be worth it to remove the default arg someday, since it's
mostly used in tests, but that can be a separate patch.

[Nate Chapin, 2016-05-02 19:33:47]

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

[Nate Chapin, 2016-05-02 19:33:47]

megjablon, is this test change OK? It basically means that a lo fi placeholder
image will still show during the reload of the real image, and get replaced once
we have bytes for the real image, instead of getting cleared as soon as
reloadIfLofi() is called.

[megjablon, 2016-05-03 01:05:00]

On 2016/05/02 19:33:47, Nate Chapin wrote:
> megjablon, is this test change OK? It basically means that a lo fi placeholder
> image will still show during the reload of the real image, and get replaced
once
> we have bytes for the real image, instead of getting cleared as soon as
> reloadIfLofi() is called.


Yep that's fine. That's the current behavior. The placeholder doesn't currently
disappear on reload.

[yhirano, 2016-05-06 10:06:03]

lgtm

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):

https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:372: clear();
On 2016/05/02 19:32:24, Nate Chapin wrote:
> On 2016/05/02 07:31:23, hiroshige wrote:
> > checkNotify() and markClientsAndObserversFinished() are not called if we
don't
> > call error() here.
> > I'm not sure whether those calles are needed, but making isLoading() false
> > without notification to Clients/Observers looks strange.
> 
> I think this just means that ResourceLoader will keep running and isLoading()
> will still return true until the ResourceLoader completes?

Is it desirable to cancel loading in such a case?

[Nate Chapin, 2016-05-06 19:59:01]

On 2016/05/06 10:06:03, yhirano wrote:
> lgtm
> 
>
https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right):
> 
>
https://codereview.chromium.org/1928823002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/fetch/ImageResource.cpp:372: clear();
> On 2016/05/02 19:32:24, Nate Chapin wrote:
> > On 2016/05/02 07:31:23, hiroshige wrote:
> > > checkNotify() and markClientsAndObserversFinished() are not called if we
> don't
> > > call error() here.
> > > I'm not sure whether those calles are needed, but making isLoading() false
> > > without notification to Clients/Observers looks strange.
> > 
> > I think this just means that ResourceLoader will keep running and
isLoading()
> > will still return true until the ResourceLoader completes?
> 
> Is it desirable to cancel loading in such a case?

If I'm reading the code on trunk correctly, the ResourceLoader currently runs to
completion: the difference is that with this change, we'll report isLoading() is
true. I'm fine with changing this behavior later, but I think this is close
enough to the status quo that I'm inclined to leave it as-is in this change.

[Nate Chapin, 2016-05-06 20:00:13]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2016-05-06 20:00:13]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1928823002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-06 20:00:39]

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

[commit-bot: I haz the power, 2016-05-06 22:17:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-06 22:19:19]

Description was changed from

==========
Simplifying finishing a load on Resource

Have a single method to be called for success (Resource::finish) or failure
(Resource::error) that take all the relevant parameters.

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

to

==========
Simplifying finishing a load on Resource

Have a single method to be called for success (Resource::finish) or failure
(Resource::error) that take all the relevant parameters.

BUG=

Committed: https://crrev.com/61e34ff7dd4ac48b8c4275eb3f541ebfb8a50266
Cr-Commit-Position: refs/heads/master@{#392177}
==========

[commit-bot: I haz the power, 2016-05-06 22:19:20]

Patchset 3 (id:??) landed as
https://crrev.com/61e34ff7dd4ac48b8c4275eb3f541ebfb8a50266
Cr-Commit-Position: refs/heads/master@{#392177}