Implementation (cc file) for background loader contents.

components/offline_pages/content/background_loader/background_loader_contents.cc

Index: components/offline_pages/content/background_loader/background_loader_contents.cc
diff --git a/components/offline_pages/content/background_loader/background_loader_contents.cc b/components/offline_pages/content/background_loader/background_loader_contents.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7e1ebdcec82f09bb3b7406a42945bcf8f715bdca
--- /dev/null
+++ b/components/offline_pages/content/background_loader/background_loader_contents.cc
@@ -0,0 +1,103 @@
+// Copyright (c) 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.
+
+#include "components/offline_pages/content/background_loader/background_loader_contents.h"
+
+#include "content/public/browser/web_contents.h"
+
+namespace background_loader {
+
+BackgroundLoaderContents::BackgroundLoaderContents(
+    content::BrowserContext* browser_context)
+    : browser_context_(browser_context) {
+  web_contents_.reset(content::WebContents::Create(
+      content::WebContents::CreateParams(browser_context_)));
+  web_contents_->SetDelegate(this);
+}
+
+BackgroundLoaderContents::~BackgroundLoaderContents() {}
+
+void BackgroundLoaderContents::LoadPage(const GURL& url) {
+  web_contents_->GetController().LoadURL(
+      url, content::Referrer(), ui::PAGE_TRANSITION_LINK, std::string());

[Dmitry Titov, 2016/11/08 00:07:18]

Could you add comment on the choice of parameter values?
Also, the std::string() param - need to add comment on what it is.

[chili, 2016/11/10 22:05:08]

Done.

+}
+
+void BackgroundLoaderContents::CloseContents(content::WebContents* source) {

[Pete Williamson, 2016/11/08 01:33:43]

Why have two methods that do the same thing?  Are this and Cancel expected to
diverge at some point?  Are we just trying to better document the intent?  Is
this just implementing an existing interface and both methods are best handled
by cancelling?

[chili, 2016/11/10 22:05:08]

Cancel() is our own - for when we want to fail early out of a request loop. 
CloseContents is inherited/overridden through the WebContentsDelegate, and
actually shouldn't be called in vast majority of cases.

I can see intent being something that differentiates the 2 - Cancel() is when we
call fail out and CloseContents is when a different tab or other management
closes this webcontents.  I can see these diverging in the future - maybe when
we are failing out, we want some sort of 'save progress' signal, or maybe we
want to ignore CloseContents calls because any random tab shouldn't be able to
just close us.

+  web_contents_->Close();
+}
+
+void BackgroundLoaderContents::Cancel() {

[Dmitry Titov, 2016/11/08 00:07:18]

The methods in cc file should be in the same order as they are in .h file.
(https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md,
"Code Formatting")

[chili, 2016/11/10 22:05:08]

Done.

+  web_contents_->Close();
+}
+
+bool BackgroundLoaderContents::ShouldSuppressDialogs(
+    content::WebContents* source) {
+  return true;

[dougarnett, 2016/11/08 17:09:41]

I wonder if it would be helpful to add a comment on what policy/intent this is
targeting in these methods so maintainers have some better clue. Eg, maybe

  // Dialog prompts are not actionable in the background. 

[chili, 2016/11/10 22:05:08]

Done.

[dougarnett, 2016/11/14 20:12:16]

Thanks, I find those really helpful

+}
+
+bool BackgroundLoaderContents::IsNeverVisible(
+    content::WebContents* web_contents) {
+  return true;
+}
+
+bool BackgroundLoaderContents::ShouldFocusPageAfterCrash() {
+  return false;
+}
+
+void BackgroundLoaderContents::CanDownload(
+    const GURL& url,
+    const std::string& request_method,
+    const base::Callback<void(bool)>& callback) {
+  callback.Run(false);

[Dmitry Titov, 2016/11/08 00:07:18]

It'd be nice to look at callers of CanDownload and check if they can consume the
reentrant call to the callback. Often it is safer to PostTask with a callback
call to make sure it comes on a clean stack. Same for other callback.Run() calls
in this file.

[dewittj, 2016/11/08 00:16:39]

aside: this could be a cool signal in our page quality metric

[chili, 2016/11/10 22:05:08]

The WebContentsDelegate method comments say that the callback can be
called/returned immediately

[chili, 2016/11/10 22:05:08]

That's interesting.  Perhaps not now, but we can add a signal to page quality
once that is established.

+}
+
+bool BackgroundLoaderContents::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) {
+  return false;
+}
+
+void BackgroundLoaderContents::AddNewContents(
+    content::WebContents* source,
+    content::WebContents* new_contents,
+    WindowOpenDisposition disposition,
+    const gfx::Rect& initial_rect,
+    bool user_gesture,
+    bool* was_blocked) {
+  if (was_blocked != nullptr)
+    *was_blocked = true;
+}
+
+void BackgroundLoaderContents::RequestMediaAccessPermission(
+    content::WebContents* contents,
+    const content::MediaStreamRequest& request,
+    const content::MediaResponseCallback& callback) {
+  // No permissions granted, act as if dismissed.
+  callback.Run(
+      content::MediaStreamDevices(),
+      content::MediaStreamRequestResult::MEDIA_DEVICE_PERMISSION_DISMISSED,
+      std::unique_ptr<content::MediaStreamUI>());
+}
+
+bool BackgroundLoaderContents::CheckMediaAccessPermission(
+    content::WebContents* contents,
+    const GURL& security_origin,
+    content::MediaStreamType type) {
+  return false;  // No permissions granted.
+}
+
+#if defined(OS_ANDROID)
+bool BackgroundLoaderContents::ShouldBlockMediaRequest(const GURL& url) {
+  return true;
+}
+#endif
+
+}  // namespace background_loader