WebView Metrics client implementation (Chromium part)

WebView Metrics client implementation (Chromium part)

BUG=546754
Committed: https://crrev.com/5bc628b55810289b6ebb269da47642b929eaf42d
Cr-Commit-Position: refs/heads/master@{#367433}

[paulmiller, 2015-11-24 00:59:40]

Description was changed from

==========
WebView Metrics prototype (Chromium part)

BUG=546754
==========

to

==========
WebView Metrics prototype (Chromium part)

BUG=546754
==========

[paulmiller, 2015-11-24 00:59:40]

paulmiller@chromium.org changed reviewers:
+ sgurun@chromium.org

[sgurun-gerrit only, 2015-12-02 00:41:43]

will look at it once you complete your experiments and ready for review. a few
simple things for now,

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:101: return 42;
?

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:191: bool
AwMetricsServiceClient::s_metrics_enabled_ = false;
drop

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:73: static bool
s_metrics_enabled_;
why static?

[paulmiller, 2015-12-08 01:44:12]

Description was changed from

==========
WebView Metrics prototype (Chromium part)

BUG=546754
==========

to

==========
WebView Metrics implementation (Chromium part)

BUG=546754
==========

[paulmiller, 2015-12-08 01:44:34]

Description was changed from

==========
WebView Metrics implementation (Chromium part)

BUG=546754
==========

to

==========
WebView Metrics client implementation (Chromium part)

BUG=546754
==========

[paulmiller, 2015-12-09 00:48:49]

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

[paulmiller, 2015-12-09 00:49:08]

talked to ath@ of Gmail, need to do some extra checks to make sure this is only
enabled for dogfood, will upload that later.

+Alexei for adding metrics component to DEPS

[paulmiller, 2015-12-09 00:50:41]

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:101: return 42;
On 2015/12/02 00:41:43, sgurun wrote:
> ?

fixed

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.cc:191: bool
AwMetricsServiceClient::s_metrics_enabled_ = false;
On 2015/12/02 00:41:43, sgurun wrote:
> drop

fixed

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1474483004/diff/40001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:73: static bool
s_metrics_enabled_;
On 2015/12/02 00:41:43, sgurun wrote:
> why static?

fixed

[Alexei Svitkine (slow), 2015-12-09 19:58:53]

lgtm

[sgurun-gerrit only, 2015-12-11 00:06:46]

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:32: class
AwMetricsServiceClient : public metrics::MetricsServiceClient {
in class doc mention about thread safety, which threads are the methods supposed
to be called from.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:37: static void
SetMetricsEnabled(bool enabled);
make in non-static, let the caller call GetInstance()

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:70: static const int
kUploadIntervalMinutes = 30;
move to .cc file in an anonymous namespace

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java
(right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:23:
private Context mApplicationContext;
drop

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:33:
mApplicationContext = applicationContext;
drop

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:34:
getMetricsServiceClient();
do a 
mMetricsServiceClient = new MetricsServiceClient(applicationContext)
here

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:69:
return mMetricsServiceClient;
drop the if, simply return mMetricsServiceClient

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java
(right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:22:
private static final String TAG = "AwMetricsServiceCli-";
"AwMetricsServiceClient"

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:23:
private static final String GMAIL = "com.google.android.gm";
let's move this logic to the bridge

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:41:
if (GMAIL.equals(applicationContext.getPackageName())) {
same, move this to the bridge.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:47:
Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + " (invocation):",
not an error. This is actually a reasonable state for AOSP webview. Log.i

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:49:
} catch (Exception e) {
I don't think you can put a blanket exception here. Need to specialize. (I think
findbugs will barf)

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:50:
Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + ":", e);
rather than repeating the Log.e, use a boolean to indicate fail state and log if
it failed.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:55:
// bridge failed, get the generic service bridge.
remove the gmail comment.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
File android_webview/native/BUILD.gn (right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
android_webview/native/BUILD.gn:33: "//third_party/protobuf",
should this be here?

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
File android_webview/native/webview_native.gyp (right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
android_webview/native/webview_native.gyp:37: '<(protoc_out_dir)',
should this be here?

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
android_webview/native/webview_native.gyp:73: 'aw_message_port_service_impl.h',
coughing :)

[paulmiller, 2015-12-18 00:15:50]

uploaded #9

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:32: class
AwMetricsServiceClient : public metrics::MetricsServiceClient {
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:37: static void
SetMetricsEnabled(bool enabled);
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
android_webview/browser/aw_metrics_service_client.h:70: static const int
kUploadIntervalMinutes = 30;
Is an anonymous namespace necessary? It's already private.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
File android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java
(right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:23:
private Context mApplicationContext;
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:33:
mApplicationContext = applicationContext;
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:34:
getMetricsServiceClient();
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:69:
return mMetricsServiceClient;
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
File
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java
(right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:22:
private static final String TAG = "AwMetricsServiceCli-";
I'd prefer that too, but there's a length limit.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:23:
private static final String GMAIL = "com.google.android.gm";
Are you sure? The Gmail detection is specific to Metrics.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:47:
Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + " (invocation):",
I think that'll actually be a ClassNotFoundException.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:49:
} catch (Exception e) {
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:50:
Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + ":", e);
But I want to log InvocationTargetException differently by calling getCause,
because InvocationTargetException otherwise gives no useful information.

https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:55:
// bridge failed, get the generic service bridge.
fixed

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
File android_webview/native/BUILD.gn (right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
android_webview/native/BUILD.gn:33: "//third_party/protobuf",
required by metrics_service_client.h; will investigate later

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
File android_webview/native/webview_native.gyp (right):

https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
android_webview/native/webview_native.gyp:37: '<(protoc_out_dir)',
required by metrics_service_client.h; will investigate later

[sgurun-gerrit only, 2015-12-21 21:47:11]

On 2015/12/18 00:15:50, paulmiller wrote:
> uploaded #9
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
> File android_webview/browser/aw_metrics_service_client.h (right):
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
> android_webview/browser/aw_metrics_service_client.h:32: class
> AwMetricsServiceClient : public metrics::MetricsServiceClient {
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
> android_webview/browser/aw_metrics_service_client.h:37: static void
> SetMetricsEnabled(bool enabled);
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/browser...
> android_webview/browser/aw_metrics_service_client.h:70: static const int
> kUploadIntervalMinutes = 30;
> Is an anonymous namespace necessary? It's already private.
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
> File
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java
> (right):
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:23:
> private Context mApplicationContext;
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:33:
> mApplicationContext = applicationContext;
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:34:
> getMetricsServiceClient();
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:69:
> return mMetricsServiceClient;
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
> File
>
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java
> (right):
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:22:
> private static final String TAG = "AwMetricsServiceCli-";
> I'd prefer that too, but there's a length limit.
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:23:
> private static final String GMAIL = "com.google.android.gm";
> Are you sure? The Gmail detection is specific to Metrics.
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:47:
> Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + " (invocation):",
> I think that'll actually be a ClassNotFoundException.
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:49:
> } catch (Exception e) {
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:50:
> Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + ":", e);
> But I want to log InvocationTargetException differently by calling getCause,
> because InvocationTargetException otherwise gives no useful information.
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/java/sr...
>
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:55:
> // bridge failed, get the generic service bridge.
> fixed
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
> File android_webview/native/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
> android_webview/native/BUILD.gn:33: "//third_party/protobuf",
> required by metrics_service_client.h; will investigate later
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
> File android_webview/native/webview_native.gyp (right):
> 
>
https://codereview.chromium.org/1474483004/diff/80001/android_webview/native/...
> android_webview/native/webview_native.gyp:37: '<(protoc_out_dir)',
> required by metrics_service_client.h; will investigate later

[sgurun-gerrit only, 2015-12-21 21:47:26]

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:23: }
nit: add // namespace to indicate the end of namespace

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:48: 
please file a bug that blocks main UMA bug to add other metrics service
providers.

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:69: if (enabled) {
nit: no brackets when if block is oneliner.

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:98: // for WebView.
this seems to need its own bug (or 564962 is satisfactory?)

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:158: return is_enabled_;
this is called as a callback from metrics service. Add a
dcheck_currently_on(Browserthread::UI) here to ease catching any regressions,
since it is set from java side (in main thread)

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:70: bool
IsReportingEnabled();
rename it to is_reporting_enabled(), trivial set/get methods in chromium source
code are not named in camelcase.

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:71: void
StoreClientInfo(const metrics::ClientInfo& client_info);
move to anonymous namespace in .cc

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:72:
scoped_ptr<metrics::ClientInfo> LoadClientInfo();
move to anonymous namespace in .cc

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java
(right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:23:
nativeSetMetricsEnabled(enabled.booleanValue());
java unboxing takes care of that. drop .booleanvalue

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
(right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:15:
* This class manages platform-specific services. (i.e. Google Services) The
platform
It is actually the Google Webview implementation that should extend this class,
not the platform. please reword.

https://codereview.chromium.org/1474483004/diff/180001/android_webview/native...
File android_webview/native/aw_metrics_switch.cc (right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/native...
android_webview/native/aw_metrics_switch.cc:17: static_cast<bool>(enabled));
static_cast is unnecessary.

[paulmiller, 2015-12-22 19:42:05]

uploaded #11

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:23: }
done

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:48: 
I'll just copy all the relevant-looking ones from
ChromeMetricsServiceClient::Initialize. Except for AndroidMetricsProvider, since
that one's under chrome/. Maybe there should be a non-blocking bug to refactor
AndroidMetricsProvider into components/metrics/ ?

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:69: if (enabled) {
done

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:98: // for WebView.
The purpose of 564962 is to get the log source, so I think that's sufficient.

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:158: return is_enabled_;
done

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.h (right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:70: bool
IsReportingEnabled();
done

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:71: void
StoreClientInfo(const metrics::ClientInfo& client_info);
done

https://codereview.chromium.org/1474483004/diff/180001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.h:72:
scoped_ptr<metrics::ClientInfo> LoadClientInfo();
done

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java
(right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/AwMetricsServiceClient.java:23:
nativeSetMetricsEnabled(enabled.booleanValue());
done

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
(right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:15:
* This class manages platform-specific services. (i.e. Google Services) The
platform
Google WebView is part of the "platform". Since this is the non-Google-specific
part of the code, I worded it to be non-Google-specific.

https://codereview.chromium.org/1474483004/diff/180001/android_webview/native...
File android_webview/native/aw_metrics_switch.cc (right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/native...
android_webview/native/aw_metrics_switch.cc:17: static_cast<bool>(enabled));
done

[sgurun-gerrit only, 2015-12-23 00:08:56]

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
File
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
(right):

https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:15:
* This class manages platform-specific services. (i.e. Google Services) The
platform
On 2015/12/22 19:42:04, paulmiller wrote:
> Google WebView is part of the "platform". Since this is the
non-Google-specific
> part of the code, I worded it to be non-Google-specific.

Himm, Webview is part of the frameworks actually. But I think thinking about it,
the platform specific implementation of webview needs to overwrite this if it
wants to, so yeah I can live with it :)

https://codereview.chromium.org/1474483004/diff/200001/android_webview/BUILD.gn
File android_webview/BUILD.gn (right):

https://codereview.chromium.org/1474483004/diff/200001/android_webview/BUILD....
android_webview/BUILD.gn:597:
"java/src/org/chromium/android_webview/PlatformServiceBridge.java",
nit: the file seems to have some violations on the order but let's still keep it
neat. P should be smaller than p in Ascii

https://codereview.chromium.org/1474483004/diff/200001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1474483004/diff/200001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:142: // TODO(paulmiller)
Update this once we have channel info.
sorry for missing this. Since we have alpha beta and stable channels, it is
important enough to address this before release. Can you file a crbug blocking
main bug to investigate further

[sgurun-gerrit only, 2015-12-23 00:42:52]

On 2015/12/23 00:08:56, sgurun wrote:
>
https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
> File
>
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
> (right):
> 
>
https://codereview.chromium.org/1474483004/diff/180001/android_webview/java/s...
>
android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:15:
> * This class manages platform-specific services. (i.e. Google Services) The
> platform
> On 2015/12/22 19:42:04, paulmiller wrote:
> > Google WebView is part of the "platform". Since this is the
> non-Google-specific
> > part of the code, I worded it to be non-Google-specific.
> 
> Himm, Webview is part of the frameworks actually. But I think thinking about
it,
> the platform specific implementation of webview needs to overwrite this if it
> wants to, so yeah I can live with it :)
> 
>
https://codereview.chromium.org/1474483004/diff/200001/android_webview/BUILD.gn
> File android_webview/BUILD.gn (right):
> 
>
https://codereview.chromium.org/1474483004/diff/200001/android_webview/BUILD....
> android_webview/BUILD.gn:597:
> "java/src/org/chromium/android_webview/PlatformServiceBridge.java",
> nit: the file seems to have some violations on the order but let's still keep
it
> neat. P should be smaller than p in Ascii
> 
>
https://codereview.chromium.org/1474483004/diff/200001/android_webview/browse...
> File android_webview/browser/aw_metrics_service_client.cc (right):
> 
>
https://codereview.chromium.org/1474483004/diff/200001/android_webview/browse...
> android_webview/browser/aw_metrics_service_client.cc:142: // TODO(paulmiller)
> Update this once we have channel info.
> sorry for missing this. Since we have alpha beta and stable channels, it is
> important enough to address this before release. Can you file a crbug blocking
> main bug to investigate further

lgtm after my comments are addressed

[paulmiller, 2016-01-04 22:41:11]

https://codereview.chromium.org/1474483004/diff/200001/android_webview/BUILD.gn
File android_webview/BUILD.gn (right):

https://codereview.chromium.org/1474483004/diff/200001/android_webview/BUILD....
android_webview/BUILD.gn:597:
"java/src/org/chromium/android_webview/PlatformServiceBridge.java",
obviated by rebase. This section of the file was removed by
https://codereview.chromium.org/1538393003

https://codereview.chromium.org/1474483004/diff/200001/android_webview/browse...
File android_webview/browser/aw_metrics_service_client.cc (right):

https://codereview.chromium.org/1474483004/diff/200001/android_webview/browse...
android_webview/browser/aw_metrics_service_client.cc:142: // TODO(paulmiller)
Update this once we have channel info.
done: crbug.com/574226

[paulmiller, 2016-01-05 00:07:32]

The CQ bit was checked by paulmiller@chromium.org

[paulmiller, 2016-01-05 00:07:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, sgurun@chromium.org
Link to the patchset: https://codereview.chromium.org/1474483004/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-05 00:08:43]

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

[commit-bot: I haz the power, 2016-01-05 00:50:30]

Description was changed from

==========
WebView Metrics client implementation (Chromium part)

BUG=546754
==========

to

==========
WebView Metrics client implementation (Chromium part)

BUG=546754
==========

[commit-bot: I haz the power, 2016-01-05 00:50:31]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-01-05 00:51:33]

Description was changed from

==========
WebView Metrics client implementation (Chromium part)

BUG=546754
==========

to

==========
WebView Metrics client implementation (Chromium part)

BUG=546754

Committed: https://crrev.com/5bc628b55810289b6ebb269da47642b929eaf42d
Cr-Commit-Position: refs/heads/master@{#367433}
==========

[commit-bot: I haz the power, 2016-01-05 00:51:39]

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