Remove the discrepencies between chrome and android_webview print

Remove the discrepencies between chrome and android_webview print
messages to enable componentizing printing. Basically, we have
modified two print messages
PrintHostMsg_AllocateTempFileForPrinting
PrintHostMsg_TempFileForPrintingWritten

to a routed message rather then a control message to simplify
implementation. However, this prevents moving the code to components.

BUG=446509
Committed: https://crrev.com/ee20aa9a5b382180944c5f0367fdddd5f7c22aa5
Cr-Commit-Position: refs/heads/master@{#311544}

[sgurun-gerrit only, 2015-01-14 01:08:33]

sgurun@chromium.org changed reviewers:
+ hush@chromium.org

[sgurun-gerrit only, 2015-01-14 01:09:26]

sgurun@chromium.org changed reviewers:
+ nasko@chromium.org

[sgurun-gerrit only, 2015-01-14 01:10:28]

On 2015/01/14 01:09:26, sgurun wrote:
> mailto:sgurun@chromium.org changed reviewers:
> + mailto:nasko@chromium.org

nasko: please review android_webview/common/print_messages.h
hush: please review everything else.

A quick review is appreciated since it is blocking
https://codereview.chromium.org/822133003/

[sgurun-gerrit only, 2015-01-14 01:14:20]

sgurun@chromium.org changed reviewers:
+ dgn@chromium.org

[sgurun-gerrit only, 2015-01-14 01:18:23]

On 2015/01/14 01:10:28, sgurun wrote:
> On 2015/01/14 01:09:26, sgurun wrote:
> > mailto:sgurun@chromium.org changed reviewers:
> > + mailto:nasko@chromium.org
> 
> nasko: please review android_webview/common/print_messages.h
> hush: please review everything else.
> 
> A quick review is appreciated since it is blocking
> https://codereview.chromium.org/822133003/

dgn: FYI

[boliu, 2015-01-14 01:41:01]

boliu@chromium.org changed reviewers:
+ boliu@chromium.org

[boliu, 2015-01-14 01:41:02]

drive-by:

owndership of PrintManager doesn't need to change I think? AwContents has a
FromID method, and can dig out PrintManager from there.

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
File android_webview/native/aw_pdf_exporter.cc (right):

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
android_webview/native/aw_pdf_exporter.cc:53: print_manager =
Why call CreateForWebContents with the exact same params twice?

Also indent

[hush (inactive), 2015-01-14 01:55:47]

lgtm with some minor comments

https://codereview.chromium.org/854513002/diff/1/android_webview/browser/rend...
File android_webview/browser/renderer_host/print_manager.cc (right):

https://codereview.chromium.org/854513002/diff/1/android_webview/browser/rend...
android_webview/browser/renderer_host/print_manager.cc:36: 
stale line.

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
File android_webview/native/aw_pdf_exporter.cc (left):

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
android_webview/native/aw_pdf_exporter.cc:7: #include
"android_webview/browser/renderer_host/print_manager.h"
we still need print manager in this file?

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
File android_webview/native/aw_pdf_exporter.cc (right):

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
android_webview/native/aw_pdf_exporter.cc:53: print_manager =
print_manager is assigned 2 times?

[sgurun-gerrit only, 2015-01-14 01:59:55]

On 2015/01/14 01:41:02, boliu wrote:
> drive-by:
> 
> owndership of PrintManager doesn't need to change I think? AwContents has a
> FromID method, and can dig out PrintManager from there.

Yes, that is what I wanted to do initially, but then AwPrintingMessageFilter
lives in browser, and AwContents is in native :( causing layer violation. this
turned out to be simpler.
> 
>
https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
> File android_webview/native/aw_pdf_exporter.cc (right):
> 
>
https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
> android_webview/native/aw_pdf_exporter.cc:53: print_manager =
> Why call CreateForWebContents with the exact same params twice?
> 
> Also indent
left over experiment to make sure it really is destroyed

[sgurun-gerrit only, 2015-01-14 02:03:18]

https://codereview.chromium.org/854513002/diff/1/android_webview/browser/rend...
File android_webview/browser/renderer_host/print_manager.cc (right):

https://codereview.chromium.org/854513002/diff/1/android_webview/browser/rend...
android_webview/browser/renderer_host/print_manager.cc:36: 
On 2015/01/14 01:55:47, hush wrote:
> stale line.

Done.

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
File android_webview/native/aw_pdf_exporter.cc (left):

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
android_webview/native/aw_pdf_exporter.cc:7: #include
"android_webview/browser/renderer_host/print_manager.h"
On 2015/01/14 01:55:47, hush wrote:
> we still need print manager in this file?

aw_pdf_exporter.h includes it since it implements PrintManagerDelegate.

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
File android_webview/native/aw_pdf_exporter.cc (right):

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
android_webview/native/aw_pdf_exporter.cc:53: print_manager =
On 2015/01/14 01:55:47, hush wrote:
> print_manager is assigned 2 times?

Done.

https://codereview.chromium.org/854513002/diff/1/android_webview/native/aw_pd...
android_webview/native/aw_pdf_exporter.cc:53: print_manager =
On 2015/01/14 01:41:02, boliu wrote:
> Why call CreateForWebContents with the exact same params twice?
> 
> Also indent

Done.

[boliu, 2015-01-14 02:18:52]

On 2015/01/14 01:59:55, sgurun wrote:
> On 2015/01/14 01:41:02, boliu wrote:
> > drive-by:
> > 
> > owndership of PrintManager doesn't need to change I think? AwContents has a
> > FromID method, and can dig out PrintManager from there.
> 
> Yes, that is what I wanted to do initially, but then AwPrintingMessageFilter
> lives in browser, and AwContents is in native :( causing layer violation. this
> turned out to be simpler.

I remember something like this, but then double checked again and seems not be
the case:
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...

Anyway, not terribly important.

[sgurun-gerrit only, 2015-01-14 02:31:41]

On 2015/01/14 02:18:52, boliu wrote:
> On 2015/01/14 01:59:55, sgurun wrote:
> > On 2015/01/14 01:41:02, boliu wrote:
> > > drive-by:
> > > 
> > > owndership of PrintManager doesn't need to change I think? AwContents has
a
> > > FromID method, and can dig out PrintManager from there.
> > 
> > Yes, that is what I wanted to do initially, but then AwPrintingMessageFilter
> > lives in browser, and AwContents is in native :( causing layer violation.
this
> > turned out to be simpler.
> 
> I remember something like this, but then double checked again and seems not be
> the case:
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/br...
> 
> Anyway, not terribly important.

You are definitely right, the layering was quite strict (we went into a lot of
pain to keep it that way, i.e. delegates, etc) but then it means somebody added
a DEPS later. I will assume the DEPS was added as a result of a mistake until we
discuss it and everybody agrees to that.

[dgn, 2015-01-14 11:19:19]

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_printing_message_filter.cc (right):

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
android_webview/browser/aw_printing_message_filter.cc:25: if (rvh == nullptr)
nit: if(!rvh)  
same for other occurrences below

[sgurun-gerrit only, 2015-01-14 18:55:58]

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_printing_message_filter.cc (right):

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
android_webview/browser/aw_printing_message_filter.cc:25: if (rvh == nullptr)
On 2015/01/14 11:19:19, dgn wrote:
> nit: if(!rvh)  
> same for other occurrences below

thanks, I think both ways are pretty prevalent in chromium code. This code will
probably be removed anyway at the end of print refactoring so leaving it as it
is now.

[sgurun-gerrit only, 2015-01-14 18:56:47]

On 2015/01/14 18:55:58, sgurun wrote:
>
https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
> File android_webview/browser/aw_printing_message_filter.cc (right):
> 
>
https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
> android_webview/browser/aw_printing_message_filter.cc:25: if (rvh == nullptr)
> On 2015/01/14 11:19:19, dgn wrote:
> > nit: if(!rvh)  
> > same for other occurrences below
> 
> thanks, I think both ways are pretty prevalent in chromium code. This code
will
> probably be removed anyway at the end of print refactoring so leaving it as it
> is now.

nasko@ ping for reviewing android_webview/common/print_messages.h

[nasko, 2015-01-14 19:16:33]

The main comment is to transition this CL from RenderView(Host) to
RenderFrame(Host).

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_printing_message_filter.cc (right):

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
android_webview/browser/aw_printing_message_filter.cc:23:
content::RenderViewHost* rvh = content::RenderViewHost::FromID(
Replace the implementation of this method to use RenderFrameHost as
RenderViewHost is going away.

https://codereview.chromium.org/854513002/diff/20001/android_webview/common/p...
File android_webview/common/print_messages.h (right):

https://codereview.chromium.org/854513002/diff/20001/android_webview/common/p...
android_webview/common/print_messages.h:380: int /* render_view_id */,
Unless you have a really good reason, don't use RenderView(Host) routing ids.
Use the top-level RenderFrame(Host) routing id.

[sgurun-gerrit only, 2015-01-14 19:35:04]

On 2015/01/14 19:16:33, nasko wrote:
> The main comment is to transition this CL from RenderView(Host) to
> RenderFrame(Host).
> 
>
https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
> File android_webview/browser/aw_printing_message_filter.cc (right):
> 
>
https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
> android_webview/browser/aw_printing_message_filter.cc:23:
> content::RenderViewHost* rvh = content::RenderViewHost::FromID(
> Replace the implementation of this method to use RenderFrameHost as
> RenderViewHost is going away.
> 
>
https://codereview.chromium.org/854513002/diff/20001/android_webview/common/p...
> File android_webview/common/print_messages.h (right):
> 
>
https://codereview.chromium.org/854513002/diff/20001/android_webview/common/p...
> android_webview/common/print_messages.h:380: int /* render_view_id */,
> Unless you have a really good reason, don't use RenderView(Host) routing ids.
> Use the top-level RenderFrame(Host) routing id.

[sgurun-gerrit only, 2015-01-14 19:35:20]

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
File android_webview/browser/aw_printing_message_filter.cc (right):

https://codereview.chromium.org/854513002/diff/20001/android_webview/browser/...
android_webview/browser/aw_printing_message_filter.cc:23:
content::RenderViewHost* rvh = content::RenderViewHost::FromID(
On 2015/01/14 19:16:33, nasko wrote:
> Replace the implementation of this method to use RenderFrameHost as
> RenderViewHost is going away.

Again this code is to bring webview code in line with chrome printing code: 
see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...

I filed crbug.com/448852 for printing folks.

https://codereview.chromium.org/854513002/diff/20001/android_webview/common/p...
File android_webview/common/print_messages.h (right):

https://codereview.chromium.org/854513002/diff/20001/android_webview/common/p...
android_webview/common/print_messages.h:380: int /* render_view_id */,
On 2015/01/14 19:16:33, nasko wrote:
> Unless you have a really good reason, don't use RenderView(Host) routing ids.
> Use the top-level RenderFrame(Host) routing id.

This file is a direct replica of print_messages.h in chrome layer:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/prin...

We are now componentizing it remove the duplicate code. I think it is better for
printing folks to address it after moving it to components.

[nasko, 2015-01-14 19:40:31]

Thanks for filing the bug. Since you are replicating an existing codepath, LGTM.

[sgurun-gerrit only, 2015-01-14 19:47:33]

The CQ bit was checked by sgurun@chromium.org

[commit-bot: I haz the power, 2015-01-14 19:48:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/854513002/20001

[commit-bot: I haz the power, 2015-01-14 21:00:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-01-14 21:00:54]

Patchset 2 (id:??) landed as
https://crrev.com/ee20aa9a5b382180944c5f0367fdddd5f7c22aa5
Cr-Commit-Position: refs/heads/master@{#311544}