Offline Pages: Replace Observer::OfflinePageModelChanged with OfflinePageAdded.

components/offline_pages/offline_page_model.h

Index: components/offline_pages/offline_page_model.h
diff --git a/components/offline_pages/offline_page_model.h b/components/offline_pages/offline_page_model.h
index 4222b53917dd5cfd5aa95916c6aea2a670b35d64..9f4b078a888ca6a5b3345c2b0e7732469a4b2857 100644
--- a/components/offline_pages/offline_page_model.h
+++ b/components/offline_pages/offline_page_model.h
@@ -51,18 +51,19 @@ class OfflinePageModel : public base::SupportsUserData {
   class Observer {

[fgorski, 2016/11/02 15:51:06]

https://bugs.chromium.org/p/chromium/issues/detail?id=565575 use this bug
please.

[dewittj, 2016/11/02 16:34:37]

Done.

    public:
     // Invoked when the model has finished loading.
-    virtual void OfflinePageModelLoaded(OfflinePageModel* model) = 0;
+    virtual void OfflinePageModelLoaded(OfflinePageModel* model);

[Dmitry Titov, 2016/11/02 07:15:37]

What prompted you to make this a non-abstract interface? I think typically these
are abstract, and if there is one with gazillion methods that there is a
separate class which is an impl of interface with empty methods provided, for
convenience... But in this case, it doesn't seem to save you much.

If you'd like to consider an actual class rather than an abstract interface, I'd
left empty methods definitions in .h file (so it's clearly visible they are
empty) and moved "model" parameter out of them, instead having
offline_page_model() accessor on the Observer class (somewhat like
WebContentsObserver).

[dewittj, 2016/11/02 16:34:37]

Most observers don't implement all the methods, it was leading to a
proliferation of empty code.  But since there are still only 3 methods, I
suppose we could leave it abstract.

 
-    // Invoked when the model is being updated, due to adding, removing or
-    // updating an offline page.
-    virtual void OfflinePageModelChanged(OfflinePageModel* model) = 0;
+    // Invoked when the model is being updated due to adding an offline page.
+    virtual void OfflinePageAdded(OfflinePageModel* model,

[fgorski, 2016/11/02 15:51:06]

This hits me as problematic. I would expect another method for notifying
observers that an offline page has been changed/updated.

A short design document would be great for that.

[dewittj, 2016/11/02 16:34:37]

This patch is not trying to change any behavior of the model.  I agree that we
lack an updated event, but today it is not possible to update a page except by
marking it as accessed.

Future patch could add support for update and an updated method on the observer.
 I'll make a doc for that change.

+                                  SavePageResult result,

[fgorski, 2016/11/02 15:51:06]

Is this necessary?

I think you should not be informing anyone about failed add attempts. Therefore,
if you report page added, you are always not null and there is no need to pass
results, as SUCCESS is implied.

Also, I haven't seen it used in this patch.

[dewittj, 2016/11/02 16:34:37]

Not necessary now, I agree that observers won't care about failure except
possibly for the event log.  Removed.

+                                  const OfflinePageItem* added);

[Dmitry Titov, 2016/11/02 07:15:37]

added -> added_page?

Also. needs a comment on when it can be a nullptr.

[dewittj, 2016/11/02 16:34:37]

Done.

 
     // Invoked when an offline copy related to |offline_id| was deleted.
     virtual void OfflinePageDeleted(int64_t offline_id,
-                                    const ClientId& client_id) = 0;
+                                    const ClientId& client_id);
 
    protected:
-    virtual ~Observer() = default;
+    virtual ~Observer();
   };
 
   using CheckPagesExistOfflineResult =