Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2388253002:
Use the previews black list for offline previews (Closed)
|
Created:
4 years, 2 months ago by RyanSturm Modified:
4 years, 2 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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse the previews black list for offline previews
This injects PreviewsDecider into the
OfflinePagesURLRequestInterceptor to be used in determining if URLs are
allowed to be shown an offline preview.
BUG=639087
Committed: https://crrev.com/2a6ce9d25ba082fbe28eb155dce045ca6d2ee124
Cr-Commit-Position: refs/heads/master@{#423591}
Patch Set 1 #
Total comments: 10
Patch Set 2 : jianli + tbansal comments #Patch Set 3 : git cl try #
Total comments: 6
Patch Set 4 : jianli comments #
Total comments: 1
Patch Set 5 : jianli comment #
Total comments: 11
Patch Set 6 : tbansal mmenke comments #
Total comments: 6
Patch Set 7 : mmenke comments #Depends on Patchset: Messages
Total messages: 59 (38 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...
ryansturm@chromium.org changed reviewers: + tbansal@chromium.org
tbansal: Can you give this a look?
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...)
jianli@chromium.org changed reviewers: + jianli@chromium.org
https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_interceptor.h (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_interceptor.h:38: previews::PreviewsBlackList* previews_black_list_; Who owns this? What's lifetime model for this? It just seems risky to pass a raw pointer without any hints. https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_job.cc:122: if (previews_black_list && Is it possible to merge this black list into NetworkQualityEstimator or introduce another new thing like PreviewEstimator that provides both NQE, black list and any other future addition? Otherwise for every place that calls NetworkQualityEstimator, we also need to pass this new black list. For example, do we also need to pass the black list to RequestCoordinator?
https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_interceptor.h (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_interceptor.h:38: previews::PreviewsBlackList* previews_black_list_; On 2016/10/03 22:56:05, jianli wrote: > Who owns this? What's lifetime model for this? It just seems risky to pass a raw > pointer without any hints. This is owned by PreviewsIOData, which is created and initialized for each profile before OfflinePageRequestInterceptor. Both of these are ultimately owned by ProfileImpl, and will both be destroyed during shutdown. This should be safe as a raw pointer, and I'm not sure of a better way to get this in besides dependency injection. The only other option would be to put it in URLRequestContext, which really makes no sense as previews have nothing to do with net/. I'm not sure what you mean by hints, but I can add a comment that the embedder should guarantee that |previews_black_list| outlives |this|. WDYT? https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_job.cc:122: if (previews_black_list && On 2016/10/03 22:56:05, jianli wrote: > Is it possible to merge this black list into NetworkQualityEstimator or > introduce another new thing like PreviewEstimator that provides both NQE, black > list and any other future addition? NQE is global, the blacklist is per profile. Embedding the blacklist in URLRequestContext would be a layering violation, and I'm not really sure how else we could get this here. > > Otherwise for every place that calls NetworkQualityEstimator, we also need to > pass this new black list. For example, do we also need to pass the black list to > RequestCoordinator? > > It shouldn't need to be plumbed anywhere else in OfflinePages. I haven't looked extensively at RequestCoordinator, but the NQE usage in it isn't related to previews AFAIK.
https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_interceptor.h (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_interceptor.h:27: OfflinePageRequestInterceptor( explicit https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_job.cc:123: !previews_black_list->IsLoadedAndAllowed( I suppose the part where the black list is notified of navigation completions (and opt-outs) will come later? https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_job.h:98: // Used to determine if an URLRequest is eligible for offline previews. Comment on the lifetime: e.g., is it possible that |previews_black_list_| changes to null during the lifetime of |this|? If yes, then this should be a weak ptr.
Since you both raised the same concern, I will switch to passing in a WeakPtr to PreviewsIOData (same lifetime, managed by Profile), and I will access the blacklist from that within the URLRequestJob. That will allow for more flexibility with future changes to the lifetime management of other objects.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 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...
Patchset #3 (id:40001) has been deleted
jianli, tbansal: PTAL @ * https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_interceptor.h (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_interceptor.h:27: OfflinePageRequestInterceptor( On 2016/10/04 12:38:04, tbansal1 wrote: > explicit Done. https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_job.cc:123: !previews_black_list->IsLoadedAndAllowed( On 2016/10/04 12:38:04, tbansal1 wrote: > I suppose the part where the black list is notified of navigation completions > (and opt-outs) will come later? Yes. https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/offline_page_request_job.h (right): https://codereview.chromium.org/2388253002/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/offline_page_request_job.h:98: // Used to determine if an URLRequest is eligible for offline previews. On 2016/10/04 12:38:04, tbansal1 wrote: > Comment on the lifetime: e.g., is it possible that |previews_black_list_| > changes to null during the lifetime of |this|? If yes, then this should be a > weak ptr. Done.
Please also update the issue description. https://codereview.chromium.org/2388253002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2388253002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:202: class TestPreviewsDecider : public previews::PreviewsDecider { Since you're overriding PreviewsDecider, I think you can remove ScopedEnableProbihibitivelySlowNetwork and make TestPreviewsDecider directly control turning on preview offline. https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... components/previews/core/previews_decider.h:23: PreviewsType type) = 0; nit: add const modifier https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.h:60: bool ShouldAllowPreview(net::URLRequest* request, PreviewsType type) override; Since you're not going to change request object, probably adding "const" to its type is better.
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
jianli, tbansal: PTAL, thanks! https://codereview.chromium.org/2388253002/diff/60001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2388253002/diff/60001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:202: class TestPreviewsDecider : public previews::PreviewsDecider { On 2016/10/05 01:13:02, jianli wrote: > Since you're overriding PreviewsDecider, I think you can remove > ScopedEnableProbihibitivelySlowNetwork and make TestPreviewsDecider directly > control turning on preview offline. Done. https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... components/previews/core/previews_decider.h:23: PreviewsType type) = 0; On 2016/10/05 01:13:02, jianli wrote: > nit: add const modifier Done. https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2388253002/diff/60001/components/previews/cor... components/previews/core/previews_io_data.h:60: bool ShouldAllowPreview(net::URLRequest* request, PreviewsType type) override; On 2016/10/05 01:13:02, jianli wrote: > Since you're not going to change request object, probably adding "const" to its > type is better. Done.
Description was changed from ========== Use the previews black list for offline previews This injects the Previews black list into the OfflinePagesURLRequestInterceptor to be used in determining if URLs are allowed to be shown an offline preview. BUG=639087 ========== to ========== Use the previews black list for offline previews This injects PreviewsDecider into the OfflinePagesURLRequestInterceptor to be used in determining if URLs are allowed to be shown an offline preview. BUG=639087 ==========
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...)
lgtm https://codereview.chromium.org/2388253002/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc (right): https://codereview.chromium.org/2388253002/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_request_job_unittest.cc:572: test_previews_decider()->set_should_allow_preview(true); nit: might be better to restore its value at the end of the function in case that multiple test cases are run together.
ryansturm@chromium.org changed reviewers: + mmenke@chromium.org
mmenke: PTAL @ profile_impl_io_data and previews/core/DEPS adding //net tbansal: PTAL Thanks!
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...)
https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:124: previews_decider->ShouldAllowPreview(*request, Can you elaborate a bit more here or at some other appropriate place. e.g., is the blacklist consulted only when the connection is slow (but not actually offline)? https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:553: previews_io_data()->PreviewsDeciderWeakPtr()))); Is there a guarantee that previews decider will outlive the OfflinePageRequestInterceptor? I am asking because generally it is considered an anti-pattern to expose weak ptrs publically. https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... components/previews/core/previews_decider.h:18: PreviewsDecider() {} you can make the ctor and dtor protected. https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... components/previews/core/previews_io_data.h:56: return weak_factory_.GetWeakPtr(); This should be in .cc file protected by some sort of thread checker. Also, it is not clear why it is public.
https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... components/previews/core/previews_io_data.cc:76: !previews_black_list_->IsLoadedAndAllowed(request.url(), type)) { Also, how does a host that is blacklisted gets off the blacklist? e.g., one naive approach could be that show a previews with probability = 0.1 (just an example) even if the host is blacklisted. Then, depending on the user's opt-out interaction, the host may automatically be removed from the blacklist. This is probably outside the scope of this CL, but in that case it might be useful to add a TODO here.
https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:553: previews_io_data()->PreviewsDeciderWeakPtr()))); On 2016/10/06 14:51:04, tbansal1 wrote: > Is there a guarantee that previews decider will outlive the > OfflinePageRequestInterceptor? I am asking because generally it is considered an > anti-pattern to expose weak ptrs publically. +1 to not exposing a WeakPtr this way. Can we pass ownership instead (And while we're at it, we could even remove PreviewsIOData from ProfileIOData, and instead make it part of LazyParams, I think? We'd have to create it a little later, if that's not a problem. If it is, we could at least move it to ProfileImplIOData from ProfileIOData)
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 review, PTAL https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/android... File chrome/browser/android/offline_pages/offline_page_request_job.cc (right): https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/android... chrome/browser/android/offline_pages/offline_page_request_job.cc:124: previews_decider->ShouldAllowPreview(*request, On 2016/10/06 14:51:04, tbansal1 wrote: > Can you elaborate a bit more here or at some other appropriate place. e.g., is > the blacklist consulted only when the connection is slow (but not actually > offline)? Done. https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2388253002/diff/100001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:553: previews_io_data()->PreviewsDeciderWeakPtr()))); On 2016/10/06 15:02:46, mmenke wrote: > On 2016/10/06 14:51:04, tbansal1 wrote: > > Is there a guarantee that previews decider will outlive the > > OfflinePageRequestInterceptor? I am asking because generally it is considered > an > > anti-pattern to expose weak ptrs publically. > > +1 to not exposing a WeakPtr this way. Can we pass ownership instead (And while > we're at it, we could even remove PreviewsIOData from ProfileIOData, and instead > make it part of LazyParams, I think? We'd have to create it a little later, if > that's not a problem. If it is, we could at least move it to ProfileImplIOData > from ProfileIOData) The PreviewsIOData is used in more places than just offline pages (or it will be), so I'll move it to ProfileImplIOData, as I don't think it will be needed on ProfileIOData. I'll pass it as a raw pointer and add a comment about the embedder guaranteeing it outlives the interceptor. https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... File components/previews/core/previews_decider.h (right): https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... components/previews/core/previews_decider.h:18: PreviewsDecider() {} On 2016/10/06 14:51:04, tbansal1 wrote: > you can make the ctor and dtor protected. Done. https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... File components/previews/core/previews_io_data.cc (right): https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... components/previews/core/previews_io_data.cc:76: !previews_black_list_->IsLoadedAndAllowed(request.url(), type)) { On 2016/10/06 14:55:52, tbansal1 wrote: > Also, how does a host that is blacklisted gets off the blacklist? e.g., one > naive approach could be that show a previews with probability = 0.1 (just an > example) even if the host is blacklisted. Then, depending on the user's opt-out > interaction, the host may automatically be removed from the blacklist. > > This is probably outside the scope of this CL, but in that case it might be > useful to add a TODO here. It's a timing mechanism (90 days I believe) controlled by field trial. If the host's last opt out was x days ago, we allow the site to be shown. https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... File components/previews/core/previews_io_data.h (right): https://codereview.chromium.org/2388253002/diff/100001/components/previews/co... components/previews/core/previews_io_data.h:56: return weak_factory_.GetWeakPtr(); On 2016/10/06 14:51:04, tbansal1 wrote: > This should be in .cc file protected by some sort of thread checker. > > Also, it is not clear why it is public. Done.
lgtm https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:819: std::unique_ptr<previews::PreviewsIOData> previews_io_data) const { Can this really be a const function?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM, modulo comments. https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.h:208: } I don't think we need this method - it's not used externally, so can just use previews_io_data_.get() inline. https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.h:211: std::unique_ptr<previews::PreviewsIOData> previews_io_data) const; Again, don't think we need this - can just set it explicitly. Handle is a friend, and Handle::Init already reaches int the ProfileImpleIOData's guts.
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...
https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:819: std::unique_ptr<previews::PreviewsIOData> previews_io_data) const { On 2016/10/06 16:32:33, tbansal1 wrote: > Can this really be a const function? mutable. It's going away anyway. https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.h (right): https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.h:208: } On 2016/10/06 16:57:45, mmenke wrote: > I don't think we need this method - it's not used externally, so can just use > previews_io_data_.get() inline. Done. https://codereview.chromium.org/2388253002/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.h:211: std::unique_ptr<previews::PreviewsIOData> previews_io_data) const; On 2016/10/06 16:57:45, mmenke wrote: > Again, don't think we need this - can just set it explicitly. Handle is a > friend, and Handle::Init already reaches int the ProfileImpleIOData's guts. Good point. Didn't think enough about the refactor.
profiles/ LGTM, thanks for the refactors!
The CQ bit was unchecked by ryansturm@chromium.org
The CQ bit was checked by ryansturm@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org, tbansal@chromium.org Link to the patchset: https://codereview.chromium.org/2388253002/#ps140001 (title: "mmenke 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 ========== Use the previews black list for offline previews This injects PreviewsDecider into the OfflinePagesURLRequestInterceptor to be used in determining if URLs are allowed to be shown an offline preview. BUG=639087 ========== to ========== Use the previews black list for offline previews This injects PreviewsDecider into the OfflinePagesURLRequestInterceptor to be used in determining if URLs are allowed to be shown an offline preview. BUG=639087 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Use the previews black list for offline previews This injects PreviewsDecider into the OfflinePagesURLRequestInterceptor to be used in determining if URLs are allowed to be shown an offline preview. BUG=639087 ========== to ========== Use the previews black list for offline previews This injects PreviewsDecider into the OfflinePagesURLRequestInterceptor to be used in determining if URLs are allowed to be shown an offline preview. BUG=639087 Committed: https://crrev.com/2a6ce9d25ba082fbe28eb155dce045ca6d2ee124 Cr-Commit-Position: refs/heads/master@{#423591} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/2a6ce9d25ba082fbe28eb155dce045ca6d2ee124 Cr-Commit-Position: refs/heads/master@{#423591} |
