[Offline pages] Use the new policy bits

components/offline_pages/offline_page_model_impl.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_MODEL_IMPL_H_
 #define COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_MODEL_IMPL_H_
 
 #include <stdint.h>
 
 #include <map>
@@ skipping 240 matching lines @@
   void ClearStorageIfNeeded(
       const OfflinePageStorageManager::ClearStorageCallback& callback);
 
   // Callback completing storage clearing.
   void OnStorageCleared(size_t expired_page_count,
                         OfflinePageStorageManager::ClearStorageResult result);
 
   // Post task to clear storage.
   void PostClearStorageIfNeededTask();
 
   // Check if |offline_page| is user-requested.

[romax, 2016/09/19 23:00:24]

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 :)

[chili, 2016/09/20 00:02:33]

Updated comment.

I'm inclined to keep the name as is, to be parallel with the naming in the
ClientPolicyController... 

-  bool IsUserRequestedPage(const OfflinePageItem& offline_page) const;
+  bool IsRemovedOnCacheReset(const OfflinePageItem& offline_page) const;

[Pete Williamson, 2016/09/20 00:01:59]

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.

[chili, 2016/09/20 00:06:21]

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...

[romax, 2016/09/20 00:19:01]

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?

 
   void RunWhenLoaded(const base::Closure& job);
 
   base::Time GetCurrentTime() const;
 
   // Persistent store for offline page metadata.
   std::unique_ptr<OfflinePageMetadataStore> store_;
 
   // Location where all of the archive files will be stored.
   base::FilePath archives_dir_;
@@ skipping 30 matching lines @@
   base::Clock* testing_clock_;
 
   base::WeakPtrFactory<OfflinePageModelImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OfflinePageModelImpl);
 };
 
 }  // namespace offline_pages
 
 #endif  // COMPONENTS_OFFLINE_PAGES_OFFLINE_PAGE_MODEL_IMPL_H_