Inform disk cache of WebKit memory cache hits.

Inform disk cache of WebKit memory cache hits.

BUG=37112
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=95145

[darin (slow to review), 2011-07-27 22:55:01]

On Wed, Jul 27, 2011 at 3:48 PM, Darin Fisher <darin@chromium.org> wrote:

> On Tue, Jul 26, 2011 at 8:09 PM, <simonjam@chromium.org> wrote:
>
>> Reviewers: rvargas, darin,
>>
>> Description:
>> Inform disk cache of WebKit memory cache hits.
>>
>> BUG=37112
>> TEST=net_unittests
>>
>>
>> Please review this at
http://codereview.chromium.**org/7461106/<http://codereview.chromium.org/7461...
>>
>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>>
>> Affected files:
>>  M chrome/browser/renderer_host/**chrome_render_message_filter.h
>>  M chrome/browser/renderer_host/**chrome_render_message_filter.**cc
>>  M content/common/view_messages.h
>>  M content/renderer/render_view.**cc
>>  M net/disk_cache/backend_**unittest.cc
>>  M net/disk_cache/disk_cache.h
>>  M net/disk_cache/entry_impl.h
>>  M net/disk_cache/entry_impl.cc
>>  M net/disk_cache/in_flight_**backend_io.h
>>  M net/disk_cache/in_flight_**backend_io.cc
>>  M net/disk_cache/mem_entry_impl.**h
>>  M net/disk_cache/mem_entry_impl.**cc
>>  M net/http/http_cache.h
>>  M net/http/http_cache.cc
>>  M net/http/http_cache_**transaction.h
>>  M net/http/http_cache_**transaction.cc
>>  M net/http/http_cache_unittest.**cc
>>  M net/http/http_network_**transaction.h
>>  M net/http/http_transaction.h
>>  M net/http/http_transaction_**unittest.h
>>  M net/http/http_transaction_**unittest.cc
>>
>>
>>
> codereview.chromium.org seems to be done, so copying my comments here:
>
> ------
>
> it slightly bugs me that we have DidLoadResourceFromMemoryCache and
> DidAccessResourceInMemoryCache.  those sound like synonyms, and it seems
> odd
> that we should need to inform the browser twice about the same event.
>
> i understand that you are doing this because one is a routed message and
> the
> other is a control message.  one is handled on the UI thread of the browser
> and
> the other is handled on the IO thread of the browser.
>
> unfortunately, we do not have good infrastructure for intercepting a
> message
> bound for the browser's UI thread, inspecting the message, and then letting
> the
> message proceed on to the UI thread.  we have IPC MessageFilter which can
> be
> used to intercept the IPCs on the IO thread, but that terminates the
> handling of
> the IPC.
>
> ideally, just piggy back on ViewHostMsg_DidLoadResourceFromMemoryCache for
> this
> feature.  in the IO thread, we'd intercept the IPC, act on it, and then let
> the
> IPC continue on to the UI thread.
>
> maybe this is something to explore in the future?  but maybe there is an
> easy
> way to do this now?
>
>

jam informed me that IPC::ChannelProxy::MessageFilter::OnMessageReceived can
return false to allow normal message processing to continue.  Can you look
into
using that so we don't need to create a redundant message?

-Darin

[darin (slow to review), 2011-07-27 22:55:51]

On Tue, Jul 26, 2011 at 8:09 PM, <simonjam@chromium.org> wrote:

> Reviewers: rvargas, darin,
>
> Description:
> Inform disk cache of WebKit memory cache hits.
>
> BUG=37112
> TEST=net_unittests
>
>
> Please review this at
http://codereview.chromium.**org/7461106/<http://codereview.chromium.org/7461...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>  M chrome/browser/renderer_host/**chrome_render_message_filter.h
>  M chrome/browser/renderer_host/**chrome_render_message_filter.**cc
>  M content/common/view_messages.h
>  M content/renderer/render_view.**cc
>  M net/disk_cache/backend_**unittest.cc
>  M net/disk_cache/disk_cache.h
>  M net/disk_cache/entry_impl.h
>  M net/disk_cache/entry_impl.cc
>  M net/disk_cache/in_flight_**backend_io.h
>  M net/disk_cache/in_flight_**backend_io.cc
>  M net/disk_cache/mem_entry_impl.**h
>  M net/disk_cache/mem_entry_impl.**cc
>  M net/http/http_cache.h
>  M net/http/http_cache.cc
>  M net/http/http_cache_**transaction.h
>  M net/http/http_cache_**transaction.cc
>  M net/http/http_cache_unittest.**cc
>  M net/http/http_network_**transaction.h
>  M net/http/http_transaction.h
>  M net/http/http_transaction_**unittest.h
>  M net/http/http_transaction_**unittest.cc
>
>
>
codereview.chromium.org seems to be done, so copying my comments here:

------

it slightly bugs me that we have DidLoadResourceFromMemoryCache and
DidAccessResourceInMemoryCache.  those sound like synonyms, and it seems odd
that we should need to inform the browser twice about the same event.

i understand that you are doing this because one is a routed message and the
other is a control message.  one is handled on the UI thread of the browser
and
the other is handled on the IO thread of the browser.

unfortunately, we do not have good infrastructure for intercepting a message
bound for the browser's UI thread, inspecting the message, and then letting
the
message proceed on to the UI thread.  we have IPC MessageFilter which can be
used to intercept the IPCs on the IO thread, but that terminates the
handling of
the IPC.

ideally, just piggy back on ViewHostMsg_DidLoadResourceFromMemoryCache for
this
feature.  in the IO thread, we'd intercept the IPC, act on it, and then let
the
IPC continue on to the UI thread.

maybe this is something to explore in the future?  but maybe there is an
easy
way to do this now?

[James Simonsen, 2011-07-27 23:14:02]

On Wed, Jul 27, 2011 at 3:48 PM, Darin Fisher <darin@chromium.org> wrote:

> On Tue, Jul 26, 2011 at 8:09 PM, <simonjam@chromium.org> wrote:
>
>> Reviewers: rvargas, darin,
>>
>> Description:
>> Inform disk cache of WebKit memory cache hits.
>>
>> BUG=37112
>> TEST=net_unittests
>>
>>
>> Please review this at
http://codereview.chromium.**org/7461106/<http://codereview.chromium.org/7461...
>>
>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>>
>> Affected files:
>>  M chrome/browser/renderer_host/**chrome_render_message_filter.h
>>  M chrome/browser/renderer_host/**chrome_render_message_filter.**cc
>>  M content/common/view_messages.h
>>  M content/renderer/render_view.**cc
>>  M net/disk_cache/backend_**unittest.cc
>>  M net/disk_cache/disk_cache.h
>>  M net/disk_cache/entry_impl.h
>>  M net/disk_cache/entry_impl.cc
>>  M net/disk_cache/in_flight_**backend_io.h
>>  M net/disk_cache/in_flight_**backend_io.cc
>>  M net/disk_cache/mem_entry_impl.**h
>>  M net/disk_cache/mem_entry_impl.**cc
>>  M net/http/http_cache.h
>>  M net/http/http_cache.cc
>>  M net/http/http_cache_**transaction.h
>>  M net/http/http_cache_**transaction.cc
>>  M net/http/http_cache_unittest.**cc
>>  M net/http/http_network_**transaction.h
>>  M net/http/http_transaction.h
>>  M net/http/http_transaction_**unittest.h
>>  M net/http/http_transaction_**unittest.cc
>>
>>
>>
> codereview.chromium.org seems to be done, so copying my comments here:
>
> ------
>
> it slightly bugs me that we have DidLoadResourceFromMemoryCache and
> DidAccessResourceInMemoryCache.  those sound like synonyms, and it seems
> odd
> that we should need to inform the browser twice about the same event.
>
> i understand that you are doing this because one is a routed message and
> the
> other is a control message.  one is handled on the UI thread of the browser
> and
> the other is handled on the IO thread of the browser.
>

Yeah, that was definitely the main reason for the new message. It could have
other uses though. I thought someday we might also want to periodically ping
resources that are still in active use. We could send this new message to
indicate that. The idea being that your Gmail resources, which are rarely
loaded, will then stay resident in the disk cache and the next time you
restart Chrome, Gmail will load quicker.

However, I don't plan on doing that work right now, so it's probably jumping
the gun to add a message for something we may not use in the future. I'll
try returning false and report back.

James

[rvargas (doing something else), 2011-07-27 23:26:17]

Thanks for working on this!

http://codereview.chromium.org/7461106/diff/1/chrome/browser/renderer_host/ch...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7461106/diff/1/chrome/browser/renderer_host/ch...
chrome/browser/renderer_host/chrome_render_message_filter.cc:542:
request_context_->GetURLRequestContext()->http_transaction_factory()->
I'm not sure, but doesn't this belong to content?

http://codereview.chromium.org/7461106/diff/1/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/7461106/diff/1/content/renderer/render_view.cc...
content/renderer/render_view.cc:2724: Send(new
ViewHostMsg_DidAccessResourceInMemoryCache(
It seems unfortunate to send two IPCs messages for the same signal.

http://codereview.chromium.org/7461106/diff/1/net/disk_cache/backend_unittest.cc
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/7461106/diff/1/net/disk_cache/backend_unittest...
net/disk_cache/backend_unittest.cc:2122: TEST_F(DiskCacheBackendTest,
UpdateRankForExternalCacheHit) {
I know the distinction is not that clear, but this test should go to
entry_unittest.cc

http://codereview.chromium.org/7461106/diff/1/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/7461106/diff/1/net/disk_cache/disk_cache.h#new...
net/disk_cache/disk_cache.h:295: // directly. This allows feedback from the
renderer's memory cache to improve
net is not aware of what a renderer is. I think that the name of this method is
a bit too specific... it implies that the caller knows what a "rank" is, and the
comments talks about LRU...

Maybe something closer to OnExternalCacheHit(), OnEntryReused(),
EntryReused()...

Also, this should be a member of the backend and not the entry: if the user has
an open entry there is little to do, and if it has to open the entry that should
do the trick by itself. (note that making this change implies that my previous
comment about the location of the test is no longer valid)

http://codereview.chromium.org/7461106/diff/1/net/http/http_cache.cc
File net/http/http_cache.cc (right):

http://codereview.chromium.org/7461106/diff/1/net/http/http_cache.cc#newcode466
net/http/http_cache.cc:466: class ReportExternalCacheHitJob {
Maybe we can simplify this code if the disk cache method doesn't return a value
(and if  we don't worry about the ActiveEntry). Then, there is no need for a
transaction :)

http://codereview.chromium.org/7461106/diff/1/net/http/http_cache.h
File net/http/http_cache.h (right):

http://codereview.chromium.org/7461106/diff/1/net/http/http_cache.h#newcode185
net/http/http_cache.h:185: void ReportExternalCacheHit(const GURL& url, const
std::string& http_method);
Maybe OnExternalCacheHit? I'm not so fond of OnXxx, but from the point of view
of the caller this method doesn't report anything :(

[James Simonsen, 2011-07-28 01:25:19]

Thanks for all the feedback!

Returning false from the message handler did indeed work.

Also, moving the code to the backend dramatically simplified things. Thanks!

http://codereview.chromium.org/7461106/diff/1/chrome/browser/renderer_host/ch...
File chrome/browser/renderer_host/chrome_render_message_filter.cc (right):

http://codereview.chromium.org/7461106/diff/1/chrome/browser/renderer_host/ch...
chrome/browser/renderer_host/chrome_render_message_filter.cc:542:
request_context_->GetURLRequestContext()->http_transaction_factory()->
On 2011/07/27 23:26:17, rvargas wrote:
> I'm not sure, but doesn't this belong to content?

Sorry, I don't know what you mean by "content." I mostly work in WebKit, so I'm
not as familiar with the Chrome side.

In case this is along the lines what you're asking... I've verified that both
incognito and the normal windows successfully update the rank of the entries
while browsing around google.com's press information.

[rvargas (doing something else), 2011-07-28 22:27:01]

What I meant by the ChromeRenderMessageFilter is that src\content is meant to
have things that any embedder should be interested in having (as opposed to
things that are chrome specific: how we show them to the user). This interaction
of resources and the cache seem to belong to the first group.

Also, URlRequestContext at this point is not necessarily the one that that you
want... we have regular requests, incognito requests, media requests and media
for incognito. They all use different caches :(.

It's probably fine to ignore media requests (most likely they are not stored in
the webkit cache 'cause we normally use range requests for them), but we should
use the right cache anyway.

Regular request context determination goes through BrowserRenderProcessHost
(ResourceMessageFilter, ResourceDispatcherHost)

See RendererURLRequestContextSelector::GetRequestContext.

http://codereview.chromium.org/7461106/diff/3004/net/disk_cache/backend_impl.cc
File net/disk_cache/backend_impl.cc (right):

http://codereview.chromium.org/7461106/diff/3004/net/disk_cache/backend_impl....
net/disk_cache/backend_impl.cc:691: }
Leaking the entry.

We should also check that this is an ENTRY_NORMAL before calling UpdateRank

http://codereview.chromium.org/7461106/diff/3004/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/7461106/diff/3004/net/disk_cache/disk_cache.h#...
net/disk_cache/disk_cache.h:142: // been used directly. This allows feedback
from the renderer's memory cache
There should not be a reference to the renderer's memory cache here.

Also, stating that there is an LRU cache is not a good idea, because in fact the
default cache is not LRU... and what does it means to improve the rank of a
resource? That is an implementation detail.

We should just limit the documentation to the invariant part: That this method
should be called whenever there is an  external cache that is serving this entry
and we expect that entry to be stored by this cache. What we do with that
information is implementation specific.

http://codereview.chromium.org/7461106/diff/3004/net/http/http_cache.h
File net/http/http_cache.h (right):

http://codereview.chromium.org/7461106/diff/3004/net/http/http_cache.h#newcod...
net/http/http_cache.h:182: // Indicates that the renderer's memory cache hit the
resource specified by
What is the renderer's memory cache?

http://codereview.chromium.org/7461106/diff/3004/net/http/http_cache_unittest.cc
File net/http/http_cache_unittest.cc (right):

http://codereview.chromium.org/7461106/diff/3004/net/http/http_cache_unittest...
net/http/http_cache_unittest.cc:283: ++rank_;
I wouldn't actually bother with this. Now you are writing a test that verifies
that the mock cache behaves as the cache implementation (sort of).

I would be fine leaving that method untested (it's just a simple forwarder). But
you already wrote the tests so you can leave them in if you think they are
useful.

[James Simonsen, 2011-07-30 01:22:55]

Thanks for explaining everything. I think I've addressed everything you
mentioned.

[darin (slow to review), 2011-07-30 05:09:32]

LGTM for content/ and webkit/

http://codereview.chromium.org/7461106/diff/1/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/7461106/diff/1/content/common/view_messages.h#...
content/common/view_messages.h:1418:
IPC_MESSAGE_CONTROL2(ViewHostMsg_DidAccessResourceInMemoryCache,
it slightly bugs me that we have DidLoadResourceFromMemoryCache and
DidAccessResourceInMemoryCache.  those sound like synonyms, and it seems odd
that we should need to inform the browser twice about the same event.

i understand that you are doing this because one is a routed message and the
other is a control message.  one is handled on the UI thread of the browser and
the other is handled on the IO thread of the browser.

unfortunately, we do not have good infrastructure for intercepting a message
bound for the browser's UI thread, inspecting the message, and then letting the
message proceed on to the UI thread.  we have IPC MessageFilter which can be
used to intercept the IPCs on the IO thread, but that terminates the handling of
the IPC.

ideally, just piggy back on ViewHostMsg_DidLoadResourceFromMemoryCache for this
feature.  in the IO thread, we'd intercept the IPC, act on it, and then let the
IPC continue on to the UI thread.

maybe this is something to explore in the future?  but maybe there is an easy
way to do this now?

edit: jam points out that MessageFilter::OnMessageReceived can return false to
allow the message to be normally processed!

http://codereview.chromium.org/7461106/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7461106/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:348: if
(message.type() == ViewHostMsg_DidLoadResourceFromMemoryCache::ID) {
it is a tad unfortunate that this code can't be inside the handler function, but
unless we have more code like this, it is probably not worth creating framework
to support a cleaner solution.

http://codereview.chromium.org/7461106/diff/17001/webkit/glue/resource_type.h
File webkit/glue/resource_type.h (right):

http://codereview.chromium.org/7461106/diff/17001/webkit/glue/resource_type.h...
webkit/glue/resource_type.h:42: static Type
FromTargetType(WebKit::WebURLRequest::TargetType type) {
this is sizeable enough that it should not be implemented inline.

[darin (slow to review), 2011-07-30 05:10:49]

On 2011/07/30 05:09:32, darin wrote:
> it slightly bugs me that we have DidLoadResourceFromMemoryCache and
> DidAccessResourceInMemoryCache.  those sound like synonyms, and it 

^^^ hrm, not sure why that comment was repeated.  oh well :-/

[darin (slow to review), 2011-07-30 05:10:50]

On 2011/07/30 05:09:32, darin wrote:
> it slightly bugs me that we have DidLoadResourceFromMemoryCache and
> DidAccessResourceInMemoryCache.  those sound like synonyms, and it 

^^^ hrm, not sure why that comment was repeated.  oh well :-/

[rvargas (doing something else), 2011-08-01 18:18:06]

LGTM, Thanks!

[James Simonsen, 2011-08-02 00:14:27]

http://codereview.chromium.org/7461106/diff/17001/content/browser/renderer_ho...
File content/browser/renderer_host/resource_dispatcher_host.cc (right):

http://codereview.chromium.org/7461106/diff/17001/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:348: if
(message.type() == ViewHostMsg_DidLoadResourceFromMemoryCache::ID) {
On 2011/07/30 05:09:32, darin wrote:
> it is a tad unfortunate that this code can't be inside the handler function,
but
> unless we have more code like this, it is probably not worth creating
framework
> to support a cleaner solution.

I could only find one other case. They use IPC_MESSAGE_HANDLER_GENERIC instead,
but they're only making a simple function call (no args).

It'd be easy to add a new IPC macro, but this would be the only real use case.

http://codereview.chromium.org/7461106/diff/18009/content/browser/renderer_ho...
content/browser/renderer_host/resource_dispatcher_host.cc:693:
filter_->GetURLRequestContext(resource_type)->http_transaction_factory()->
I had to add this check, because WebKit was sending us file: urls, which tripped
a DCHECK under GenerateKey.

[darin (slow to review), 2011-08-02 03:49:22]

On Mon, Aug 1, 2011 at 5:14 PM, <simonjam@chromium.org> wrote:

>
> http://codereview.chromium.**org/7461106/diff/17001/**
>
content/browser/renderer_host/**resource_dispatcher_host.cc<http://codereview.chromium.org/7461106/diff/17001/content/browser/renderer_host/resource_dispatcher_host.cc>
> File content/browser/renderer_host/**resource_dispatcher_host.cc (right):
>
> http://codereview.chromium.**org/7461106/diff/17001/**
>
content/browser/renderer_host/**resource_dispatcher_host.cc#**newcode348<http://codereview.chromium.org/7461106/diff/17001/content/browser/renderer_host/resource_dispatcher_host.cc#newcode348>
> content/browser/renderer_host/**resource_dispatcher_host.cc:**348: if
> (message.type() == ViewHostMsg_**DidLoadResourceFromMemoryCache**::ID) {
> On 2011/07/30 05:09:32, darin wrote:
>
>> it is a tad unfortunate that this code can't be inside the handler
>>
> function, but
>
>> unless we have more code like this, it is probably not worth creating
>>
> framework
>
>> to support a cleaner solution.
>>
>
> I could only find one other case. They use IPC_MESSAGE_HANDLER_GENERIC
> instead, but they're only making a simple function call (no args).
>
> It'd be easy to add a new IPC macro, but this would be the only real use
> case.
>

Probably best to leave it as you have it for now.



>
> http://codereview.chromium.**org/7461106/diff/18009/**
>
content/browser/renderer_host/**resource_dispatcher_host.cc#**newcode693<http://codereview.chromium.org/7461106/diff/18009/content/browser/renderer_host/resource_dispatcher_host.cc#newcode693>
> content/browser/renderer_host/**resource_dispatcher_host.cc:**693:
> filter_->GetURLRequestContext(**resource_type)->http_**
> transaction_factory()->
> I had to add this check, because WebKit was sending us file: urls, which
> tripped a DCHECK under GenerateKey.


Makes sense.

LGTM!

Do we have any histograms that can help us know how much impact this change
is making?

-Darin



>
>
>
http://codereview.chromium.**org/7461106/<http://codereview.chromium.org/7461...
>

[James Simonsen, 2011-08-02 17:55:46]

On 2011/08/02 03:49:22, darin wrote:
> Do we have any histograms that can help us know how much impact this change
> is making?

I think our best bet is DiskCache.0.EntryAccessRate. I'll keep an eye on it in
the dev channel as well as the overall PLT metrics. Perhaps Ricardo knows
others?

[darin (slow to review), 2011-08-02 17:58:04]

On Tue, Aug 2, 2011 at 10:55 AM, <simonjam@chromium.org> wrote:

> On 2011/08/02 03:49:22, darin wrote:
>
>> Do we have any histograms that can help us know how much impact this
>> change
>> is making?
>>
>
> I think our best bet is DiskCache.0.EntryAccessRate. I'll keep an eye on it
> in
> the dev channel as well as the overall PLT metrics. Perhaps Ricardo knows
> others?


Should we roll this CL out as an A/B test?

-Darin



>
>
>
http://codereview.chromium.**org/7461106/<http://codereview.chromium.org/7461...
>

[darin (slow to review), 2011-08-02 17:58:43]

On Tue, Aug 2, 2011 at 10:57 AM, Darin Fisher <darin@chromium.org> wrote:

> On Tue, Aug 2, 2011 at 10:55 AM, <simonjam@chromium.org> wrote:
>
>> On 2011/08/02 03:49:22, darin wrote:
>>
>>> Do we have any histograms that can help us know how much impact this
>>> change
>>> is making?
>>>
>>
>> I think our best bet is DiskCache.0.EntryAccessRate. I'll keep an eye on
>> it in
>> the dev channel as well as the overall PLT metrics. Perhaps Ricardo knows
>> others?
>
>
> Should we roll this CL out as an A/B test?
>
> -Darin
>

I should add that I'm pretty darn confident that this patch is going to make
a significant improvement, but it'd be nice to be able to quantify that
improvement :-)


>
>
>
>>
>>
>>
http://codereview.chromium.**org/7461106/<http://codereview.chromium.org/7461...
>>
>
>

[rvargas (doing something else), 2011-08-02 18:01:51]

On 2011/08/02 03:49:22, darin wrote:
> Do we have any histograms that can help us know how much impact this change
> is making?

We don't. I guess we'd have to setup an experiment and disable this change for
some people to be able to have meaningful measurements. It's hard to evaluate
the real impact because the theory is that it will only affect a few sites for a
few users, so most likely any signal will be completely lost  in the noise.

I'm currently running an experiment on the disk cache, so I would prefer not
having another one at the same time... I guess it's fine to roll this up an
measure later?

[James Simonsen, 2011-08-02 18:02:57]

On 2011/08/02 17:58:04, darin wrote:
> Should we roll this CL out as an A/B test?

How about we monitor canary/dev channel and if the results are at all ambiguous,
then we can do a holdback A/B test by disabling it for half the users? We're
really early into M15, so there's no hurry.

[darin (slow to review), 2011-08-02 18:17:12]

Agreed.
-Darin

On Tue, Aug 2, 2011 at 11:02 AM, <simonjam@chromium.org> wrote:

> On 2011/08/02 17:58:04, darin wrote:
>
>> Should we roll this CL out as an A/B test?
>>
>
> How about we monitor canary/dev channel and if the results are at all
> ambiguous,
> then we can do a holdback A/B test by disabling it for half the users?
> We're
> really early into M15, so there's no hurry.
>
>
>
http://codereview.chromium.**org/7461106/<http://codereview.chromium.org/7461...
>

[commit-bot: I haz the power, 2011-08-02 20:13:32]

Change committed as 95145