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

components/offline_pages/offline_page_model_impl.cc

Index: components/offline_pages/offline_page_model_impl.cc
diff --git a/components/offline_pages/offline_page_model_impl.cc b/components/offline_pages/offline_page_model_impl.cc
index e36afd3e03f4d78d96240341e9b9b105ef92b7df..5c72d5d436b05805d2dfed64441c229f39561291 100644
--- a/components/offline_pages/offline_page_model_impl.cc
+++ b/components/offline_pages/offline_page_model_impl.cc
@@ -175,7 +175,7 @@ void OfflinePageModelImpl::SavePage(
 
   // 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;