DomDistiller: fix 0 document width

DomDistiller: fix 0 document width

bug:
When distilling a page with new WebContents, document.offsetWidth in
domdistiller.js is always 0, because the newly created RenderView doesn't have a
size.

fix:
DistillerPageWebContents extends WebContentsDelegate and implements
GetNewSizeForRenderView() to return the size for the new RenderView.  This size
is the container bounds of the WebContents from where the distillation is
triggered.  The size is plumbed through the pipeline from the following sources
all the way down to DistillerPageWebContents.
1) WebUI: WebContents of chrome:://dom-distiller
3) ReadingList: WebContents currently associated with the extension
2) standalone ContentExtractor: WebContents of content::Shell
As for the reused WebContents (via "Distill page" in wrench menu), it already
has a size, so we just use it.

BUG=368941, 367254
TBR=danakj (ui/gfx/size.h)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284094

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

Awesome! Glad to see this. I added another bug which I think is related whereby
the distiller runs too early. I'm hoping that with this fixed, we can wait until
the body height is non-0 which may help some of those pages

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:107:
create_params.initially_hidden = true;
I think you can do this more easily by just setting create_params.initial_size
here. I think that removes the need for WebContentsDelegate.

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:127: DVLOG(1) <<
"Using default RenderView size 800x600";
Can this actually happen?

[kuan, 2014-07-15 16:52:52]

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:107:
create_params.initially_hidden = true;
On 2014/07/15 01:46:27, Yaron wrote:
> I think you can do this more easily by just setting create_params.initial_size
> here. I think that removes the need for WebContentsDelegate.

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.

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:127: DVLOG(1) <<
"Using default RenderView size 800x600";
On 2014/07/15 01:46:27, Yaron wrote:
> Can this actually happen?

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

[nyquist, 2014-07-15 17:13:03]

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:123:
web_contents->GetContainerBounds().size();
was this supposed to say |size = wc->GCB().size()| ?

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_ui.cc:56:
web_ui->GetWebContents()->GetContainerBounds().size()));
What would happen if the user resizes the browser before opening an article?
Could this be done on-the-fly in the handler instead of stored as a member?

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

https://codereview.chromium.org/396503003/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc
(right):

https://codereview.chromium.org/396503003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc:5:
#include
"chrome/browser/extensions/api/reading_list_private/reading_list_private_api.h"
You'll need a chrome/browser/extensions OWNER

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:123:
web_contents->GetContainerBounds().size();
On 2014/07/15 17:13:02, nyquist wrote:
> was this supposed to say |size = wc->GCB().size()| ?

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

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:127: DVLOG(1) <<
"Using default RenderView size 800x600";
On 2014/07/15 16:52:51, kuan wrote:
> On 2014/07/15 01:46:27, Yaron wrote:
> > Can this actually happen?
> 
> i hope not, but i figured i shld handle and log it just in case.

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

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_ui.cc:56:
web_ui->GetWebContents()->GetContainerBounds().size()));
On 2014/07/15 17:13:03, nyquist wrote:
> What would happen if the user resizes the browser before opening an article?
> Could this be done on-the-fly in the handler instead of stored as a member?

Agreed. I thought these handlers were actually long-lived (not just for a single
chrome:// page) so instead use web_ui()->GetWebContents() in |HandleAddArticle|

[nyquist, 2014-07-16 19:20:49]

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_ui.cc:56:
web_ui->GetWebContents()->GetContainerBounds().size()));
On 2014/07/16 17:46:28, Yaron wrote:
> On 2014/07/15 17:13:03, nyquist wrote:
> > What would happen if the user resizes the browser before opening an article?
> > Could this be done on-the-fly in the handler instead of stored as a member?
> 
> Agreed. I thought these handlers were actually long-lived (not just for a
single
> chrome:// page) so instead use web_ui()->GetWebContents() in
|HandleAddArticle|

I think they're created through |WebContentsImpl::CreateWebUI ->
ChromeWebUIControllerFactory::CreateWebUIControllerForURL -> DomDistillerUi
factory function| which is used by RenderFrameHostManager::SetPendingWebUI which
is called during navigation of a RenderFrame, so I think it's linked to the
RenderFrame navigation entry lifetime, so not long-lived-forever.

Anyway, the previous comments still apply.

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

mike: pls provide owners approval for chrome/browser/extensions.  thx.

yaron, tommy:
i've addressed all comments in patch set 2.  ptal.  thx.

https://codereview.chromium.org/396503003/diff/1/chrome/browser/extensions/ap...
File
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc
(right):

https://codereview.chromium.org/396503003/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/reading_list_private/reading_list_private_api.cc:5:
#include
"chrome/browser/extensions/api/reading_list_private/reading_list_private_api.h"
On 2014/07/16 17:46:28, Yaron wrote:
> You'll need a chrome/browser/extensions OWNER

Acknowledged.

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
File components/dom_distiller/content/distiller_page_web_contents.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:123:
web_contents->GetContainerBounds().size();
On 2014/07/15 17:13:02, nyquist wrote:
> was this supposed to say |size = wc->GCB().size()| ?

Done.  gd catch!

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:123:
web_contents->GetContainerBounds().size();
On 2014/07/16 17:46:28, Yaron wrote:
> On 2014/07/15 17:13:02, nyquist wrote:
> > was this supposed to say |size = wc->GCB().size()| ?
> 
> +1 (or early return but I think Tommy's suggestion is better)

Done.

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/con...
components/dom_distiller/content/distiller_page_web_contents.cc:127: DVLOG(1) <<
"Using default RenderView size 800x600";
On 2014/07/16 17:46:28, Yaron wrote:
> On 2014/07/15 16:52:51, kuan wrote:
> > On 2014/07/15 01:46:27, Yaron wrote:
> > > Can this actually happen?
> > 
> > i hope not, but i figured i shld handle and log it just in case.
> 
> Hmm. Maybe just do a fullscreen result in that case?
> gfx::Screen::GetNativeScreen()->GetPrimaryDisplay()->size() ?

Done.

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
File components/dom_distiller/webui/dom_distiller_ui.cc (right):

https://codereview.chromium.org/396503003/diff/1/components/dom_distiller/web...
components/dom_distiller/webui/dom_distiller_ui.cc:56:
web_ui->GetWebContents()->GetContainerBounds().size()));
On 2014/07/16 19:20:49, nyquist wrote:
> On 2014/07/16 17:46:28, Yaron wrote:
> > On 2014/07/15 17:13:03, nyquist wrote:
> > > What would happen if the user resizes the browser before opening an
article?
> > > Could this be done on-the-fly in the handler instead of stored as a
member?
> > 
> > Agreed. I thought these handlers were actually long-lived (not just for a
> single
> > chrome:// page) so instead use web_ui()->GetWebContents() in
> |HandleAddArticle|
> 
> I think they're created through |WebContentsImpl::CreateWebUI ->
> ChromeWebUIControllerFactory::CreateWebUIControllerForURL -> DomDistillerUi
> factory function| which is used by RenderFrameHostManager::SetPendingWebUI
which
> is called during navigation of a RenderFrame, so I think it's linked to the
> RenderFrame navigation entry lifetime, so not long-lived-forever.
> 
> Anyway, the previous comments still apply.

Done.

[Yaron, 2014-07-17 17:40:03]

lgtm

[miket_OOO, 2014-07-17 19:36:14]

OWNERS LGTM

[nyquist, 2014-07-17 20:15:17]

lgtm

[kuan, 2014-07-17 20:32:10]

The CQ bit was checked by kuan@chromium.org

[commit-bot: I haz the power, 2014-07-17 20:36:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kuan@chromium.org/396503003/20001

[kuan, 2014-07-17 21:54:17]

The CQ bit was checked by kuan@chromium.org

[commit-bot: I haz the power, 2014-07-17 21:56:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kuan@chromium.org/396503003/40001

[kuan, 2014-07-17 22:25:11]

danakj@, cld u pls provide owners approval for adding ui/gfx to
components/dom_distiller/DEPS?  i added to use ui/gfx/size.h.  thx!

[commit-bot: I haz the power, 2014-07-18 02:35:19]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-18 02:39:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-18 02:40:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[kuan, 2014-07-18 14:17:21]

tbring danakj for ui/gfx/size.h

[kuan, 2014-07-18 14:17:30]

The CQ bit was checked by kuan@chromium.org

[commit-bot: I haz the power, 2014-07-18 14:20:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kuan@chromium.org/396503003/40001

[commit-bot: I haz the power, 2014-07-18 14:26:17]

Change committed as 284094