[ServiceWorkerCache] Put should replace an existing entry to meet spec.

[ServiceWorkerCache] Put should replace an existing entry to meet spec.

Put now calls Delete before CreateEntry, this requires a new callback PutDidDeleteEntry.

BUG=426562
Committed: https://crrev.com/4b83c33732fb77da5d5100c339ad16325047ebed
Cr-Commit-Position: refs/heads/master@{#302072}

[jkarlin, 2014-10-24 19:19:15]

jkarlin@chromium.org changed reviewers:
+ falken@chromium.org, michaeln@chromium.org

[jkarlin, 2014-10-24 19:19:16]

michaeln@chromium.org: Please review changes in all

falken@chromium.org: Please review changes in all

So many thanks guys!

[jkarlin, 2014-10-24 21:16:49]

On 2014/10/24 19:19:16, jkarlin wrote:
> mailto:michaeln@chromium.org: Please review changes in all
> 
> mailto:falken@chromium.org: Please review changes in all
> 
> So many thanks guys!

Hold off on reviewing this for a few hours, I need to update this so that any
deleted data is reported to the quota manager.  I'll likely just call
ServiceWorkerCache::Delete first (which does the reporting).

[jsbell, 2014-10-24 22:44:42]

Can you add a note to the commit comment that this will cause a blink layout
test (http/tests/serviceworker/cache-put.html) to "fail" since one case will
start passing, and that a rebase is fine.

(Not that I've tried the patch, I'm just assuming it will be awesome!)

Alternately, you could do the whole TestExpectations dance, but yuck! If this
doesn't land until Monday when there's active monitoring I wouldn't bother.

[jkarlin, 2014-10-24 23:27:44]

On 2014/10/24 22:44:42, jsbell wrote:
> Can you add a note to the commit comment that this will cause a blink layout
> test (http/tests/serviceworker/cache-put.html) to "fail" since one case will
> start passing, and that a rebase is fine.
> 
> (Not that I've tried the patch, I'm just assuming it will be awesome!)
> 
> Alternately, you could do the whole TestExpectations dance, but yuck! If this
> doesn't land until Monday when there's active monitoring I wouldn't bother.

Due to a big rebase due on the upstream branch, this won't land until Monday
evening at the earliest.  So I'll have to do the expectations dance.

[falken, 2014-10-27 04:09:46]

code change lg...

> Hold off on reviewing this for a few hours, I need to update this so that any
> deleted data is reported to the quota manager. 

I guess this also is still planned?

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

https://codereview.chromium.org/649203005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_cache_unittest.cc:345:
callback_response_->url.spec().c_str());
I'm a bit confused how this tests the new behavior. I'd expect you Put things
with the same request and different responses to see it was replaced?

[jkarlin, 2014-10-27 12:15:28]

Ready for review again, thanks for the early feedback Falken.

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

https://codereview.chromium.org/649203005/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_cache_unittest.cc:345:
callback_response_->url.spec().c_str());
On 2014/10/27 04:09:45, falken wrote:
> I'm a bit confused how this tests the new behavior. I'd expect you Put things
> with the same request and different responses to see it was replaced?

Done. Thank you for catching this!

[jkarlin, 2014-10-27 21:46:09]

Some tests are failing, will fix tomorrow.

[jkarlin, 2014-10-28 11:55:29]

jkarlin@chromium.org changed reviewers:
- falken@chromium.org, michaeln@chromium.org

[jkarlin, 2014-10-28 13:09:55]

On 2014/10/27 21:46:09, jkarlin wrote:
> Some tests are failing, will fix tomorrow.

The tests are now failing on Windows due to a known SimpleCache bug.  Gavin is
fixing that in https://codereview.chromium.org/651063002/ and I'll post this CL
back up for review once that lands.

[jkarlin, 2014-10-29 13:49:41]

jkarlin@chromium.org changed reviewers:
+ falken@chromium.org, michaeln@chromium.org

[jkarlin, 2014-10-29 13:49:41]

michaeln@chromium.org: Please review changes in all

falken@chromium.org: Please review changes in all

Alright, game on!  Sorry for the interruption in reviewing.

[michaeln, 2014-10-30 00:31:40]

lgtm

does it matter that (delete + add) isn't atomic?

[jkarlin, 2014-10-30 11:36:09]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-10-30 11:37:31]

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

[jkarlin, 2014-10-30 11:38:07]

On 2014/10/30 00:31:40, michaeln wrote:
> lgtm
> 
> does it matter that (delete + add) isn't atomic?

Thanks.  It matters in the long run, but add isn't atomic because SimpleCache
doesn't support atomic operations yet.  That's for post MVP.

[commit-bot: I haz the power, 2014-10-30 12:19:08]

Committed patchset #8 (id:130001)

[commit-bot: I haz the power, 2014-10-30 12:19:48]

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