Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2342443006:
[Offline pages] Use the new policy bits (Closed)
|
Created:
4 years, 3 months ago by chili Modified:
4 years, 2 months ago CC:
chromium-reviews, ntp-dev+reviews_chromium.org, 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. |
Description[Offline pages] Use the new policy bits
BUG=641053
Committed: https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb
Cr-Commit-Position: refs/heads/master@{#423402}
Patch Set 1 #Patch Set 2 : also rename isUserRequested in offline page model impl #
Total comments: 9
Patch Set 3 : code review update #
Total comments: 1
Patch Set 4 : add client policy controller to request coordinator and use it #Patch Set 5 : fix download bridge and ntp suggestions to use correct policy controller #
Total comments: 6
Patch Set 6 : Resolve code review comment #Patch Set 7 : test fix #Patch Set 8 : test fix #Patch Set 9 : fix memory leak caused by test fix. #Patch Set 10 : #Patch Set 11 : i think i got it! Made sure it compiles #Messages
Total messages: 59 (33 generated)
The CQ bit was checked by chili@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-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 chili@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.
chili@chromium.org changed reviewers: + dimich@chromium.org, fgorski@chromium.org, romax@chromium.org
Thought I sent this out last week. Oops. I've also a doc regarding the feature policy struct and client policy in general at https://docs.google.com/a/google.com/document/d/1mkPiaV9nSJlCu83plqHorro741jv... Thanks!
mostly looks good https://codereview.chromium.org/2342443006/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2342443006/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:77: DownloadUIAdapter::IsVisibleInUI(request->client_id())) { Have you considered calling the policy directly here? What would the trade-offs be? https://codereview.chromium.org/2342443006/diff/20001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2342443006/diff/20001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:49: return offline_pages::ClientPolicyController().IsSupportedByDownload( Does the comment/todo still make sense here? Could you please update it?
https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.h:261: // Check if |offline_page| is user-requested. I think the function name change would also affect the comment :) And would 'WillBe*' be better here? I'm bad at naming and I'm fine if it's okay to others :)
petewil@chromium.org changed reviewers: + petewil@chromium.org
https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.h:262: bool IsRemovedOnCacheReset(const OfflinePageItem& offline_page) const; Drive-by: Why rename the function? While the concept of whether a page is user-requested or not is not that important at the offline page model level, it is an important concept to the background offliner, which treats user-requested pages differently (with higher priority). If we rename it, I'm a bit concerned that somewhere we will be passing the value (is_removed_on_cache_reset) to a function taking (is_user_requested). I do understand that on the policy level, the only place that offline page model interacts with user requested pages is about whether they should be removed on cache reset.
https://codereview.chromium.org/2342443006/diff/20001/chrome/browser/android/... File chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc (right): https://codereview.chromium.org/2342443006/diff/20001/chrome/browser/android/... chrome/browser/android/offline_pages/downloads/offline_page_download_bridge.cc:77: DownloadUIAdapter::IsVisibleInUI(request->client_id())) { On 2016/09/19 22:18:54, fgorski wrote: > Have you considered calling the policy directly here? > What would the trade-offs be? At first I picked DownloadUIAdapter because this is the download bridge and it there was a reference to DownloadUIAdapter. Later, I realized the IsVisibleInUI method was static. In reality though, it's the same effect as calling the policy directly, minus an extra stack frame. Taking another look at this, though, this will actually create n policy controllers, where n is # of requests... made the change. https://codereview.chromium.org/2342443006/diff/20001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2342443006/diff/20001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:49: return offline_pages::ClientPolicyController().IsSupportedByDownload( On 2016/09/19 22:18:54, fgorski wrote: > Does the comment/todo still make sense here? Could you please update it? Done. https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.h:261: // Check if |offline_page| is user-requested. On 2016/09/19 23:00:24, romax wrote: > I think the function name change would also affect the comment :) > And would 'WillBe*' be better here? I'm bad at naming and I'm fine if it's okay > to others :) Updated comment. I'm inclined to keep the name as is, to be parallel with the naming in the ClientPolicyController...
https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.h:262: bool IsRemovedOnCacheReset(const OfflinePageItem& offline_page) const; On 2016/09/20 00:01:59, Pete Williamson wrote: > Drive-by: > > Why rename the function? While the concept of whether a page is user-requested > or not is not that important at the offline page model level, it is an important > concept to the background offliner, which treats user-requested pages > differently (with higher priority). If we rename it, I'm a bit concerned that > somewhere we will be passing the value (is_removed_on_cache_reset) to a function > taking (is_user_requested). > > I do understand that on the policy level, the only place that offline page model > interacts with user requested pages is about whether they should be removed on > cache reset. This came from comment on the original policy-bit CL (https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl...) But perhaps the solution is to add another bit for user requested...
https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... File components/offline_pages/offline_page_model_impl.h (right): https://codereview.chromium.org/2342443006/diff/20001/components/offline_page... components/offline_pages/offline_page_model_impl.h:262: bool IsRemovedOnCacheReset(const OfflinePageItem& offline_page) const; On 2016/09/20 00:06:21, chili wrote: > On 2016/09/20 00:01:59, Pete Williamson wrote: > > Drive-by: > > > > Why rename the function? While the concept of whether a page is > user-requested > > or not is not that important at the offline page model level, it is an > important > > concept to the background offliner, which treats user-requested pages > > differently (with higher priority). If we rename it, I'm a bit concerned that > > somewhere we will be passing the value (is_removed_on_cache_reset) to a > function > > taking (is_user_requested). > > > > I do understand that on the policy level, the only place that offline page > model > > interacts with user requested pages is about whether they should be removed on > > cache reset. > > This came from comment on the original policy-bit CL > (https://codereview.chromium.org/2289143005/diff/1/components/offline_pages/cl...) > But perhaps the solution is to add another bit for user requested... It seems that we can get rid of this method and use the policy check (the new implementation) where this method is called? The intention was not to write the namespace check at multiple places. Would it be better if we remove this helper function and use the policy_controller_->* here?
https://codereview.chromium.org/2342443006/diff/40001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2342443006/diff/40001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:47: return offline_pages::ClientPolicyController().IsSupportedByDownload( This is unorthodox way to use a constructor. A better way would be if either those methods were static, so you would do CPC::IsFoo(..), or use the existing OfflinePageModel::getPolicyController(). I think calling a ctor w/o 'new' is a dangerous thing...
Hi all, Reviving this CL after a long-winded loop. TL;DR is that trying to use the same client policy controller is hard and will cause us to rethink the bigger picture of how our code is laid out. For the time being, we can deal with having 2 client policy controllers and work on combining once we figure out the best layout/directory structure for code PTAL
friendly nudge :D PTAL
mostly looks good! sorry for the delayed response... however i still have the question about not using static methods to check for client_ids... sorry i just forgot the conclusion from the discussion... :S https://codereview.chromium.org/2342443006/diff/80001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2342443006/diff/80001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:53: return offline_page_model_->GetPolicyController()->IsSupportedByDownload( sorry i remembered we had a discussion on this but i forgot now... what was the reason we didn't want to have a static IsXXX(ClientId& client_id) method here?
https://codereview.chromium.org/2342443006/diff/80001/components/ntp_snippets... File components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc (right): https://codereview.chromium.org/2342443006/diff/80001/components/ntp_snippets... components/ntp_snippets/offline_pages/offline_page_suggestions_provider.cc:53: return offline_page_model_->GetPolicyController()->IsSupportedByDownload( On 2016/10/04 17:50:34, romax wrote: > sorry i remembered we had a discussion on this but i forgot now... what was the > reason we didn't want to have a static IsXXX(ClientId& client_id) method here? If it's static, we'd either --- Get the OfflinePageModel from keyed service to get the ClientPolicyController (which means we'd have to find the BrowserContext and depend on content/, which means stuff we'd have to change if we provide iOS support) --- Create a new ClientPolicyController (which means we aren't using the one on OfflinePageModel, which means if there are policy changes being applied, we won't be getting the latest version, and these kind of issues are hard to catch)
lgtm https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... File components/offline_pages/downloads/download_notifying_observer.h (right): https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... components/offline_pages/downloads/download_notifying_observer.h:45: explicit DownloadNotifyingObserver( no need for this to be marked explicit. https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:470: predicate.Run(id_page_pair.second.url)) please add {}, because the condition of if spans multiple lines.
https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... File components/offline_pages/downloads/download_notifying_observer.h (right): https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... components/offline_pages/downloads/download_notifying_observer.h:45: explicit DownloadNotifyingObserver( On 2016/10/04 18:28:50, fgorski wrote: > no need for this to be marked explicit. Done. https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... File components/offline_pages/offline_page_model_impl.cc (right): https://codereview.chromium.org/2342443006/diff/80001/components/offline_page... components/offline_pages/offline_page_model_impl.cc:470: predicate.Run(id_page_pair.second.url)) On 2016/10/04 18:28:50, fgorski wrote: > please add {}, because the condition of if spans multiple lines. Done.
lgtm
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2342443006/#ps100001 (title: "Resolve code review comment")
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-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 chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2342443006/#ps120001 (title: "test fix")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2342443006/#ps140001 (title: "test fix")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2342443006/#ps160001 (title: "fix memory leak caused by test fix.")
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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chili@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_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by chili@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 chili@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org, dimich@chromium.org Link to the patchset: https://codereview.chromium.org/2342443006/#ps200001 (title: "i think i got it! Made sure it compiles")
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.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Use the new policy bits BUG=641053 ========== to ========== [Offline pages] Use the new policy bits BUG=641053 Committed: https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb Cr-Commit-Position: refs/heads/master@{#423402} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f3de4523eff1856d2fe99c80a76bb7692d1e8beb Cr-Commit-Position: refs/heads/master@{#423402} |
