Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1412813007:
ExternalDataUseObserver JNI integration (Closed)
DescriptionExternalDataUseObserver JNI integration
- ExternalDataUseObserver fetches matching rules over JNI
periodically.
- ExternalDataUseObserver buffers data use reports until
enough number of bytes have been buffered. Once enough
bytes have been buffered, data use report is sent over JNI.
- Field trial is used to control the value of various
constants.
BUG=540061
Committed: https://crrev.com/0f4c24aa707bbecc888e6c77ba03fec58ed011d1
Cr-Commit-Position: refs/heads/master@{#358511}
Patch Set 1 : First patch set #
Total comments: 28
Patch Set 2 : Addressed sclittle comments #
Total comments: 4
Patch Set 3 : Addressed comments #Patch Set 4 : static cast #
Messages
Total messages: 16 (8 generated)
Description was changed from ========== Fetch matching rules periodically. BUG= ========== to ========== Fetch matching rules periodically. Buffer data use reports until enough number of bytes have been buffered. Use field trial to control the value of various constants. BUG= ==========
Description was changed from ========== Fetch matching rules periodically. Buffer data use reports until enough number of bytes have been buffered. Use field trial to control the value of various constants. BUG= ========== to ========== ExternalDataUseObserver JNI integration - ExternalDataUseObserver fetches matching rules over JNI periodically. - ExternalDataUseObserver buffers data use reports until enough number of bytes have been buffered. Once enough bytes have been buffered, data use report is sent over JNI. - Field trial is used to control the value of various constants. BUG= ==========
Description was changed from ========== ExternalDataUseObserver JNI integration - ExternalDataUseObserver fetches matching rules over JNI periodically. - ExternalDataUseObserver buffers data use reports until enough number of bytes have been buffered. Once enough bytes have been buffered, data use report is sent over JNI. - Field trial is used to control the value of various constants. BUG= ========== to ========== ExternalDataUseObserver JNI integration - ExternalDataUseObserver fetches matching rules over JNI periodically. - ExternalDataUseObserver buffers data use reports until enough number of bytes have been buffered. Once enough bytes have been buffered, data use report is sent over JNI. - Field trial is used to control the value of various constants. BUG=540061 ==========
Patchset #1 (id:1) has been deleted
tbansal@chromium.org changed reviewers: + sclittle@chromium.org
sclittle: PTAL. Thanks!
https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:29: const int kDefaultFetchMatchingRulesDurationSeconds = 60 * 15; // 15 minutes nit: add period at the end of the comment. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:33: const int64_t kDefaultDataUseReportMinBytes = 100 * 1000; // 100 KB. nit: 1 KB = 1024 bytes https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:99: uint64_t data_use_report_min_bytes = 0; Use an int64_t here instead of a uint64_t. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:104: data_use_report_min_bytes_ = data_use_report_min_bytes; Please don't convert a uint64_t to an int64_t without a static cast or something. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:118: ExternalDataUseObserver::~ExternalDataUseObserver() { To help prevent losing any information, could we pass any remaining data use to the Java observer here in the destructor? It wouldn't be a guarantee, because another report could be being submitted right at this time anyways, but it could at least help. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:213: last_matching_rules_fetch_time_ = base::TimeTicks::Now(); Could you add a quick comment around here describing what this does? https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:266: // Remove the first entry. Why do you remove the first entry? They aren't in any order anyways, since it's a hash map, so why not just ignore the new entry? If you really want to remove the oldest entry, then you should use a linked hash map: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/linked_ha... https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:285: DCHECK_LE(buffered_data_reports_.size(), nit: fix indentation https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:84: const std::string GetExternalDataUseObserverFieldTrialName() const; Make this a static constant. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:140: struct DataUseReport { For a future CL: This DataUseReport stuff should really be moved out to its own separate files, and the implementations of all the constructors/methods should be in the .cc file, not the header file. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:192: typedef std::unordered_map<DataUseReportKey, Change this to use base::hash_map from https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/ha... Chromium code isn't allowed to use C++11 STL containers like unordered_map. See https://chromium-cpp.appspot.com/ I'm surprised this compiles at all - last time I checked, Android was one of the platforms that didn't support C++11 STL coolness. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:225: void PopulateFieldTrialParams(); Could this just be a few anonymous functions in the .cc file instead of a method?
Patchset #2 (id:40001) has been deleted
ptal. thanks! https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:29: const int kDefaultFetchMatchingRulesDurationSeconds = 60 * 15; // 15 minutes On 2015/11/06 23:42:49, sclittle wrote: > nit: add period at the end of the comment. Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:33: const int64_t kDefaultDataUseReportMinBytes = 100 * 1000; // 100 KB. On 2015/11/06 23:42:48, sclittle wrote: > nit: 1 KB = 1024 bytes Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:99: uint64_t data_use_report_min_bytes = 0; On 2015/11/06 23:42:50, sclittle wrote: > Use an int64_t here instead of a uint64_t. Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:104: data_use_report_min_bytes_ = data_use_report_min_bytes; On 2015/11/06 23:42:50, sclittle wrote: > Please don't convert a uint64_t to an int64_t without a static cast or > something. Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:118: ExternalDataUseObserver::~ExternalDataUseObserver() { On 2015/11/06 23:42:49, sclittle wrote: > To help prevent losing any information, could we pass any remaining data use to > the Java observer here in the destructor? > > It wouldn't be a guarantee, because another report could be being submitted > right at this time anyways, but it could at least help. The data use reports are asynchronously submitted on UI thread by the Java object. The posted function on UI thread may get called after the destructor has finished. This makes it impossible to post any reports from the destructor. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:213: last_matching_rules_fetch_time_ = base::TimeTicks::Now(); On 2015/11/06 23:42:50, sclittle wrote: > Could you add a quick comment around here describing what this does? Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:266: // Remove the first entry. On 2015/11/06 23:42:49, sclittle wrote: > Why do you remove the first entry? They aren't in any order anyways, since it's > a hash map, so why not just ignore the new entry? > > If you really want to remove the oldest entry, then you should use a linked hash > map: > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/linked_ha... Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:285: DCHECK_LE(buffered_data_reports_.size(), On 2015/11/06 23:42:49, sclittle wrote: > nit: fix indentation Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:84: const std::string GetExternalDataUseObserverFieldTrialName() const; On 2015/11/06 23:42:50, sclittle wrote: > Make this a static constant. This function would be used by Raj to control other params in his classes (e.g., how many tabs to maintain the memory etc.) https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:140: struct DataUseReport { On 2015/11/06 23:42:50, sclittle wrote: > For a future CL: This DataUseReport stuff should really be moved out to its own > separate files, and the implementations of all the constructors/methods should > be in the .cc file, not the header file. Agree with moving the implementation to .cc but why is it useful to move them to separate files since the inner classes are only used here. http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in.... https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:192: typedef std::unordered_map<DataUseReportKey, On 2015/11/06 23:42:50, sclittle wrote: > Change this to use base::hash_map from > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/ha... > > Chromium code isn't allowed to use C++11 STL containers like unordered_map. See > https://chromium-cpp.appspot.com/ > > I'm surprised this compiles at all - last time I checked, Android was one of the > platforms that didn't support C++11 STL coolness. Interesting. Fixed it. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:225: void PopulateFieldTrialParams(); On 2015/11/06 23:42:50, sclittle wrote: > Could this just be a few anonymous functions in the .cc file instead of a > method? Done.
LGTM % nits https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:84: const std::string GetExternalDataUseObserverFieldTrialName() const; On 2015/11/07 01:32:43, tbansal1 wrote: > On 2015/11/06 23:42:50, sclittle wrote: > > Make this a static constant. > > This function would be used by Raj to control other params in his classes (e.g., > how many tabs to maintain the memory etc.) Could it just be a publicly visible static constant then? https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:140: struct DataUseReport { On 2015/11/07 01:32:43, tbansal1 wrote: > On 2015/11/06 23:42:50, sclittle wrote: > > For a future CL: This DataUseReport stuff should really be moved out to its > own > > separate files, and the implementations of all the constructors/methods should > > be in the .cc file, not the header file. > > Agree with moving the implementation to .cc but why is it useful to move them to > separate files since the inner classes are only used here. > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in.... Good point, it might make sense to move them to separate files later if they get complicated though. https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:268: total_bytes_buffered_ -= (data_use->rx_bytes + data_use->tx_bytes); nit: Instead of adding the bytes above and subtracting them here, could you just add them after the insert or merge steps below? https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:111: DataUseReportKey(const DataUseReportKey& other) nit: This copy constructor is unnecessary, since it's the same as the default copy constructor.
https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:84: const std::string GetExternalDataUseObserverFieldTrialName() const; On 2015/11/07 02:05:32, sclittle wrote: > On 2015/11/07 01:32:43, tbansal1 wrote: > > On 2015/11/06 23:42:50, sclittle wrote: > > > Make this a static constant. > > > > This function would be used by Raj to control other params in his classes > (e.g., > > how many tabs to maintain the memory etc.) > > Could it just be a publicly visible static constant then? Done. https://codereview.chromium.org/1412813007/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:140: struct DataUseReport { On 2015/11/07 02:05:32, sclittle wrote: > On 2015/11/07 01:32:43, tbansal1 wrote: > > On 2015/11/06 23:42:50, sclittle wrote: > > > For a future CL: This DataUseReport stuff should really be moved out to its > > own > > > separate files, and the implementations of all the constructors/methods > should > > > be in the .cc file, not the header file. > > > > Agree with moving the implementation to .cc but why is it useful to move them > to > > separate files since the inner classes are only used here. > > > > > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Move-in.... > > Good point, it might make sense to move them to separate files later if they get > complicated though. Acknowledged. https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.cc (right): https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.cc:268: total_bytes_buffered_ -= (data_use->rx_bytes + data_use->tx_bytes); On 2015/11/07 02:05:32, sclittle wrote: > nit: Instead of adding the bytes above and subtracting them here, could you just > add them after the insert or merge steps below? Done. https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... File chrome/browser/android/data_usage/external_data_use_observer.h (right): https://codereview.chromium.org/1412813007/diff/60001/chrome/browser/android/... chrome/browser/android/data_usage/external_data_use_observer.h:111: DataUseReportKey(const DataUseReportKey& other) On 2015/11/07 02:05:32, sclittle wrote: > nit: This copy constructor is unnecessary, since it's the same as the default > copy constructor. Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/1412813007/#ps100001 (title: "static cast")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1412813007/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1412813007/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0f4c24aa707bbecc888e6c77ba03fec58ed011d1 Cr-Commit-Position: refs/heads/master@{#358511} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
