[Background loader] .h file draft outline of background loader

components/background_loader/background_loader_contents.h

Index: components/background_loader/background_loader_contents.h
diff --git a/components/background_loader/background_loader_contents.h b/components/background_loader/background_loader_contents.h
new file mode 100644
index 0000000000000000000000000000000000000000..9e3640b7a3c9281c037aabec1b608a6ec81f576a
--- /dev/null
+++ b/components/background_loader/background_loader_contents.h
@@ -0,0 +1,88 @@
+// Copyright (c) 2016 The Chromium Authors. All rights reserved.

[fgorski, 2016/10/13 22:32:51]

drop the (c) it's not required.

[chili, 2016/10/24 23:56:41]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_
+#define COMPONENTS_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_

[fgorski, 2016/10/13 22:32:51]

About location of that file (from the meeting discussion):
* Perhaps you want to start the new folder structure with new files and put it
directly in the components/offline_pages/content/background_loader

[chili, 2016/10/24 23:56:41]

Done.

+
+#include <string>
+
+#include "base/callback.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "url/gurl.h"
+
+namespace background {
+
+// This class maintains a WebContents used in the background. It can host
+// a renderer but does not have any visible display.
+class BackgroundLoaderContents : public content::WebContentsDelegate,
+                                 public content::WebContentsObserver {
+ public:
+  // Nested observer class for listening to events.
+  class Observer {
+   public:
+    Observer() {}

[Dmitry Titov, 2016/10/13 01:03:26]

no need to have constructor, usually the Observers follow the pattern:
Observer {
public:
 virtual ~Observer = default;

 virtual void Method1() = 0;
 virtual void Method2() = 0;
};

So they are pure abstract classes and don't need ctor as such.

[chili, 2016/10/13 01:18:58]

I'm not too familiar about this part of C++.  I was hoping to have an interface
similar to the WebContentsObserver - where you only need to override the methods
that you care about.  Wouldn't having all the methods be virtual force you to
implement all of the methods?  What would be a better way to implement what I
want to do?

[dougarnett, 2016/10/13 15:38:04]

So at least remove ctor here and define dtor as virtual default.

[chili, 2016/10/24 23:56:42]

Removed Observer altogether in favor of plain WebContentsObserver

+    ~Observer() {}
+    void OnPageLoadStarted() {}
+    void OnPageLoaded() {}

[Dmitry Titov, 2016/10/13 01:03:26]

Will this correspond to OnLoad event or to some signal from SnapshotController?
Who and how will determine that page is loaded? 

[chili, 2016/10/13 01:18:58]

This will correspond to the OnLoad event from the WebContentsObserver.  The
WebContents determines that.  I imagine when we fully integrate the
SnapshotController in, this might change to provide more granular detail

[dougarnett, 2016/10/13 15:38:04]

Might be helpful to mention in the class comment that the "events" are
WebContentsObserver events then and per method comment what such event it
corresponds to (eg, DidStartLoading?  DocumentOnLoadCompletedInMainFrame?)

[chili, 2016/10/24 23:56:41]

Acknowledged.

+  };
+
+  BackgroundLoaderContents(

[Dmitry Titov, 2016/10/13 01:03:26]

Needs comment explaining the parameters (eg why passing the
sessions_storage_namespace and where to get one). Also, the next ctor does not
have one - when shoudl the user use one or another?

[chili, 2016/10/24 23:56:41]

Done.

+      content::BrowserContext* browser_context,
+      content::SessionStorageNamespace* session_storage_namespace);
+  BackgroundLoaderContents(content::BrowserContext* browser_context);
+  ~BackgroundLoaderContents() override;
+
+  content::WebContents* web_contents() const { return web_contents_.get(); }

[Dmitry Titov, 2016/10/13 01:03:26]

I'm still not sure this should be exposed. WebContents is internal object that
is constantly changing - it is unclear how it can be a 'result' of a load? Maybe
it should be passed to Observer::OnPageLoaded()?

Alternatively, this class could never expose web_contents and instead implement
an AbstractOffliner interface that would do all the work of observing the
ongoing loading process and produce a snapshot of contents.

Even more alternatively, this class would expose something like TabHelper that a
client can implement and attach to receive WebContentsObserver calls.

[chili, 2016/10/24 23:56:42]

removed

+  void LoadPage(const GURL& url);
+  void StopLoading();

[Dmitry Titov, 2016/10/13 01:03:26]

If StopLoading() is called, what call will Observer receive? 

[chili, 2016/10/24 23:56:42]

Renamed to Cancel, per design doc.  This will call Close() on WebContents, which
should fire DidStopLoading() and later WebContentsDestroyed()

+  void SetObserver(Observer* observer);

[Dmitry Titov, 2016/10/13 01:03:26]

These usually follow patter on AddObserver/RemoveObserver and use ObserverList
for implementation.

[chili, 2016/10/13 01:18:58]

I had a TODO comment somewhere for whether this should be a single observer vs
list.  I originally picked single for ease of management, but will change to
List

+
+  // content::WebContentsDelegate Implementation:
+  void CloseContents(content::WebContents* source) override;
+  bool ShouldSuppressDialogs(content::WebContents* source) override;
+  bool IsNeverVisible(content::WebContents* web_contents) override;
+  bool ShouldFocusLocationBarByDefault(content::WebContents* source) override;
+  bool ShouldFocusPageAfterCrash() override;
+  void CanDownload(const GURL& url,
+                   const std::string& request_method,
+                   const base::Callback<void(bool)>& callback) override;
+  bool ShouldCreateWebContents(
+      content::WebContents* contents,
+      int32_t route_id,
+      int32_t main_frame_route_id,
+      int32_t main_frame_widget_route_id,
+      WindowContainerType window_container_type,
+      const std::string& frame_name,
+      const GURL& target_url,
+      const std::string& partition_id,
+      content::SessionStorageNamespace* session_storage_namespace) override;
+  void RequestMediaAccessPermission(
+      content::WebContents* contents,
+      const content::MediaStreamRequest& request,
+      const content::MediaResponseCallback& callback) override;
+  bool CheckMediaAccessPermission(content::WebContents* contents,
+                                  const GURL& security_origin,
+                                  content::MediaStreamType type) override;
+
+  // content::WebContentsObserver Implementation:
+  void RenderProcessGone(base::TerminationStatus status) override;
+  void DidStartLoading() override;
+  void DidStopLoading() override;
+
+ protected:
+  BackgroundLoaderContents();

[fgorski, 2016/10/13 22:32:51]

Hey, I don't understand that part. Why is this protected?

[chili, 2016/10/24 23:56:41]

Removed

+
+ private:
+  std::unique_ptr<content::WebContents> web_contents_;
+  content::SessionStorageNamespaceMap session_storage_namespace_map_;
+  content::BrowserContext* browser_context_;
+  Observer* observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(BackgroundLoaderContents);
+};
+
+}  // namespace background
+#endif  // COMPONENTS_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_