DomDistiller: fix 0 document width

components/dom_distiller/content/distiller_page_web_contents.cc

 // Copyright 2013 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/dom_distiller/content/distiller_page_web_contents.h"
 
 #include "base/callback.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/utf_string_conversions.h"
 #include "components/dom_distiller/content/web_contents_main_frame_observer.h"
@@ skipping 15 matching lines @@
   DCHECK(web_contents_);
 }
 
 SourcePageHandleWebContents::~SourcePageHandleWebContents() {
 }
 
 scoped_ptr<content::WebContents> SourcePageHandleWebContents::GetWebContents() {
   return web_contents_.Pass();
 }
 
-scoped_ptr<DistillerPage> DistillerPageWebContentsFactory::CreateDistillerPage()
-    const {
+scoped_ptr<DistillerPage> DistillerPageWebContentsFactory::CreateDistillerPage(
+    const gfx::Size& render_view_size) const {
   DCHECK(browser_context_);
   return scoped_ptr<DistillerPage>(new DistillerPageWebContents(
-      browser_context_, scoped_ptr<SourcePageHandleWebContents>()));
+      browser_context_, render_view_size,
+      scoped_ptr<SourcePageHandleWebContents>()));
 }
 
 scoped_ptr<DistillerPage>
 DistillerPageWebContentsFactory::CreateDistillerPageWithHandle(
     scoped_ptr<SourcePageHandle> handle) const {
   DCHECK(browser_context_);
   scoped_ptr<SourcePageHandleWebContents> web_contents_handle =
       scoped_ptr<SourcePageHandleWebContents>(
           static_cast<SourcePageHandleWebContents*>(handle.release()));
   return scoped_ptr<DistillerPage>(new DistillerPageWebContents(
-      browser_context_, web_contents_handle.Pass()));
+      browser_context_, gfx::Size(), web_contents_handle.Pass()));
 }
 
 DistillerPageWebContents::DistillerPageWebContents(
     content::BrowserContext* browser_context,
+    const gfx::Size& render_view_size,
     scoped_ptr<SourcePageHandleWebContents> optional_web_contents_handle)
-    : state_(IDLE), browser_context_(browser_context) {
+    : state_(IDLE), browser_context_(browser_context),
+      render_view_size_(render_view_size) {
   if (optional_web_contents_handle) {
     web_contents_ = optional_web_contents_handle->GetWebContents().Pass();
+    if (render_view_size.IsEmpty())
+      render_view_size_ = web_contents_->GetContainerBounds().size();
   }
 }
 
 DistillerPageWebContents::~DistillerPageWebContents() {
+  if (web_contents_)
+    web_contents_->SetDelegate(NULL);
 }
 
 void DistillerPageWebContents::DistillPageImpl(const GURL& url,
                                                const std::string& script) {
   DCHECK(browser_context_);
   DCHECK(state_ == IDLE);
   state_ = LOADING_PAGE;
   script_ = script;
 
   if (web_contents_ && web_contents_->GetLastCommittedURL() == url) {
@@ skipping 16 matching lines @@
       CreateNewWebContents(url);
     }
   } else {
     CreateNewWebContents(url);
   }
 }
 
 void DistillerPageWebContents::CreateNewWebContents(const GURL& url) {
   // Create new WebContents to use for distilling the content.
   content::WebContents::CreateParams create_params(browser_context_);
   create_params.initially_hidden = true;

[Yaron, 2014/07/15 01:46:27]

I think you can do this more easily by just setting create_params.initial_size
here. I think that removes the need for WebContentsDelegate.

[kuan, 2014/07/15 16:52:51]

that was the first thing i tried, but it didn't work, and i didn't debug why.
at ur suggestion, i just tried it again, w/ the same result.

   web_contents_.reset(content::WebContents::Create(create_params));
   DCHECK(web_contents_.get());
 
+  web_contents_->SetDelegate(this);
+
   // Start observing WebContents and load the requested URL.
   content::WebContentsObserver::Observe(web_contents_.get());
   content::NavigationController::LoadURLParams params(url);
   web_contents_->GetController().LoadURLWithParams(params);
 }
 
+gfx::Size DistillerPageWebContents::GetSizeForNewRenderView(
+    content::WebContents* web_contents) const {
+  gfx::Size size(render_view_size_);
+  if (size.IsEmpty())
+    web_contents->GetContainerBounds().size();

[nyquist, 2014/07/15 17:13:02]

was this supposed to say |size = wc->GCB().size()| ?

[Yaron, 2014/07/16 17:46:28]

+1 (or early return but I think Tommy's suggestion is better)

[kuan, 2014/07/17 09:57:25]

Done.  gd catch!

[kuan, 2014/07/17 09:57:25]

Done.

+  // If size is still empty, set an arbitary size so that document.offsetWidth
+  // in the executed domdistiller.js won't be 0.
+  if (size.IsEmpty()) {
+    DVLOG(1) << "Using default RenderView size 800x600";

[Yaron, 2014/07/15 01:46:27]

Can this actually happen?

[kuan, 2014/07/15 16:52:51]

i hope not, but i figured i shld handle and log it just in case.

[Yaron, 2014/07/16 17:46:28]

Hmm. Maybe just do a fullscreen result in that case?
gfx::Screen::GetNativeScreen()->GetPrimaryDisplay()->size() ?

[kuan, 2014/07/17 09:57:26]

Done.

+    size = gfx::Size(800, 600);
+  }
+  return size;
+}
+
 void DistillerPageWebContents::DocumentLoadedInFrame(
     content::RenderFrameHost* render_frame_host) {
   if (render_frame_host == web_contents_->GetMainFrame()) {
     ExecuteJavaScript();
   }
 }
 
 void DistillerPageWebContents::DidFailLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url,
@@ skipping 25 matching lines @@
 
 void DistillerPageWebContents::OnWebContentsDistillationDone(
     const GURL& page_url,
     const base::Value* value) {
   DCHECK(state_ == PAGELOAD_FAILED || state_ == EXECUTING_JAVASCRIPT);
   state_ = IDLE;
   DistillerPage::OnDistillationDone(page_url, value);
 }
 
 }  // namespace dom_distiller