Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2166363003:
Offline pages using NQE 2G Slow (Closed)
|
Created:
4 years, 5 months ago by RyanSturm Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOffline pages using NQE 2G Slow
This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a
FieldTrial.
BUG=615563
Committed: https://crrev.com/9d5928c1f4cdfc2ce617c8c4c0c925951a5eb861
Cr-Commit-Position: refs/heads/master@{#408511}
Patch Set 1 #Patch Set 2 : nits #Patch Set 3 : Added tests and field trial #Patch Set 4 : changes in switch statement #Patch Set 5 : changed the field trial name and logic #Patch Set 6 : clarifying comment #
Total comments: 18
Patch Set 7 : tbansal nits (switching to consuming previews component) #
Total comments: 21
Patch Set 8 : Addressing tbansal's comments (and moving clock to delegate) #Patch Set 9 : removing unneeded comment #Patch Set 10 : rebasing, renaming method to match rebase #
Total comments: 8
Patch Set 11 : thestig comments #Depends on Patchset: Messages
Total messages: 72 (52 generated)
The CQ bit was checked by ryansturm@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 ryansturm@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by ryansturm@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 ryansturm@chromium.org to run a CQ dry run
Description was changed from ========== Draft of Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prhibitively slow network. BUG= ========== to ========== Draft of Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a FieldTrial. BUG=615563 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Draft of Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a FieldTrial. BUG=615563 ========== to ========== Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a FieldTrial. BUG=615563 ==========
The CQ bit was checked by ryansturm@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...
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: PTAL at *, thanks
https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:280: RedirectResult::PAGE_NOT_FRESH_ON_PROHIBITVELY_SLOW_NETWORK); Why is this needed? https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:25: void SetEffectiveConnectionTypeForTest( s/ForTest/ForTesting/ https://cs.chromium.org/chromium/src/styleguide/c%2B%2B/c%2B%2B.md?rcl=0&l=41 https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:131: FieldTrialList::FindFullName(kClientSideExperimentsFieldTrial) Instead of checking the group name, I think you should read the field trial params, and see if value of param "show_offline_pages" is 1 or not. https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:80: bool IsIncludedInOfflinePagesSlowConnectionFieldTrial(); This should not be in DRP (Offline can be enabled when DRP is off). Use some more general name for field trial, and move the field trial to somewhere in chrome/browser/ chrome/browser/io_thread.cc might be okay if there is no better place.
More comments. https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:118: if (data_reduction_proxy::params:: Might be cleaner to move this to a separate function. https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:125: net::NetworkQualityEstimator::EFFECTIVE_CONNECTION_TYPE_SLOW_2G) { Instead of doing this, I will do something like ect = nqe::GetEct(); if(ect >=_OFFLINE && ect <= _SLOW_2G) { // do something } https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:231: result == RedirectResult::REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK); May be: s/PROHIBITVELY_// bengr likes PROHIBITVELY_ but I do not. I think it is redundant :) https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:259: break; Why not return here? https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:61: REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK = 10, What's the difference between Flaky network and Slow network? May be add some comments (at least for the new enum values that you added).
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 ryansturm@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...
tbansal: PTAL https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:118: if (data_reduction_proxy::params:: On 2016/07/25 21:43:22, tbansal1 wrote: > Might be cleaner to move this to a separate function. Done. https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:125: net::NetworkQualityEstimator::EFFECTIVE_CONNECTION_TYPE_SLOW_2G) { On 2016/07/25 21:43:22, tbansal1 wrote: > Instead of doing this, I will do something like > ect = nqe::GetEct(); > if(ect >=_OFFLINE && ect <= _SLOW_2G) { > // do something > } I'm using the NCN::IsOffline above; either way I'd have to change the RedirectResult, so I will do (ect > offline && ect <= slow_2g) https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:231: result == RedirectResult::REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK); On 2016/07/25 21:43:22, tbansal1 wrote: > May be: s/PROHIBITVELY_// > bengr likes PROHIBITVELY_ but I do not. I think it is redundant :) I think slow is <4G, but prohibitively slow is the point where I would not bother using my phone anymore (slow 2g), so I don't find it redundant. https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:259: break; On 2016/07/25 21:43:22, tbansal1 wrote: > Why not return here? Done. https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:280: RedirectResult::PAGE_NOT_FRESH_ON_PROHIBITVELY_SLOW_NETWORK); On 2016/07/25 21:35:26, tbansal1 wrote: > Why is this needed? It gives us the amount of times that we had a slow network and the page was not fresh enough. Every other case is accounted for in this UMA, and I think there is a difference between page not found and page not fresh enough for slow network. I'm willing to turn this into PAGE_NOT_FOUND_ON_SLOW_NETWORK if you think that is more sound. https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:61: REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK = 10, On 2016/07/25 21:43:22, tbansal1 wrote: > What's the difference between Flaky network and Slow network? > > May be add some comments (at least for the new enum values that you added). Done. https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/net/nqe... File chrome/browser/net/nqe/ui_network_quality_estimator_service.h (right): https://codereview.chromium.org/2166363003/diff/100001/chrome/browser/net/nqe... chrome/browser/net/nqe/ui_network_quality_estimator_service.h:25: void SetEffectiveConnectionTypeForTest( On 2016/07/25 21:35:26, tbansal1 wrote: > s/ForTest/ForTesting/ > https://cs.chromium.org/chromium/src/styleguide/c%2B%2B/c%2B%2B.md?rcl=0&l=41 Done. https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:131: FieldTrialList::FindFullName(kClientSideExperimentsFieldTrial) On 2016/07/25 21:35:26, tbansal1 wrote: > Instead of checking the group name, I think you should read the field trial > params, and see if value of param "show_offline_pages" is 1 or not. Done. https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.h (right): https://codereview.chromium.org/2166363003/diff/100001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.h:80: bool IsIncludedInOfflinePagesSlowConnectionFieldTrial(); On 2016/07/25 21:35:26, tbansal1 wrote: > This should not be in DRP (Offline can be enabled when DRP is off). > Use some more general name for field trial, and move the field trial to > somewhere in chrome/browser/ > > chrome/browser/io_thread.cc might be okay if there is no better place. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:52: if (!previews::IsIncludedInOfflinePagesSlowConnectionFieldTrial()) { braces not needed :) https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:62: if (effective_connection_type > s/>/>=/ https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:287: base::TimeDelta::FromDays(1)) { Where else is this being "freshness test" done? Would it make sense to move "base::TimeDelta::FromDays(1)" to a common const to ensure every body is using the same duration? https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:64: // connection of 2G slow. Do not put "2G slow" in the comment as NQE could have returned E_C_T_OFFLINE. May be simply saying that these enums are used when offline page was used because NQE reported the network as slow. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:65: REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK = 10, s/TV/TIV/ https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:385: std::map<std::string, std::string> params; #include <map> #include <string> https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:386: params["show_offline_pages"] = "1"; s/1/true/ https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:388: "DataReductionProxyClientSideExperimentsFieldTrial", "Enabled", params)); You can expose this from the previews component, and call that function here directly. https://codereview.chromium.org/2166363003/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2166363003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:121: This seems unrelated to this CL.
dimich@chromium.org changed reviewers: + dimich@chromium.org
https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:77: void SetClockForTesting(std::unique_ptr<base::Clock> clock); Drive-by: Note how we use DefaultDelegate to inject dependencies into this class. Tests will then create their own Delegate and call SetDelegateForTest(). This way, all test code is in Delegate.
tbansal: PTAL, thanks https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:52: if (!previews::IsIncludedInOfflinePagesSlowConnectionFieldTrial()) { On 2016/07/26 20:12:48, tbansal1 wrote: > braces not needed :) Done. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:62: if (effective_connection_type > On 2016/07/26 20:12:48, tbansal1 wrote: > s/>/>=/ I'll make the change, but does this differ from NCN::IsOffline? https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:287: base::TimeDelta::FromDays(1)) { On 2016/07/26 20:12:48, tbansal1 wrote: > Where else is this being "freshness test" done? Would it make sense to move > "base::TimeDelta::FromDays(1)" to a common const to ensure every body is using > the same duration? We are using 1 day for freshness based on different requirements for slow networks. The database keeps entries for weeks before evicting them. This is the only place that 1 day is used for freshness. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:64: // connection of 2G slow. On 2016/07/26 20:12:48, tbansal1 wrote: > Do not put "2G slow" in the comment as NQE could have returned E_C_T_OFFLINE. > May be simply saying that these enums are used when offline page was used > because NQE reported the network as slow. Done. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:65: REDIRECTED_ON_PROHIBITVELY_SLOW_NETWORK = 10, On 2016/07/26 20:12:48, tbansal1 wrote: > s/TV/TIV/ Done. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:77: void SetClockForTesting(std::unique_ptr<base::Clock> clock); On 2016/07/26 21:10:36, Dmitry Titov wrote: > Drive-by: Note how we use DefaultDelegate to inject dependencies into this > class. Tests will then create their own Delegate and call SetDelegateForTest(). > This way, all test code is in Delegate. Done. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:385: std::map<std::string, std::string> params; On 2016/07/26 20:12:48, tbansal1 wrote: > #include <map> > #include <string> Acknowledged. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:386: params["show_offline_pages"] = "1"; On 2016/07/26 20:12:48, tbansal1 wrote: > s/1/true/ Acknowledged. https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:388: "DataReductionProxyClientSideExperimentsFieldTrial", "Enabled", params)); On 2016/07/26 20:12:48, tbansal1 wrote: > You can expose this from the previews component, and call that function here > directly. Done. https://codereview.chromium.org/2166363003/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc (right): https://codereview.chromium.org/2166363003/diff/120001/components/data_reduct... components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:121: On 2016/07/26 20:12:48, tbansal1 wrote: > This seems unrelated to this CL. Done. :(
The CQ bit was checked by ryansturm@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 ryansturm@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...
lgtm https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/120001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:287: base::TimeDelta::FromDays(1)) { On 2016/07/26 21:48:12, RyanSturm wrote: > On 2016/07/26 20:12:48, tbansal1 wrote: > > Where else is this being "freshness test" done? Would it make sense to move > > "base::TimeDelta::FromDays(1)" to a common const to ensure every body is using > > the same duration? > > We are using 1 day for freshness based on different requirements for slow > networks. The database keeps entries for weeks before evicting them. This is the > only place that 1 day is used for freshness. Acknowledged.
dimich: PTAL, thanks
lgtm, thanks for updating!
Thanks for the review, I'll have to wait to land the previews CL before landing this.
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 ryansturm@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 ryansturm@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 ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2166363003/#ps170001 (title: "rebasing, renaming method to match rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
thestig: PTAL @ chrome/browser/BUILD.gn and chrome/chrome_browser.gypi (adding dependency for new component, previews).
ryansturm@chromium.org changed reviewers: + thestig@chromium.org
thestig: PTAL @ chrome/browser/BUILD.gn and chrome/chrome_browser.gypi (adding dependency for new component, previews).
lgtm https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:59: if (effective_connection_type >= if (comparision_evals_to_bool) return true; return false; is the same as: return comparision_evals_to_bool; https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: virtual base::Time Now() const = 0; a) I'm not sure you can forward declare if it's returning an object. I thought it only worked for pointers to the forward declared object. b) https://google.github.io/styleguide/cppguide.html#Forward_Declarations at some point decided to just include the header. (I didn't make the decision, but that's what it says) https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:389: EXPECT_EQ(nullptr, item); Maybe just EXPECT_FALSE(item); ? https://codereview.chromium.org/2166363003/diff/170001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3441: '../components/components.gyp:previews', alphabetical order please
The CQ bit was checked by ryansturm@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...
Thanks for the quick review. https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.cc (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.cc:59: if (effective_connection_type >= On 2016/07/28 20:41:52, Lei Zhang wrote: > if (comparision_evals_to_bool) > return true; > return false; > > is the same as: > > return comparision_evals_to_bool; Done. https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper.h (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper.h:39: virtual base::Time Now() const = 0; On 2016/07/28 20:41:52, Lei Zhang wrote: > a) I'm not sure you can forward declare if it's returning an object. I thought > it only worked for pointers to the forward declared object. > > b) https://google.github.io/styleguide/cppguide.html#Forward_Declarations at > some point decided to just include the header. (I didn't make the decision, but > that's what it says) Done. You're absolutely right. Time is included via offline_page_types.h, offline_page_item.h. This wouldn't compile without the actual header include. https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_tab_helper_unittest.cc:389: EXPECT_EQ(nullptr, item); On 2016/07/28 20:41:52, Lei Zhang wrote: > Maybe just EXPECT_FALSE(item); ? Done. https://codereview.chromium.org/2166363003/diff/170001/chrome/chrome_browser.... File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/2166363003/diff/170001/chrome/chrome_browser.... chrome/chrome_browser.gypi:3441: '../components/components.gyp:previews', On 2016/07/28 20:41:52, Lei Zhang wrote: > alphabetical order please Done.
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 ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, dimich@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2166363003/#ps190001 (title: "thestig comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a FieldTrial. BUG=615563 ========== to ========== Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a FieldTrial. BUG=615563 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a FieldTrial. BUG=615563 ========== to ========== Offline pages using NQE 2G Slow This allows use of the offline pages feature when on a prohibitively slow network. This feature is behind a FieldTrial. BUG=615563 Committed: https://crrev.com/9d5928c1f4cdfc2ce617c8c4c0c925951a5eb861 Cr-Commit-Position: refs/heads/master@{#408511} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/9d5928c1f4cdfc2ce617c8c4c0c925951a5eb861 Cr-Commit-Position: refs/heads/master@{#408511} |
