[OfflinePages] RequestCoordinatorFactory return null for incognito

components/offline_pages/background/request_coordinator.cc

Index: components/offline_pages/background/request_coordinator.cc
diff --git a/components/offline_pages/background/request_coordinator.cc b/components/offline_pages/background/request_coordinator.cc
index 6b7753c181d47c4e5b388760d7dbf6234c36e070..7538887b81eb5c78352e8b9f41a3a19815e3bce5 100644
--- a/components/offline_pages/background/request_coordinator.cc
+++ b/components/offline_pages/background/request_coordinator.cc
@@ -194,6 +194,10 @@ int64_t RequestCoordinator::SavePageLater(const GURL& url,
                                           RequestAvailability availability) {
   DVLOG(2) << "URL is " << url << " " << __func__;
 
+  // Make sure this is called on actual instance (as the factory
+  // may not return one for incognito profile).
+  CHECK(this);

[fgorski, 2016/11/22 22:46:33]

I am still not OK with this, and it does not seem to be a pattern used in
chromium.

https://cs.chromium.org/search/?q=CHECK%5C(this%5C)&sq=package:chromium&type=cs

[Pete Williamson, 2016/11/22 22:59:32]

This might be better as a DCHECK.  The idea is to catch new calling code paths
that don't set request_coordinator first.

[jianli, 2016/11/22 23:03:55]

I agree with Filip that this was not a good pattern to use in Chrome. If we add
this here, do we want to add for other methods?

We should rely on crash caused by accessing null pointer.

[dougarnett, 2016/11/23 18:19:52]

Done.

+
   if (!OfflinePageModel::CanSaveURL(url)) {
     DVLOG(1) << "Not able to save page for requested url: " << url;
     return 0L;