Service Worker script sizes in database.

Adding size and total size to the service worker database.

Tests currently do not pass because the 'resources_total_size_bytes' is not populated in the database tests, as I moved this population to the service_worker_storage class. Waiting for guidance on the best place to populate this before modifying too many tests.

BUG=

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

[dmurph, 2014-10-17 00:51:50]

dmurph@chromium.org changed reviewers:
+ jsbell@chromium.org, michaeln@chromium.org

[dmurph, 2014-10-17 00:51:50]

Hello all,

Please take a look at service_worker_storage.cc and service_worker_database.cc. 
I can put the total calculation of all of the resources in either of these, but
to maintain the current const-ness of the call chain I have to do this in the
service_Worker_storage.

This seems to be reasonable, but I wanted to check with you guys before I
started modifying a bunch of tests.

Thanks!
Daniel

[michaeln, 2014-10-17 02:01:40]

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_script_cache_map.cc (right):

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_script_cache_map.cc:62:
resource_map_[url].size_bytes = static_cast<int64>(size_bytes);
is the static_cast<> needed?

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:187:
*bytes_read = net_bytes_read;
I don't think be here when net_bytes_read > 0. From the point of view of the
urlrequestjob's consumer the read has not yet completed, the read will complete
after its been written to disk. The job is not yet done with the IOBuffer and
the returned status indicates io_pending.

otherwise, this cl looks good

[chromium-reviews, 2014-10-17 02:20:20]

Wait, so, do you think it makes sense to compute the total size in the
storage class? Or should that be done right before saving in the database
class

On Thu, Oct 16, 2014, 7:01 PM null <michaeln@chromium.org> wrote:

>
> https://codereview.chromium.org/647953003/diff/1/content/
> browser/service_worker/service_worker_script_cache_map.cc
> File content/browser/service_worker/service_worker_script_cache_map.cc
> (right):
>
> https://codereview.chromium.org/647953003/diff/1/content/
> browser/service_worker/service_worker_script_cache_map.cc#newcode62
> content/browser/service_worker/service_worker_script_cache_map.cc:62
>
<https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...>
> :
> resource_map_[url].size_bytes = static_cast<int64>(size_bytes);
> is the static_cast<> needed?
>
> https://codereview.chromium.org/647953003/diff/1/content/
> browser/service_worker/service_worker_write_to_cache_job.cc
> File content/browser/service_worker/service_worker_write_to_cache_job.cc
> (right):
>
> https://codereview.chromium.org/647953003/diff/1/content/
> browser/service_worker/service_worker_write_to_cache_job.cc#newcode187
> content/browser/service_worker/service_worker_write_to_cache_job.cc:187
>
<https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...>
> :
> *bytes_read = net_bytes_read;
> I don't think be here when net_bytes_read > 0. From the point of view of
> the urlrequestjob's consumer the read has not yet completed, the read
> will complete after its been written to disk. The job is not yet done
> with the IOBuffer and the returned status indicates io_pending.
>
> otherwise, this cl looks good
>
> https://codereview.chromium.org/647953003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-10-17 02:20:53]

(Just making sure you saw my question on the first email)

On Thu, Oct 16, 2014, 7:20 PM Daniel Murphy <dmurph@google.com> wrote:

> Wait, so, do you think it makes sense to compute the total size in the
> storage class? Or should that be done right before saving in the database
> class
>
> On Thu, Oct 16, 2014, 7:01 PM null <michaeln@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/647953003/diff/1/content/bro
>> wser/service_worker/service_worker_script_cache_map.cc
>> File content/browser/service_worker/service_worker_script_cache_map.cc
>> (right):
>>
>> https://codereview.chromium.org/647953003/diff/1/content/bro
>> wser/service_worker/service_worker_script_cache_map.cc#newcode62
>> content/browser/service_worker/service_worker_script_cache_map.cc:62
>>
<https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...>
>> :
>> resource_map_[url].size_bytes = static_cast<int64>(size_bytes);
>> is the static_cast<> needed?
>>
>> https://codereview.chromium.org/647953003/diff/1/content/bro
>> wser/service_worker/service_worker_write_to_cache_job.cc
>> File content/browser/service_worker/service_worker_write_to_cache_job.cc
>> (right):
>>
>> https://codereview.chromium.org/647953003/diff/1/content/bro
>> wser/service_worker/service_worker_write_to_cache_job.cc#newcode187
>> content/browser/service_worker/service_worker_write_to_cache_job.cc:187
>>
<https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...>
>> :
>> *bytes_read = net_bytes_read;
>> I don't think be here when net_bytes_read > 0. From the point of view of
>> the urlrequestjob's consumer the read has not yet completed, the read
>> will complete after its been written to disk. The job is not yet done
>> with the IOBuffer and the returned status indicates io_pending.
>>
>> otherwise, this cl looks good
>>
>> https://codereview.chromium.org/647953003/
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaeln, 2014-10-17 20:16:20]

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:187:
*bytes_read = net_bytes_read;
On 2014/10/17 02:01:40, michaeln wrote:
> I don't think be here when net_bytes_read > 0. From the point of view of the
> urlrequestjob's consumer the read has not yet completed, the read will
complete
> after its been written to disk. The job is not yet done with the IOBuffer and
> the returned status indicates io_pending.
> 
> otherwise, this cl looks good

> > Wait, so, do you think it makes sense to compute the total size in the
> > storage class? Or should that be done right before saving in the database
> > class

I'm just saying that this newly added line 187 doesn't need to be here and
should be removed from the CL.

Notice line 191 returns IO_PENDING when non-zero bytes have been read from the
net. The ServiceWorkerWriteToCacheJob reads from the net request, writes to the
disk, and then returns the data to its caller. It's caller doesn't see any bytes
read until the write has completed too.

[michaeln, 2014-10-17 20:29:53]

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_database.cc:295:
version_id(kInvalidServiceWorkerVersionId),
oh... also zero init the new data member here

[dmurph, 2014-10-17 22:55:19]

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_database.cc:295:
version_id(kInvalidServiceWorkerVersionId),
On 2014/10/17 20:29:53, michaeln wrote:
> oh... also zero init the new data member here

Done.

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_script_cache_map.cc (right):

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_script_cache_map.cc:62:
resource_map_[url].size_bytes = static_cast<int64>(size_bytes);
On 2014/10/17 02:01:40, michaeln wrote:
> is the static_cast<> needed?

Nope not anymore, removed.

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/647953003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_write_to_cache_job.cc:187:
*bytes_read = net_bytes_read;
On 2014/10/17 20:16:20, michaeln wrote:
> On 2014/10/17 02:01:40, michaeln wrote:
> > I don't think be here when net_bytes_read > 0. From the point of view of the
> > urlrequestjob's consumer the read has not yet completed, the read will
> complete
> > after its been written to disk. The job is not yet done with the IOBuffer
and
> > the returned status indicates io_pending.
> > 
> > otherwise, this cl looks good
> 
> > > Wait, so, do you think it makes sense to compute the total size in the
> > > storage class? Or should that be done right before saving in the database
> > > class
> 
> I'm just saying that this newly added line 187 doesn't need to be here and
> should be removed from the CL.
> 
> Notice line 191 returns IO_PENDING when non-zero bytes have been read from the
> net. The ServiceWorkerWriteToCacheJob reads from the net request, writes to
the
> disk, and then returns the data to its caller. It's caller doesn't see any
bytes
> read until the write has completed too.

Done.

[michaeln, 2014-10-20 22:03:17]

lgtm!

[dmurph, 2014-10-20 22:05:58]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2014-10-20 22:08:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647953003/20001

[commit-bot: I haz the power, 2014-10-20 22:15:38]

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

[commit-bot: I haz the power, 2014-10-20 22:15:39]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/70976)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmurph, 2014-10-21 00:03:31]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:04:44]

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

[commit-bot: I haz the power, 2014-10-21 01:12:04]

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

[commit-bot: I haz the power, 2014-10-21 01:12:05]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmurph, 2014-10-21 01:56:00]

The CQ bit was checked by dmurph@chromium.org

[commit-bot: I haz the power, 2014-10-21 01:57:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/647953003/60001

[commit-bot: I haz the power, 2014-10-21 03:40:52]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 03:41:46]

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