Tie multibuffers to URLs

media/blink/url_index.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <set>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/message_loop/message_loop.h"
+#include "base/time/time.h"
+#include "media/blink/resource_multibuffer_data_provider.h"
+#include "media/blink/url_index.h"
+
+namespace media {
+
+const int kBlockSizeShift = 15;  // 1<<15 == 32kb
+const int kUrlMappingTimeoutSeconds = 300;
+
+ResourceMultiBuffer::ResourceMultiBuffer(UrlData* url_data)
+    : MultiBuffer(kBlockSizeShift, url_data->url_index_->lru_),
+      url_data_(url_data),
+      frame_(url_data->url_index_->frame()) {}

[xhwang, 2015/11/19 23:34:18]

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

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

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

+
+ResourceMultiBuffer::~ResourceMultiBuffer() {}
+
+MultiBuffer::DataProvider* ResourceMultiBuffer::CreateWriter(
+    const MultiBufferBlockId& pos) {
+  ResourceMultiBufferDataProvider* ret =
+      new ResourceMultiBufferDataProvider(url_data_, pos);
+  ret->Start();
+  return ret;

[xhwang, 2015/11/19 23:34:18]

See comments above. If the caller owns the writer, return a scoped_ptr?

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

Done.

+}
+
+bool ResourceMultiBuffer::RangeSupported() const {
+  return url_data_->range_supported_;
+}
+
+void ResourceMultiBuffer::OnEmpty() {
+  url_data_->OnEmpty();
+}
+
+UrlData::UrlData(const GURL& url,
+                 CORSMode cors_mode,
+                 const base::WeakPtr<UrlIndex>& url_index)
+    : url_(url),
+      cors_mode_(cors_mode),
+      url_index_(url_index),
+      length_(kPositionNotSpecified),
+      range_supported_(false),
+      cacheable_(false),
+      last_used_(),
+      multibuffer_(this) {}
+
+UrlData::~UrlData() {
+  if (url_index_)
+    url_index_->RemoveUrlData(this);
+}
+
+std::pair<GURL, UrlData::CORSMode> UrlData::key() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return std::make_pair(url(), cors_mode());
+}
+
+void UrlData::set_valid_until(base::TimeTicks t) {

[xhwang, 2015/11/19 23:34:18]

nit: s/t/valid_until?

following the "no abbreviation" rule...

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

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  valid_until_ = t;
+}
+
+void UrlData::MergeFrom(const scoped_refptr<UrlData>& other) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  valid_until_ = std::max(valid_until_, other->valid_until_);

[xhwang, 2015/11/19 23:34:18]

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"?

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

Comment added.

+  set_length(other->length_);

[xhwang, 2015/11/19 23:34:18]

Could you please explain why?

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

Done.

+  cacheable_ &= other->cacheable_;
+  range_supported_ |= other->range_supported_;

[xhwang, 2015/11/19 23:34:18]

why it's "or"?

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

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

+  if (last_modified_.is_null()) {
+    last_modified_ = other->last_modified_;
+  }
+  multibuffer()->MergeFrom(other->multibuffer());
+}
+
+void UrlData::set_cacheable(bool cacheable) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  cacheable_ = cacheable;
+}
+
+void UrlData::set_length(int64 length) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (length != kPositionNotSpecified) {
+    length_ = length;
+  }
+}
+
+void UrlData::RedirectTo(const scoped_refptr<UrlData>& url_data) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Copy any cached data over to the new location.
+  url_data->multibuffer()->MergeFrom(multibuffer());
+
+  std::vector<RedirectCB> redirect_callbacks;
+  redirect_callbacks.swap(redirect_callbacks_);
+  for (const RedirectCB& cb : redirect_callbacks) {
+    cb.Run(url_data);
+  }
+}
+
+void UrlData::Fail() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  // Handled similar to a redirect.
+  std::vector<RedirectCB> redirect_callbacks;
+  redirect_callbacks.swap(redirect_callbacks_);
+  for (const RedirectCB& cb : redirect_callbacks) {
+    cb.Run(nullptr);
+  }
+}
+
+void UrlData::OnRedirect(const RedirectCB& cb) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  redirect_callbacks_.push_back(cb);
+}
+
+void UrlData::Use() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  last_used_ = base::TimeTicks::Now();
+}
+
+void UrlData::OnEmpty() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (url_index_)
+    url_index_->RemoveUrlData(this);
+}
+
+bool UrlData::Valid() const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  base::TimeTicks now = base::TimeTicks::Now();
+  if (!range_supported_)
+    return false;
+  // When ranges are not supported, we cannot re-use cached data.
+  if (valid_until_ > now)
+    return true;
+  if (now - last_used_ <
+      base::TimeDelta::FromSeconds(kUrlMappingTimeoutSeconds))
+    return true;
+  return false;
+}
+
+void UrlData::set_last_modified(base::Time last_modified) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  last_modified_ = last_modified;
+}
+
+void UrlData::set_range_supported() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  range_supported_ = true;
+}
+
+ResourceMultiBuffer* UrlData::multibuffer() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return &multibuffer_;
+}
+
+size_t UrlData::CachedSize() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return multibuffer()->map().size();
+}
+
+UrlIndex::UrlIndex(blink::WebFrame* frame)
+    : frame_(frame), lru_(new MultiBuffer::GlobalLRU()), weak_factory_(this) {}
+
+UrlIndex::~UrlIndex() {}
+
+void UrlIndex::RemoveUrlData(const UrlData* url_data) {
+  auto i = by_url_.find(url_data->key());
+  if (i != by_url_.end() && i->second == url_data)
+    by_url_.erase(i);
+}
+
+scoped_refptr<UrlData> UrlIndex::GetByUrl(const GURL& gurl,
+                                          UrlData::CORSMode cors_mode) {
+  auto i = by_url_.find(std::make_pair(gurl, cors_mode));
+  if (i != by_url_.end() && i->second->Valid()) {
+    return i->second;
+  }
+  return NewUrlData(gurl, cors_mode);
+}
+
+UrlData* UrlIndex::NewUrlData(const GURL& url, UrlData::CORSMode cors_mode) {
+  return new UrlData(url, cors_mode, weak_factory_.GetWeakPtr());
+}
+
+scoped_refptr<UrlData> UrlIndex::TryInsert(
+    const scoped_refptr<UrlData>& urldata) {
+  std::map<UrlData::KeyType, scoped_refptr<UrlData>>::iterator i;
+  bool urldata_valid = urldata->Valid();

[xhwang, 2015/11/19 23:34:18]

s/urldata/url_data

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

Done.

+  if (urldata_valid) {
+    i = by_url_.insert(std::make_pair(urldata->key(), urldata)).first;
+  } else {
+    i = by_url_.find(urldata->key());
+    if (i == by_url_.end())
+      return urldata;
+  }
+  if (i->second == urldata)

[xhwang, 2015/11/19 23:34:18]

give i->second a more meaningful by assign it to a local variable? Then the code
could be more readable and concise.

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

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

+    return urldata;
+
+  // TODO(hubbe): Support etag validation.
+  if (!urldata->last_modified().is_null()) {
+    if (i->second->last_modified() != urldata->last_modified()) {
+      if (urldata_valid)
+        i->second = urldata;
+      return urldata;
+    }
+  }
+  // Check if we should replace the in-cache url data with our
+  // url data.
+  if (urldata_valid) {
+    if ((!i->second->Valid() ||
+         urldata->CachedSize() > i->second->CachedSize())) {
+      i->second = urldata;
+    } else {
+      i->second->MergeFrom(urldata);
+    }
+  }
+  return i->second;
+}
+
+}  // namespace media