Eliminate //chrome dependencies from MetricsServicesManager

Eliminate //chrome dependencies from MetricsServicesManager

This CL eliminates //chrome dependencies from MetricsServicesManager in
preparation for componentizing that file to share with iOS. To do so, we
introduce MetricsServicesManagerClient, which encapsulates the embedder
information that MetricsServicesManager previously internalized.

BUG=508025
TBR=thestig, droger

Committed: https://crrev.com/40b14cfa3f3458ae0d6f655886d952389aabc25c
Cr-Commit-Position: refs/heads/master@{#355255}

Committed: https://crrev.com/fecea528dd9085f28a577add02a6c62e7c4685ce
Cr-Commit-Position: refs/heads/master@{#355266}

[blundell, 2015-10-16 16:12:38]

blundell@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-10-19 16:47:19]

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:538: if (!metrics_services_manager_)
Nit: {}'s

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right):

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:25: void
PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) {
Nit: Put anon namespace around this.

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:26: return
client_->GetMetricsService(GetMetricsServiceClient());
This is a bit convoluted. Can we just expose metrics_service() on the metrics
service client interface, and then we wouldn't need this extra
client_->GetMetricsService call/interface?

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:47:
GetMetricsServiceClient()->OnPluginLoadingError(plugin_path);
Hmm, I wonder if we should just plumb this through to metrics_service directly,
avoiding the metrics service client.

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:53: if
(!metrics_service_client_) {
Nit: No {}'s

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:101: base::Unretained(this)))
Nit: Make a local var for this callback to make the if statement more concise.
Also, can you add a comment about why base::Unretained is safe here or if it's
not, use a weak ptr?

[blundell, 2015-10-20 08:25:32]

Patchset #3 (id:40001) has been deleted

[blundell, 2015-10-20 08:36:03]

Patchset #3 (id:60001) has been deleted

[blundell, 2015-10-20 09:15:36]

Patchset #3 (id:80001) has been deleted

[blundell, 2015-10-20 10:36:45]

Thanks!

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:538: if (!metrics_services_manager_)
On 2015/10/19 16:47:18, Alexei Svitkine wrote:
> Nit: {}'s

Done.

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/chro...
File chrome/browser/metrics/chrome_metrics_services_manager_client.cc (right):

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/chro...
chrome/browser/metrics/chrome_metrics_services_manager_client.cc:25: void
PostStoreMetricsClientInfo(const metrics::ClientInfo& client_info) {
On 2015/10/19 16:47:19, Alexei Svitkine wrote:
> Nit: Put anon namespace around this.

Done.

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_services_manager.cc (right):

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:26: return
client_->GetMetricsService(GetMetricsServiceClient());
On 2015/10/19 16:47:19, Alexei Svitkine wrote:
> This is a bit convoluted. Can we just expose metrics_service() on the metrics
> service client interface, and then we wouldn't need this extra
> client_->GetMetricsService call/interface?

Yep! I meant to ask you about whether you thought that would be a better
approach and forgot. Done.

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:47:
GetMetricsServiceClient()->OnPluginLoadingError(plugin_path);
On 2015/10/19 16:47:19, Alexei Svitkine wrote:
> Hmm, I wonder if we should just plumb this through to metrics_service
directly,
> avoiding the metrics service client.

The functionality here is implemented at the //chrome level, so we'd still have
to ultimately call out to the metrics service client. Are you suggesting burying
that callout within the metrics service itself?

If we were to change this structure, what I would propose is to expose a
GetMetricsServiceClient() method on MetricsServicesManager, have the
//chrome-level callsite of this method get the metrics service client, cast it
to a ChromeMetricsServiceClient, and then call OnPluginLoadingError directly on
what could now be just the //chrome-level interface. Then we could eliminate
going from //chrome to //components back to //chrome altogether for this flow.

I don't feel strongly about it though. I think the current structure is also OK.

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:53: if
(!metrics_service_client_) {
On 2015/10/19 16:47:19, Alexei Svitkine wrote:
> Nit: No {}'s

Done.

https://codereview.chromium.org/1411593004/diff/1/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_services_manager.cc:101: base::Unretained(this)))
On 2015/10/19 16:47:19, Alexei Svitkine wrote:
> Nit: Make a local var for this callback to make the if statement more concise.
> Also, can you add a comment about why base::Unretained is safe here or if it's
> not, use a weak ptr?

Done.

[Alexei Svitkine (slow), 2015-10-20 15:02:20]

LGTM

https://codereview.chromium.org/1411593004/diff/100001/components/metrics/met...
File components/metrics/metrics_service_client.h (right):

https://codereview.chromium.org/1411593004/diff/100001/components/metrics/met...
components/metrics/metrics_service_client.h:37: virtual metrics::MetricsService*
GetMetricsService() = 0;
Nit: Remove metrics:: since this is in it.

[blundell, 2015-10-21 06:15:39]

Description was changed from

==========
Eliminate //chrome dependencies from MetricsServicesManager

This CL eliminates //chrome dependencies from MetricsServicesManager in
preparation for componentizing that file to share with iOS. To do so, we
introduce MetricsServicesManagerClient, which encapsulates the embedder
information that MetricsServicesManager previously internalized.

BUG=508025
==========

to

==========
Eliminate //chrome dependencies from MetricsServicesManager

This CL eliminates //chrome dependencies from MetricsServicesManager in
preparation for componentizing that file to share with iOS. To do so, we
introduce MetricsServicesManagerClient, which encapsulates the embedder
information that MetricsServicesManager previously internalized.

BUG=508025
TBR=thestig, droger
==========

[blundell, 2015-10-21 06:15:39]

blundell@chromium.org changed reviewers:
+ droger@chromium.org, thestig@chromium.org

[blundell, 2015-10-21 06:16:04]

TBR=thestig for //chrome/browser/browser_process_impl
TBR=droger for //ios/chrome

[blundell, 2015-10-21 06:16:08]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-10-21 06:16:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1411593004/#ps120001
(title: "Response to review")

[commit-bot: I haz the power, 2015-10-21 06:16:23]

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

[Lei Zhang, 2015-10-21 06:35:38]

On 2015/10/21 06:16:04, blundell wrote:
> TBR=thestig for //chrome/browser/browser_process_impl

lgtm

[Lei Zhang, 2015-10-21 06:36:01]

Just fix the nits later.

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/metrics_services_manager_client.h (right):

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/metrics_services_manager_client.h:9: #include
"base/callback.h"
base/callback_forward.h

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/metrics_services_manager_client.h:11: #include
"base/threading/thread_checker.h"
not used

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/metrics_services_manager_client.h:14: class
MetricsService;
not referenced

[commit-bot: I haz the power, 2015-10-21 07:42:43]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-10-21 07:43:27]

Patchset 4 (id:??) landed as
https://crrev.com/40b14cfa3f3458ae0d6f655886d952389aabc25c
Cr-Commit-Position: refs/heads/master@{#355255}

[vasilii, 2015-10-21 08:33:30]

A revert of this CL (patchset #4 id:120001) has been created in
https://codereview.chromium.org/1418763002/ by vasilii@chromium.org.

The reason for reverting is: Broke compilation on Google Chrome ChromeOS Google
Chrome Linux x64 Google Chrome Mac

http://build.chromium.org/p/chromium.chrome/buildstatus?builder=Google%20Chro...

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/chrome/browser/metrics/browser.metrics_services_manager.o.d
-DV8_DEPRECATION_WARNINGS -DCLD_VERSION=2
-D__ASSERT_MACROS_DEFINE_VERSIONS_WITHOUT_UNDERSCORE=0 -DGOOGLE_CHROME_BUILD
-DCR_CLANG_REVISION=247874-1 -DENABLE_RLZ -DUSE_LIBJPEG_TURBO=1
-DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_WEBRTC=1
-DENABLE_MEDIA_ROUTER=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DSYSTEM_NATIVELY_SIGNALS_MEMORY_PRESSURE -DENABLE_TASK_MANAGER=1
-DENABLE_EXTENSIONS=1 -DENABLE_PDF=1 -DENABLE_PLUGIN_INSTALLATION=1
-DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1
-DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PROD_WALLET_SERVICE=1 -DENABLE_BACKGROUND=1
-DENABLE_GOOGLE_NOW=1 -DENABLE_PRINTING=1 -DENABLE_BASIC_PRINTING=1
-DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1 -DUSE_BROWSER_SPELLCHECKER=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_SERVICE_DISCOVERY=1
-DENABLE_WIFI_BOOTSTRAPPING=1 -DENABLE_HANGOUT_SERVICES_EXTENSION=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DMOJO_USE_SYSTEM_IMPL -DTOOLKIT_VIEWS=1
-DPROTOBUF_USE_DLLS -DGOOGLE_PROTOBUF_NO_RTTI
-DGOOGLE_PROTOBUF_NO_STATIC_INITIALIZER -DSK_SUPPORT_GPU=1
-DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS -DU_USING_ICU_NAMESPACE=0
-DU_ENABLE_DYLOAD=0 -DU_STATIC_IMPLEMENTATION -DLIBXML_STATIC
'-DI18N_ADDRESS_VALIDATION_DATA_URL="https://i18napis.appspot.com/ssl-aggregate-address/"'
-DI18N_ADDRESSINPUT_USE_BASICTYPES_OVERRIDE=1 -DWEBRTC_CHROMIUM_BUILD
-DWEBRTC_MAC -DWEBRTC_POSIX -DMESA_EGL_NO_X11_HEADERS -DHUNSPELL_STATIC
-DHUNSPELL_CHROME_CLIENT -DUSE_HUNSPELL -DUSE_CUPS -DUSE_LIBPCI=1
-DUSE_OPENSSL=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DNDEBUG
-DOFFICIAL_BUILD -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2
-Igen -I../.. -Iobj/chrome/browser.gen -I../../skia/config
-I../../third_party/khronos -I../../gpu -Igen/angle
-I../../third_party/WebKit/Source -Igen/protoc_out -I../../third_party/protobuf
-I../../third_party/protobuf/src
-I../../third_party/dom_distiller_js/dist/proto_gen -Igen/chrome
-Igen/components/strings -I../../third_party/WebKit -I../../third_party/mojo/src
-Igen/third_party/mojo/src -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../third_party/skia/include/utils/mac -I../../skia/ext
-I../../third_party/cacheinvalidation/overrides
-I../../third_party/cacheinvalidation/src
-I../../third_party/cacheinvalidation/google/cacheinvalidation
-I../../third_party/icu/source/i18n -I../../third_party/icu/source/common
-I../../third_party/libxml/mac/include -I../../third_party/libxml/src/include
-I../../third_party/zlib -Igen/ui/resources -I../../third_party/re2
-Igen/components -Igen/content/app/resources/
-I../../third_party/opus/src/include
-I../../third_party/leveldatabase/src/include
-I../../third_party/leveldatabase/src -I../../third_party/leveldatabase
-I../../third_party/libaddressinput/chromium/override
-I../../third_party/libaddressinput/src/cpp/include
-Igen/third_party/libaddressinput/ -I../../third_party/libyuv/include
-I../../third_party/libyuv -I../../third_party/webrtc_overrides
-I../../third_party -I../../third_party/mesa/src/include -I../../v8/include
-Igen/ui/views/resources -Igen/policy -I../../third_party/google_toolbox_for_mac
-I../../third_party/google_toolbox_for_mac/src
-I../../third_party/google_toolbox_for_mac/src/AppKit
-I../../third_party/google_toolbox_for_mac/src/DebugUtils
-I../../third_party/google_toolbox_for_mac/src/Foundation -isysroot
/Applications/Xcode5.1.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.10.sdk
-O2 -gdwarf-2 -fvisibility=hidden -Werror -Wnewline-eof
-mmacosx-version-min=10.6 -arch x86_64 -Wall -Wendif-labels -Wextra
-Wno-unused-parameter -Wno-missing-field-initializers
-Wno-selector-type-mismatch -Wpartial-availability -Wheader-hygiene
-Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wno-bitfield-width -Wexit-time-destructors -std=c++11
-fno-rtti -fno-exceptions -fvisibility-inlines-hidden -fno-threadsafe-statics
-Xclang -load -Xclang
/b/build/slave/google-chrome-rel-mac/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.dylib
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics
-fno-strict-aliasing -fno-standalone-debug  -c
../../chrome/browser/metrics/metrics_services_manager.cc -o
obj/chrome/browser/metrics/browser.metrics_services_manager.o
../../chrome/browser/metrics/metrics_services_manager.cc:107:5:error: expected
')'
    recording_groups |= rappor::SAFEBROWSING_RAPPOR_GROUP;
    ^
../../chrome/browser/metrics/metrics_services_manager.cc:106:6: note: to match
this '('
  if (client_->IsSafeBrowsingEnabled(on_safe_browsing_update_callback)
     ^
1 error generated.
ninja: build stopped: subcommand failed.
.

[blundell, 2015-10-21 09:14:28]

Description was changed from

==========
Eliminate //chrome dependencies from MetricsServicesManager

This CL eliminates //chrome dependencies from MetricsServicesManager in
preparation for componentizing that file to share with iOS. To do so, we
introduce MetricsServicesManagerClient, which encapsulates the embedder
information that MetricsServicesManager previously internalized.

BUG=508025
TBR=thestig, droger

Committed: https://crrev.com/40b14cfa3f3458ae0d6f655886d952389aabc25c
Cr-Commit-Position: refs/heads/master@{#355255}
==========

to

==========
Eliminate //chrome dependencies from MetricsServicesManager

This CL eliminates //chrome dependencies from MetricsServicesManager in
preparation for componentizing that file to share with iOS. To do so, we
introduce MetricsServicesManagerClient, which encapsulates the embedder
information that MetricsServicesManager previously internalized.

BUG=508025
TBR=thestig, droger

Committed: https://crrev.com/40b14cfa3f3458ae0d6f655886d952389aabc25c
Cr-Commit-Position: refs/heads/master@{#355255}
==========

[blundell, 2015-10-21 09:15:35]

PS5 has the fix for the official build. Going to reland. Will address thestig's
nits in a separate followup.

[blundell, 2015-10-21 09:16:06]

The CQ bit was checked by blundell@chromium.org

[blundell, 2015-10-21 09:16:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1411593004/#ps140001
(title: "Fix official build")

[commit-bot: I haz the power, 2015-10-21 09:16:29]

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

[commit-bot: I haz the power, 2015-10-21 10:10:33]

Committed patchset #5 (id:140001)

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

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

[droger, 2015-10-21 15:40:15]

lgtm

[blundell, 2016-01-22 10:30:10]

On 2015/10/21 06:36:01, Lei Zhang (OOO) wrote:
> Just fix the nits later.
> 
>
https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
> File chrome/browser/metrics/metrics_services_manager_client.h (right):
> 
>
https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
> chrome/browser/metrics/metrics_services_manager_client.h:9: #include
> "base/callback.h"
> base/callback_forward.h
> 
>
https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
> chrome/browser/metrics/metrics_services_manager_client.h:11: #include
> "base/threading/thread_checker.h"
> not used
> 
>
https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
> chrome/browser/metrics/metrics_services_manager_client.h:14: class
> MetricsService;
> not referenced

https://codereview.chromium.org/1612383003/ :)

[blundell, 2016-01-22 10:30:24]

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
File chrome/browser/metrics/metrics_services_manager_client.h (right):

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/metrics_services_manager_client.h:9: #include
"base/callback.h"
On 2015/10/21 06:36:01, Lei Zhang (OOO) wrote:
> base/callback_forward.h

Done.

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/metrics_services_manager_client.h:11: #include
"base/threading/thread_checker.h"
On 2015/10/21 06:36:01, Lei Zhang (OOO) wrote:
> not used

Done.

https://codereview.chromium.org/1411593004/diff/120001/chrome/browser/metrics...
chrome/browser/metrics/metrics_services_manager_client.h:14: class
MetricsService;
On 2015/10/21 06:36:01, Lei Zhang (OOO) wrote:
> not referenced

Done.