ImageResource: Reset the encoded image size after receiving all data

ImageResource: Reset the encoded image size after receiving all data

Before this CL, an ImageResource's encoded image size remains non-zero
even after the encoded data is cleared. This CL fixes this to make
Resource::onMemoryDump report more accurate size.

BUG=618623, 643135
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*

Committed: https://crrev.com/66658a10410c9f6d90dfbabbc3b3d6c9a366e0e2
Cr-Commit-Position: refs/heads/master@{#426754}

[hajimehoshi, 2016-10-05 05:57:31]

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

[commit-bot: I haz the power, 2016-10-05 05:57:43]

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

[hajimehoshi, 2016-10-05 05:58:10]

hajimehoshi@chromium.org changed reviewers:
+ haraken@chromium.org, hiroshige@chromium.org, yhirano@chromium.org

[hajimehoshi, 2016-10-05 05:58:10]

PTAL

[haraken, 2016-10-05 05:59:39]

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

https://codereview.chromium.org/2393113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:423: setEncodedSize(0);

Would it be better to move setEncodedSize into clearData?

[hajimehoshi, 2016-10-05 06:04:31]

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

https://codereview.chromium.org/2393113002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ImageResource.cpp:423: setEncodedSize(0);
On 2016/10/05 05:59:39, haraken wrote:
> 
> Would it be better to move setEncodedSize into clearData?

I agree but hiroshige@ will do this in another CL. That would not be simple
according to hiroshige@.

[haraken, 2016-10-05 06:04:57]

LGTM

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

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

[commit-bot: I haz the power, 2016-10-05 07:05:17]

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

[yhirano, 2016-10-06 01:32:20]

Tests are failing.

[hajimehoshi, 2016-10-06 10:08:36]

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

[commit-bot: I haz the power, 2016-10-06 10:08:50]

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

[hajimehoshi, 2016-10-06 10:10:30]

On 2016/10/06 01:32:20, yhirano wrote:
> Tests are failing.

Couldn't reproduce them on my local machine. Let me cl-try again.

[hajimehoshi, 2016-10-06 10:53:49]

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

[commit-bot: I haz the power, 2016-10-06 10:54:02]

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-10-06 12:09:22]

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

[commit-bot: I haz the power, 2016-10-06 12:09:23]

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_...)

[hajimehoshi, 2016-10-07 08:59:06]

On 2016/10/06 10:10:30, hajimehoshi wrote:
> On 2016/10/06 01:32:20, yhirano wrote:
> > Tests are failing.
> 
> Couldn't reproduce them on my local machine. Let me cl-try again.

Found that InspectorPageAgent::cachedResourceContent returns empty result when
its encoded size is 0. +hiroshige@ do you have any good idea to fix this?

[ssid, 2016-10-13 02:59:35]

On 2016/10/07 08:59:06, hajimehoshi wrote:
> On 2016/10/06 10:10:30, hajimehoshi wrote:
> > On 2016/10/06 01:32:20, yhirano wrote:
> > > Tests are failing.
> > 
> > Couldn't reproduce them on my local machine. Let me cl-try again.
> 
> Found that InspectorPageAgent::cachedResourceContent returns empty result when
> its encoded size is 0. +hiroshige@ do you have any good idea to fix this?

ping.

[hajimehoshi, 2016-10-13 11:24:41]

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

[commit-bot: I haz the power, 2016-10-13 11:24:50]

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

[hajimehoshi, 2016-10-13 11:26:28]

On 2016/10/13 02:59:35, ssid wrote:
> On 2016/10/07 08:59:06, hajimehoshi wrote:
> > On 2016/10/06 10:10:30, hajimehoshi wrote:
> > > On 2016/10/06 01:32:20, yhirano wrote:
> > > > Tests are failing.
> > > 
> > > Couldn't reproduce them on my local machine. Let me cl-try again.
> > 
> > Found that InspectorPageAgent::cachedResourceContent returns empty result
when
> > its encoded size is 0. +hiroshige@ do you have any good idea to fix this?
> 
> ping.

As discussed offline with hiroshige, I've added encodedSizeMemoryUsage along
with encodedSize. This could have a better name. PTAL

[haraken, 2016-10-13 11:28:21]

LGTM

[hajimehoshi, 2016-10-13 11:32:12]

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

[commit-bot: I haz the power, 2016-10-13 11:32:27]

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-10-13 12:58:01]

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

[commit-bot: I haz the power, 2016-10-13 12:58:04]

Dry run: This issue passed the CQ dry run.

[hajimehoshi, 2016-10-14 09:12:40]

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

[commit-bot: I haz the power, 2016-10-14 09:12:51]

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

[hajimehoshi, 2016-10-14 09:13:11]

yhirano: As we discussed offline, I've removed unnecessary change in
MemoryCache, which was my misunderstanding. PTAL

[yhirano, 2016-10-14 09:20:00]

https://codereview.chromium.org/2393113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2393113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:803: void
Resource::setEncodedSizeMemoryUsage(size_t encodedSize) {
Can you remove this function, and set m_encodedSizeMemoryUsage to 0 in
clearData?

https://codereview.chromium.org/2393113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:808:
memoryCache()->update(this, oldSize, size());
This is now not needed, right?

[hajimehoshi, 2016-10-14 09:29:52]

Thank you!

https://codereview.chromium.org/2393113002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2393113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:803: void
Resource::setEncodedSizeMemoryUsage(size_t encodedSize) {
On 2016/10/14 09:20:00, yhirano wrote:
> Can you remove this function, and set m_encodedSizeMemoryUsage to 0 in
> clearData?

Hmm, your suggestion would affect other resources than ImageResource, which I
don't want to do in this CL.

https://codereview.chromium.org/2393113002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/Resource.cpp:808:
memoryCache()->update(this, oldSize, size());
On 2016/10/14 09:20:00, yhirano wrote:
> This is now not needed, right?

Done.

[hajimehoshi, 2016-10-14 09:30:11]

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

[commit-bot: I haz the power, 2016-10-14 09:30:21]

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-10-14 10:46:03]

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

[commit-bot: I haz the power, 2016-10-14 10:46:05]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-17 05:25:16]

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:185: size_t
encodedSizeMemoryUsage() const { return m_encodedSizeMemoryUsage; }
Can you add ForTest[ing] suffix?

[yhirano, 2016-10-17 09:00:14]

lgtm

[hajimehoshi, 2016-10-18 04:37:43]

hiroshige: Are you happy with this change?

[yhirano, 2016-10-19 00:41:26]

On 2016/10/18 04:37:43, hajimehoshi wrote:
> hiroshige: Are you happy with this change?

hajimehoshi@: Please see #37.

[hiroshige, 2016-10-19 04:04:01]

lgtm.

Could you add 643135 to BUG in the CL description?

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:185: size_t
encodedSizeMemoryUsage() const { return m_encodedSizeMemoryUsage; }
On 2016/10/17 05:25:16, yhirano wrote:
> Can you add ForTest[ing] suffix?

I'm neutral about this; encodedSizeMemoryUsage() is not only for testing but
also used for memory-infra.
(and both of encodedSize() and encodedSizeMemoryUsage() is equally not
recommended)

[yhirano, 2016-10-19 05:36:11]

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:185: size_t
encodedSizeMemoryUsage() const { return m_encodedSizeMemoryUsage; }
On 2016/10/19 04:04:01, hiroshige wrote:
> On 2016/10/17 05:25:16, yhirano wrote:
> > Can you add ForTest[ing] suffix?
> 
> I'm neutral about this; encodedSizeMemoryUsage() is not only for testing but
> also used for memory-infra.
> (and both of encodedSize() and encodedSizeMemoryUsage() is equally not
> recommended)

The field is not for testing but this getter is for testing only.

[hajimehoshi, 2016-10-21 06:26:51]

Description was changed from

==========
ImageResource: Reset the encoded image size after receiving all data

Before this CL, an ImageResource's encoded image size remains non-zero
even after the encoded data is cleared. This CL fixes this to make
Resource::onMemoryDump report more accurate size.

BUG=618623
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*
==========

to

==========
ImageResource: Reset the encoded image size after receiving all data

Before this CL, an ImageResource's encoded image size remains non-zero
even after the encoded data is cleared. This CL fixes this to make
Resource::onMemoryDump report more accurate size.

BUG=618623, 643135
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*
==========

[hajimehoshi, 2016-10-21 06:41:23]

Thank you!

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/fetch/Resource.h (right):

https://codereview.chromium.org/2393113002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/fetch/Resource.h:185: size_t
encodedSizeMemoryUsage() const { return m_encodedSizeMemoryUsage; }
On 2016/10/19 05:36:11, yhirano wrote:
> On 2016/10/19 04:04:01, hiroshige wrote:
> > On 2016/10/17 05:25:16, yhirano wrote:
> > > Can you add ForTest[ing] suffix?
> > 
> > I'm neutral about this; encodedSizeMemoryUsage() is not only for testing but
> > also used for memory-infra.
> > (and both of encodedSize() and encodedSizeMemoryUsage() is equally not
> > recommended)
> 
> The field is not for testing but this getter is for testing only.

Done.

[hajimehoshi, 2016-10-21 06:41:29]

The CQ bit was checked by hajimehoshi@chromium.org

[hajimehoshi, 2016-10-21 06:41:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, hiroshige@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2393113002/#ps120001
(title: "Rename encodedSizeMemoryUsage -> encodedSizeMemoryUsageForTesting")

[commit-bot: I haz the power, 2016-10-21 06:41:47]

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-10-21 08:57:42]

Description was changed from

==========
ImageResource: Reset the encoded image size after receiving all data

Before this CL, an ImageResource's encoded image size remains non-zero
even after the encoded data is cleared. This CL fixes this to make
Resource::onMemoryDump report more accurate size.

BUG=618623, 643135
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*
==========

to

==========
ImageResource: Reset the encoded image size after receiving all data

Before this CL, an ImageResource's encoded image size remains non-zero
even after the encoded data is cleared. This CL fixes this to make
Resource::onMemoryDump report more accurate size.

BUG=618623, 643135
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*
==========

[commit-bot: I haz the power, 2016-10-21 08:57:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-21 13:28:18]

Description was changed from

==========
ImageResource: Reset the encoded image size after receiving all data

Before this CL, an ImageResource's encoded image size remains non-zero
even after the encoded data is cleared. This CL fixes this to make
Resource::onMemoryDump report more accurate size.

BUG=618623, 643135
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*
==========

to

==========
ImageResource: Reset the encoded image size after receiving all data

Before this CL, an ImageResource's encoded image size remains non-zero
even after the encoded data is cleared. This CL fixes this to make
Resource::onMemoryDump report more accurate size.

BUG=618623, 643135
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*

Committed: https://crrev.com/66658a10410c9f6d90dfbabbc3b3d6c9a366e0e2
Cr-Commit-Position: refs/heads/master@{#426754}
==========

[commit-bot: I haz the power, 2016-10-21 13:28:19]

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