Store, use and send etags.

Store, use and send etags.

We use them to validate/invalide cached but expired data.
We send Match-If headers to prevent mixing of old/new cached data.

This should all be safe to do, since the HTTP cache already sends
etags to manage it's internal cached state, this CL just extends
this behavior to apply to client-side in-memory cached data as well.

BUG=504194
Committed: https://crrev.com/fd30799f6f8a7d1366d97693b72b0b20be94d5c0
Cr-Commit-Position: refs/heads/master@{#419264}

[hubbe, 2016-09-13 20:49:55]

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

[commit-bot: I haz the power, 2016-09-13 20:50:15]

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

[hubbe, 2016-09-13 22:23:31]

hubbe@chromium.org changed reviewers:
+ dalecurtis@google.com

[DaleCurtis, 2016-09-13 23:30:12]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org, tombergan@chromium.org
- dalecurtis@google.com

[DaleCurtis, 2016-09-13 23:30:12]

lg2m, but +tombergan to verify.

[DaleCurtis, 2016-09-13 23:31:52]

https://codereview.chromium.org/2338963002/diff/1/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/2338963002/diff/1/media/blink/url_index.cc#ne...
media/blink/url_index.cc:224: bool HasStrongEtag(const scoped_refptr<UrlData>&
entry) {
Don't pass scoped_refptr by const& anymore. I'd just pass a const& directly to
the etag here.

[commit-bot: I haz the power, 2016-09-13 23:59:07]

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

[commit-bot: I haz the power, 2016-09-13 23:59:08]

Dry run: This issue passed the CQ dry run.

[hubbe, 2016-09-14 00:22:31]

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

[hubbe, 2016-09-14 00:23:06]

https://codereview.chromium.org/2338963002/diff/1/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/2338963002/diff/1/media/blink/url_index.cc#ne...
media/blink/url_index.cc:224: bool HasStrongEtag(const scoped_refptr<UrlData>&
entry) {
On 2016/09/13 23:31:52, DaleCurtis wrote:
> Don't pass scoped_refptr by const& anymore. I'd just pass a const& directly to
> the etag here.

Done.

[commit-bot: I haz the power, 2016-09-14 00:23:06]

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-09-14 02:26:29]

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

[commit-bot: I haz the power, 2016-09-14 02:26:30]

Dry run: This issue passed the CQ dry run.

[Tom Bergan, 2016-09-14 19:04:23]

(Can't reply on the codereview tool right now because I left my phone at
home, so I can't get past two-factor auth :-/)

> media/blink/resource_multibuffer_data_provider.cc:88
> request.setHTTPHeaderField(WebString::fromUTF8("Match-If"),

Two questions about this: First, did you mean to say If-Match, not Match-if?
https://tools.ietf.org/html/rfc7232#section-3.1

Second, I think If-Range is more appropriate since this is a Range request
(see line 82 above)?
https://tools.ietf.org/html/rfc7232#section-3.5

> media/blink/url_index.cc:227
I think lines 227-229 can simplify to:
return etag[0] == '"';   // <- that's checking if the first character is a
double quote

https://tools.ietf.org/html/rfc7232#section-2.3

On Tue, Sep 13, 2016 at 5:23 PM, <hubbe@chromium.org> wrote:

>
> https://codereview.chromium.org/2338963002/diff/1/media/blink/url_index.cc
> File media/blink/url_index.cc (right):
>
> https://codereview.chromium.org/2338963002/diff/1/media/
> blink/url_index.cc#newcode224
> media/blink/url_index.cc:224: bool HasStrongEtag(const
> scoped_refptr<UrlData>& entry) {
> On 2016/09/13 23:31:52, DaleCurtis wrote:
> > Don't pass scoped_refptr by const& anymore. I'd just pass a const&
> directly to
> > the etag here.
>
> Done.
>
> https://codereview.chromium.org/2338963002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom Bergan, 2016-09-14 19:07:47]

On Wed, Sep 14, 2016 at 12:04 PM, Tom Bergan <tombergan@chromium.org> wrote:

> (Can't reply on the codereview tool right now because I left my phone at
> home, so I can't get past two-factor auth :-/)
>
> > media/blink/resource_multibuffer_data_provider.cc:88
> > request.setHTTPHeaderField(WebString::fromUTF8("Match-If"),
>
> Two questions about this: First, did you mean to say If-Match, not
> Match-if?
> https://tools.ietf.org/html/rfc7232#section-3.1
>
> Second, I think If-Range is more appropriate since this is a Range request
> (see line 82 above)?
> https://tools.ietf.org/html/rfc7232#section-3.5
>

Just remembered from the bug why you suggested If-Match instead of If-Range
-- because the player cannot proceed if the video changed, so there's no
point in returning the full video. However, note that the cache layer uses
If-Range but does not use If-Match (to my knowledge), so from a deployment
perspective, this may be more risky than If-Range. Not sure though.


> > media/blink/url_index.cc:227
> I think lines 227-229 can simplify to:
> return etag[0] == '"';   // <- that's checking if the first character is a
> double quote
>
> https://tools.ietf.org/html/rfc7232#section-2.3
>
> On Tue, Sep 13, 2016 at 5:23 PM, <hubbe@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2338963002/diff/1/media/blin
>> k/url_index.cc
>> File media/blink/url_index.cc (right):
>>
>> https://codereview.chromium.org/2338963002/diff/1/media/blin
>> k/url_index.cc#newcode224
>> media/blink/url_index.cc:224: bool HasStrongEtag(const
>> scoped_refptr<UrlData>& entry) {
>> On 2016/09/13 23:31:52, DaleCurtis wrote:
>> > Don't pass scoped_refptr by const& anymore. I'd just pass a const&
>> directly to
>> > the etag here.
>>
>> Done.
>>
>> https://codereview.chromium.org/2338963002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hubbe, 2016-09-16 19:02:40]

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

[commit-bot: I haz the power, 2016-09-16 19:03:09]

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

[hubbe, 2016-09-16 19:06:07]

PTAL

On 2016/09/14 19:07:47, Tom Bergan wrote:
> On Wed, Sep 14, 2016 at 12:04 PM, Tom Bergan <mailto:tombergan@chromium.org>
wrote:
> 
> > (Can't reply on the codereview tool right now because I left my phone at
> > home, so I can't get past two-factor auth :-/)
> >
> > > media/blink/resource_multibuffer_data_provider.cc:88
> > > request.setHTTPHeaderField(WebString::fromUTF8("Match-If"),
> >
> > Two questions about this: First, did you mean to say If-Match, not
> > Match-if?
> > https://tools.ietf.org/html/rfc7232#section-3.1
> >

Yes, done.


> > Second, I think If-Range is more appropriate since this is a Range request
> > (see line 82 above)?
> > https://tools.ietf.org/html/rfc7232#section-3.5
> >
> 
> Just remembered from the bug why you suggested If-Match instead of If-Range
> -- because the player cannot proceed if the video changed, so there's no
> point in returning the full video. However, note that the cache layer uses
> If-Range but does not use If-Match (to my knowledge), so from a deployment
> perspective, this may be more risky than If-Range. Not sure though.
> 


I agree, there is a small additional risk in using If-Match instead of If-Range.
I think it's pretty small though. The problem with If-Range is that people just
get confused about "I responded with a 200, why doesn't it work??".


> 
> > > media/blink/url_index.cc:227
> > I think lines 227-229 can simplify to:
> > return etag[0] == '"';   // <- that's checking if the first character is a
> > double quote
> >
> > https://tools.ietf.org/html/rfc7232#section-2.3
> >

Done.

[Tom Bergan, 2016-09-16 19:30:21]

One more question, otherwise LGTM

https://codereview.chromium.org/2338963002/diff/40001/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/2338963002/diff/40001/media/blink/url_index.c...
media/blink/url_index.cc:238: return false;
Missed this on the first read ... What if the response doesn't have any
validator headers (neither Etag nor Last-Modified)? This happens frequently. In
this case, I believe the code currently assumes the content hasn't changed, and
I think we should continue making that assumption.

Basically, to scope this change as narrowly as possible:

- If the response has a strong etag, then combine only if the etag matches the
buffered version
- Otherwise, do the same thing as before this change

[hubbe, 2016-09-16 19:33:41]

https://codereview.chromium.org/2338963002/diff/40001/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/2338963002/diff/40001/media/blink/url_index.c...
media/blink/url_index.cc:238: return false;
On 2016/09/16 19:30:21, Tom Bergan wrote:
> Missed this on the first read ... What if the response doesn't have any
> validator headers (neither Etag nor Last-Modified)? This happens frequently.
In
> this case, I believe the code currently assumes the content hasn't changed,
and
> I think we should continue making that assumption.
> 
> Basically, to scope this change as narrowly as possible:
> 
> - If the response has a strong etag, then combine only if the etag matches the
> buffered version
> - Otherwise, do the same thing as before this change

That is exactly what this code does.
(Unless I wrote it wrong somehow?)

[Tom Bergan, 2016-09-16 19:45:37]

lgtm

Thanks for doing this!

https://codereview.chromium.org/2338963002/diff/40001/media/blink/url_index.cc
File media/blink/url_index.cc (right):

https://codereview.chromium.org/2338963002/diff/40001/media/blink/url_index.c...
media/blink/url_index.cc:238: return false;
On 2016/09/16 19:33:41, hubbe wrote:
> On 2016/09/16 19:30:21, Tom Bergan wrote:
> > Missed this on the first read ... What if the response doesn't have any
> > validator headers (neither Etag nor Last-Modified)? This happens frequently.
> In
> > this case, I believe the code currently assumes the content hasn't changed,
> and
> > I think we should continue making that assumption.
> > 
> > Basically, to scope this change as narrowly as possible:
> > 
> > - If the response has a strong etag, then combine only if the etag matches
the
> > buffered version
> > - Otherwise, do the same thing as before this change
> 
> That is exactly what this code does.
> (Unless I wrote it wrong somehow?)

It is, I misread the meaning of "false" in this context (for some reason I
thought this should return "true" if the response had no validators). Sorry!

[hubbe, 2016-09-16 19:56:44]

The CQ bit was unchecked by hubbe@chromium.org

[hubbe, 2016-09-16 19:56:48]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-09-16 19:57:00]

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-09-16 20:10:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-16 20:13:34]

Description was changed from

==========
Store, use and send etags.

We use them to validate/invalide cached but expired data.
We send Match-If headers to prevent mixing of old/new cached data.

This should all be safe to do, since the HTTP cache already sends
etags to manage it's internal cached state, this CL just extends
this behavior to apply to client-side in-memory cached data as well.

BUG=504194
==========

to

==========
Store, use and send etags.

We use them to validate/invalide cached but expired data.
We send Match-If headers to prevent mixing of old/new cached data.

This should all be safe to do, since the HTTP cache already sends
etags to manage it's internal cached state, this CL just extends
this behavior to apply to client-side in-memory cached data as well.

BUG=504194

Committed: https://crrev.com/fd30799f6f8a7d1366d97693b72b0b20be94d5c0
Cr-Commit-Position: refs/heads/master@{#419264}
==========

[commit-bot: I haz the power, 2016-09-16 20:13:35]

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