Cleanup of PrepareFrameAndViewForPrint code.

Cleanup of PrepareFrameAndViewForPrint code.

Hide PrepareFrameAndViewForPrint in cc file.
Remove references to PrepareFrameAndViewForPrint from platform specific code.
Deleted dead code.

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

[gene, 2013-01-22 22:33:31]

lgtm,
overall looks ok, but I might be not the best person to review this code. May be
ask thestig@ to take a quick look too?

[Lei Zhang, 2013-01-23 08:31:38]

Mostly good.

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:492: bool StartPrinting();
Why return a bool if you never check it?

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:563: NULL);
|use_browser_overlays_| has been disconnected for a long time and no one has
complained. If we are assuming it's safe to remove, we should file a bug with
webkit to remove this parameter from the Chromium WebFrame header / impl.

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:573: if (frame_)
If you get rid of |finished_|, you may want to document that the validity of
|frame_| and |web_view_| are expected to stay in sync.

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper_linux.cc (right):

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper_linux.cc:131: int page_count,
With |page_count| coming from outside, RenderPages() can return void, because
you already know if it will fail or not. Lines 134-145 can also be moved
outside, so you won't even need |page_count|, and |printed_pages| can be an in
parameter.

Of course, doing all that would strip RenderPages() down to a 6 line function.
At which point, maybe we don't even need it anymore. I think if you do get rid
of it, then the workflow on all platforms become PrintPages() ->
PrintPagesNative() -> PrintPagesInternal(), right?

[Vitaly Buka (NO REVIEWS), 2013-01-23 09:32:31]

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:492: bool StartPrinting();
On 2013/01/23 08:31:38, Lei Zhang wrote:
> Why return a bool if you never check it?

Done.

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:563: NULL);
I am not assuming, just see that it's not used anywhere.
I can file bug.

On 2013/01/23 08:31:38, Lei Zhang wrote:
> |use_browser_overlays_| has been disconnected for a long time and no one has
> complained. If we are assuming it's safe to remove, we should file a bug with
> webkit to remove this parameter from the Chromium WebFrame header / impl.

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:573: if (frame_)
Actually only frame is necessary.

On 2013/01/23 08:31:38, Lei Zhang wrote:
> If you get rid of |finished_|, you may want to document that the validity of
> |frame_| and |web_view_| are expected to stay in sync.

Done.

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper_linux.cc (right):

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper_linux.cc:131: int page_count,
I can remove RenderPage.

I see that loop in PrintPagesNative is very similar on all platforms. Only
problems that Linux sends fake messages after entire doc is rendered. I'd like
to think about that later.


On 2013/01/23 08:31:38, Lei Zhang wrote:
> With |page_count| coming from outside, RenderPages() can return void, because
> you already know if it will fail or not. Lines 134-145 can also be moved
> outside, so you won't even need |page_count|, and |printed_pages| can be an in
> parameter.



> 
> Of course, doing all that would strip RenderPages() down to a 6 line function.
> At which point, maybe we don't even need it anymore. I think if you do get rid
> of it, then the workflow on all platforms become PrintPages() ->
> PrintPagesNative() -> PrintPagesInternal(), right?

[Lei Zhang, 2013-01-23 19:50:24]

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:563: NULL);
On 2013/01/23 09:32:31, Vitaly Buka wrote:
> I am not assuming, just see that it's not used anywhere.
> I can file bug.

Thanks. Please do that and add a TODO here referring to that bug.

https://codereview.chromium.org/11953010/diff/2018/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/11953010/diff/2018/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:557: if (WebKit::WebView*
web_view = frame_->view()) {
Why do we need this check? Same with line 574.

[Vitaly Buka (NO REVIEWS), 2013-01-23 20:25:57]

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/11953010/diff/3012/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:563: NULL);
On 2013/01/23 19:50:24, Lei Zhang wrote:
> On 2013/01/23 09:32:31, Vitaly Buka wrote:
> > I am not assuming, just see that it's not used anywhere.
> > I can file bug.
> 
> Thanks. Please do that and add a TODO here referring to that bug.

Done.

https://codereview.chromium.org/11953010/diff/2018/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper.cc (right):

https://codereview.chromium.org/11953010/diff/2018/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper.cc:557: if (WebKit::WebView*
web_view = frame_->view()) {
On 2013/01/23 19:50:24, Lei Zhang wrote:
> Why do we need this check? Same with line 574.

Done.

[Lei Zhang, 2013-01-23 20:48:43]

lgtm

[commit-bot: I haz the power, 2013-01-23 20:56:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vitalybuka@chromium.org/11953010/3015

[commit-bot: I haz the power, 2013-01-24 00:37:11]

Change committed as 178439