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

components/offline_pages/content/background_loader/background_loader_contents.h

+// 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.
+
+#ifndef COMPONENTS_OFFLINE_PAGES_CONTENT_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_
+#define COMPONENTS_OFFLINE_PAGES_CONTENT_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "content/public/browser/web_contents.h"

[fgorski, 2016/10/25 12:15:36]

This file has a lot of dependencies included only through the web_contents.*\.h
headers. Can we discuss in the office if we are OK with that, or whether it
makes sense to be more explicit?

[chili, 2016/10/25 18:48:00]

Removed a few where we only need the definition.  will add these to the cc file
instead.  Will poke you in office to see if this works

+#include "content/public/browser/web_contents_delegate.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "url/gurl.h"
+
+namespace background {

[Dmitry Titov, 2016/10/25 00:31:09]

As a global namespace, this may easily overlap with something else (although it
currently does not). How about:

namespace offline_pages
namespace background

or 
namespace background_loader

Either one is narrower.

[fgorski, 2016/10/25 12:15:36]

Is there a reason to put it in a different namespace than offline_pages?

[chili, 2016/10/25 18:48:00]

It's in a different namespace mainly because at one point I put it in a
different component.  I kept it different for ease of moving it around.  what do
you think?

+
+// 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:
+  // Creates BackgroundLoaderContents with specified |browser_context| and
+  // |session_storage_namespace|.  This ctor should only be used if a different
+  // session storage should be used.

[fgorski, 2016/10/25 12:15:36]

Could you add an example of such case?
What if nullptr is passed for the latter parameter?

[chili, 2016/10/25 18:48:00]

Not sure what you mean by example.  session storage namespace should not be null

+  BackgroundLoaderContents(
+      content::BrowserContext* browser_context,
+      content::SessionStorageNamespace* session_storage_namespace);
+
+  // Creates BackgroundLoaderContents with specified |browser_context|. Uses
+  // default session storage space.
+  BackgroundLoaderContents(content::BrowserContext* browser_context);

[Dmitry Titov, 2016/10/25 00:31:09]

should be 'explicit' contructor:
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

[fgorski, 2016/10/25 12:15:36]

Rule applies to all constructors with a single parameter.

[chili, 2016/10/25 18:48:00]

Done.

+  ~BackgroundLoaderContents() override;
+
+  // Loads the URL in a WebContents. Will call observers to observe

[Dmitry Titov, 2016/10/25 00:31:09]

Is there incomplete sentence?

[chili, 2016/10/25 18:48:00]

not really, but expanded the sentence to be more clear

+  void LoadPage(const GURL& url);
+  // Cancels loading on the current page. Calls Close() on internal WebContents.

[Dmitry Titov, 2016/10/25 00:31:09]

"loading on" -> "loading of"?

[chili, 2016/10/25 18:48:00]

Done.

+  void Cancel();
+
+  void AddObserver(content::WebContentsObserver* observer);
+  void RemoveObserver(content::WebContentsObserver* observer);
+
+  // content::WebContentsDelegate Implementation:

[fgorski, 2016/10/25 12:15:36]

s/I/i/

[chili, 2016/10/25 18:48:00]

Done.

+  bool IsNeverVisible(content::WebContents* web_contents) override;
+  void CloseContents(content::WebContents* source) override;
+  bool ShouldSuppressDialogs(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 AddNewContents(WebContents* source,
+                      WebContents* new_contents,
+                      WindowOpenDisposition disposition,
+                      const gfx::Rect& initial_rect,
+                      bool user_gesture,
+                      bool* was_blocked) override;
+
+#if defined(OS_ANDROID)
+  bool ShouldBlockMediaRequest(const GURL& url) override;
+#endif
+
+  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;
+
+ private:
+  std::unique_ptr<content::WebContents> web_contents_;
+  content::SessionStorageNamespaceMap session_storage_namespace_map_;
+  content::BrowserContext* browser_context_;
+  std::vector<WebContentsObserver*> observers_;

[fgorski, 2016/10/25 12:15:36]

Is there a reason why this cannot be a base::ObserverList?

https://cs.chromium.org/chromium/src/base/observer_list.h

[chili, 2016/10/25 18:48:00]

did not know it existed :)

+
+  DISALLOW_COPY_AND_ASSIGN(BackgroundLoaderContents);
+};
+
+}  // namespace background
+#endif  // COMPONENTS_OFFLINE_PAGES_CONTENT_BACKGROUND_LOADER_BACKGROUND_LOADER_CONTENTS_H_