setLoadFinishTime on ImageResource for ImageDocument.

setLoadFinishTime on ImageResource for ImageDocument.

The loadFinishTime was never set on ImageResource used for the generated
image element inside an ImageDocument. We discovered this checking for
that time stamp in an internal Opera project.

Set the finish time of the ImageResource to that of the document (which
is the actual image).

Committed: https://crrev.com/bf9fbf106a8360f176ed1f7de9f3b87146e11f35
Cr-Commit-Position: refs/heads/master@{#352730}

[rune, 2015-10-01 23:22:03]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-01 23:22:14]

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

[rune, 2015-10-01 23:28:09]

rune@opera.com changed reviewers:
+ tkent@chromium.org

[rune, 2015-10-01 23:28:09]

@tkent:

I don't know if setting the timestamp will affect memory cache eviction, or if
it was intentional to leave the timestamp at 0 in the first place?

I have not written any tests yet. Do you know how I can/should test this?

[commit-bot: I haz the power, 2015-10-01 23:29:55]

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

[commit-bot: I haz the power, 2015-10-01 23:29:56]

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

[tkent, 2015-10-02 00:08:29]

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

[tkent, 2015-10-02 00:09:02]

I'm not familiar with it.  japhet@ may know.

[Nate Chapin, 2015-10-02 22:41:58]

On 2015/10/01 23:28:09, rune wrote:
> @tkent:
> 
> I don't know if setting the timestamp will affect memory cache eviction, or if
> it was intentional to leave the timestamp at 0 in the first place?
> 
> I have not written any tests yet. Do you know how I can/should test this?

This is most likely an oversight, since this code has been basically unchanged
for several years.

You can probably test this pretty easily in Source/web/tests/WebFrameTest.cpp.
Load an image into an frame, and you should be able to verify it's an
ImageDocument and poke at its cacheImage(). There might be a way to unit test it
closer to ImageDocument, but that's likely to require a bunch of stubbing.

[rune, 2015-10-04 17:56:11]

On 2015/10/02 22:41:58, Nate Chapin wrote:
> On 2015/10/01 23:28:09, rune wrote:
> > @tkent:
> > 
> > I don't know if setting the timestamp will affect memory cache eviction, or
if
> > it was intentional to leave the timestamp at 0 in the first place?
> > 
> > I have not written any tests yet. Do you know how I can/should test this?
> 
> This is most likely an oversight, since this code has been basically unchanged
> for several years.
> 
> You can probably test this pretty easily in Source/web/tests/WebFrameTest.cpp.
> Load an image into an frame, and you should be able to verify it's an
> ImageDocument and poke at its cacheImage(). There might be a way to unit test
it
> closer to ImageDocument, but that's likely to require a bunch of stubbing.

Done. PTAL.

[rune, 2015-10-04 18:02:57]

https://codereview.chromium.org/1380163006/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/1380163006/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:160:
cachedImage->setResponse(loader->response());
Moved setResponse() before finish() because that's the invocation order for
normal resource fetching.

[rune, 2015-10-05 22:11:34]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-05 22:13:49]

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

[commit-bot: I haz the power, 2015-10-05 22:41:54]

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

[commit-bot: I haz the power, 2015-10-05 22:41:54]

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

[rune, 2015-10-06 07:27:10]

The CQ bit was checked by rune@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-06 07:27:52]

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

[commit-bot: I haz the power, 2015-10-06 08:28:19]

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

[commit-bot: I haz the power, 2015-10-06 08:28:20]

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

[Nate Chapin, 2015-10-06 21:36:17]

lgtm. Trybot failure looks like it might be real, though.

[rune, 2015-10-06 22:54:33]

The CQ bit was checked by rune@opera.com

[rune, 2015-10-06 22:54:34]

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

[commit-bot: I haz the power, 2015-10-06 22:56:13]

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

[commit-bot: I haz the power, 2015-10-07 00:43:47]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-07 00:44:37]

Patchset 7 (id:??) landed as
https://crrev.com/bf9fbf106a8360f176ed1f7de9f3b87146e11f35
Cr-Commit-Position: refs/heads/master@{#352730}