Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
651443005:
Use no-op statistics collecting object for WebView (Closed)
|
Created:
6 years, 2 months ago by hush (inactive) Modified:
6 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@refactor2 Project:
chromium Visibility:
Public. |
DescriptionUse no-op statistics collecting object for WebView.
WebView does not gather data_reduction_proxy related
statistics at the moment. WebView's statistics object writes
the data to pref service which does not upload the data to
anywhere.
BUG=426140
Committed: https://crrev.com/4441cc1d6e7ac564a885ad425b2e8914977438d7
Cr-Commit-Position: refs/heads/master@{#300613}
Patch Set 1 : #Patch Set 2 : Selim and Ben's comments #
Total comments: 21
Patch Set 3 : address some of the comments. #Patch Set 4 : #
Total comments: 4
Patch Set 5 : rewrite with simpler changes #
Messages
Total messages: 34 (8 generated)
Patchset #1 (id:1) has been deleted
hush@chromium.org changed reviewers: + bengr@chromium.org, sgurun@chromium.org
hush@chromium.org changed required reviewers: + bengr@chromium.org, sgurun@chromium.org
internal bug: b/17998706
On 2014/10/16 20:16:44, hush wrote: > internal bug: b/17998706 I don't understand this approach. Is it not just that the preferences aren't being registered in time? Can you just register the preferences?
On 2014/10/16 20:16:44, hush wrote: > internal bug: b/17998706 I don't understand this approach. Is it not just that the preferences aren't being registered in time? Can't you just register them?
On 2014/10/16 20:16:44, hush wrote: > internal bug: b/17998706 I don't understand this approach. Is it not just that the preferences aren't being registered in time? Can't you just register them?
On 2014/10/16 20:24:17, bengr1 wrote: > On 2014/10/16 20:16:44, hush wrote: > > internal bug: b/17998706 > > I don't understand this approach. Is it not just that the preferences aren't > being registered in time? Can't you just register them? Sorry I said something wrong the in the internal bug. This is not a timing bug (I don't think...) The preferences object are not registered for webview at all. SetDataReductionProxyStatisticsPrefs is only called in chrome/ code. > Can't you just register them? Yes. I am registering them in this patch. But at the moment, we don't want to gather these stats for android webview yet (for privacy concerns), so I passed a dummy preference object that collects nothing.
On 2014/10/16 21:04:33, hush wrote: > On 2014/10/16 20:24:17, bengr1 wrote: > > On 2014/10/16 20:16:44, hush wrote: > > > internal bug: b/17998706 > > > > I don't understand this approach. Is it not just that the preferences aren't > > being registered in time? Can't you just register them? > > Sorry I said something wrong the in the internal bug. This is not a timing bug > (I don't think...) > The preferences object are not registered for webview at all. > SetDataReductionProxyStatisticsPrefs is > only called in chrome/ code. > > > Can't you just register them? > Yes. I am registering them in this patch. But at the moment, we don't want to > gather these stats for android webview yet (for privacy concerns), so I passed a > dummy preference object > that collects nothing. I see. So in that case, would it make more sense to have the current StatisticsPrefs object subclass a no-op implementation instead of the other way around?
On 2014/10/16 21:12:14, bengr1 wrote: > On 2014/10/16 21:04:33, hush wrote: > > On 2014/10/16 20:24:17, bengr1 wrote: > > > On 2014/10/16 20:16:44, hush wrote: > > > > internal bug: b/17998706 > > > > > > I don't understand this approach. Is it not just that the preferences aren't > > > being registered in time? Can't you just register them? > > > > Sorry I said something wrong the in the internal bug. This is not a timing bug > > (I don't think...) > > The preferences object are not registered for webview at all. > > SetDataReductionProxyStatisticsPrefs is > > only called in chrome/ code. > > > > > Can't you just register them? > > Yes. I am registering them in this patch. But at the moment, we don't want to > > gather these stats for android webview yet (for privacy concerns), so I passed > a > > dummy preference object > > that collects nothing. > > I see. So in that case, would it make more sense to have the current > StatisticsPrefs object subclass a no-op implementation instead of the other way > around? I talked to boliu@ about this general problem. How about a pure virtual base class (an interface) that both DataReductionProxyStatisticsPrefs and AwDataReductionProxyStatisticsPrefs inherit from? A base class with implementation is kind of bad.
On 2014/10/16 21:42:58, hush wrote: > On 2014/10/16 21:12:14, bengr1 wrote: > > On 2014/10/16 21:04:33, hush wrote: > > > On 2014/10/16 20:24:17, bengr1 wrote: > > > > On 2014/10/16 20:16:44, hush wrote: > > > > > internal bug: b/17998706 > > > > > > > > I don't understand this approach. Is it not just that the preferences > aren't > > > > being registered in time? Can't you just register them? > > > > > > Sorry I said something wrong the in the internal bug. This is not a timing > bug > > > (I don't think...) > > > The preferences object are not registered for webview at all. > > > SetDataReductionProxyStatisticsPrefs is > > > only called in chrome/ code. > > > > > > > Can't you just register them? > > > Yes. I am registering them in this patch. But at the moment, we don't want > to > > > gather these stats for android webview yet (for privacy concerns), so I > passed > > a > > > dummy preference object > > > that collects nothing. > > > > I see. So in that case, would it make more sense to have the current > > StatisticsPrefs object subclass a no-op implementation instead of the other > way > > around? > > I talked to boliu@ about this general problem. > How about a pure virtual base class (an interface) that both > DataReductionProxyStatisticsPrefs > and AwDataReductionProxyStatisticsPrefs inherit from? > A base class with implementation is kind of bad. I'm fine with that.
On 2014/10/16 22:09:39, bengr1 wrote: > On 2014/10/16 21:42:58, hush wrote: > > On 2014/10/16 21:12:14, bengr1 wrote: > > > On 2014/10/16 21:04:33, hush wrote: > > > > On 2014/10/16 20:24:17, bengr1 wrote: > > > > > On 2014/10/16 20:16:44, hush wrote: > > > > > > internal bug: b/17998706 > > > > > > > > > > I don't understand this approach. Is it not just that the preferences > > aren't > > > > > being registered in time? Can't you just register them? > > > > > > > > Sorry I said something wrong the in the internal bug. This is not a timing > > bug > > > > (I don't think...) > > > > The preferences object are not registered for webview at all. > > > > SetDataReductionProxyStatisticsPrefs is > > > > only called in chrome/ code. > > > > > > > > > Can't you just register them? > > > > Yes. I am registering them in this patch. But at the moment, we don't want > > to > > > > gather these stats for android webview yet (for privacy concerns), so I > > passed > > > a > > > > dummy preference object > > > > that collects nothing. > > > > > > I see. So in that case, would it make more sense to have the current > > > StatisticsPrefs object subclass a no-op implementation instead of the other > > way > > > around? > > > > I talked to boliu@ about this general problem. > > How about a pure virtual base class (an interface) that both > > DataReductionProxyStatisticsPrefs > > and AwDataReductionProxyStatisticsPrefs inherit from? > > A base class with implementation is kind of bad. > > I'm fine with that. Cool. I will do so in the next patch set.
The name statisticsprefs is confusing to me. I thought they were some prefs to
enable collection of some statistics but it does not look like the case, rather
they are the stats themselves, i.e.:
registry->RegisterInt64Pref(
prefs::kHttpReceivedContentLength, 0);
If these are really the stats then they may actually would be useful to webview
at some point of time. I am still looking at the code though.
Actually, I think it is better to push such logic to Data Reduction Proxy component as much as possible to make the layers less tightly integrated. We can have a default implementation of DataReductionProxyStatisticsPrefs base class in the DRP component that does nothing and if DRP requires to instantiate a object of this class, it can create one by default. Chrome or any other/future clients of DRP can inject a settings prefs object through DataReductionProxySettings::SetDataReductionProxyStatisticsPrefs() which they already need to do. Looking at the size of this CL, this would move close to 100 lines of DRP related code from webview to DRP component.
PTAL
just looked webview side and very little to drp changes. https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.cc:47: using data_reduction_proxy::DataReductionProxyStatisticsPrefs; stale? https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.cc:242: // which means a default dummy DataReductionProxyStatistics will be used. I don't think we will need this comment anymore, since default behaviour will work,. https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.h:33: class AwDataReductionProxyStatisticsPrefs; stale? https://codereview.chromium.org/651443005/diff/40001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/651443005/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:79: settings_->SetDataReductionProxyStatistics(statistics_prefs_.get()); why change name? https://codereview.chromium.org/651443005/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/651443005/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:673: data_reduction_proxy_chrome_settings->SetDataReductionProxyStatistics( same here. why change name?
https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.cc:47: using data_reduction_proxy::DataReductionProxyStatisticsPrefs; On 2014/10/17 20:29:38, sgurun wrote: > stale? Done. https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.h:33: class AwDataReductionProxyStatisticsPrefs; On 2014/10/17 20:29:38, sgurun wrote: > stale? Done. https://codereview.chromium.org/651443005/diff/40001/chrome/browser/net/spdyp... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc (right): https://codereview.chromium.org/651443005/diff/40001/chrome/browser/net/spdyp... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc:79: settings_->SetDataReductionProxyStatistics(statistics_prefs_.get()); On 2014/10/17 20:29:38, sgurun wrote: > why change name? the idea of the base class "DataReductionProxyStatistics" is that it is an object that contains the drp stats, regardless of what is used to implement it. DataReductionProxyStatisticsPrefs writes the stats into pref_service, but it is an implementation detail. I don't feel too strongly about this though.. If you guys don't like this name, I will keep it and have a smaller CL
https://codereview.chromium.org/651443005/diff/40001/chrome/browser/profiles/... File chrome/browser/profiles/profile_impl.cc (right): https://codereview.chromium.org/651443005/diff/40001/chrome/browser/profiles/... chrome/browser/profiles/profile_impl.cc:673: data_reduction_proxy_chrome_settings->SetDataReductionProxyStatistics( On 2014/10/17 20:29:38, sgurun wrote: > same here. why change name? I like the name change, because the statistics object doesn't necessarily have to be backed by prefs. https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:122: DataReductionProxyStatistics* drp_statistics); Don't use abbreviations like "drp". Can you just call this "statistics"? https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:313: DataReductionProxyStatistics* drp_statistics_; I don't understand why you need two statistics pointers. Why can't Webview manage the lifetime of its statistics object? https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.cc (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.cc:23: return list_value_.get(); This seems very strange to me. Why not do what the derived class does and maintain maps for lists and int64s to make this class functional? Then the derived class can used these data structures instead of creating its own. https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h:17: // This class collects the statistics of data reduction proxy. The statistics Add a blank line above. https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h:26: // Sets the int64 stats corresponding to |stats_name|. Add a blank line between each of these methods.
https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:313: DataReductionProxyStatistics* drp_statistics_; On 2014/10/17 23:40:27, bengr1 wrote: > I don't understand why you need two statistics pointers. Why can't Webview > manage the lifetime of its statistics object? From my discussion with Selim yesterday, I think our goal is to make data_reduction_proxy and webview as loose coupled as possible. Since webview does not care about stats gathering at the moment, I want to push the responsibility of statistics object lifetime management to data_reduction_proxy component. https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.cc (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.cc:23: return list_value_.get(); On 2014/10/17 23:40:27, bengr1 wrote: > This seems very strange to me. Why not do what the derived class does and > maintain maps for lists and int64s to make this class functional? Then the > derived class can used these data structures instead of creating its own. Yes, I thought about this... WebView has to use a no-op DataReductionProxyStatistics at the moment (otherwise we may need to have the user's consent that WebView is gathering such data). I can do what you said. But we need to make sure the WriteStats() function is empty. Since the data is already somewhere else in memory, I see no harm in putting it to a map in memory. But we can't send it anywhere else (which is what WriteStats does in the derived class) https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h:17: // This class collects the statistics of data reduction proxy. The statistics On 2014/10/17 23:40:27, bengr1 wrote: > Add a blank line above. Done. https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h:26: // Sets the int64 stats corresponding to |stats_name|. On 2014/10/17 23:40:27, bengr1 wrote: > Add a blank line between each of these methods. Done. https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics_prefs.h:62: void Init(); https://code.google.com/p/chromium/codesearch#chromium/src/components/data_re... actually I don't think the unit test needs Init(). Init() is already called in the constructor, so there is no need to call it again right after the constructor? I will move it to private. What do you guys think?
https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/651443005/diff/40001/android_webview/browser/... android_webview/browser/net/aw_url_request_context_getter.cc:242: // which means a default dummy DataReductionProxyStatistics will be used. On 2014/10/17 20:29:38, sgurun wrote: > I don't think we will need this comment anymore, since default behaviour will > work,. Done. https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h (right): https://codereview.chromium.org/651443005/diff/40001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_settings.h:122: DataReductionProxyStatistics* drp_statistics); On 2014/10/17 23:40:27, bengr1 wrote: > Don't use abbreviations like "drp". Can you just call this "statistics"? Done.
bengr@ PTAL
https://codereview.chromium.org/651443005/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.cc (right): https://codereview.chromium.org/651443005/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.cc:15: int64 DataReductionProxyStatistics::GetInt64(const char* stats_name) { Check if the pointer is not null? https://codereview.chromium.org/651443005/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.cc:16: std::map<const char*, int64>::iterator iter = int64_map_.find(stats_name); Can this be a const_iterator? https://codereview.chromium.org/651443005/diff/80001/components/data_reductio... File components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h (right): https://codereview.chromium.org/651443005/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h:27: virtual int64 GetInt64(const char* stats_name); These need tests. https://codereview.chromium.org/651443005/diff/80001/components/data_reductio... components/data_reduction_proxy/core/browser/data_reduction_proxy_statistics.h:41: // Writes the stats to some persistence object depending on implementation. persistent
Patchset #5 (id:100001) has been deleted
sorry about going back and forth on this CL.... Me and Selim had another discussion today. The conclusion is splitting up the data_reduction_proxy_statistics_prefs object is too messy and it is not worth it. It will introduce too much code churn for data_reduction_proxy. Since data reduction proxy is written in the way that it assumes the embedder manages the life time of the statistics_prefs object, android webview (as an embedder) should just create a statistics_prefs object and pass it to DataReductionProxySettings. Selim just told me that even if data_reduction_proxy writes the data to the pref service, it won't be uploaded for WebView. In PS5, webview just creates a statistics prefs object with WebView's pref service. In this way, if webview decides to upload the UMA data somewhere, we can just hook up our pref service to upload. Maybe in the future, data_reduction_proxy could manage the lifetime of statistics prefs object itself? Instead of letting the embedder creating it and pass it in.
lgtm
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/651443005/120001
The CQ bit was unchecked by commit-bot@chromium.org
All required reviewers (with asterisk prefixes) have not yet approved this CL. No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
hush@chromium.org changed required reviewers: - bengr@chromium.org
The CQ bit was checked by hush@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/651443005/120001
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/4441cc1d6e7ac564a885ad425b2e8914977438d7 Cr-Commit-Position: refs/heads/master@{#300613} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
