New Task Manager - Phase 1.1: Implement Browser Process Task Providing

chrome/chrome_browser.gypi

 # Copyright 2012 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.
 
 {
   'variables': {
     # All .cc, .h, .m, and .mm files under browser except for:
     # * tests and mocks.
     # * code below browser/chromeos
     # * code below browser/extensions
@@ skipping 2825 matching lines @@
       'browser/sync/supervised_user_signin_manager_wrapper.h',
       'browser/sync/sync_error_controller.cc',
       'browser/sync/sync_error_controller.h',
       'browser/sync/sync_startup_tracker.cc',
       'browser/sync/sync_startup_tracker.h',
       'browser/sync/sync_stopped_reporter.cc',
       'browser/sync/sync_stopped_reporter.h',
       'browser/sync/sync_type_preference_provider.h',
     ],
     'chrome_browser_task_manager_sources': [
+      # New Task Manager Sources:
+      'browser/task_management/providers/browser_process/browser_process_task_provider.cc',

[Lei Zhang, 2015/04/01 22:47:52]

Do you need to have a browser_process sub-directory, and files within it named
browser_process_foo? Maybe just drop the sub-directory?

ls chrome/browser|wc -l returns 349
and
ls chrome/browser/extensions|wc -l return 519

and nobody is crying yet. (AFAIK)

[afakhry, 2015/04/03 01:51:01]

The idea was to keep the private interfaces in the top directory and related
implementation files are grouped together in sub-directories.

[Lei Zhang, 2015/04/03 02:39:37]

OTOH, #include
"chrome/browser/task_management/providers/browser_process/browser_process_task_provider.h"
doesn't fit within 80 columns. I think you are trying way too hard to give your
files hierarchy.

how about
chrome/browser/task_management/providers/browser_process_task_provider_impl.h?
One can easily spot the interface files as the shorter named files without _impl
in their names.

[ncarter (slow), 2015/04/03 04:09:03]

FWIW, no levels of hierarchy after providers/ was my suggestion as well earlier
in the review. I still would prefer at most one level of directory hierarchy,
because the convention in chrome seems to be a relatively flat organization.

I'm not sure I like the _impl pattern that Lei suggests. Usually when we have
foo_impl.cc it's the sole non-test implementation of the interface foo.h. In
this case we have many implementations of the taskprovider and task interfaces;
seems clumsy to me to call them all 'Impls.

I'm not bothered by the idea of task.h living alongside browser_process_task.h
and web_contents_task.h.

[afakhry, 2015/04/03 16:04:27]

Ok, I yielded although I'm not too happy about it :(.
At least for web_contents in the future I strongly believe they should have
their own sub-dir. There will be a lot of related files.

+      'browser/task_management/providers/browser_process/browser_process_task_provider.h',
+      'browser/task_management/providers/browser_process/browser_process_task.cc',
+      'browser/task_management/providers/browser_process/browser_process_task.h',
+      'browser/task_management/providers/task_provider_observer.h',
+      'browser/task_management/providers/task_provider.cc',
+      'browser/task_management/providers/task_provider.h',
+      'browser/task_management/providers/task.cc',
+      'browser/task_management/providers/task.h',
+      
+      # Old Task Manager Sources:
       'browser/task_manager/background_information.cc',
       'browser/task_manager/background_information.h',
       'browser/task_manager/browser_process_resource_provider.cc',
       'browser/task_manager/browser_process_resource_provider.h',
       'browser/task_manager/child_process_resource_provider.cc',
       'browser/task_manager/child_process_resource_provider.h',
       'browser/task_manager/extension_information.cc',
       'browser/task_manager/extension_information.h',
       'browser/task_manager/guest_information.cc',
       'browser/task_manager/guest_information.h',
@@ skipping 953 matching lines @@
           'variables': {
             'proto_in_dir': 'browser/sync_file_system/drive_backend',
             'proto_out_dir': 'chrome/browser/sync_file_system/drive_backend',
           },
           'includes': [ '../build/protoc.gypi' ]
         },
       ],
     },],
   ],
 }