Tie multibuffers to URLs

Provides an a way to have one multibuffer per URL and the data providers needed to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing. More CLs to follow.

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS1E/edit

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719
Committed: https://crrev.com/dd6151100c52e52929d5e09f5f7fb15653c67f14
Cr-Commit-Position: refs/heads/master@{#362242}

[hubbe, 2015-10-08 23:16:39]

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

[hubbe, 2015-10-14 22:37:12]

Time to actually start reviewing this CL.

[liberato (no reviews please), 2015-10-16 21:50:36]

still going...

-fl

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
File media/blink/multibuffer_data_source.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_data_source.cc:146: // A factory method to create
BufferedResourceLoader using the read parameters.
comment is out of date.  also, it doesn't create anything with the words
'resource' or 'loader' in it.

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_data_source.cc:329: }
else?

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_data_source.cc:434: // make sure that we update
has_single_origin_ now.
single_origin_

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_data_source.cc:486: if ((!!loader_ &&
loader_->IsLoading()) != loading_) {
extra points to use s/!=/^ :)

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
File media/blink/multibuffer_data_source.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_data_source.h:128: // Check if we've moved to a new url
and update has_signgle_origin_.
s/igng/ing/

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_data_source.h:168: scoped_ptr<MultiBufferReader>
loader_;
|reader_|?

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
File media/blink/multibuffer_resource_loader.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_resource_loader.cc:191: if
(original_url_data_->cors_mode() == UrlData::kUnspecified) {
we'll probably want somebody to check this whole CL for security issues.

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_resource_loader.cc:362: FROM_HERE,
base::Bind(&ResourceMultiBufferDataProvider::Start,
not sure that this works -- it starts fetching from |pos_|, but that isn't
updated until it's provided from Read().  won't we double-fetch, and send twice
to the multibuffer, the data that's already in |fifo_|?

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_resource_loader.cc:395: FROM_HERE,
base::Bind(&ResourceMultiBufferDataProvider::Start,
same concern about |pos_| + |fifo_| here.

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
File media/blink/multibuffer_resource_loader.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/120001/media/blink/mul...
media/blink/multibuffer_resource_loader.h:1: // Copyright 2015 The Chromium
Authors. All rights reserved.
this file is confusingly named -- it has no MultibufferResourceLoader in it.

[hubbe, 2015-10-16 23:47:06]

First batch of comments addressed.

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
File media/blink/multibuffer_data_source.cc (right):

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_data_source.cc:146: // A factory method to create
BufferedResourceLoader using the read parameters.
On 2015/10/16 21:50:36, liberato wrote:
> comment is out of date.  also, it doesn't create anything with the words
> 'resource' or 'loader' in it.

Removed the comment.

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_data_source.cc:329: }
On 2015/10/16 21:50:36, liberato wrote:
> else?

Sure, why not?

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_data_source.cc:434: // make sure that we update
has_single_origin_ now.
On 2015/10/16 21:50:36, liberato wrote:
> single_origin_

Done.

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_data_source.cc:486: if ((!!loader_ &&
loader_->IsLoading()) != loading_) {
On 2015/10/16 21:50:36, liberato wrote:
> extra points to use s/!=/^ :)

Acknowledged.

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
File media/blink/multibuffer_data_source.h (right):

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_data_source.h:168: scoped_ptr<MultiBufferReader>
loader_;
On 2015/10/16 21:50:36, liberato wrote:
> |reader_|?

Done.

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
File media/blink/multibuffer_resource_loader.cc (right):

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_resource_loader.cc:191: if
(original_url_data_->cors_mode() == UrlData::kUnspecified) {
On 2015/10/16 21:50:36, liberato wrote:
> we'll probably want somebody to check this whole CL for security issues.

Acknowledged.

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_resource_loader.cc:362: FROM_HERE,
base::Bind(&ResourceMultiBufferDataProvider::Start,
On 2015/10/16 21:50:36, liberato wrote:
> not sure that this works -- it starts fetching from |pos_|, but that isn't
> updated until it's provided from Read().  won't we double-fetch, and send
twice
> to the multibuffer, the data that's already in |fifo_|?

Added fifo_.clear() to Start with a comment.

https://codereview.chromium.org/1399603003/diff/120001/media/blink/multibuffe...
media/blink/multibuffer_resource_loader.cc:395: FROM_HERE,
base::Bind(&ResourceMultiBufferDataProvider::Start,
On 2015/10/16 21:50:36, liberato wrote:
> same concern about |pos_| + |fifo_| here.

and the same solution.

[liberato (no reviews please), 2015-11-05 19:03:18]

few things that i noticed before realizing i was looking at the older
multibuffer CL.

thanks
-fl

https://codereview.chromium.org/1399603003/diff/240001/components/html_viewer...
File components/html_viewer/media_factory.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/components/html_viewer...
components/html_viewer/media_factory.cc:105: if (!resource_multibuffer_.get() ||
perhaps this should be protected by kMediaUseMultibuffer also.

https://codereview.chromium.org/1399603003/diff/240001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2129: if (!resource_multibuffer_.get() ||
kMediaUseMultibuffer?

https://codereview.chromium.org/1399603003/diff/240001/media/blink/cache_util.cc
File media/blink/cache_util.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/cache_util...
media/blink/cache_util.cc:112: if
(Time::FromString(response.httpHeaderField("Date").utf8().data(), &date) &&
i think that max-age is supposed to take precedence over Expires.

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
File media/blink/resource_multibuffer.h (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
media/blink/resource_multibuffer.h:38: friend class
ResourceMultiBufferDataProvider;
why is this needed?  didn't see anything other than |frame_|, which has a public
accessor.

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:65:
resource_multibuffer_->frame_->setReferrerForRequest(request,
frame_ => frame() (and elsewhere)?

https://codereview.chromium.org/1399603003/diff/240001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:247: switches::kMediaUseMultibuffer)) {
&& |resource_multibuffer_| (maybe instead)

[hubbe, 2015-11-13 22:56:28]

Description was changed from

==========
Integrate multibuffer

Integrates multibuffer (https://codereview.chromium.org/1165903002/) with the
buffered_data_source, update test and remove the old code.

The overall effect should be:
  o better cache usage, cache isn't cleared when seeking
  o smaller wait times for seeks than 2Mb (previous code would download all the
data)
  o Multiple <video> tags can share data.
==========

to

==========
Provides an a way to have one multibuffer per URL and the data providers needed
to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing.
More CLs to follow.

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719
==========

[hubbe, 2015-11-13 22:56:52]

https://codereview.chromium.org/1399603003/diff/240001/media/blink/cache_util.cc
File media/blink/cache_util.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/cache_util...
media/blink/cache_util.cc:112: if
(Time::FromString(response.httpHeaderField("Date").utf8().data(), &date) &&
On 2015/11/05 19:03:18, liberato wrote:
> i think that max-age is supposed to take precedence over Expires.

Done.

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
File media/blink/resource_multibuffer.h (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
media/blink/resource_multibuffer.h:38: friend class
ResourceMultiBufferDataProvider;
On 2015/11/05 19:03:18, liberato wrote:
> why is this needed?  didn't see anything other than |frame_|, which has a
public
> accessor.

Gone.

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:65:
resource_multibuffer_->frame_->setReferrerForRequest(request,
On 2015/11/05 19:03:18, liberato wrote:
> frame_ => frame() (and elsewhere)?

Done.

https://codereview.chromium.org/1399603003/diff/240001/media/blink/webmediapl...
File media/blink/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/1399603003/diff/240001/media/blink/webmediapl...
media/blink/webmediaplayer_impl.cc:247: switches::kMediaUseMultibuffer)) {
On 2015/11/05 19:03:18, liberato wrote:
> && |resource_multibuffer_| (maybe instead)

Not sure which is better here...
I see your point, but I also like having the flag code in only one place and
creating an instance of the cache is pretty cheap. (And hopefully it won't be
too long until we flip the flag to on...)

[hubbe, 2015-11-13 22:58:19]

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

[xhwang, 2015-11-16 22:49:12]

I don't know enough about MultiBuffer to review this CL. I could read the design
doc and learn more about it (actually I like to know more about it) but that'll
take time. With that I'd defer the review to liberato. Feel free to replace me
with any other reviewer who is more familiar with this code.

[hubbe, 2015-11-16 22:58:42]

Description was changed from

==========
Provides an a way to have one multibuffer per URL and the data providers needed
to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing.
More CLs to follow.

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719
==========

to

==========
Provides an a way to have one multibuffer per URL and the data providers needed
to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing.
More CLs to follow.

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719
==========

[hubbe, 2015-11-16 22:59:29]

On 2015/11/16 22:49:12, xhwang wrote:
> I don't know enough about MultiBuffer to review this CL. I could read the
design
> doc and learn more about it (actually I like to know more about it) but
that'll
> take time. With that I'd defer the review to liberato. Feel free to replace me
> with any other reviewer who is more familiar with this code.

Updated with design doc, take a look and if that's not enough let me know where
I need to improve the docs and comments.

[xhwang, 2015-11-19 23:34:19]

I didn't review the test yet. Here are my first round of comments. Mostly nits
and questions asking for clarification.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/cac...
File media/blink/cache_util.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/cac...
media/blink/cache_util.cc:119: date > Time() && expires > Time()) {
What if expires <= date? We'll return some time that is earlier than Now(),
which seems okay. But maybe make this clear in a comment.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
File media/blink/multibuffer.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
media/blink/multibuffer.cc:261: }
tiny nit: braces can be omitted here. I actually saw mixed style in this file.
Is there any special rule you are following?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
media/blink/multibuffer.cc:264: void MultiBuffer::OnEmpty() {}
What's the purpose of this? Will some test class override this?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
File media/blink/multibuffer.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
media/blink/multibuffer.h:26: typedef int32_t MultiBufferBlockId;
Add a comment?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
media/blink/multibuffer.h:75: class MultiBuffer;
not needed?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
media/blink/multibuffer.h:243: void DataProviderEvent(DataProvider* provider);
In media/ callbacks are often called OnXxx, not sure whether OnDataProviderEvent
would make sense. Also "event" is vague. Can we make it more clear?
OnDataAvailable doesn't cover the wake up case...

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
media/blink/multibuffer.h:248: virtual DataProvider* CreateWriter(const BlockId&
pos) = 0;
Who owns the created Writer? If the caller owns it, return a scoped_ptr?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
File media/blink/multibuffer_unittest.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/mul...
media/blink/multibuffer_unittest.cc:24:
std::vector<FakeMultiBufferDataProvider*> writers;
move this to an anonymous or internal namespace?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:37: static const int
kMaxRetries = 3;
"static" not needed, also be consistent with l.33

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:52: void
ResourceMultiBufferDataProvider::Start() {
definition order doesn't match declaration order

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:88:
url_data_->multibuffer()->frame()->createAssociatedURLLoader(options));
It seems we plumb the |frame| all the way here through multiple layers only to
create the loader. ResourceMultiBufferDataProvider doesn't really need to know
WebFrame (which is a pretty big inteface) other than this.

Since we pretty much get everything from url_data_ to create the loader. Does it
make sense to ask url_data_ to create the loader for us? Or get some callback in
the constructor that can do so?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:98: // MultiBufferDataProvider
implementation.
MultiBuffer::DataProvider

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:122: if (active_loader_) {
nit: We like to return early

if (!active_loader)
  return;

// handle normal cases

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:168: : "Unknown")
Have a helper function to convert WebURLResponse::HTTPVersion_* to a string?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:228:
url_data_->set_range_supported();
We do similar operations in multiple places. Can we make a helper function to
so?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:231: // return kOk. It's also
possible for a server to support range requests
I don't see where "kOk" is returned.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:240: } else if
(response.httpStatusCode() == 416) {
We don't like hardcoded numbers, define a const similar to kHttpPartialContent?

Actually I am surprised to see net/ code also use numbers directly...

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:245:
url_data_->multibuffer()->DataProviderEvent(this);
We are using the multibuffer a lot, I kinda feel we have a layering issue... But
I am not familiar with the code enough to give better suggestions.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:269: url_data_ =
destination_url_data.get();
Can we remove the .get() part?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:273:
old_url_data->RedirectTo(destination_url_data);
These operations are confusing. Can we wrap 263-273 into a function, something
like swap()?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:290: if (fifo_.empty() ||
fifo_.back()->data_size() == block_size()) {
nit: fifo_.back()->data_size() appeared 4 times, worth a local variable? e.g.
block_data_size or existing_data_size?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
File media/blink/resource_multibuffer_data_provider.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:24: class MediaLog;
not used?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:25: class ResourceMultiBuffer;
not used as well?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:34: // MultiBufferDataProvider
implementation
MultiBuffer::DataProvider ?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:70: // VerifyPartialResponse (a
private method).
Not seeing how this will be tested... 

Given we have two test classes as friend, do we still need this to be public?

Also, I wonder whether we should test implementation detail instead of
interfaces. If the latter, we can just define it as a file scope static function
in the .cc file.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:76: // Virtual for testing
purposes.
I don't see how this is used for testing and why it needs to be virtual here...

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:79: protected:
Why not private?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:85: int64_t block_size() const;
nit: empty line here

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:95: UrlData* url_data_;
const?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
File media/blink/url_index.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:22: frame_(url_data->url_index_->frame()) {}
So the DataProvider gets the frame from url_data_->multibuffer()->frame(), and
the multibuffer gets the frame from url_data->url_index_->frame()...

See comments above about accessing the frame. The layering doesn't seem clean...

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:31: return ret;
See comments above. If the caller owns the writer, return a scoped_ptr?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:64: void UrlData::set_valid_until(base::TimeTicks t) {
nit: s/t/valid_until?

following the "no abbreviation" rule...

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:71: valid_until_ = std::max(valid_until_,
other->valid_until_);
Sorry for the lack of context... Could you please explain why it's std::max
instead of std::min? So some data that is expired could be valid again after
it's merged into another "data"?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:72: set_length(other->length_);
Could you please explain why?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:74: range_supported_ |= other->range_supported_;
why it's "or"?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:192: bool urldata_valid = urldata->Valid();
s/urldata/url_data

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.cc:200: if (i->second == urldata)
give i->second a more meaningful by assign it to a local variable? Then the code
could be more readable and concise.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
File media/blink/url_index.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:36: // MultiBuffer overrides
s/overrides/implementation to be consistent with other code in this CL?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:42: blink::WebFrame* frame() const { return frame_; }
See my comment above about passing WebFrame all the way down to create the
loader...

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:56: enum CORSMode { kUnspecified, kAnonymous,
kUseCredentials };
nit: We use UPPER_CASE for enums.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:146: int64 length_;
what length is this? in bytes?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:165: virtual ~UrlIndex();
empty line

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:169: // after initializing relevant parameters, like
weather it support
s/after//
s/weather/whether

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:175: // is already present in the index, return it
instead. Please make sure to
otherwise, what to return?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:187: scoped_refptr<UrlData> TryInsert(const
scoped_refptr<UrlData>& urldata);
nit: UrlData means Url and Data are two words, s/urldata/url_data ?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:187: scoped_refptr<UrlData> TryInsert(const
scoped_refptr<UrlData>& urldata);
empty line

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:196: virtual UrlData* NewUrlData(const GURL& url,
UrlData::CORSMode cors_mode);
return scoped_refptr<UrlData>? Typically it's a bad pattern to pass around a raw
pointer to a ref-counted object.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
File media/blink/url_index_unittest.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index_unittest.cc:19: UrlIndexTest() : url_index_(NULL) {}
nit: s/NULL/nullptr

[hubbe, 2015-11-20 23:24:24]

PTAL

https://codereview.chromium.org/1399603003/diff/380001/media/blink/cache_util.cc
File media/blink/cache_util.cc (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/cache_util...
media/blink/cache_util.cc:119: date > Time() && expires > Time()) {
On 2015/11/19 23:34:17, xhwang wrote:
> What if expires <= date? We'll return some time that is earlier than Now(),
> which seems okay. But maybe make this clear in a comment.

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
media/blink/multibuffer.cc:261: }
On 2015/11/19 23:34:17, xhwang wrote:
> tiny nit: braces can be omitted here. I actually saw mixed style in this file.
> Is there any special rule you are following?

No, unfortunately I'm still all mixed up since I used to work in a group where
we required braces everywhere.

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
media/blink/multibuffer.cc:264: void MultiBuffer::OnEmpty() {}
On 2015/11/19 23:34:17, xhwang wrote:
> What's the purpose of this? Will some test class override this?

Comment added in multibuffer.h
See ResourceMultiBuffer::OnEmpty in url_index.h/cc

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
media/blink/multibuffer.h:26: typedef int32_t MultiBufferBlockId;
On 2015/11/19 23:34:17, xhwang wrote:
> Add a comment?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
media/blink/multibuffer.h:75: class MultiBuffer;
On 2015/11/19 23:34:17, xhwang wrote:
> not needed?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
media/blink/multibuffer.h:243: void DataProviderEvent(DataProvider* provider);
On 2015/11/19 23:34:17, xhwang wrote:
> In media/ callbacks are often called OnXxx, not sure whether
OnDataProviderEvent
> would make sense. Also "event" is vague. Can we make it more clear?
> OnDataAvailable doesn't cover the wake up case... 

OnDataProviderEvent() seems ok, can't think of something better right now...
Event here is used in the same sense as "event loop", but I'm not sure how to
make that clear either.

DataProviderHandler() ?
OnDataProviderNeedsToDoSomething() ?
DataProviderCallback() ?

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
media/blink/multibuffer.h:248: virtual DataProvider* CreateWriter(const BlockId&
pos) = 0;
On 2015/11/19 23:34:17, xhwang wrote:
> Who owns the created Writer? If the caller owns it, return a scoped_ptr?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
File media/blink/multibuffer_unittest.cc (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/multibuffe...
media/blink/multibuffer_unittest.cc:24:
std::vector<FakeMultiBufferDataProvider*> writers;
On 2015/11/19 23:34:17, xhwang wrote:
> move this to an anonymous or internal namespace?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:37: static const int
kMaxRetries = 3;
On 2015/11/19 23:34:17, xhwang wrote:
> "static" not needed, also be consistent with l.33

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:52: void
ResourceMultiBufferDataProvider::Start() {
On 2015/11/19 23:34:17, xhwang wrote:
> definition order doesn't match declaration order

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:88:
url_data_->multibuffer()->frame()->createAssociatedURLLoader(options));
On 2015/11/19 23:34:17, xhwang wrote:
> It seems we plumb the |frame| all the way here through multiple layers only to
> create the loader. ResourceMultiBufferDataProvider doesn't really need to know
> WebFrame (which is a pretty big inteface) other than this.
> 
> Since we pretty much get everything from url_data_ to create the loader. Does
it
> make sense to ask url_data_ to create the loader for us? Or get some callback
in
> the constructor that can do so?
> 
> 

To me, it makes sense to have "creating a loader" in the same module as the
blink::WebUrlLoaderClient implementation. I've moved the frame() function from
the multibuffer to the url_data_, which simplifies things a bit, but doesn't
address the issue of making a broad interface available.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:98: // MultiBufferDataProvider
implementation.
On 2015/11/19 23:34:17, xhwang wrote:
> MultiBuffer::DataProvider

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:122: if (active_loader_) {
On 2015/11/19 23:34:17, xhwang wrote:
> nit: We like to return early
> 
> if (!active_loader)
>   return;
> 
> // handle normal cases

I tend to prefer non-negative if-statements over returning early, but when in
rome...

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:168: : "Unknown")
On 2015/11/19 23:34:17, xhwang wrote:
> Have a helper function to convert WebURLResponse::HTTPVersion_* to a string?

Helper function seems overkill since it's just for debugging.
This was pretty until git cl format got it's grubby hands on it.
Better now?

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:228:
url_data_->set_range_supported();
On 2015/11/19 23:34:17, xhwang wrote:
> We do similar operations in multiple places. Can we make a helper function to
> so?

A helper function that calls url_data_->set_range_supported()?
Or am I missing something?

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:231: // return kOk. It's also
possible for a server to support range requests
On 2015/11/19 23:34:17, xhwang wrote:
> I don't see where "kOk" is returned.

Removed reference to kOk.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:240: } else if
(response.httpStatusCode() == 416) {
On 2015/11/19 23:34:17, xhwang wrote:
> We don't like hardcoded numbers, define a const similar to
kHttpPartialContent?
> 
> Actually I am surprised to see net/ code also use numbers directly...

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:245:
url_data_->multibuffer()->DataProviderEvent(this);
On 2015/11/19 23:34:17, xhwang wrote:
> We are using the multibuffer a lot, I kinda feel we have a layering issue...
But
> I am not familiar with the code enough to give better suggestions.

To me, using url_data_ seems more questionable than using the multibuffer..

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:269: url_data_ =
destination_url_data.get();
On 2015/11/19 23:34:17, xhwang wrote:
> Can we remove the .get() part?

No.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:273:
old_url_data->RedirectTo(destination_url_data);
On 2015/11/19 23:34:17, xhwang wrote:
> These operations are confusing. Can we wrap 263-273 into a function, something
> like swap()?

I decided to add a bunch of comments instead.
Let me know if you still prefer a helper function.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:290: if (fifo_.empty() ||
fifo_.back()->data_size() == block_size()) {
On 2015/11/19 23:34:17, xhwang wrote:
> nit: fifo_.back()->data_size() appeared 4 times, worth a local variable? e.g.
> block_data_size or existing_data_size?

Done (but only for 3 out of the 4 cases.)

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.h (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:24: class MediaLog;
On 2015/11/19 23:34:17, xhwang wrote:
> not used?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:25: class ResourceMultiBuffer;
On 2015/11/19 23:34:18, xhwang wrote:
> not used as well?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:34: // MultiBufferDataProvider
implementation
On 2015/11/19 23:34:18, xhwang wrote:
> MultiBuffer::DataProvider ?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:70: // VerifyPartialResponse (a
private method).
On 2015/11/19 23:34:18, xhwang wrote:
> Not seeing how this will be tested... 
> 
> Given we have two test classes as friend, do we still need this to be public?
> 
> Also, I wonder whether we should test implementation detail instead of
> interfaces. If the latter, we can just define it as a file scope static
function
> in the .cc file.

Test is in a later CL I'm afraid.
I don't think it needs to be public, fixed.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:76: // Virtual for testing
purposes.
On 2015/11/19 23:34:18, xhwang wrote:
> I don't see how this is used for testing and why it needs to be virtual
here...

Unfortunately tests are in a later CL.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:79: protected:
On 2015/11/19 23:34:18, xhwang wrote:
> Why not private?

Comment added.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:85: int64_t block_size() const;
On 2015/11/19 23:34:18, xhwang wrote:
> nit: empty line here

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:95: UrlData* url_data_;
On 2015/11/19 23:34:18, xhwang wrote:
> const?

Unfortunately no, we change it when we change owners...

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

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:22: frame_(url_data->url_index_->frame()) {}
On 2015/11/19 23:34:18, xhwang wrote:
> So the DataProvider gets the frame from url_data_->multibuffer()->frame(), and
> the multibuffer gets the frame from url_data->url_index_->frame()...
> 
> See comments above about accessing the frame. The layering doesn't seem
clean...

moved to UrlData, not sure if you think that's better or not though...

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:31: return ret;
On 2015/11/19 23:34:18, xhwang wrote:
> See comments above. If the caller owns the writer, return a scoped_ptr?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:64: void UrlData::set_valid_until(base::TimeTicks t) {
On 2015/11/19 23:34:18, xhwang wrote:
> nit: s/t/valid_until?
> 
> following the "no abbreviation" rule...

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:71: valid_until_ = std::max(valid_until_,
other->valid_until_);
On 2015/11/19 23:34:18, xhwang wrote:
> Sorry for the lack of context... Could you please explain why it's std::max
> instead of std::min? So some data that is expired could be valid again after
> it's merged into another "data"?

Comment added.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:72: set_length(other->length_);
On 2015/11/19 23:34:18, xhwang wrote:
> Could you please explain why?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:74: range_supported_ |= other->range_supported_;
On 2015/11/19 23:34:18, xhwang wrote:
> why it's "or"?

Or is right, the & above it is probably not though.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:192: bool urldata_valid = urldata->Valid();
On 2015/11/19 23:34:18, xhwang wrote:
> s/urldata/url_data

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.cc:200: if (i->second == urldata)
On 2015/11/19 23:34:18, xhwang wrote:
> give i->second a more meaningful by assign it to a local variable? Then the
code
> could be more readable and concise.

Hmm, I changed it, but I'm not sure it's actually better...
(And it's not shorter either.)

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index.h
File media/blink/url_index.h (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:36: // MultiBuffer overrides
On 2015/11/19 23:34:18, xhwang wrote:
> s/overrides/implementation to be consistent with other code in this CL?

Done. (Although technically OnBuffer() is an override since a default
implementation exists, but whatever....)

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:42: blink::WebFrame* frame() const { return frame_; }
On 2015/11/19 23:34:18, xhwang wrote:
> See my comment above about passing WebFrame all the way down to create the
> loader...

moved to UrlData, better?

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:56: enum CORSMode { kUnspecified, kAnonymous,
kUseCredentials };
On 2015/11/19 23:34:18, xhwang wrote:
> nit: We use UPPER_CASE for enums.

This was copied from buffered_resource_loader.h which doesn't seem to follow
that rule at all.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:146: int64 length_;
On 2015/11/19 23:34:18, xhwang wrote:
> what length is this? in bytes?

Comment added. (And to several other variables.)

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:165: virtual ~UrlIndex();
On 2015/11/19 23:34:18, xhwang wrote:
> empty line

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:169: // after initializing relevant parameters, like
weather it support
On 2015/11/19 23:34:19, xhwang wrote:
> s/after//
> s/weather/whether

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:175: // is already present in the index, return it
instead. Please make sure to
On 2015/11/19 23:34:18, xhwang wrote:
> otherwise, what to return?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:187: scoped_refptr<UrlData> TryInsert(const
scoped_refptr<UrlData>& urldata);
On 2015/11/19 23:34:18, xhwang wrote:
> nit: UrlData means Url and Data are two words, s/urldata/url_data ?

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:187: scoped_refptr<UrlData> TryInsert(const
scoped_refptr<UrlData>& urldata);
On 2015/11/19 23:34:18, xhwang wrote:
> empty line

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:196: virtual UrlData* NewUrlData(const GURL& url,
UrlData::CORSMode cors_mode);
On 2015/11/19 23:34:18, xhwang wrote:
> return scoped_refptr<UrlData>? Typically it's a bad pattern to pass around a
raw
> pointer to a ref-counted object.

Done.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index_...
File media/blink/url_index_unittest.cc (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index_...
media/blink/url_index_unittest.cc:19: UrlIndexTest() : url_index_(NULL) {}
On 2015/11/19 23:34:19, xhwang wrote:
> nit: s/NULL/nullptr

Done.

[liberato (no reviews please), 2015-11-23 19:07:10]

https://codereview.chromium.org/1399603003/diff/400001/media/blink/cache_util.h
File media/blink/cache_util.h (right):

https://codereview.chromium.org/1399603003/diff/400001/media/blink/cache_util...
media/blink/cache_util.h:20: // Reasons that a cached WebURLResponse will *not*
prevent a future request to
can all of this be moved into WebURLResponse?

https://codereview.chromium.org/1399603003/diff/400001/media/blink/cache_util...
media/blink/cache_util.h:44: GetMemoryCacheValidUntil(const
blink::WebURLResponse& response);
not sure that 'Memory' adds much here -- aren't the rules the same?

https://codereview.chromium.org/1399603003/diff/400001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1399603003/diff/400001/media/blink/multibuffe...
media/blink/multibuffer.h:245: // some data for us. Also called when it might be
apprperiate
/appreperiat/appropriate/

https://codereview.chromium.org/1399603003/diff/400001/media/blink/multibuffe...
media/blink/multibuffer.h:258: // that goes with it.
'destroy this object': as in 'delete this'?

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

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.cc:38: url_data_->OnEmpty();
i'm not a big fan of self-deleters, especially since it's being deleted by an
object that contains an inline instance of a ResourceMultiBuffer.

having this (or UrlData::OnEmpty) post to something like
UrlIndex::OnEmpty(scoped_refptr<UrlData>), and then let it delete seems safer to
me.

i agree that it works as written, though.  if you choose to keep it, then please
add comments in the whole OnEmpty chain about it.

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.cc:55: url_index_->RemoveUrlData(this);
how would this happen?  urlindex has a strong ref to to urldata.

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index.h
File media/blink/url_index.h (right):

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.h:30: class MEDIA_BLINK_EXPORT ResourceMultiBuffer
since folks will want to create a ResourceMultiBuffer, might move this to
resource_multi_buffer.h .

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.h:43: UrlData* url_data_;
needs a comment that this cannot be accessed safely in the destructor.

[hubbe, 2015-11-23 20:17:30]

https://codereview.chromium.org/1399603003/diff/400001/media/blink/cache_util.h
File media/blink/cache_util.h (right):

https://codereview.chromium.org/1399603003/diff/400001/media/blink/cache_util...
media/blink/cache_util.h:20: // Reasons that a cached WebURLResponse will *not*
prevent a future request to
On 2015/11/23 19:07:10, liberato wrote:
> can all of this be moved into WebURLResponse?

Possibly.
I think we need to send some sort of signal as to weather content is getting
cached or not from the cache down to the client instead of making our own
guesses. However, I think that is not in the scope of this CL.

https://codereview.chromium.org/1399603003/diff/400001/media/blink/cache_util...
media/blink/cache_util.h:44: GetMemoryCacheValidUntil(const
blink::WebURLResponse& response);
On 2015/11/23 19:07:10, liberato wrote:
> not sure that 'Memory' adds much here -- aren't the rules the same?

s/GetMemoryCacheValidUntil/GetCacheValidUntil/g

https://codereview.chromium.org/1399603003/diff/400001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1399603003/diff/400001/media/blink/multibuffe...
media/blink/multibuffer.h:245: // some data for us. Also called when it might be
apprperiate
On 2015/11/23 19:07:10, liberato wrote:
> /appreperiat/appropriate/

Done.

https://codereview.chromium.org/1399603003/diff/400001/media/blink/multibuffe...
media/blink/multibuffer.h:258: // that goes with it.
On 2015/11/23 19:07:10, liberato wrote:
> 'destroy this object': as in 'delete this'?

s/destroy/free/

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

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.cc:38: url_data_->OnEmpty();
On 2015/11/23 19:07:10, liberato wrote:
> i'm not a big fan of self-deleters, especially since it's being deleted by an
> object that contains an inline instance of a ResourceMultiBuffer.
> 
> having this (or UrlData::OnEmpty) post to something like
> UrlIndex::OnEmpty(scoped_refptr<UrlData>), and then let it delete seems safer
to
> me.
> 
> i agree that it works as written, though.  if you choose to keep it, then
please
> add comments in the whole OnEmpty chain about it.

Agreed, updated UrlData::OnEmpty()

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.cc:55: url_index_->RemoveUrlData(this);
On 2015/11/23 19:07:10, liberato wrote:
> how would this happen?  urlindex has a strong ref to to urldata.

I was thinking that UrlData might not be in the index...
Of course, if they aren't in the index they also don't need to be removed from
the index...

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index.h
File media/blink/url_index.h (right):

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.h:30: class MEDIA_BLINK_EXPORT ResourceMultiBuffer
On 2015/11/23 19:07:10, liberato wrote:
> since folks will want to create a ResourceMultiBuffer, might move this to
> resource_multi_buffer.h .

Ignored, as per discussion.

https://codereview.chromium.org/1399603003/diff/400001/media/blink/url_index....
media/blink/url_index.h:43: UrlData* url_data_;
On 2015/11/23 19:07:10, liberato wrote:
> needs a comment that this cannot be accessed safely in the destructor.

Done.

[xhwang, 2015-11-23 23:09:21]

lgtm % nits/comments

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:168: : "Unknown")
On 2015/11/20 23:24:23, hubbe wrote:
> On 2015/11/19 23:34:17, xhwang wrote:
> > Have a helper function to convert WebURLResponse::HTTPVersion_* to a string?
> 
> Helper function seems overkill since it's just for debugging.
> This was pretty until git cl format got it's grubby hands on it.
> Better now?

Acknowledged.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.cc:228:
url_data_->set_range_supported();
On 2015/11/20 23:24:23, hubbe wrote:
> On 2015/11/19 23:34:17, xhwang wrote:
> > We do similar operations in multiple places. Can we make a helper function
to
> > so?
> 
> A helper function that calls url_data_->set_range_supported()?
> Or am I missing something?

Sorry for not being clear. A helper function that reads out data for a field in
a |response|.

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
File media/blink/resource_multibuffer_data_provider.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:76: // Virtual for testing
purposes.
On 2015/11/20 23:24:23, hubbe wrote:
> On 2015/11/19 23:34:18, xhwang wrote:
> > I don't see how this is used for testing and why it needs to be virtual
> here...
> 
> Unfortunately tests are in a later CL.

Then does it make sense to add Start() in that later CL?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/res...
media/blink/resource_multibuffer_data_provider.h:95: UrlData* url_data_;
On 2015/11/20 23:24:23, hubbe wrote:
> On 2015/11/19 23:34:18, xhwang wrote:
> > const?
> 
> Unfortunately no, we change it when we change owners...

Interesting fact! Worth a comment?

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
File media/blink/url_index.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/380001/media/blink/url...
media/blink/url_index.h:56: enum CORSMode { kUnspecified, kAnonymous,
kUseCredentials };
On 2015/11/20 23:24:24, hubbe wrote:
> On 2015/11/19 23:34:18, xhwang wrote:
> > nit: We use UPPER_CASE for enums.
> 
> This was copied from buffered_resource_loader.h which doesn't seem to follow
> that rule at all.

We don't update old code, but new code should follow this rule.

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
File media/blink/multibuffer.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
media/blink/multibuffer.cc:164: provider = writer_index_[pos] =
CreateWriter(pos).release();
release() seems hacky. Can we switch to use std::map<BlockId,
scoped_ptr<DataProvider>> asap?

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
File media/blink/multibuffer.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
media/blink/multibuffer.h:27: // Our blocks are 32kb (1 << 15), so our maximum
cacheable file  size
s/\ \ /\ /

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
media/blink/multibuffer.h:247: void OnDataProviderEvent(DataProvider* provider);
On...Event seems okay since you have multiple events to handle.

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
media/blink/multibuffer.h:305: std::map<BlockId, DataProvider*> writer_index_;
Use std::map<BlockId, scoped_ptr<DataProvider>>:

https://code.google.com/p/chromium/issues/detail?id=554291

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
File media/blink/multibuffer_unittest.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
media/blink/multibuffer_unittest.cc:35: media::TestRandom* rnd)
s/media:://

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
media/blink/multibuffer_unittest.cc:127: media::TestRandom* rnd)
ditto

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/mul...
media/blink/multibuffer_unittest.cc:219: class MultiBufferTest : public
testing::Test {
Is there any reason these tests are not in media namespace?

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/url...
File media/blink/url_index.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/url...
media/blink/url_index.cc:129: scoped_refptr<UrlData>(this)));
will s/scoped_refptr<UrlData>(this))/this/ work?

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/url...
File media/blink/url_index_unittest.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/420001/media/blink/url...
media/blink/url_index_unittest.cc:31: 
Is there any test to test the redirection case where we change the owner?

[hubbe, 2015-11-24 22:55:11]

PTAL

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.cc:228:
url_data_->set_range_supported();
On 2015/11/23 23:09:21, xhwang wrote:
> On 2015/11/20 23:24:23, hubbe wrote:
> > On 2015/11/19 23:34:17, xhwang wrote:
> > > We do similar operations in multiple places. Can we make a helper function
> to
> > > so?
> > 
> > A helper function that calls url_data_->set_range_supported()?
> > Or am I missing something?
> 
> Sorry for not being clear. A helper function that reads out data for a field
in
> a |response|.

Doesn't seem to be a lot of re-usable code here.
Just read out the data for a field is already a single line.
I looked at some of the functions in cache_util to see if there are some common
patterns, but the only thing that seems potentially worth it is to make a
function that reads out "Date" and "Expires" and returns a base::Time.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
File media/blink/resource_multibuffer_data_provider.h (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:76: // Virtual for testing
purposes.
On 2015/11/23 23:09:21, xhwang wrote:
> On 2015/11/20 23:24:23, hubbe wrote:
> > On 2015/11/19 23:34:18, xhwang wrote:
> > > I don't see how this is used for testing and why it needs to be virtual
> > here...
> > 
> > Unfortunately tests are in a later CL.
> 
> Then does it make sense to add Start() in that later CL?

No, Start() is needed already, but I could remove "virtual" for now if you like.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/resource_m...
media/blink/resource_multibuffer_data_provider.h:95: UrlData* url_data_;
On 2015/11/23 23:09:21, xhwang wrote:
> On 2015/11/20 23:24:23, hubbe wrote:
> > On 2015/11/19 23:34:18, xhwang wrote:
> > > const?
> > 
> > Unfortunately no, we change it when we change owners...
> 
> Interesting fact! Worth a comment?

Absolutely.

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index.h
File media/blink/url_index.h (right):

https://codereview.chromium.org/1399603003/diff/380001/media/blink/url_index....
media/blink/url_index.h:56: enum CORSMode { kUnspecified, kAnonymous,
kUseCredentials };
On 2015/11/23 23:09:21, xhwang wrote:
> On 2015/11/20 23:24:24, hubbe wrote:
> > On 2015/11/19 23:34:18, xhwang wrote:
> > > nit: We use UPPER_CASE for enums.
> > 
> > This was copied from buffered_resource_loader.h which doesn't seem to follow
> > that rule at all.
> 
> We don't update old code, but new code should follow this rule.

Better?

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
File media/blink/multibuffer.cc (right):

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
media/blink/multibuffer.cc:164: provider = writer_index_[pos] =
CreateWriter(pos).release();
On 2015/11/23 23:09:21, xhwang wrote:
> release() seems hacky. Can we switch to use std::map<BlockId,
> scoped_ptr<DataProvider>> asap?

Done.

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
media/blink/multibuffer.h:27: // Our blocks are 32kb (1 << 15), so our maximum
cacheable file  size
On 2015/11/23 23:09:21, xhwang wrote:
> s/\ \ /\ /

Done.

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
media/blink/multibuffer.h:247: void OnDataProviderEvent(DataProvider* provider);
On 2015/11/23 23:09:21, xhwang wrote:
> On...Event seems okay since you have multiple events to handle.

Acknowledged.

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
media/blink/multibuffer.h:305: std::map<BlockId, DataProvider*> writer_index_;
On 2015/11/23 23:09:21, xhwang wrote:
> Use std::map<BlockId, scoped_ptr<DataProvider>>:
> 
> https://code.google.com/p/chromium/issues/detail?id=554291

Done.

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
File media/blink/multibuffer_unittest.cc (right):

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
media/blink/multibuffer_unittest.cc:35: media::TestRandom* rnd)
On 2015/11/23 23:09:21, xhwang wrote:
> s/media:://

Done
(Here and everywhere else)

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
media/blink/multibuffer_unittest.cc:127: media::TestRandom* rnd)
On 2015/11/23 23:09:21, xhwang wrote:
> ditto

Done.

https://codereview.chromium.org/1399603003/diff/420001/media/blink/multibuffe...
media/blink/multibuffer_unittest.cc:219: class MultiBufferTest : public
testing::Test {
On 2015/11/23 23:09:21, xhwang wrote:
> Is there any reason these tests are not in media namespace?

Nope, done.

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

https://codereview.chromium.org/1399603003/diff/420001/media/blink/url_index....
media/blink/url_index.cc:129: scoped_refptr<UrlData>(this)));
On 2015/11/23 23:09:21, xhwang wrote:
> will s/scoped_refptr<UrlData>(this))/this/ work?

no:
../../base/bind.h:102:3: error: static_assert failed
"a_parameter_is_refcounted_type_and_needs_scoped_refptr"

Plus, I think it's helpful for the reader.

https://codereview.chromium.org/1399603003/diff/420001/media/blink/url_index_...
File media/blink/url_index_unittest.cc (right):

https://codereview.chromium.org/1399603003/diff/420001/media/blink/url_index_...
media/blink/url_index_unittest.cc:31: 
On 2015/11/23 23:09:21, xhwang wrote:
> Is there any test to test the redirection case where we change the owner?

There is now. :)

[xhwang, 2015-11-25 01:01:44]

lgtm with nits

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/mul...
File media/blink/multibuffer.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/mul...
media/blink/multibuffer.cc:266: writer_index_[pos] = provider.Pass();
nit: the new recommendation is to use std::move() instead of .Pass(). But we can
do a batch replacement later.

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
File media/blink/resource_multibuffer_data_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
2015

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:42: static const
char* kHttpUrl = "http://test";
Use const array

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:47: static const int
kHttpPartialContent = 206;
l.42-47: static not needed

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:64: :
view_(WebView::create(NULL)),
nullptr

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
File media/blink/url_index.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
media/blink/url_index.cc:171: weak_factory_(this) {}
You can use Delegated Constructors here:

https://chromium-cpp.appspot.com/

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
File media/blink/url_index.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
media/blink/url_index.h:185: explicit UrlIndex(blink::WebFrame*, int
block_shift);
explicit not needed

[hubbe, 2015-11-25 01:15:10]

Will submit tomorrow unless further comments show up. :)

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/mul...
File media/blink/multibuffer.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/mul...
media/blink/multibuffer.cc:266: writer_index_[pos] = provider.Pass();
On 2015/11/25 01:01:44, xhwang wrote:
> nit: the new recommendation is to use std::move() instead of .Pass(). But we
can
> do a batch replacement later.

Fixed.

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
File media/blink/resource_multibuffer_data_provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:1: // Copyright 2013
The Chromium Authors. All rights reserved.
On 2015/11/25 01:01:44, xhwang wrote:
> 2015

Done.

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:42: static const
char* kHttpUrl = "http://test";
On 2015/11/25 01:01:44, xhwang wrote:
> Use const array

Done.

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:47: static const int
kHttpPartialContent = 206;
On 2015/11/25 01:01:44, xhwang wrote:
> l.42-47: static not needed

Done.

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/res...
media/blink/resource_multibuffer_data_provider_unittest.cc:64: :
view_(WebView::create(NULL)),
On 2015/11/25 01:01:44, xhwang wrote:
> nullptr

Done.

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
File media/blink/url_index.cc (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
media/blink/url_index.cc:171: weak_factory_(this) {}
On 2015/11/25 01:01:44, xhwang wrote:
> You can use Delegated Constructors here:
> 
> https://chromium-cpp.appspot.com/

Lots of fancy new features. :)
Done.

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
File media/blink/url_index.h (right):

https://chromiumcodereview.appspot.com/1399603003/diff/460001/media/blink/url...
media/blink/url_index.h:185: explicit UrlIndex(blink::WebFrame*, int
block_shift);
On 2015/11/25 01:01:44, xhwang wrote:
> explicit not needed

Done.

[hubbe, 2015-11-30 19:44:32]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2015-11-30 19:44:32]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/1399603003/#ps560001
(title: "swap TimeTicks for Time in cacheing logic")

[commit-bot: I haz the power, 2015-11-30 19:46:42]

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

[commit-bot: I haz the power, 2015-11-30 20:14:56]

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

[commit-bot: I haz the power, 2015-11-30 20:14:57]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[hubbe, 2015-11-30 21:47:23]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2015-11-30 21:47:23]

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

[commit-bot: I haz the power, 2015-11-30 21:49:10]

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

[commit-bot: I haz the power, 2015-11-30 22:22:21]

Description was changed from

==========
Provides an a way to have one multibuffer per URL and the data providers needed
to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing.
More CLs to follow.

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719
==========

to

==========
Provides an a way to have one multibuffer per URL and the data providers needed
to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing.
More CLs to follow.

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719
==========

[commit-bot: I haz the power, 2015-11-30 22:22:21]

Committed patchset #30 (id:580001)

[commit-bot: I haz the power, 2015-11-30 22:23:21]

Description was changed from

==========
Provides an a way to have one multibuffer per URL and the data providers needed
to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing.
More CLs to follow.

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719
==========

to

==========
Provides an a way to have one multibuffer per URL and the data providers needed
to populate those multibuffers.

This CL used to be bigger, but I've split it into pieces for easier reviewing.
More CLs to follow.

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

Depends on:
https://codereview.chromium.org/1420883004/

BUG=514719

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

[commit-bot: I haz the power, 2015-11-30 22:23:22]

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