Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781323004:
Create a new Data Saver settings page that adds the site breakdown (Closed)
|
Created:
3 years, 8 months ago by megjablon Modified:
3 years, 8 months ago CC:
chromium-reviews, srahim+watch_chromium.org, cbentzel+watch_chromium.org, net-reviews_chromium.org, tbansal+watch-data-reduction-proxy_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate a new Data Saver settings page that adds the site breakdown
The new Data Saver settings page moves the savings percent and stats above the
graph, changes formatting, and adds a reset button and site breakdown. The site
breakdown displays the top ten sites with the most data usage and their data
savings. The compression stats in native are quieried and a list of
DataReductionDataUseItem is passed to the view to display.
BUG=615560
Review-Url: https://codereview.chromium.org/2781323004
Cr-Commit-Position: refs/heads/master@{#463732}
Committed: https://chromium.googlesource.com/chromium/src/+/1956acf3a49b71307e53b33129dad628b64326e8
Patch Set 1 #Patch Set 2 : one preference to inflate two different layouts #
Total comments: 28
Patch Set 3 : rebase for test fix #Patch Set 4 : rebase #Patch Set 5 : twellington comments #
Total comments: 2
Patch Set 6 : rebase and use feature #Patch Set 7 : twellington comments #Patch Set 8 : move comparator #
Total comments: 14
Patch Set 9 : sclittle comments #
Total comments: 38
Patch Set 10 : remove file from supressions #Patch Set 11 : sclittle and dfalcantara comments #Patch Set 12 : check that the results object is not null #Patch Set 13 : rebase #Messages
Total messages: 87 (62 generated)
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #2 (id:20001) has been deleted
megjablon@chromium.org changed reviewers: + twellington@chromium.org
Hi Theresa, I haven't had time to write up a design doc yet because I'm trying to move quickly to get this in ASAP. Can you take a look at this for general design, while I get that written up? I'll still need to make some adjustments to get this to spec and also add in sorting by column, which I'll do in a separate cl. Thanks!
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:1) has been deleted
From a high-level, the user of a table layout makes sense to me. The old DataReductionOldStatsPreference.java and DataReductionStatsPreference.java share a lot of code, and it looks like there have been active changes in the last few months (although you would likely know better what those were for). Is it possible for these two classes to share a base class? Or for the new implementation to extend the old for now?
On 2017/03/31 18:29:07, Theresa wrote: > From a high-level, the user of a table layout makes sense to me. The old > DataReductionOldStatsPreference.java and DataReductionStatsPreference.java share > a lot of code, and it looks like there have been active changes in the last few > months (although you would likely know better what those were for). Is it > possible for these two classes to share a base class? Or for the new > implementation to extend the old for now? The reason I did it this way is since we're moving quickly I wanted to isolate the changes as much as possible to reduce the chance of bugs. I thought having the flag inflate different layouts would minimize the risk and make it so we can fall back to the old menu if any issues arise. I plan on stripping out the old menu code once we have transitioned over and then a base class won't be necessary. Does that make sense? I could extend the old for now, but I feel like that would get a bit more intertwined and harder to adapt for other changes we'll be making to the settings page.
On 2017/03/31 18:56:07, megjablon wrote: > On 2017/03/31 18:29:07, Theresa wrote: > > From a high-level, the user of a table layout makes sense to me. The old > > DataReductionOldStatsPreference.java and DataReductionStatsPreference.java > share > > a lot of code, and it looks like there have been active changes in the last > few > > months (although you would likely know better what those were for). Is it > > possible for these two classes to share a base class? Or for the new > > implementation to extend the old for now? > > The reason I did it this way is since we're moving quickly I wanted to isolate > the changes as much as possible to reduce the chance of bugs. I thought having > the flag inflate different layouts would minimize the risk and make it so we can > fall back to the old menu if any issues arise. I plan on stripping out the old > menu code once we have transitioned over and then a base class won't be > necessary. Does that make sense? I could extend the old for now, but I feel like > that would get a bit more intertwined and harder to adapt for other changes > we'll be making to the settings page. I think it primarily depends on whether it's possible that there will be bug fixes or other changes that are now required in two places rather than one since the code was forked. If the possibility is really low, then I think a fork is fine.
Ok this now uses one Preference and should be ready to review!
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2781323004/diff/80001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2781323004/diff/80001/build/android/lint/supp... build/android/lint/suppressions.xml:437: <ignore regexp="chrome/android/java/res/layout/data_usage_breakdown.xml"/> Out of curiosity, what's the useless parent? https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/data_usage_breakdown.xml (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown.xml:47: android:singleLine="true" nit: a lot of this styling can be pulled out into a rule in styles.xml. It looks like the paddingBottom, paddintGop, paddingStart, singleLine, textSize, and textAppearance are all the same between these three text views. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/data_usage_breakdown_row.xml (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown_row.xml:20: android:singleLine="true" Same comment about a style rule here. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown_row.xml:23: <TextView nit: blank line above this one and the other <TextView below https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown_row.xml:30: android:paddingStart="10dp" nit: Can this be paddingStart 10dp w/ no paddingEnd on the previous TextView for consistency with the TextView below. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:285: public void queryDataUsage( nit: JavaDoc https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:286: DataReductionSiteBreakdownView dataReductionBreakdownView, int numDays) { Instead of passing the view directly, I suggest using a "Callback<List<DataReductionDataUseItem>> queryDataUsageCallback". https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionDataUseItem.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionDataUseItem.java:19: * Constructor for a DataReductionDataUseItem which associates a hostname with it's data usage nit: s/it's/its https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionDataUseItem.java:33: * Returns the hostname for this data usage. nit: s/data usage/data use item https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:42: * Display the data use items one they have been fetched from the compression stats. nit: s/one/once https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:60: mTableLayout.removeViews(1, mTableLayout.getChildCount() - 1); This is removing all of the old views except the header, correct? https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java:108: mRightPosition = mCurrentTime + DateUtils.HOUR_IN_MILLIS nit: mLastUpdateTime would be a better name, but it looks like it's been mCreationTime forever so it's okay to leave it. https://codereview.chromium.org/2781323004/diff/80001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:113: // items that by group data use by hostname. nit: s/by group/groups: ...adds them to a list... that groups data use by hostname
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
https://codereview.chromium.org/2781323004/diff/80001/build/android/lint/supp... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2781323004/diff/80001/build/android/lint/supp... build/android/lint/suppressions.xml:437: <ignore regexp="chrome/android/java/res/layout/data_usage_breakdown.xml"/> On 2017/04/03 15:05:20, Theresa wrote: > Out of curiosity, what's the useless parent? The data_reduction_proxy_breakdown_table with only one row that is then populated dynamically. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/data_usage_breakdown.xml (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown.xml:47: android:singleLine="true" On 2017/04/03 15:05:20, Theresa wrote: > nit: a lot of this styling can be pulled out into a rule in styles.xml. It looks > like the paddingBottom, paddintGop, paddingStart, singleLine, textSize, and > textAppearance are all the same between these three text views. Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... File chrome/android/java/res/layout/data_usage_breakdown_row.xml (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown_row.xml:20: android:singleLine="true" On 2017/04/03 15:05:20, Theresa wrote: > Same comment about a style rule here. Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown_row.xml:23: <TextView On 2017/04/03 15:05:20, Theresa wrote: > nit: blank line above this one and the other <TextView below Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/res... chrome/android/java/res/layout/data_usage_breakdown_row.xml:30: android:paddingStart="10dp" On 2017/04/03 15:05:20, Theresa wrote: > nit: Can this be paddingStart 10dp w/ no paddingEnd on the previous TextView for > consistency with the TextView below. Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:285: public void queryDataUsage( On 2017/04/03 15:05:20, Theresa wrote: > nit: JavaDoc Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:286: DataReductionSiteBreakdownView dataReductionBreakdownView, int numDays) { On 2017/04/03 15:05:20, Theresa wrote: > Instead of passing the view directly, I suggest using a > "Callback<List<DataReductionDataUseItem>> queryDataUsageCallback". Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionDataUseItem.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionDataUseItem.java:19: * Constructor for a DataReductionDataUseItem which associates a hostname with it's data usage On 2017/04/03 15:05:21, Theresa wrote: > nit: s/it's/its Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionDataUseItem.java:33: * Returns the hostname for this data usage. On 2017/04/03 15:05:20, Theresa wrote: > nit: s/data usage/data use item Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:42: * Display the data use items one they have been fetched from the compression stats. On 2017/04/03 15:05:21, Theresa wrote: > nit: s/one/once Done. https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:60: mTableLayout.removeViews(1, mTableLayout.getChildCount() - 1); On 2017/04/03 15:05:21, Theresa wrote: > This is removing all of the old views except the header, correct? Yep! https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java:108: mRightPosition = mCurrentTime + DateUtils.HOUR_IN_MILLIS On 2017/04/03 15:05:21, Theresa wrote: > nit: mLastUpdateTime would be a better name, but it looks like it's been > mCreationTime forever so it's okay to leave it. Acknowledged. https://codereview.chromium.org/2781323004/diff/80001/chrome/browser/net/spdy... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/browser/net/spdy... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:113: // items that by group data use by hostname. On 2017/04/03 15:05:21, Theresa wrote: > nit: s/by group/groups: > > ...adds them to a list... that groups data use by hostname Done.
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #6 (id:150001) has been deleted
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
chrome/android/java/src/org/chromium/chrome/browser/preferences/ and chrome/android/java/res lgtm https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:60: mTableLayout.removeViews(1, mTableLayout.getChildCount() - 1); On 2017/04/03 20:33:45, megjablon wrote: > On 2017/04/03 15:05:21, Theresa wrote: > > This is removing all of the old views except the header, correct? > > Yep! It might be helpful to add a comment to that effect. https://codereview.chromium.org/2781323004/diff/130014/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/2781323004/diff/130014/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:320: * @param queryDataUsageCallback Callback to give the list of queryDataUsageCallback on query nit: Callback to give the list of DataReductionDataUseItems on query completion?
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #8 (id:210001) has been deleted
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #8 (id:230001) has been deleted
https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2781323004/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:60: mTableLayout.removeViews(1, mTableLayout.getChildCount() - 1); On 2017/04/03 22:17:31, Theresa wrote: > On 2017/04/03 20:33:45, megjablon wrote: > > On 2017/04/03 15:05:21, Theresa wrote: > > > This is removing all of the old views except the header, correct? > > > > Yep! > > It might be helpful to add a comment to that effect. Done. https://codereview.chromium.org/2781323004/diff/130014/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java (right): https://codereview.chromium.org/2781323004/diff/130014/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/net/spdyproxy/DataReductionProxySettings.java:320: * @param queryDataUsageCallback Callback to give the list of queryDataUsageCallback on query On 2017/04/03 22:17:31, Theresa wrote: > nit: Callback to give the list of DataReductionDataUseItems on query completion? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
megjablon@chromium.org changed reviewers: + dfalcantara@chromium.org, pasko@chromium.org, sclittle@chromium.org
PTAL, thanks! pasko: suppressions.xml dfalcantara: DataReductionProxySettings.java and android_chrome_strings.grd sclittle: data_reduction_proxy_settings_android.*
https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:43: const int BUCKETS_PER_DAY = nit: Change this to constexpr int kBucketsPerDay = ... https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:231: std::unordered_map<std::string, std::unique_ptr<base::DictionaryValue>> Instead of using DictionaryValue here, you could just define a struct in the anonymous namespace above, e.g. something like: struct DataUsageForHost { DataUsageForHost() : data_used(0), original_size(0) {} int64_t data_used; int64_t original_size; }; Then, you could just have a std::map<base::StringPiece, DataUsageForHost> per_site_usage_map; and replace the body of the innermost for-loop below with: for (const auto& site_usage : connection_usage.site_usage()) { DataUsageForHost& usage = per_site_usage_map[site_usage.hostname()]; usage.data_usage += site_usage.data_used(); usage.original_size += site_usage.original_size(); } https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:236: for (int i = data_usage_size - 1; Using signed ints for indices like this seems a bit sketchy, could you replace this with something like: const size_t num_buckets_to_display = num_day_for_query_ * kBucketsPerDay; for (auto data_usage_it = data_usage->size() > num_buckets_to_display ? data_usage->end() - num_buckets_to_display : data_usage->begin(); data_usage_it != data_usage->end(); ++data_usage_it) { // ... inner for loops ... } https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:245: usage->second->GetDouble("data_used", &data_used); Why are these doubles? Could they just be int64s? https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:268: Java_DataReductionProxySettings_createDataUseItemAndAddToList( Instead of adding the items one-by-one making a jni-call for each hostname, is there some way you could send them all over at once in a single batch? https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:36: // For testing purposes. nit: Is there some way this test-only constructor could be hidden such that only tests would be able to access it? https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:119: std::unique_ptr<std::vector<data_reduction_proxy::DataUsageBucket>> Does this need to be a unique_ptr to a vector? Passing just a const std::vector<DataUsageBucket>& seems like it would work just as well.
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:43: const int BUCKETS_PER_DAY = On 2017/04/06 23:27:43, sclittle wrote: > nit: Change this to constexpr int kBucketsPerDay = ... Done. https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:231: std::unordered_map<std::string, std::unique_ptr<base::DictionaryValue>> On 2017/04/06 23:27:43, sclittle wrote: > Instead of using DictionaryValue here, you could just define a struct in the > anonymous namespace above, e.g. something like: > > struct DataUsageForHost { > DataUsageForHost() : data_used(0), original_size(0) {} > > int64_t data_used; > int64_t original_size; > }; > > Then, you could just have a std::map<base::StringPiece, DataUsageForHost> > per_site_usage_map; and replace the body of the innermost for-loop below with: > > for (const auto& site_usage : connection_usage.site_usage()) { > DataUsageForHost& usage = > per_site_usage_map[site_usage.hostname()]; > usage.data_usage += site_usage.data_used(); > usage.original_size += site_usage.original_size(); > } Done. https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:236: for (int i = data_usage_size - 1; On 2017/04/06 23:27:43, sclittle wrote: > Using signed ints for indices like this seems a bit sketchy, could you replace > this with something like: > > const size_t num_buckets_to_display = > num_day_for_query_ * kBucketsPerDay; > for (auto data_usage_it = > data_usage->size() > num_buckets_to_display > ? data_usage->end() - num_buckets_to_display > : data_usage->begin(); > data_usage_it != data_usage->end(); ++data_usage_it) { > > // ... inner for loops ... > > } Done. https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:245: usage->second->GetDouble("data_used", &data_used); On 2017/04/06 23:27:43, sclittle wrote: > Why are these doubles? Could they just be int64s? This is obsolete. https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:268: Java_DataReductionProxySettings_createDataUseItemAndAddToList( On 2017/04/06 23:27:43, sclittle wrote: > Instead of adding the items one-by-one making a jni-call for each hostname, is > there some way you could send them all over at once in a single batch? I don't know of a way. I followed the same pattern as we use to get history items and create a list: https://cs.chromium.org/chromium/src/chrome/browser/android/history/browsing_... https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:36: // For testing purposes. On 2017/04/06 23:27:43, sclittle wrote: > nit: Is there some way this test-only constructor could be hidden such that only > tests would be able to access it? Friended the class and made it private https://codereview.chromium.org/2781323004/diff/250001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:119: std::unique_ptr<std::vector<data_reduction_proxy::DataUsageBucket>> On 2017/04/06 23:27:43, sclittle wrote: > Does this need to be a unique_ptr to a vector? Passing just a const > std::vector<DataUsageBucket>& seems like it would work just as well. That's the callback signature. That'll affect desktop so if we want to change that, we should do that separately. https://cs.chromium.org/chromium/src/components/data_reduction_proxy/core/bro...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:436: <ignore regexp="chrome/android/java/res/layout/data_reduction_promo_screen.xml"/> note: this file does not exist in the tree, remove it? https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:437: <ignore regexp="chrome/android/java/res/layout/data_usage_breakdown.xml"/> I don't understand these suppressions and the reasons why they may be necessary in this case. Please ask one of chrome/android/java/res/OWNERS to take another look at it. I can rubberstamp after that.
While I review this: are there any screenshots to compare the mocks against?
I can send you screenshots if you'd like, but this isn't to spec yet. I still have to add sorting the different columns and plan to update to spec separately. I'm just trying to get this ASAP so it can be demoed. https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:436: <ignore regexp="chrome/android/java/res/layout/data_reduction_promo_screen.xml"/> On 2017/04/07 11:13:46, pasko wrote: > note: this file does not exist in the tree, remove it? Done. https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:437: <ignore regexp="chrome/android/java/res/layout/data_usage_breakdown.xml"/> On 2017/04/07 11:13:46, pasko wrote: > I don't understand these suppressions and the reasons why they may be necessary > in this case. Please ask one of chrome/android/java/res/OWNERS to take another > look at it. I can rubberstamp after that. twellington took a look and commented previously. It's necessary because the TableLayout only has one child row and is populated dynamically.
https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/re... File chrome/android/java/res/layout/data_usage_breakdown.xml (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/re... chrome/android/java/res/layout/data_usage_breakdown.xml:75: android:textSize="16sp" The mock says to use 14sp here. Check the sidebar when you click the button in the mock. https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:54: private static class DataUsedComparator final? https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:97: hostnameView.setText(hostname); just inline hostname here like you do for the other ones? https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java:74: setLayout(); In general these constructors should just call each other. e.g. this(context, null) at this location Do you really need all three of these, though? https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:259: Should you check if either of the java object pointers are null at this point? Will this break if you call QueryDataUsage twice in a row quickly, like if you enter the prefs page, leave it, then re-enter it? https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:122: base::android::ScopedJavaGlobalRef<jobject> j_settings_obj_; If the Java side creates and owns this class, this should be a JavaObjectWeakGlobalRef instead. AIUI ScopedJavaGlobalRefs are supposed to be used when the native side owns the java side and is trying to prevent it from ever being deallocated.
https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:9: #include <algorithm> nit: remove unnecessary include https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:11: #include <unordered_map> nit: remove unnecessary include https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:12: #include <vector> nit: Move this include to the .h file https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:16: #include "base/time/time.h" nit: Is this include necessary? https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:43: constexpr int kBucketsPerDay = minor style nit: I'd recommend explicitly making this a size_t, since that's how it's used below, but this isn't a big deal. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:234: std::map<base::StringPiece, DataUsageForHost> per_site_usage_map; nit: Could you add includes for <map> and base/strings/string_piece.h? https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:243: for (const auto& connection_usage : (*data_usage_it).connection_usage()) { micro style nit: you could use data_usage_it->connection_usage() instead of (*data_usage_it).connection_usage(), since they're equivalent. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:253: Java_DataReductionProxySettings_createDataUseItemAndAddToList( In order to send all the counts at once instead of calling into Java for each item, could you change the Java method signature to take in 3 arrays, one for each of hostnames, data_used counts, and original_size counts? You could use stuff like base::android::ToJavaLongArray(...) to construct those arrays. WDYT? https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:256: (long)site_bucket.second.data_used, Do you need to cast these to long here? |long| in C++ is often just 32-bit, but I think |long| is 64-bit in Java. What happens if the number of bytes is > max 32-bit in (i.e. >2GB)? Even if this cast to long here is removed, does it get truncated to 32-bits at some other point in the process of calling into the Java method? https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:10: #include <memory> nit: Add include for <vector>
Patchset #11 (id:310001) has been deleted
https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/re... File chrome/android/java/res/layout/data_usage_breakdown.xml (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/re... chrome/android/java/res/layout/data_usage_breakdown.xml:75: android:textSize="16sp" On 2017/04/07 20:21:58, dfalcantara (load balance plz) wrote: > The mock says to use 14sp here. Check the sidebar when you click the button in > the mock. Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:54: private static class DataUsedComparator On 2017/04/07 20:21:58, dfalcantara (load balance plz) wrote: > final? Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionSiteBreakdownView.java:97: hostnameView.setText(hostname); On 2017/04/07 20:21:58, dfalcantara (load balance plz) wrote: > just inline hostname here like you do for the other ones? Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/datareduction/DataReductionStatsPreference.java:74: setLayout(); On 2017/04/07 20:21:58, dfalcantara (load balance plz) wrote: > In general these constructors should just call each other. > e.g. this(context, null) at this location > > Do you really need all three of these, though? Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:9: #include <algorithm> On 2017/04/07 20:44:16, sclittle wrote: > nit: remove unnecessary include Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:11: #include <unordered_map> On 2017/04/07 20:44:16, sclittle wrote: > nit: remove unnecessary include Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:12: #include <vector> On 2017/04/07 20:44:16, sclittle wrote: > nit: Move this include to the .h file Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:16: #include "base/time/time.h" On 2017/04/07 20:44:16, sclittle wrote: > nit: Is this include necessary? Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:43: constexpr int kBucketsPerDay = On 2017/04/07 20:44:16, sclittle wrote: > minor style nit: I'd recommend explicitly making this a size_t, since that's how > it's used below, but this isn't a big deal. Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:234: std::map<base::StringPiece, DataUsageForHost> per_site_usage_map; On 2017/04/07 20:44:16, sclittle wrote: > nit: Could you add includes for <map> and base/strings/string_piece.h? I've gotten too use to Eclipse taking care of this in Java :P https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:243: for (const auto& connection_usage : (*data_usage_it).connection_usage()) { On 2017/04/07 20:44:16, sclittle wrote: > micro style nit: you could use data_usage_it->connection_usage() instead of > (*data_usage_it).connection_usage(), since they're equivalent. Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:253: Java_DataReductionProxySettings_createDataUseItemAndAddToList( On 2017/04/07 20:44:16, sclittle wrote: > In order to send all the counts at once instead of calling into Java for each > item, could you change the Java method signature to take in 3 arrays, one for > each of hostnames, data_used counts, and original_size counts? > > You could use stuff like base::android::ToJavaLongArray(...) to construct those > arrays. WDYT? I'm not a huge fan of this option. It doesn't seem clean to send over three different arrays where each index is expected to match up. Also we then have to construct the objects in Java or keep passing around all three of the arrays there. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:256: (long)site_bucket.second.data_used, On 2017/04/07 20:44:16, sclittle wrote: > Do you need to cast these to long here? |long| in C++ is often just 32-bit, but > I think |long| is 64-bit in Java. > > What happens if the number of bytes is > max 32-bit in (i.e. >2GB)? Even if this > cast to long here is removed, does it get truncated to 32-bits at some other > point in the process of calling into the Java method? Removed cast. Tried it out and it works as expected with longs. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:259: On 2017/04/07 20:21:58, dfalcantara (load balance plz) wrote: > Should you check if either of the java object pointers are null at this point? > Will this break if you call QueryDataUsage twice in a row quickly, like if you > enter the prefs page, leave it, then re-enter it? The Settings object is a singleton. It has the callback to the site breakdown and checks that that's not null before giving it the list. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:10: #include <memory> On 2017/04/07 20:44:16, sclittle wrote: > nit: Add include for <vector> Done. https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.h:122: base::android::ScopedJavaGlobalRef<jobject> j_settings_obj_; On 2017/04/07 20:21:58, dfalcantara (load balance plz) wrote: > If the Java side creates and owns this class, this should be a > JavaObjectWeakGlobalRef instead. AIUI ScopedJavaGlobalRefs are supposed to be > used when the native side owns the java side and is trying to prevent it from > ever being deallocated. Done.
build/android/lint rubberstamp lgtm based on review by twellington@ https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... File build/android/lint/suppressions.xml (right): https://codereview.chromium.org/2781323004/diff/270001/build/android/lint/sup... build/android/lint/suppressions.xml:437: <ignore regexp="chrome/android/java/res/layout/data_usage_breakdown.xml"/> On 2017/04/07 18:00:23, megjablon wrote: > On 2017/04/07 11:13:46, pasko wrote: > > I don't understand these suppressions and the reasons why they may be > necessary > > in this case. Please ask one of chrome/android/java/res/OWNERS to take another > > look at it. I can rubberstamp after that. > > twellington took a look and commented previously. It's necessary because the > TableLayout only has one child row and is populated dynamically. Thank you for explanation, I did not notice.
lgtm
chrome/browser/net/spdyproxy/* LGTM https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... File chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc (right): https://codereview.chromium.org/2781323004/diff/270001/chrome/browser/net/spd... chrome/browser/net/spdyproxy/data_reduction_proxy_settings_android.cc:253: Java_DataReductionProxySettings_createDataUseItemAndAddToList( On 2017/04/08 01:52:57, megjablon wrote: > On 2017/04/07 20:44:16, sclittle wrote: > > In order to send all the counts at once instead of calling into Java for each > > item, could you change the Java method signature to take in 3 arrays, one for > > each of hostnames, data_used counts, and original_size counts? > > > > You could use stuff like base::android::ToJavaLongArray(...) to construct > those > > arrays. WDYT? > > I'm not a huge fan of this option. It doesn't seem clean to send over three > different arrays where each index is expected to match up. Also we then have to > construct the objects in Java or keep passing around all three of the arrays > there. Acknowledged.
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, pasko@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2781323004/#ps350001 (title: "check that the results object is not null")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by megjablon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, twellington@chromium.org, dfalcantara@chromium.org, sclittle@chromium.org Link to the patchset: https://codereview.chromium.org/2781323004/#ps370001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 370001, "attempt_start_ts": 1491938132603340,
"parent_rev": "94ad9a25b35bb0bb2fbb488c40239f5ece40b277", "commit_rev":
"1956acf3a49b71307e53b33129dad628b64326e8"}
Message was sent while issue was closed.
Description was changed from ========== Create a new Data Saver settings page that adds the site breakdown The new Data Saver settings page moves the savings percent and stats above the graph, changes formatting, and adds a reset button and site breakdown. The site breakdown displays the top ten sites with the most data usage and their data savings. The compression stats in native are quieried and a list of DataReductionDataUseItem is passed to the view to display. BUG=615560 ========== to ========== Create a new Data Saver settings page that adds the site breakdown The new Data Saver settings page moves the savings percent and stats above the graph, changes formatting, and adds a reset button and site breakdown. The site breakdown displays the top ten sites with the most data usage and their data savings. The compression stats in native are quieried and a list of DataReductionDataUseItem is passed to the view to display. BUG=615560 Review-Url: https://codereview.chromium.org/2781323004 Cr-Commit-Position: refs/heads/master@{#463732} Committed: https://chromium.googlesource.com/chromium/src/+/1956acf3a49b71307e53b33129da... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:370001) as https://chromium.googlesource.com/chromium/src/+/1956acf3a49b71307e53b33129da... |
