OfflinePageModel cleanups - improved CanSavePage() and moved static kInvalidOfflineId to impl

components/offline_pages/offline_page_model_impl.cc

 // 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.
 
 #include "components/offline_pages/offline_page_model_impl.h"
 
 #include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
@@ skipping 157 matching lines @@
 
 void OfflinePageModelImpl::SavePage(
     const GURL& url,
     const ClientId& client_id,
     std::unique_ptr<OfflinePageArchiver> archiver,
     const SavePageCallback& callback) {
   DCHECK(is_loaded_);
 
   // Skip saving the page that is not intended to be saved, like local file
   // page.
-  if (url.is_valid() && !CanSavePage(url)) {
+  if (url.is_valid() && !url.SchemeIsHTTPOrHTTPS()) {

[dewittj, 2016/06/01 16:05:04]

Why not CanSavePage?

[dougarnett, 2016/06/01 16:18:25]

Good question here about where CanSaveURL() should live. Should we put in a
utils within components (so it is available to use here) rather than the utils
in browser/android? 

On the other hand, this is a little bit different of a test (is_valid and !) and
only used in one place - so wasn't clear to define a new method for it.

[dougarnett, 2016/06/01 19:27:09]

Justin and I chatted about this offline wondering if we need this check here
anyway. In particular, why can't OfflinePageBridge::SavePage() do the CanSave*
check? If so, then the Bridge code might then need to run the callback with
SavePageResult::SKIPPED.

[fgorski, 2016/06/01 20:21:03]

url.is_valid() was added recently, by jianli. We think there is a test that
depends on it. Perhaps updating the code to if (!CanSavePage) and making sure
the tests work is a better solution. 

If that works I think this method can still be a static on the model, until we
find a better place for it. I don't think this would violate the pure abstract
property of this class.

CanSaveURL can be a fall through to this method. That way we can also add more
conditions where saving URL should not be possible.
The reason I would be reluctant to do it is that we call InformSavePageDone
below, which collects UMA stats. If we short-circut that higher up, we will have
hard time tracking it properly. We already hit that problem in the past.

Additionally not all calls to SavePage will be done using the Bridge, which
justifies this check even more.

[dougarnett, 2016/06/01 22:40:22]

Ok put CanSave method back on the Model with url.is_valid() addition but did
keep the newer name for now.

     InformSavePageDone(callback, SavePageResult::SKIPPED, client_id,
                        kInvalidOfflineId);
     return;
   }
 
   // The web contents is not available if archiver is not created and passed.
   if (!archiver.get()) {
     InformSavePageDone(callback, SavePageResult::CONTENT_UNAVAILABLE, client_id,
                        kInvalidOfflineId);
     return;
@@ skipping 715 matching lines @@
 void OfflinePageModelImpl::RunWhenLoaded(const base::Closure& task) {
   if (!is_loaded_) {
     delayed_tasks_.push_back(task);
     return;
   }
 
   base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, task);
 }
 
 }  // namespace offline_pages