Move webCacheManager to //components.

WebCacheManager::ClearCacheOnNavigation() is used by webrequest api, which will be moved to //extensions. So we need to resolve its the dependency on WebCacheManager by moving WebCacheManager to //components.

There are two ipc messages sent in WebCacheManger:
-ChromeViewMsg_SetCacheCapacities
-ChromeViewMsg_ClearCache

Both ipc handlers were in ChromeRenderProcessObserver. These two ipcs are renamed as WebCacheMsg_XXX, and we introduce WebCacheRenderProcessObserver and handle these ipcs in //components.

BUG=352293, 411437
TBR=agl@chromium.org

Committed: https://crrev.com/149b92db00bc1deab22599e7d3da3fc8be2c0d8b
Cr-Commit-Position: refs/heads/master@{#294475}

[Xi Han, 2014-09-03 15:36:32]

Patchset #1 (id:1) has been deleted

[Xi Han, 2014-09-03 16:11:35]

Patchset #1 (id:20001) has been deleted

[Xi Han, 2014-09-04 15:22:05]

Patchset #2 (id:60001) has been deleted

[Xi Han, 2014-09-04 15:32:13]

Patchset #2 (id:80001) has been deleted

[Xi Han, 2014-09-04 17:26:12]

Patchset #2 (id:100001) has been deleted

[Xi Han, 2014-09-04 20:09:07]

Patchset #4 (id:160001) has been deleted

[Xi Han, 2014-09-04 20:09:29]

Patchset #3 (id:140001) has been deleted

[Xi Han, 2014-09-04 20:21:28]

Patchset #3 (id:180001) has been deleted

[Xi Han, 2014-09-04 21:24:43]

Patchset #3 (id:200001) has been deleted

[Xi Han, 2014-09-04 21:32:54]

hanxi@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Xi Han, 2014-09-04 21:32:55]

Please review this CL and let me know if you have any idea about the failures in
try-bot servers. Thanks!

[Fady Samuel, 2014-09-04 22:38:52]

https://codereview.chromium.org/528363002/diff/220001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.cc (right):

https://codereview.chromium.org/528363002/diff/220001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:358:
UMA_HISTOGRAM_COUNTS_100("Cache.ActiveTabs", active_renderers_.size());
The windows compiler doesn't seem to like these, but I can't explain why :(

[Xi Han, 2014-09-05 14:19:18]

hanxi@chromium.org changed reviewers:
+ jamescook@chromium.org

[Xi Han, 2014-09-05 14:19:19]

+jamescook@chromium.org
James: please review this CL. Do you have any idea about the failures in android
and windows?

[James Cook, 2014-09-05 16:32:44]

On 2014/09/05 14:19:19, hanxi wrote:
> mailto:+jamescook@chromium.org
> James: please review this CL. Do you have any idea about the failures in
android
> and windows?

The Windows failure is because you introduced a new build target without this
warning suppression:

      # Disable c4267 warnings until we fix size_t to int truncations.
      'msvs_disabled_warnings': [ 4267, ],
      'conditions': [
        ['OS=="win" and win_use_allocator_shim==1', {
          'dependencies': [
            '../base/allocator/allocator.gyp:allocator',
          ],
        }],
      ],

It's in all the build targets, probably as part of the win x64 bringup. I have
no idea if it will ever get fixed.

If you would prefer to avoid that suppression you can static_cast<> the size_t
to int32_t

[James Cook, 2014-09-05 16:53:18]

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_time_tracker.cc
(left):

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_time_tracker.cc:13:
#include "content/public/browser/browser_thread.h"
Hooray for cleaning up unused includes!

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/renderer...
File chrome/browser/renderer_host/web_cache_manager_browsertest.cc (right):

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/renderer...
chrome/browser/renderer_host/web_cache_manager_browsertest.cc:23: class
WebCacheManagerBrowserTest : public InProcessBrowserTest {
So what's the plan for these tests? It's weird that the test source files live
in src/chrome but the code they are testing lives in src/components.

I suggest doing something like we do for src/extensions unit tests. Move this
file into src/components along with the code under test, fix
src/components/web_cache/DEPS to allow src/chrome dependencies just test files,
then compile and run the tests as part of Chrome's browser_tests suite. Then
leave a comment and file a bug to track refactoring the tests so they can run in
components_browsertests.

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/task_man...
File chrome/browser/task_manager/task_manager.h (right):

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager.h:21: #include
"components/web_cache/browser/web_cache_manager.h"
Is this header used?

https://codereview.chromium.org/528363002/diff/240001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/528363002/diff/240001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1180:
'browser/renderer_host/web_cache_manager_unittest.cc',
Hooray for moving the unit test!

https://codereview.chromium.org/528363002/diff/240001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/528363002/diff/240001/components/BUILD.gn#new...
components/BUILD.gn:98: "//components/web_cache/browser",
nit: alphabetize above web_modal

https://codereview.chromium.org/528363002/diff/240001/components/web_cache.gypi
File components/web_cache.gypi (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache.gy...
components/web_cache.gypi:25: 'web_cache_common',
nit: put this at the bottom of the list

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
File components/web_cache/browser/BUILD.gn (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
components/web_cache/browser/BUILD.gn:16: "//content/test:test_support",
Why does this need test_support? Shouldn't that only be needed for the tests?

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.cc (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:58:
registry->RegisterIntegerPref(prefs::kMemoryCacheSize, GetDefaultCacheSize());
Do you still need this pref? It looks like you deleted the code that uses it.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
File components/web_cache/common/pref_names.h (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
components/web_cache/common/pref_names.h:14: extern const char
kMemoryCacheSize[];
Can this go away?

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
File components/web_cache/common/web_cache_messages.h (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
components/web_cache/common/web_cache_messages.h:5: //Multiply-included file, no
traditional include guard.
nit: space after //

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.cc (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
nit: no (c)

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:12: using
blink::WebCache;
I like "using" declarations. :-)

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:16: const
size_t kUnitializedCacheCapacity = UINT_MAX;
wrap in namespace { }, it helps the linker, also "kUn*in*itialized"

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.h (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.h:8: #include
"content/public/renderer/render_process_observer.h"
nit: IWYU #include "macros.h" for DISALLOW_COPY_AND_ASSIGN, compiler_specific.h
for OVERRIDE

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.h:12: class
WebCacheRenderProcessObserver: public content::RenderProcessObserver {
nit: small class comment please, space before ":"

[Xi Han, 2014-09-05 19:38:35]

On 2014/09/05 16:32:44, James Cook wrote:
> On 2014/09/05 14:19:19, hanxi wrote:
> > mailto:+jamescook@chromium.org
> > James: please review this CL. Do you have any idea about the failures in
> android
> > and windows?
> 
> The Windows failure is because you introduced a new build target without this
> warning suppression:
> 
>       # Disable c4267 warnings until we fix size_t to int truncations.
>       'msvs_disabled_warnings': [ 4267, ],
>       'conditions': [
>         ['OS=="win" and win_use_allocator_shim==1', {
>           'dependencies': [
>             '../base/allocator/allocator.gyp:allocator',
>           ],
>         }],
>       ],
> 
> It's in all the build targets, probably as part of the win x64 bringup. I have
> no idea if it will ever get fixed.
> 
> If you would prefer to avoid that suppression you can static_cast<> the size_t
> to int32_t

Thanks a lot for finding the problem. It has bothered me for a while and haven't
got any clue. Waiting for try-bot server to run again:)

[Xi Han, 2014-09-05 19:39:25]

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_request/web_request_time_tracker.cc
(left):

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/extensio...
chrome/browser/extensions/api/web_request/web_request_time_tracker.cc:13:
#include "content/public/browser/browser_thread.h"
On 2014/09/05 16:53:18, James Cook wrote:
> Hooray for cleaning up unused includes!

Yeah! I will remove the last unneeded one
"chrome/browser/extensions/extension_service.h".

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/renderer...
File chrome/browser/renderer_host/web_cache_manager_browsertest.cc (right):

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/renderer...
chrome/browser/renderer_host/web_cache_manager_browsertest.cc:23: class
WebCacheManagerBrowserTest : public InProcessBrowserTest {
On 2014/09/05 16:53:18, James Cook wrote:
> So what's the plan for these tests? It's weird that the test source files live
> in src/chrome but the code they are testing lives in src/components.
> 
> I suggest doing something like we do for src/extensions unit tests. Move this
> file into src/components along with the code under test, fix
> src/components/web_cache/DEPS to allow src/chrome dependencies just test
files,
> then compile and run the tests as part of Chrome's browser_tests suite. Then
> leave a comment and file a bug to track refactoring the tests so they can run
in
> components_browsertests.


I feel the same way. Thanks a lot for all the details, and I have moved it to
components:)

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/task_man...
File chrome/browser/task_manager/task_manager.h (right):

https://codereview.chromium.org/528363002/diff/240001/chrome/browser/task_man...
chrome/browser/task_manager/task_manager.h:21: #include
"components/web_cache/browser/web_cache_manager.h"
On 2014/09/05 16:53:18, James Cook wrote:
> Is this header used?

Removed.

https://codereview.chromium.org/528363002/diff/240001/chrome/chrome_tests_uni...
File chrome/chrome_tests_unit.gypi (left):

https://codereview.chromium.org/528363002/diff/240001/chrome/chrome_tests_uni...
chrome/chrome_tests_unit.gypi:1180:
'browser/renderer_host/web_cache_manager_unittest.cc',
On 2014/09/05 16:53:18, James Cook wrote:
> Hooray for moving the unit test!

:)

https://codereview.chromium.org/528363002/diff/240001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/528363002/diff/240001/components/BUILD.gn#new...
components/BUILD.gn:98: "//components/web_cache/browser",
On 2014/09/05 16:53:18, James Cook wrote:
> nit: alphabetize above web_modal

Done.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache.gypi
File components/web_cache.gypi (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache.gy...
components/web_cache.gypi:25: 'web_cache_common',
On 2014/09/05 16:53:18, James Cook wrote:
> nit: put this at the bottom of the list

Done.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
File components/web_cache/browser/BUILD.gn (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
components/web_cache/browser/BUILD.gn:16: "//content/test:test_support",
On 2014/09/05 16:53:18, James Cook wrote:
> Why does this need test_support? Shouldn't that only be needed for the tests?

Great catch, thanks!

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.cc (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.cc:58:
registry->RegisterIntegerPref(prefs::kMemoryCacheSize, GetDefaultCacheSize());
On 2014/09/05 16:53:18, James Cook wrote:
> Do you still need this pref? It looks like you deleted the code that uses it.

You are absolutely right. Removed.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
File components/web_cache/common/pref_names.h (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
components/web_cache/common/pref_names.h:14: extern const char
kMemoryCacheSize[];
On 2014/09/05 16:53:18, James Cook wrote:
> Can this go away?

Yeas, both prefs_names.h and prefs_names.cc are gone.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
File components/web_cache/common/web_cache_messages.h (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/co...
components/web_cache/common/web_cache_messages.h:5: //Multiply-included file, no
traditional include guard.
On 2014/09/05 16:53:18, James Cook wrote:
> nit: space after //

Done.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.cc (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:1: //
Copyright (c) 2014 The Chromium Authors. All rights reserved.
On 2014/09/05 16:53:18, James Cook wrote:
> nit: no (c)

Removed.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:12: using
blink::WebCache;
On 2014/09/05 16:53:18, James Cook wrote:
> I like "using" declarations. :-)

Me too:)

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:16: const
size_t kUnitializedCacheCapacity = UINT_MAX;
On 2014/09/05 16:53:18, James Cook wrote:
> wrap in namespace { }, it helps the linker, also "kUn*in*itialized"

Added.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.h (right):

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.h:8: #include
"content/public/renderer/render_process_observer.h"
On 2014/09/05 16:53:18, James Cook wrote:
> nit: IWYU #include "macros.h" for DISALLOW_COPY_AND_ASSIGN,
compiler_specific.h
> for OVERRIDE

Done. Also updated web_cache.gypi.

https://codereview.chromium.org/528363002/diff/240001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.h:12: class
WebCacheRenderProcessObserver: public content::RenderProcessObserver {
On 2014/09/05 16:53:18, James Cook wrote:
> nit: small class comment please, space before ":"

Done.

[James Cook, 2014-09-05 19:47:58]

LGTM with nits

https://codereview.chromium.org/528363002/diff/260001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/528363002/diff/260001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:866:
'../components/web_cache/browser/web_cache_manager_browsertest.cc',
While you're in this file can you sort all the "../components" and
"../extensions" pieces together?  Also, please file a bug to move this test to
components_browsertests and reference it in a comment.

https://codereview.chromium.org/528363002/diff/260001/components/web_cache.gypi
File components/web_cache.gypi (right):

https://codereview.chromium.org/528363002/diff/260001/components/web_cache.gy...
components/web_cache.gypi:51: '<(DEPTH)/base/base.gyp:base',
This is not a source file. If it's a dependency it belongs in the 'dependencies'
section above.

https://codereview.chromium.org/528363002/diff/260001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.h (right):

https://codereview.chromium.org/528363002/diff/260001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.h:14: // This
class filters the incomming cache related control messages.
nit: "incomming" -> "incoming"

[Xi Han, 2014-09-05 20:53:05]

Patchset #6 (id:280001) has been deleted

[Xi Han, 2014-09-05 21:01:41]

hanxi@chromium.org changed reviewers:
+ brettw@chromium.org

[Xi Han, 2014-09-05 21:01:42]

+brettw@chromium.org: Please review changes in:
BUILD.gn files.
Thanks!

https://codereview.chromium.org/528363002/diff/260001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/528363002/diff/260001/chrome/chrome_tests.gyp...
chrome/chrome_tests.gypi:866:
'../components/web_cache/browser/web_cache_manager_browsertest.cc',
On 2014/09/05 19:47:58, James Cook wrote:
> While you're in this file can you sort all the "../components" and
> "../extensions" pieces together?  Also, please file a bug to move this test to
> components_browsertests and reference it in a comment.

Sorry for missing the file a big part. Done.

https://codereview.chromium.org/528363002/diff/260001/components/web_cache.gypi
File components/web_cache.gypi (right):

https://codereview.chromium.org/528363002/diff/260001/components/web_cache.gy...
components/web_cache.gypi:51: '<(DEPTH)/base/base.gyp:base',
On 2014/09/05 19:47:58, James Cook wrote:
> This is not a source file. If it's a dependency it belongs in the
'dependencies'
> section above.

Sorry, put it in the wrong place.

https://codereview.chromium.org/528363002/diff/260001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.h (right):

https://codereview.chromium.org/528363002/diff/260001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.h:14: // This
class filters the incomming cache related control messages.
On 2014/09/05 19:47:58, James Cook wrote:
> nit: "incomming" -> "incoming"

Done.

[Xi Han, 2014-09-05 21:04:16]

hanxi@chromium.org changed reviewers:
+ sky@chromium.org

[Xi Han, 2014-09-05 21:04:16]

sky@chromium.org: Please review changes in 

-chrome/browser/*
-chrome/renderer/*

Thanks!

[Xi Han, 2014-09-05 21:05:47]

hanxi@chromium.org changed reviewers:
+ kenrb@chromium.org

[Xi Han, 2014-09-05 21:05:48]

+kenrb@chromium.org: Please review changes in 

-chrome/common/render_messages.h
-ipc/ipc_message_start.h

Thanks!

[Xi Han, 2014-09-05 21:28:49]

Brett: I have updated the components/GUILD.gn file:)

[Xi Han, 2014-09-05 21:31:06]

hanxi@chromium.org changed reviewers:
+ erikwright@chromium.org

[Xi Han, 2014-09-05 21:31:07]

erikwright@chromium.org: Please review changes in 
-components/*. 
Thanks!

[sky, 2014-09-05 22:13:07]

LGTM

[brettw, 2014-09-05 23:30:29]

gn  build  lgtm

[Xi Han, 2014-09-08 14:24:44]

Hi James: I updated the ShellContentRenderClient and initialized
WebCacheRenderProcessObserver there. Please take another look. Thanks!

[Xi Han, 2014-09-08 15:31:06]

Patchset #8 (id:340001) has been deleted

[kenrb, 2014-09-08 15:58:54]

Can you please add an OWNERS file to components/web_cache/common with the
per-file overrides for IPC security reviews?

You can use the same one as in components/pdf/common.

[Xi Han, 2014-09-08 16:12:31]

On 2014/09/08 15:58:54, kenrb wrote:
> Can you please add an OWNERS file to components/web_cache/common with the
> per-file overrides for IPC security reviews?
> 
> You can use the same one as in components/pdf/common.

Updated. Please take another look. Thanks!

[kenrb, 2014-09-08 16:49:28]

ipc lgtm with very minor nit.

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/co...
File components/web_cache/common/OWNERS (right):

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/co...
components/web_cache/common/OWNERS:2: # # new sandbox escapes.
The extra # shouldn't be needed.

[James Cook, 2014-09-08 17:32:32]

lgtm but does app_shell need WebCacheRenderProcessObserver in
extensions/shell/renderer/shell_content_renderer_client.cc?

[erikwright (departed), 2014-09-08 18:17:50]

https://codereview.chromium.org/528363002/diff/380001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/528363002/diff/380001/components/BUILD.gn#new...
components/BUILD.gn:176: "//components/web_cache/browser",  # Blocked on
content.
and blink?

https://codereview.chromium.org/528363002/diff/380001/components/BUILD.gn#new...
components/BUILD.gn:177: "//components/web_cache/renderer",  # Blocked on
content.
and blink?

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.cc (right):

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:78: if
(on_navigation || !webkit_initialized_) {
no brackets needed for this if/else

https://codereview.chromium.org/528363002/diff/400001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager_browsertest.cc (right):

https://codereview.chromium.org/528363002/diff/400001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_browsertest.cc:7: #include
"base/message_loop/message_loop.h"
How hard is it going to be to fix the chrome DEP from this file? And when do you
foresee completing that?

[Xi Han, 2014-09-08 18:23:57]

On 2014/09/08 17:32:32, James Cook wrote:
> lgtm but does app_shell need WebCacheRenderProcessObserver in
> extensions/shell/renderer/shell_content_renderer_client.cc?

When app_shell has webview to use web request api, we need a
RenderProcessObserver to listen to and handle the ipc message when calling
helpers::ClearCacheOnNavigation() in
web_request_api.cc(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/extensions/api/web_request/web_request_api.cc&rcl=1410138160&l=1354).
Am I right?

[James Cook, 2014-09-08 18:28:29]

Yes, I think that's right.

[Xi Han, 2014-09-08 18:49:33]

Patchset #11 (id:420001) has been deleted

[Xi Han, 2014-09-08 18:52:23]

https://codereview.chromium.org/528363002/diff/380001/components/BUILD.gn
File components/BUILD.gn (right):

https://codereview.chromium.org/528363002/diff/380001/components/BUILD.gn#new...
components/BUILD.gn:176: "//components/web_cache/browser",  # Blocked on
content.
On 2014/09/08 18:17:49, erikwright wrote:
> and blink?

You are right, added, thanks.

https://codereview.chromium.org/528363002/diff/380001/components/BUILD.gn#new...
components/BUILD.gn:177: "//components/web_cache/renderer",  # Blocked on
content.
On 2014/09/08 18:17:49, erikwright wrote:
> and blink?

Added.

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/co...
File components/web_cache/common/OWNERS (right):

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/co...
components/web_cache/common/OWNERS:2: # # new sandbox escapes.
On 2014/09/08 16:49:28, kenrb wrote:
> The extra # shouldn't be needed.

Done.

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/re...
File components/web_cache/renderer/web_cache_render_process_observer.cc (right):

https://codereview.chromium.org/528363002/diff/380001/components/web_cache/re...
components/web_cache/renderer/web_cache_render_process_observer.cc:78: if
(on_navigation || !webkit_initialized_) {
On 2014/09/08 18:17:49, erikwright wrote:
> no brackets needed for this if/else

Done.

https://codereview.chromium.org/528363002/diff/400001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager_browsertest.cc (right):

https://codereview.chromium.org/528363002/diff/400001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_browsertest.cc:7: #include
"base/message_loop/message_loop.h"
On 2014/09/08 18:17:49, erikwright wrote:
> How hard is it going to be to fix the chrome DEP from this file? And when do
you
> foresee completing that?

Honestly, I really can't foresee when these chrome DEP will be fix since it is
not easy to fix them now. The test requires behaviors of Browser
(chrome/browser/ui/browser.h) to deal with tabs (which is not a concept of
app_shell), that is why I have to leave it run as part of chrome as many of
other extensions tests.

[Xi Han, 2014-09-08 18:59:39]

+agl@chromium.org, Please review changes in:
-components/web_cache/common/DEPS

Thanks!

[erikwright (departed), 2014-09-08 19:20:00]

https://codereview.chromium.org/528363002/diff/440001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager_browsertest.cc (right):

https://codereview.chromium.org/528363002/diff/440001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_browsertest.cc:28:
IN_PROC_BROWSER_TEST_F(WebCacheManagerBrowserTest, CrashOnceOnly) {
As far as I can tell, this test is for a regression that would occur when a
render process crashes.

ContentBrowserTest::CreateBrowser returns instances of Shell, each of which
corresponds to a separate WebContents. I wonder if you could write this test in
terms of those, instead of in terms of tabs.

I don't know what about this test ensured that the two tabs were in distinct
processes (or if that is, in fact, a precondition for the crash), but presumably
there is a way to ensure that with the content shell, too, if necessary.

I would, of course, validate that the test is still legitimate by reverting the
fix
(http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/...)
and ensuring that the newly refactored test crashes.

[Xi Han, 2014-09-09 15:01:21]

On 2014/09/08 19:20:00, erikwright wrote:
>
https://codereview.chromium.org/528363002/diff/440001/components/web_cache/br...
> File components/web_cache/browser/web_cache_manager_browsertest.cc (right):
> 
>
https://codereview.chromium.org/528363002/diff/440001/components/web_cache/br...
> components/web_cache/browser/web_cache_manager_browsertest.cc:28:
> IN_PROC_BROWSER_TEST_F(WebCacheManagerBrowserTest, CrashOnceOnly) {
> As far as I can tell, this test is for a regression that would occur when a
> render process crashes.
> 
> ContentBrowserTest::CreateBrowser returns instances of Shell, each of which
> corresponds to a separate WebContents. I wonder if you could write this test
in
> terms of those, instead of in terms of tabs.
> 
> I don't know what about this test ensured that the two tabs were in distinct
> processes (or if that is, in fact, a precondition for the crash), but
presumably
> there is a way to ensure that with the content shell, too, if necessary.
> 
> I would, of course, validate that the test is still legitimate by reverting
the
> fix
>
(http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/renderer_host/...)
> and ensuring that the newly refactored test crashes.

After some investigation, I found the fixed function
WebCacheManager::ObserveActivity(int renderer_id) is only used in
core_tab_helper.cc
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
in chrome. Since the code path only exists in chrome, we cannot reproduce the
crash by using window in content shell. So I think this browser test has to be
run in chrome.

[erikwright (departed), 2014-09-09 15:08:02]

It seems like there is some missing unit test coverage then, and that this
would be easy to add to the existing unit test.

Once you have added that, a browser test is really not required anymore.

On Tue, Sep 9, 2014 at 11:01 AM, <hanxi@chromium.org> wrote:

> On 2014/09/08 19:20:00, erikwright wrote:
>
> https://codereview.chromium.org/528363002/diff/440001/
> components/web_cache/browser/web_cache_manager_browsertest.cc
>
>> File components/web_cache/browser/web_cache_manager_browsertest.cc
>> (right):
>>
>
>
> https://codereview.chromium.org/528363002/diff/440001/
> components/web_cache/browser/web_cache_manager_browsertest.cc#newcode28
>
>> components/web_cache/browser/web_cache_manager_browsertest.cc:28:
>> IN_PROC_BROWSER_TEST_F(WebCacheManagerBrowserTest, CrashOnceOnly) {
>> As far as I can tell, this test is for a regression that would occur when
>> a
>> render process crashes.
>>
>
>  ContentBrowserTest::CreateBrowser returns instances of Shell, each of
>> which
>> corresponds to a separate WebContents. I wonder if you could write this
>> test
>>
> in
>
>> terms of those, instead of in terms of tabs.
>>
>
>  I don't know what about this test ensured that the two tabs were in
>> distinct
>> processes (or if that is, in fact, a precondition for the crash), but
>>
> presumably
>
>> there is a way to ensure that with the content shell, too, if necessary.
>>
>
>  I would, of course, validate that the test is still legitimate by
>> reverting
>>
> the
>
>> fix
>>
>
> (http://src.chromium.org/viewvc/chrome/trunk/src/
> chrome/browser/renderer_host/web_cache_manager.cc?r1=16775&r2=16774)
>
>> and ensuring that the newly refactored test crashes.
>>
>
> After some investigation, I found the fixed function
> WebCacheManager::ObserveActivity(int renderer_id) is only used in
> core_tab_helper.cc
> (https://code.google.com/p/chromium/codesearch#chromium/
> src/chrome/browser/ui/tab_contents/core_tab_helper.cc&rcl=1410195788&l=148
> )
> in chrome. Since the code path only exists in chrome, we cannot reproduce
> the
> crash by using window in content shell. So I think this browser test has
> to be
> run in chrome.
>
>
> https://codereview.chromium.org/528363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Xi Han, 2014-09-10 15:14:26]

Patchset #12 (id:460001) has been deleted

[Xi Han, 2014-09-10 15:15:33]

Erik: a new unit test is added in web_cache_manager_unittest.cc, please take
another look. Thanks.

[Xi Han, 2014-09-10 15:21:00]

hanxi@chromium.org changed reviewers:
+ sadrul@chromium.org

[Xi Han, 2014-09-10 15:21:01]

Sadurl: please take a look at:
-web_cache_manager_unittest.cc. 

MessageLoop::QuitWhenIdleClosure() is passed in WebCacheManager as a callback to
make sure that the delayed tasks in message loop is executed before the test is
done. Thanks.

[Xi Han, 2014-09-10 15:24:21]

+agl@chromium.org: Please review changes in:

-components/web_cache/common/DEPS

Thanks!

[Xi Han, 2014-09-10 15:36:41]

hanxi@chromium.org changed reviewers:
+ agl@chromium.org

[erikwright (departed), 2014-09-10 15:49:43]

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.h (right):

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.h:88: // function that guaranty
all the delayed tasks that posted to the message
guarantees

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager_unittest.cc (right):

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:334:
base::MessageLoop::current()->Run();
Since the rest of this test suite is already using friend status to invoke
protected/private methods and members of WebCacheManager, just call
ReviseAllocationStrategy from the suite (you will need to add a thunk to do so)
rather than waiting for the delayed task to execute.

That will remove the need for this callback.

[Xi Han, 2014-09-10 17:24:37]

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.h (right):

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.h:88: // function that guaranty
all the delayed tasks that posted to the message
On 2014/09/10 15:49:43, erikwright wrote:
> guarantees

Removed.

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager_unittest.cc (right):

https://codereview.chromium.org/528363002/diff/480001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:334:
base::MessageLoop::current()->Run();
On 2014/09/10 15:49:43, erikwright wrote:
> Since the rest of this test suite is already using friend status to invoke
> protected/private methods and members of WebCacheManager, just call
> ReviseAllocationStrategy from the suite (you will need to add a thunk to do
so)
> rather than waiting for the delayed task to execute.
> 
> That will remove the need for this callback.

Done.

[erikwright (departed), 2014-09-10 18:06:55]

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager.h (right):

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
components/web_cache/browser/web_cache_manager.h:32:
FRIEND_TEST_ALL_PREFIXES(WebCacheManagerBrowserTest, CrashOnceOnly);
nit remove this friend declaration for the browsertest

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager_unittest.cc (right):

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:315:
TEST_F(WebCacheManagerTest,
Add a comment explaining that Add, Remove, and ObserveActivity trigger deferred
calls to ReviseAllocationStrategy and that we call it directly after each
operation to sidestep the need to wait for an unobservable background operation.

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:328:
manager()->ObserveActivity(kRendererID);
Add a call to ReviseAllocationStrategy here.

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:331:
manager()->ReviseAllocationStrategy();
Add something that verifies the behaviour when a renderer is removed in the
200ms between ObserveActivity and ReviseAllocationStrategy being invoked. i.e.:

Add(X)
ObserveActivity(X)
Remove(X)
ReviseAllocationStrategy()

This could be accomplished by just adding a call to
ObserveActivity(kRendererID2) before the call to Remove(kRendererID2).

Be sure to add a comment explaining what the sequence is meant to cover.

[sadrul, 2014-09-11 04:34:56]

sadrul@chromium.org changed reviewers:
- sadrul@chromium.org

[Xi Han, 2014-09-11 13:47:41]

Patchset #14 (id:520001) has been deleted

[Xi Han, 2014-09-11 13:52:15]

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
File components/web_cache/browser/web_cache_manager_unittest.cc (right):

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:315:
TEST_F(WebCacheManagerTest,
On 2014/09/10 18:06:55, erikwright wrote:
> Add a comment explaining that Add, Remove, and ObserveActivity trigger
deferred
> calls to ReviseAllocationStrategy and that we call it directly after each
> operation to sidestep the need to wait for an unobservable background
operation.


Good point, otherwise people will get confused when reading this test.

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:328:
manager()->ObserveActivity(kRendererID);
On 2014/09/10 18:06:55, erikwright wrote:
> Add a call to ReviseAllocationStrategy here.

Done.

https://codereview.chromium.org/528363002/diff/500001/components/web_cache/br...
components/web_cache/browser/web_cache_manager_unittest.cc:331:
manager()->ReviseAllocationStrategy();

As discussed offline, to prevent similar failure in the future, I added another
5 tests to cover all the orderings of these three operation: Remove(ID),
ObserveActivity(ID), Remove(ID2).



On 2014/09/10 18:06:55, erikwright wrote:
> Add something that verifies the behaviour when a renderer is removed in the
> 200ms between ObserveActivity and ReviseAllocationStrategy being invoked.
i.e.:
> 
> Add(X)
> ObserveActivity(X)
> Remove(X)
> ReviseAllocationStrategy()
> 
> This could be accomplished by just adding a call to
> ObserveActivity(kRendererID2) before the call to Remove(kRendererID2).
> 
> Be sure to add a comment explaining what the sequence is meant to cover.

[erikwright (departed), 2014-09-11 13:59:36]

LGTM.

[Xi Han, 2014-09-11 16:57:26]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-11 16:58:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/528363002/560001

[Xi Han, 2014-09-11 17:29:42]

The CQ bit was unchecked by hanxi@chromium.org

[Xi Han, 2014-09-11 17:30:16]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:40:22]

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

[commit-bot: I haz the power, 2014-09-11 17:40:24]

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

[Xi Han, 2014-09-11 17:52:10]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-11 18:09:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/528363002/560001

[commit-bot: I haz the power, 2014-09-11 18:26:50]

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

[commit-bot: I haz the power, 2014-09-11 18:26:52]

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

[Xi Han, 2014-09-11 19:19:59]

Patchset #16 (id:580001) has been deleted

[Xi Han, 2014-09-11 19:24:49]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-09-11 19:33:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/528363002/600001

[commit-bot: I haz the power, 2014-09-11 22:03:17]

Committed patchset #16 (id:600001) as f65e88815a39894cdeaab8fb4e8bb5a23084f130

[commit-bot: I haz the power, 2014-09-11 22:31:00]

Patchset 16 (id:??) landed as
https://crrev.com/149b92db00bc1deab22599e7d3da3fc8be2c0d8b
Cr-Commit-Position: refs/heads/master@{#294475}

[Lei Zhang, 2014-09-15 18:24:29]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

[Lei Zhang, 2014-09-15 18:24:31]

https://codereview.chromium.org/528363002/diff/600001/components/web_cache/OW...
File components/web_cache/OWNERS (right):

https://codereview.chromium.org/528363002/diff/600001/components/web_cache/OW...
components/web_cache/OWNERS:1: # Reviewers:
This is weird. It looks like you just copied chrome/OWNERS here, which is not
right because we are probably not the experts on web_cache. Please start a new
CL and find some proper OWNERS for components/web_cache.

[Xi Han, 2014-09-15 18:27:45]

I checked the WebCacheManager but there isn't a specified OWNERS of this file.
So I trace back all the way up till the OWNER file of chrome. Do you have any
suggestion how to add OWNERS in such case?



On 2014/09/15 18:24:31, Lei Zhang wrote:
>
https://codereview.chromium.org/528363002/diff/600001/components/web_cache/OW...
> File components/web_cache/OWNERS (right):
> 
>
https://codereview.chromium.org/528363002/diff/600001/components/web_cache/OW...
> components/web_cache/OWNERS:1: # Reviewers:
> This is weird. It looks like you just copied chrome/OWNERS here, which is not
> right because we are probably not the experts on web_cache. Please start a new
> CL and find some proper OWNERS for components/web_cache.

[Fady Samuel, 2014-09-15 18:28:24]

Try git blame on the files?

On Mon, Sep 15, 2014 at 2:27 PM, <hanxi@chromium.org> wrote:

> I checked the WebCacheManager but there isn't a specified OWNERS of this
> file.
> So I trace back all the way up till the OWNER file of chrome. Do you have
> any
> suggestion how to add OWNERS in such case?
>
>
>
>
> On 2014/09/15 18:24:31, Lei Zhang wrote:
>
> https://codereview.chromium.org/528363002/diff/600001/
> components/web_cache/OWNERS
>
>> File components/web_cache/OWNERS (right):
>>
>
>
> https://codereview.chromium.org/528363002/diff/600001/
> components/web_cache/OWNERS#newcode1
>
>> components/web_cache/OWNERS:1: # Reviewers:
>> This is weird. It looks like you just copied chrome/OWNERS here, which is
>> not
>> right because we are probably not the experts on web_cache. Please start
>> a new
>> CL and find some proper OWNERS for components/web_cache.
>>
>
>
>
> https://codereview.chromium.org/528363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Lei Zhang, 2014-09-15 18:43:29]

On 2014/09/15 18:27:45, hanxi wrote:
> I checked the WebCacheManager but there isn't a specified OWNERS of this file.
> So I trace back all the way up till the OWNER file of chrome. Do you have any
> suggestion how to add OWNERS in such case?

By this logic, I will eventually own a good portion of components/ by default
due to chrome -> components refactorings.

http://www.chromium.org/developers/owners-files#TOC-Who-should-be-in-an-OWNER...
says: "Only the people who are actively investing energy in the improvement of a
directory should be listed as OWNERS."

Consider doing some code archaeology and figure out:
- Who is a stakeholder in this code?
- Who knows this code well?

All I can tell you is I (likely) have never seen nor touched this code.

[erikwright (departed), 2014-09-15 18:49:52]

thestig@: Thanks for commenting on this. I'll keep it in mind for future
componentization CLs.

On Mon, Sep 15, 2014 at 2:43 PM, <thestig@chromium.org> wrote:

> On 2014/09/15 18:27:45, hanxi wrote:
>
>> I checked the WebCacheManager but there isn't a specified OWNERS of this
>> file.
>> So I trace back all the way up till the OWNER file of chrome. Do you have
>> any
>> suggestion how to add OWNERS in such case?
>>
>
> By this logic, I will eventually own a good portion of components/ by
> default
> due to chrome -> components refactorings.
>
> http://www.chromium.org/developers/owners-files#TOC-
> Who-should-be-in-an-OWNERS-file-
> says: "Only the people who are actively investing energy in the
> improvement of a
> directory should be listed as OWNERS."
>
> Consider doing some code archaeology and figure out:
> - Who is a stakeholder in this code?
> - Who knows this code well?
>
> All I can tell you is I (likely) have never seen nor touched this code.
>
> https://codereview.chromium.org/528363002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.