[Offline pages] Removing obsolete TODOs as part of PE fixit

Removing TODOs, which are obsolete or which make no sense to act upon
at this point in time.

BUG=685523
R=chili@chromium.org

Review-Url: https://codereview.chromium.org/2782673002
Cr-Commit-Position: refs/heads/master@{#460562}

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java

@@ -297,8 +297,6 @@ public class OfflinePageTabObserver
 
     void reinitialize(Context context, TabModel tabModel, SnackbarManager manager,
             SnackbarController controller) {
-        // TODO(fgorski): Work out if we need to also update network changes observer with the
-        // context change.
         mContext = context;
         mSnackbarManager = manager;
         mSnackbarController = controller;

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java

@@ -159,12 +159,10 @@ public class OfflinePageUtils {
         WebContents webContents = tab.getWebContents();
         ClientId clientId = ClientId.createClientIdForBookmarkId(bookmarkId);
 
-        // TODO(fgorski): Ensure that request is queued if the model is not loaded.
         offlinePageBridge.savePage(webContents, clientId, new OfflinePageBridge.SavePageCallback() {
             @Override
             public void onSavePageDone(int savePageResult, String url, long offlineId) {
-                // TODO(fgorski): Decide if we need to do anything with result.
-                // Perhaps some UMA reporting, but that can really happen someplace else.
+                // Result of the call is ignored.
             }
         });
     }

chrome/android/java/src/org/chromium/chrome/browser/offlinepages/downloads/OfflinePageDownloadItem.java

@@ -60,8 +60,6 @@ public class OfflinePageDownloadItem {
     }
 
     /** @return Path to the offline item on the disk. */
-    // TODO(fgorski): Title would be more meaningful to show in the Download UI, where the local
-    // path is shown right now.
     public String getTargetPath() {
         return mTargetPath;
     }

chrome/browser/android/offline_pages/evaluation/offline_page_evaluation_bridge.cc

@@ -146,8 +146,6 @@ std::unique_ptr<KeyedService> GetTestingRequestCoordinator(
   net::NetworkQualityEstimator::NetworkQualityProvider*
       network_quality_estimator =
           UINetworkQualityEstimatorServiceFactory::GetForProfile(profile);
-  // TODO(fgorski): Something needs to keep the handle to the Notification
-  // dispatcher.
   std::unique_ptr<RequestCoordinator> request_coordinator =
       base::MakeUnique<RequestCoordinator>(
           std::move(policy), std::move(offliner), std::move(queue),

chrome/browser/android/offline_pages/offline_page_mhtml_archiver.cc

@@ -87,9 +87,6 @@ void OfflinePageMHTMLArchiver::GenerateMHTML(
     return;
   }
 
-  // TODO(fgorski): Figure out if the actual URL can be different at
-  // the end of MHTML generation. Perhaps we should pull it out after the MHTML
-  // is generated.
   GURL url(web_contents_->GetLastCommittedURL());
   base::string16 title(web_contents_->GetTitle());
   base::FilePath file_path(

chrome/browser/android/offline_pages/request_coordinator_factory.cc

@@ -82,8 +82,6 @@ KeyedService* RequestCoordinatorFactory::BuildServiceInstanceFor(
   net::NetworkQualityEstimator::NetworkQualityProvider*
       network_quality_estimator =
           UINetworkQualityEstimatorServiceFactory::GetForProfile(profile);
-  // TODO(fgorski): Something needs to keep the handle to the Notification
-  // dispatcher.
   RequestCoordinator* request_coordinator = new RequestCoordinator(
       std::move(policy), std::move(offliner), std::move(queue),
       std::move(scheduler), network_quality_estimator);

components/offline_pages/core/background/request_queue.cc

@@ -31,10 +31,6 @@ void GetRequestsDone(const RequestQueue::GetRequestsCallback& callback,
                      std::vector<std::unique_ptr<SavePageRequest>> requests) {
   GetRequestsResult result =
       success ? GetRequestsResult::SUCCESS : GetRequestsResult::STORE_FAILURE;
-  // TODO(fgorski): Filter out expired requests based on policy.
-  // This may trigger the purging if necessary.
-  // Also this may be turned into a method on the request queue or add a policy
-  // parameter in the process.
   callback.Run(result, std::move(requests));
 }
 
@@ -78,8 +74,6 @@ void RequestQueue::GetRequests(const GetRequestsCallback& callback) {
 
 void RequestQueue::AddRequest(const SavePageRequest& request,
                               const AddRequestCallback& callback) {
-  // TODO(fgorski): check that request makes sense.
-  // TODO(fgorski): check that request does not violate policy.
   std::unique_ptr<AddRequestTask> task(new AddRequestTask(
       store_.get(), request, base::Bind(&AddRequestDone, callback, request)));
   task_queue_.AddTask(std::move(task));

components/offline_pages/core/background/request_queue_store_sql.cc

@@ -97,7 +97,7 @@ bool CreateSchema(sql::Connection* db) {
       return false;
   }
 
-  // TODO(fgorski): Add indices here.
+  // This would be a great place to add indices when we need them.
   return transaction.Commit();
 }
 
@@ -294,7 +294,6 @@ void GetRequestsByIdsSync(sql::Connection* db,
                           scoped_refptr<base::SingleThreadTaskRunner> runner,
                           const std::vector<int64_t>& request_ids,
                           const RequestQueueStore::UpdateCallback& callback) {
-  // TODO(fgorski): Perhaps add metrics here.
   std::unique_ptr<UpdateRequestsResult> result(
       new UpdateRequestsResult(StoreState::LOADED));
 
@@ -332,7 +331,6 @@ void AddRequestSync(sql::Connection* db,
                     scoped_refptr<base::SingleThreadTaskRunner> runner,
                     const SavePageRequest& request,
                     const RequestQueueStore::AddCallback& callback) {
-  // TODO(fgorski): add UMA metrics here.
   ItemActionStatus status = Insert(db, request);
   runner->PostTask(FROM_HERE, base::Bind(callback, status));
 }
@@ -341,7 +339,6 @@ void UpdateRequestsSync(sql::Connection* db,
                         scoped_refptr<base::SingleThreadTaskRunner> runner,
                         const std::vector<SavePageRequest>& requests,
                         const RequestQueueStore::UpdateCallback& callback) {
-  // TODO(fgorski): add UMA metrics here.
   std::unique_ptr<UpdateRequestsResult> result(
       new UpdateRequestsResult(StoreState::LOADED));
 
@@ -371,7 +368,6 @@ void RemoveRequestsSync(sql::Connection* db,
                         scoped_refptr<base::SingleThreadTaskRunner> runner,
                         const std::vector<int64_t>& request_ids,
                         const RequestQueueStore::UpdateCallback& callback) {
-  // TODO(fgorski): Perhaps add metrics here.
   std::unique_ptr<UpdateRequestsResult> result(
       new UpdateRequestsResult(StoreState::LOADED));
 

components/offline_pages/core/background/request_queue_unittest.cc

@@ -85,7 +85,6 @@ class RequestNotifierStub : public RequestNotifier {
   int32_t total_expired_requests_;
 };
 
-// TODO(fgorski): Add tests for store failures in add/remove/get.
 class RequestQueueTest : public testing::Test {
  public:
   RequestQueueTest();

components/offline_pages/core/background/save_page_request.cc

@@ -65,9 +65,6 @@ bool SavePageRequest::operator==(const SavePageRequest& other) const {
 
 void SavePageRequest::MarkAttemptStarted(const base::Time& start_time) {
   DCHECK_LE(activation_time_, start_time);
-  // TODO(fgorski): As part of introducing policy in GetStatus, we can make a
-  // check here to ensure we only start tasks in status pending, and bail out in
-  // other cases.
   last_attempt_time_ = start_time;
   ++started_attempt_count_;
   state_ = RequestState::OFFLINING;

components/offline_pages/core/offline_page_archiver.h

@@ -39,12 +39,6 @@ namespace offline_pages {
 // archiver whether to respond with ERROR_CONTENT_UNAVAILBLE, wait longer to
 // actually snapshot a complete page, or snapshot whatever is available at that
 // point in time (what the user sees).
-//
-// TODO(fgorski): Add ability to delete archive.
-// TODO(fgorski): Add ability to check that archive exists.
-// TODO(fgorski): Add ability to refresh an existing archive in one step.
-// TODO(fgorski): Add ability to identify all of the archives in the directory,
-// to enable to model to reconcile the archives.
 class OfflinePageArchiver {
  public:
   // Results of the archive creation.

components/offline_pages/core/offline_page_metadata_store_impl_unittest.cc

@@ -392,8 +392,6 @@ void OfflinePageMetadataStoreTest::GetOfflinePagesCallback(
 
 void OfflinePageMetadataStoreTest::AddCallback(ItemActionStatus status) {
   last_called_callback_ = ADD;
-  // TODO(fgorski): Add specific add status.
-  // last_item_status_ = status;
   last_status_ =
       status == ItemActionStatus::SUCCESS ? STATUS_TRUE : STATUS_FALSE;
 }

components/offline_pages/core/offline_page_metadata_store_sql.cc

@@ -158,7 +158,7 @@ bool CreateSchema(sql::Connection* db) {
       return false;
   }
 
-  // TODO(fgorski): Add indices here.
+  // This would be a great place to add indices when we need them.
   return transaction.Commit();
 }
 
@@ -292,7 +292,6 @@ void NotifyLoadResult(scoped_refptr<base::SingleThreadTaskRunner> runner,
                       const OfflinePageMetadataStore::LoadCallback& callback,
                       OfflinePageMetadataStore::LoadStatus status,
                       const std::vector<OfflinePageItem>& result) {
-  // TODO(fgorski): Switch to SQL specific UMA metrics.
   UMA_HISTOGRAM_ENUMERATION("OfflinePages.LoadStatus", status,
                             OfflinePageMetadataStore::LOAD_STATUS_COUNT);
   if (status == OfflinePageMetadataStore::LOAD_SUCCEEDED) {
@@ -427,7 +426,6 @@ void RemoveOfflinePagesSync(
     sql::Connection* db,
     scoped_refptr<base::SingleThreadTaskRunner> runner,
     const OfflinePageMetadataStore::UpdateCallback& callback) {
-  // TODO(fgorski): Perhaps add metrics here.
   std::unique_ptr<OfflinePagesUpdateResult> result(
       new OfflinePagesUpdateResult(StoreState::LOADED));
 

components/offline_pages/core/offline_page_model_impl.cc

@@ -572,7 +572,6 @@ const std::vector<int64_t> OfflinePageModelImpl::MaybeGetOfflineIdsForClientId(
   std::vector<int64_t> results;
 
   // We want only all pages, including those marked for deletion.
-  // TODO(fgorski): actually use an index rather than linear scan.
   for (const auto& id_page_pair : offline_pages_) {
     if (id_page_pair.second.client_id == client_id)
       results.push_back(id_page_pair.second.offline_id);
@@ -690,8 +689,6 @@ void OfflinePageModelImpl::OnCreateArchiveDone(
     int64_t file_size) {
   if (save_page_params.url != url) {
     DVLOG(1) << "Saved URL does not match requested URL.";
-    // TODO(fgorski): We have created an archive for a wrong URL. It should be
-    // deleted from here, once archiver has the right functionality.
     InformSavePageDone(callback, SavePageResult::ARCHIVE_CREATION_FAILED,
                        save_page_params.client_id, offline_id);
     DeletePendingArchiver(archiver);
@@ -956,10 +953,6 @@ void OfflinePageModelImpl::OnRemoveOfflinePagesDone(
       observer.OfflinePageDeleted(page.offline_id, page.client_id);
   }
 
-  // TODO(fgorski): React the FAILED_INITIALIZATION, FAILED_RESET here.
-  // TODO(fgorski): We need a better callback interface for the Remove action on
-  // the this class. Currently removing an item that does not exist is
-  // considered a success, but not called out as such to the caller.
   DeletePageResult delete_result;
   if (result->store_state == StoreState::LOADED)
     delete_result = DeletePageResult::SUCCESS;

components/offline_pages/core/offline_page_test_archiver.h

@@ -26,8 +26,6 @@ namespace offline_pages {
 // for an actual web contents.
 class OfflinePageTestArchiver : public OfflinePageArchiver {
  public:
-  // TODO(fgorski): Try refactoring the observer out and replace it with a
-  // callback, or completely remove the call to |SetLastPathCreatedByArchiver|.
   class Observer {
    public:
     virtual ~Observer() {}

components/offline_pages/core/offline_page_test_store.cc

@@ -44,7 +44,6 @@ void OfflinePageTestStore::GetOfflinePages(const LoadCallback& callback) {
 
 void OfflinePageTestStore::AddOfflinePage(const OfflinePageItem& offline_page,
                                           const AddCallback& callback) {
-  // TODO(fgorski): Add and cover scenario with existing item.
   ItemActionStatus result;
   if (store_state_ == StoreState::LOADED &&
       scenario_ != TestScenario::WRITE_FAILED) {
@@ -61,8 +60,6 @@ void OfflinePageTestStore::AddOfflinePage(const OfflinePageItem& offline_page,
 void OfflinePageTestStore::UpdateOfflinePages(
     const std::vector<OfflinePageItem>& pages,
     const UpdateCallback& callback) {
-  // TODO(fgorski): Cover scenario where new items are being created while they
-  // shouldn't.
   std::unique_ptr<OfflinePagesUpdateResult> result(
       new OfflinePagesUpdateResult(StoreState::LOADED));
   if (scenario_ == TestScenario::WRITE_FAILED) {

components/offline_pages/core/offline_store_types.h

@@ -14,7 +14,6 @@
 // offline page related components.
 namespace offline_pages {
 
-// TODO(fgorski): This enum is meant to replace |LoadStatus|.
 // Current store state. When LOADED, the store is operational. When
 // loading or reset fails, it is reflected appropriately.
 enum class StoreState {