Make printing work better with OOPIF.

Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/aaa2bba38197c0bcaa7cbcf15281ef4f9d568818
Cr-Commit-Position: refs/heads/master@{#432353}

[Lei Zhang, 2016-10-17 06:05:36]

Description was changed from

==========
Make printing work better with OOPIF.

BUG=631513
==========

to

==========
Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Lei Zhang, 2016-10-17 06:05:46]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 06:05:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 06:26:14]

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

[commit-bot: I haz the power, 2016-10-17 06:26:15]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[Lei Zhang, 2016-10-17 07:57:34]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-17 07:57:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-17 08:40:18]

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

[commit-bot: I haz the power, 2016-10-17 08:40:19]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nasko, 2016-10-27 20:26:25]

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

[nasko, 2016-10-27 20:26:26]

I haven't read the full CL, but a comment on the RenderFrameObserver changes.

https://codereview.chromium.org/2426503002/diff/20001/components/printing/com...
File components/printing/common/print_messages.h (right):

https://codereview.chromium.org/2426503002/diff/20001/components/printing/com...
components/printing/common/print_messages.h:315: // Tells the render frame to
initiate print preview for the entire document.
nit: s/render frame/RenderFrame/

https://codereview.chromium.org/2426503002/diff/20001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/2426503002/diff/20001/content/public/renderer...
content/public/renderer/render_frame_observer.h:127: virtual void
DidStartLoading() {}
Let's not add this, as Start/Stop Loading is global for the full page, which
isn't per frame.
DidStartLoading can be replaced by DidStartProvisionalLoad.
DidStopLoading can be replaced by combo of DidFailProvisionalLoad for failure
case and DidFinishLoad for the success case.

[Charlie Reis, 2016-10-27 20:27:26]

Description was changed from

==========
Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Lei Zhang, 2016-10-28 23:22:57]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 23:23:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-10-28 23:27:10]

Synced and added some more suggestions from nasko. Most likely still needs a bit
of work.

https://codereview.chromium.org/2426503002/diff/20001/components/printing/com...
File components/printing/common/print_messages.h (right):

https://codereview.chromium.org/2426503002/diff/20001/components/printing/com...
components/printing/common/print_messages.h:315: // Tells the render frame to
initiate print preview for the entire document.
On 2016/10/27 20:26:26, nasko (out until nov 4) wrote:
> nit: s/render frame/RenderFrame/

Done.

https://codereview.chromium.org/2426503002/diff/20001/content/public/renderer...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/2426503002/diff/20001/content/public/renderer...
content/public/renderer/render_frame_observer.h:127: virtual void
DidStartLoading() {}
On 2016/10/27 20:26:26, nasko (slow) wrote:
> Let's not add this, as Start/Stop Loading is global for the full page, which
> isn't per frame.
> DidStartLoading can be replaced by DidStartProvisionalLoad.
> DidStopLoading can be replaced by combo of DidFailProvisionalLoad for failure
> case and DidFinishLoad for the success case.

Done.

[commit-bot: I haz the power, 2016-10-29 00:14:27]

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

[commit-bot: I haz the power, 2016-10-29 00:14:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lei Zhang, 2016-10-29 02:06:24]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-29 02:06:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-10-29 02:15:46]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-29 02:15:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-29 03:26:27]

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

[commit-bot: I haz the power, 2016-10-29 03:26:28]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[nasko, 2016-11-02 04:50:37]

Thanks Lei on pushing this forward. It is starting to shape up pretty well! Some
more comments.

Overall, it will be nice to land some of the code fixups (includes, const
addition, etc) in a separate CL to keep this one easier to manage.

Also, this CL doesn't have a test case of printing from an OOPIF, does it?

https://codereview.chromium.org/2426503002/diff/120001/android_webview/render...
File android_webview/renderer/print_render_frame_observer.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/android_webview/render...
android_webview/renderer/print_render_frame_observer.cc:38:
helper->PrintNode(render_frame()->GetWebFrame()->contextMenuNode());
I hope we send the context menu to the right renderer process on Android. I
don't know for sure if we do.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:154: void
PrintViewManager::RenderProcessGone(base::TerminationStatus status) {
Should this change to RenderFrameDeleted?

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:156:
PrintViewManagerBase::RenderProcessGone(status);
Isn't that gone from the base object?

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:239: scripted_print_rfh_ =
render_frame_host;
Is this just a hack to "pass" the RFH as parameter to
OnSetupScriptedPrintPreview?

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.h (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.h:87: content::RenderFrameHost*
scripted_print_rfh_;
Can you put comments on what each of these is used for? Can they be both set at
the same time to same/different objects?

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_base.cc:107:
base::Bind(&PrintViewManagerBase::BroadcastPrintingEnabled,
minor nit: s/Broadcast/Send/, since the "broadcasting" is done by virtue of
iterating through all frames, but for each instance of a frame we send it an
update message.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_common.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_common.cc:120: :
contents->GetMainFrame();
Would this work correctly if window.print() is called from a frame that is
different than the focused one?

https://codereview.chromium.org/2426503002/diff/120001/chrome/renderer/printi...
File chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/renderer/printi...
chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc:37:
render_view->GetRoutingID()));
Why is this still using the RenderView routing id? Shouldn't it route through
the frame?

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (left):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:1012:
blink::WebLocalFrame* focusedFrame = webView->focusedFrame();
What happens now that this object is per frame if the user has an active
selection in frame A then moves focus over to frame B and frame A calls
window.print() from a script?
Generic question: Is user gesture needed for window.print()?

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:761: if
(web_frame->isWebLocalFrame())
I don't know if this is the best course of action here, but I think it is
reasonable.

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:999:
IPC_MESSAGE_HANDLER(PrintMsg_SetPrintingEnabled, SetPrintingEnabled)
nit: OnSetPrintingEnabled, since it is IPC handler.

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
File components/printing/test/print_web_view_helper_browsertest.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
components/printing/test/print_web_view_helper_browsertest.cc:262:
content::RenderFrame::FromWebFrame(GetMainFrame()));
This means that the test will not exercise out-of-process iframes printing,
right?

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:112:
focused_frame_tree_node_id_(FrameTreeNode::kFrameTreeNodeInvalidId),
I love that you are fixing code and style along the way, but it will be a
smaller diff to review and keep track of if these were committed separately.

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:1045: bool has_selection_;
nit: Put a small comment on what the meaning of this is and when it is
set/reset.

[Lei Zhang, 2016-11-02 22:09:39]

nasko: I'll make a pass and move cleanups to a separate CL. There's no test case
yet, and we probably won't pass it as is.

rockot: Can you help look at the PrintHostMsg_SetupScriptedPrintPreview usage?
It's a unicorn that uses a sync message and pumps the message loop, and the
handler on the browser side is an IPC_MESSAGE_HANDLER_DELAY_REPLY.

- In PrintViewManager::OnMessageReceived() there is a hack to pass the RFH
parameter to the IPC handler. It's a bit ugly but I think it works.
- Uglier still is nasko reported a UAF if one has this CL applied, follows the
repro steps on the bug, and quit the browser.
-- I have been looking at the PrintHostMsg_SetupScriptedPrintPreview message.
--- The sender is the sub-frame renderer, which typically has a routing_id of 4.
--- The message goes to the browser, and the browser replies when print preview
closes. (e.g. just hit cancel) The reply message also has a routing_id of 4.
--- However, the sender who is waiting for the reply does not seem to be getting
the reply.
--- It looks like the delayed sync reply message instead goes to the main frame
renderer. As seen with some additional logging at the beginning of
SyncChannel::SyncContext::OnMessageReceived().

Underneath the hood, it looks like the IPC messages all go through Mojo. Is my
code doing something wrong, or is the message being incorrectly delivered by the
IPC mechanism?

[Ken Rockot(use gerrit already), 2016-11-02 22:25:55]

On 2016/11/02 at 22:09:39, thestig wrote:
> nasko: I'll make a pass and move cleanups to a separate CL. There's no test
case yet, and we probably won't pass it as is.
> 
> rockot: Can you help look at the PrintHostMsg_SetupScriptedPrintPreview usage?
It's a unicorn that uses a sync message and pumps the message loop, and the
handler on the browser side is an IPC_MESSAGE_HANDLER_DELAY_REPLY.
> 
> - In PrintViewManager::OnMessageReceived() there is a hack to pass the RFH
parameter to the IPC handler. It's a bit ugly but I think it works.
> - Uglier still is nasko reported a UAF if one has this CL applied, follows the
repro steps on the bug, and quit the browser.
> -- I have been looking at the PrintHostMsg_SetupScriptedPrintPreview message.
> --- The sender is the sub-frame renderer, which typically has a routing_id of
4.
> --- The message goes to the browser, and the browser replies when print
preview closes. (e.g. just hit cancel) The reply message also has a routing_id
of 4.
> --- However, the sender who is waiting for the reply does not seem to be
getting the reply.
> --- It looks like the delayed sync reply message instead goes to the main
frame renderer. As seen with some additional logging at the beginning of
SyncChannel::SyncContext::OnMessageReceived().
> 
> Underneath the hood, it looks like the IPC messages all go through Mojo. Is my
code doing something wrong, or is the message being incorrectly delivered by the
IPC mechanism?

It's kind of impossible for the IPC mechanism to cross wires. Channels don't
know about each other in any capacity, and replies are ultimately always sent on
a specific Channel by some caller. Let me take a closer look at the CL.

[Lei Zhang, 2016-11-02 22:29:08]

On 2016/11/02 22:25:55, Ken Rockot wrote:
> It's kind of impossible for the IPC mechanism to cross wires. Channels don't
> know about each other in any capacity, and replies are ultimately always sent
on
> a specific Channel by some caller. Let me take a closer look at the CL.

Thanks for helping out. PrintViewManager::OnScriptedPrintPreviewReply() is the
sender of the reply message on the browser side. Let me know if you have any
other questions.

[Ken Rockot(use gerrit already), 2016-11-02 22:32:55]

On 2016/11/02 at 22:25:55, Ken Rockot wrote:
> On 2016/11/02 at 22:09:39, thestig wrote:
> > nasko: I'll make a pass and move cleanups to a separate CL. There's no test
case yet, and we probably won't pass it as is.
> > 
> > rockot: Can you help look at the PrintHostMsg_SetupScriptedPrintPreview
usage? It's a unicorn that uses a sync message and pumps the message loop, and
the handler on the browser side is an IPC_MESSAGE_HANDLER_DELAY_REPLY.
> > 
> > - In PrintViewManager::OnMessageReceived() there is a hack to pass the RFH
parameter to the IPC handler. It's a bit ugly but I think it works.
> > - Uglier still is nasko reported a UAF if one has this CL applied, follows
the repro steps on the bug, and quit the browser.
> > -- I have been looking at the PrintHostMsg_SetupScriptedPrintPreview
message.
> > --- The sender is the sub-frame renderer, which typically has a routing_id
of 4.
> > --- The message goes to the browser, and the browser replies when print
preview closes. (e.g. just hit cancel) The reply message also has a routing_id
of 4.
> > --- However, the sender who is waiting for the reply does not seem to be
getting the reply.
> > --- It looks like the delayed sync reply message instead goes to the main
frame renderer. As seen with some additional logging at the beginning of
SyncChannel::SyncContext::OnMessageReceived().
> > 
> > Underneath the hood, it looks like the IPC messages all go through Mojo. Is
my code doing something wrong, or is the message being incorrectly delivered by
the IPC mechanism?
> 
> It's kind of impossible for the IPC mechanism to cross wires. Channels don't
know about each other in any capacity, and replies are ultimately always sent on
a specific Channel by some caller. Let me take a closer look at the CL.

It looks like the reply in this case is always sent via
WebContentsObserver::Send() -> WebContents::Send() -> RVH::Send().

[Lei Zhang, 2016-11-02 22:42:35]

On 2016/11/02 22:32:55, Ken Rockot wrote:
> It looks like the reply in this case is always sent via
> WebContentsObserver::Send() -> WebContents::Send() -> RVH::Send().

Should I be using RFH::Send() instead?

[nasko, 2016-11-07 19:44:02]

On 2016/11/02 22:42:35, Lei Zhang wrote:
> On 2016/11/02 22:32:55, Ken Rockot wrote:
> > It looks like the reply in this case is always sent via
> > WebContentsObserver::Send() -> WebContents::Send() -> RVH::Send().
> 
> Should I be using RFH::Send() instead?

What matters specifically is that the routing id of the frame is used. The RFH
version of the Send is the correct one to use though, as WebContents' version
does use RVH, which might or might not do the right thing :(. I'll file a bug to
fix WebContents::Send or just remove it altogether.

[Lei Zhang, 2016-11-07 19:48:30]

On 2016/11/07 19:44:02, nasko wrote:
> On 2016/11/02 22:42:35, Lei Zhang wrote:
> > On 2016/11/02 22:32:55, Ken Rockot wrote:
> > > It looks like the reply in this case is always sent via
> > > WebContentsObserver::Send() -> WebContents::Send() -> RVH::Send().
> > 
> > Should I be using RFH::Send() instead?
> 
> What matters specifically is that the routing id of the frame is used. The RFH
> version of the Send is the correct one to use though, as WebContents' version
> does use RVH, which might or might not do the right thing :(. I'll file a bug
to
> fix WebContents::Send or just remove it altogether.

Thanks. I tried RFH::Send() (as Rob suggested on the bug as well) and my local
version of this CL works better now. Trying to fix the Android failure and then
get a new patch set out.

[Lei Zhang, 2016-11-08 11:12:34]

Patchset #8 (id:140001) has been deleted

[Lei Zhang, 2016-11-08 11:13:22]

https://codereview.chromium.org/2426503002/diff/120001/android_webview/render...
File android_webview/renderer/print_render_frame_observer.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/android_webview/render...
android_webview/renderer/print_render_frame_observer.cc:38:
helper->PrintNode(render_frame()->GetWebFrame()->contextMenuNode());
On 2016/11/02 04:50:37, nasko wrote:
> I hope we send the context menu to the right renderer process on Android. I
> don't know for sure if we do.

What's making your spidey sense tingle? I made the same change here as
ChromeRenderFrameObserver::OnPrintNodeUnderContextMenu().

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:154: void
PrintViewManager::RenderProcessGone(base::TerminationStatus status) {
On 2016/11/02 04:50:37, nasko wrote:
> Should this change to RenderFrameDeleted?

Not sure. Added a TODO and will get back to it later in this CL.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:156:
PrintViewManagerBase::RenderProcessGone(status);
On 2016/11/02 04:50:37, nasko wrote:
> Isn't that gone from the base object?

The inheritance tree is several levels deep.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:239: scripted_print_rfh_ =
render_frame_host;
On 2016/11/02 04:50:37, nasko wrote:
> Is this just a hack to "pass" the RFH as parameter to
> OnSetupScriptedPrintPreview?

Yes. IPC_MESSAGE_HANDLER_DELAY_REPLY doesn't seem to work.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.h (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.h:87: content::RenderFrameHost*
scripted_print_rfh_;
On 2016/11/02 04:50:37, nasko wrote:
> Can you put comments on what each of these is used for? Can they be both set
at
> the same time to same/different objects?

Done.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_base.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_base.cc:107:
base::Bind(&PrintViewManagerBase::BroadcastPrintingEnabled,
On 2016/11/02 04:50:37, nasko wrote:
> minor nit: s/Broadcast/Send/, since the "broadcasting" is done by virtue of
> iterating through all frames, but for each instance of a frame we send it an
> update message.

Done.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_common.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_common.cc:120: :
contents->GetMainFrame();
On 2016/11/02 04:50:37, nasko wrote:
> Would this work correctly if window.print() is called from a frame that is
> different than the focused one?

You discussed this in an earlier comment. Added a TODO for now.

https://codereview.chromium.org/2426503002/diff/120001/chrome/renderer/printi...
File chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/renderer/printi...
chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc:37:
render_view->GetRoutingID()));
On 2016/11/02 04:50:37, nasko wrote:
> Why is this still using the RenderView routing id? Shouldn't it route through
> the frame?

PrerenderContents hasn't changed in this CL, so we still need to interact with
it like before.

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (left):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:1012:
blink::WebLocalFrame* focusedFrame = webView->focusedFrame();
On 2016/11/02 04:50:37, nasko wrote:
> What happens now that this object is per frame if the user has an active
> selection in frame A then moves focus over to frame B and frame A calls
> window.print() from a script?

Good question. Will need to look a bit harder.

> Generic question: Is user gesture needed for window.print()?

Nope.

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:761: if
(web_frame->isWebLocalFrame())
On 2016/11/02 04:50:37, nasko wrote:
> I don't know if this is the best course of action here, but I think it is
> reasonable.

I think you wrote this...

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:999:
IPC_MESSAGE_HANDLER(PrintMsg_SetPrintingEnabled, SetPrintingEnabled)
On 2016/11/02 04:50:37, nasko wrote:
> nit: OnSetPrintingEnabled, since it is IPC handler.

Done.

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
File components/printing/test/print_web_view_helper_browsertest.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
components/printing/test/print_web_view_helper_browsertest.cc:262:
content::RenderFrame::FromWebFrame(GetMainFrame()));
On 2016/11/02 04:50:37, nasko wrote:
> This means that the test will not exercise out-of-process iframes printing,
> right?

Correct. We'll need to write more tests at some point.

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
content/browser/frame_host/frame_tree.cc:112:
focused_frame_tree_node_id_(FrameTreeNode::kFrameTreeNodeInvalidId),
On 2016/11/02 04:50:37, nasko wrote:
> I love that you are fixing code and style along the way, but it will be a
> smaller diff to review and keep track of if these were committed separately.

Ack. I'll re-review my own CL and strip off some of the random cleanups in an
upcoming patchset.

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2426503002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:1045: bool has_selection_;
On 2016/11/02 04:50:37, nasko wrote:
> nit: Put a small comment on what the meaning of this is and when it is
> set/reset.

Done.

[Lei Zhang, 2016-11-08 11:36:06]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-08 11:36:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 12:38:58]

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

[commit-bot: I haz the power, 2016-11-08 12:38:59]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-11-08 22:18:35]

Just some minor comments. How far is this CL now? What else needs to be done to
get it ready for commit?

https://codereview.chromium.org/2426503002/diff/120001/android_webview/render...
File android_webview/renderer/print_render_frame_observer.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/android_webview/render...
android_webview/renderer/print_render_frame_observer.cc:38:
helper->PrintNode(render_frame()->GetWebFrame()->contextMenuNode());
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > I hope we send the context menu to the right renderer process on Android. I
> > don't know for sure if we do.
> 
> What's making your spidey sense tingle? I made the same change here as
> ChromeRenderFrameObserver::OnPrintNodeUnderContextMenu().

Looking back at it, I don't know : (. This is renderer side code, so I'm not
sure why I had this comment. Sorry : (

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:154: void
PrintViewManager::RenderProcessGone(base::TerminationStatus status) {
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > Should this change to RenderFrameDeleted?
> 
> Not sure. Added a TODO and will get back to it later in this CL.

Acknowledged.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:156:
PrintViewManagerBase::RenderProcessGone(status);
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > Isn't that gone from the base object?
> 
> The inheritance tree is several levels deep.

Acknowledged.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:239: scripted_print_rfh_ =
render_frame_host;
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > Is this just a hack to "pass" the RFH as parameter to
> > OnSetupScriptedPrintPreview?
> 
> Yes. IPC_MESSAGE_HANDLER_DELAY_REPLY doesn't seem to work.

That macro is a wrapper for IPC_MESSAGE_FORWARD_DELAY_REPLY. Can we use
IPC_MESSAGE_FORWARD_DELAY_REPLY directly and pass in the |render_frame_host| as
the explicit parameter?

https://codereview.chromium.org/2426503002/diff/120001/chrome/renderer/printi...
File chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/renderer/printi...
chrome/renderer/printing/chrome_print_web_view_helper_delegate.cc:37:
render_view->GetRoutingID()));
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > Why is this still using the RenderView routing id? Shouldn't it route
through
> > the frame?
> 
> PrerenderContents hasn't changed in this CL, so we still need to interact with
> it like before.

Acknowledged.

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
File components/printing/renderer/print_web_view_helper.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/re...
components/printing/renderer/print_web_view_helper.cc:761: if
(web_frame->isWebLocalFrame())
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > I don't know if this is the best course of action here, but I think it is
> > reasonable.
> 
> I think you wrote this...

Ah, good point : ). I was looking very critically at each line of code and
didn't realize it was what I sent you as a patch. I did say it is reasonable :
).

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
File components/printing/test/print_web_view_helper_browsertest.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
components/printing/test/print_web_view_helper_browsertest.cc:262:
content::RenderFrame::FromWebFrame(GetMainFrame()));
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > This means that the test will not exercise out-of-process iframes printing,
> > right?
> 
> Correct. We'll need to write more tests at some point.

Is this something I can help with? I don't know much about printing or how to
wait for printing related events, but if you point me at which test case is
closest to the scenario we are solving, I can try to write one that mimics it
for OOPIFs.

[Lei Zhang, 2016-11-08 22:38:40]

On 2016/11/08 22:18:35, nasko wrote:
> Just some minor comments. How far is this CL now? What else needs to be done
to
> get it ready for commit?

- Most of the common printing use cases work. I'll make another pass to make
sure nothing regressed.
- There's still a bunch of TODOs, and some cases that you pointed out that I
haven't tested.

[nasko, 2016-11-09 20:22:27]

On 2016/11/08 22:38:40, Lei Zhang (slow) wrote:
> On 2016/11/08 22:18:35, nasko wrote:
> > Just some minor comments. How far is this CL now? What else needs to be done
> to
> > get it ready for commit?
> 
> - Most of the common printing use cases work. I'll make another pass to make
> sure nothing regressed.

Great! Let us know once it passes the manual testing.

> - There's still a bunch of TODOs, and some cases that you pointed out that I
> haven't tested.

The issue with focused frame and printing frame potentially being different
might be worth solving in a separate CL. I don't know if we will be hitting it
in practice, though window.print() not requiring user gesture does open the
possibility of it happening.

[Lei Zhang, 2016-11-11 21:06:48]

I still have some TODOs to look into and try to write tests.

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:239: scripted_print_rfh_ =
render_frame_host;
On 2016/11/08 22:18:35, nasko wrote:
> On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> > On 2016/11/02 04:50:37, nasko wrote:
> > > Is this just a hack to "pass" the RFH as parameter to
> > > OnSetupScriptedPrintPreview?
> > 
> > Yes. IPC_MESSAGE_HANDLER_DELAY_REPLY doesn't seem to work.
> 
> That macro is a wrapper for IPC_MESSAGE_FORWARD_DELAY_REPLY. Can we use
> IPC_MESSAGE_FORWARD_DELAY_REPLY directly and pass in the |render_frame_host|
as
> the explicit parameter?

I ventured into ipc/ land and came back with a new
IPC_MESSAGE_HANDLER_WITH_PARAM_DELAY_REPLY macro to make this work.
|scripted_print_rfh_| is gone.

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
File components/printing/test/print_web_view_helper_browsertest.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
components/printing/test/print_web_view_helper_browsertest.cc:262:
content::RenderFrame::FromWebFrame(GetMainFrame()));
On 2016/11/08 22:18:35, nasko wrote:
> On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> > On 2016/11/02 04:50:37, nasko wrote:
> > > This means that the test will not exercise out-of-process iframes
printing,
> > > right?
> > 
> > Correct. We'll need to write more tests at some point.
> 
> Is this something I can help with? I don't know much about printing or how to
> wait for printing related events, but if you point me at which test case is
> closest to the scenario we are solving, I can try to write one that mimics it
> for OOPIFs.

PrintWithIframe below might be the closest, but it looks old and crufty.

I'll take a shot at writing some more test cases here. If I write a test case,
will the linux_site_isolation bot automatically run the same test with
--site-per-process ?

[Łukasz Anforowicz, 2016-11-11 21:29:01]

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

[Łukasz Anforowicz, 2016-11-11 21:29:02]

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
File components/printing/test/print_web_view_helper_browsertest.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/components/printing/te...
components/printing/test/print_web_view_helper_browsertest.cc:262:
content::RenderFrame::FromWebFrame(GetMainFrame()));
On 2016/11/11 21:06:48, Lei Zhang (slow) wrote:
> On 2016/11/08 22:18:35, nasko wrote:
> > On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> > > On 2016/11/02 04:50:37, nasko wrote:
> > > > This means that the test will not exercise out-of-process iframes
> printing,
> > > > right?
> > > 
> > > Correct. We'll need to write more tests at some point.
> > 
> > Is this something I can help with? I don't know much about printing or how
to
> > wait for printing related events, but if you point me at which test case is
> > closest to the scenario we are solving, I can try to write one that mimics
it
> > for OOPIFs.
> 
> PrintWithIframe below might be the closest, but it looks old and crufty.
> 
> I'll take a shot at writing some more test cases here. If I write a test case,
> will the linux_site_isolation bot automatically run the same test with
> --site-per-process ?

Correct - linux_site_isolation trybot will run a bunch of tests with
--site-per-process.  This includes content_browsertests, browser_tests,
components_browsertests, layout tests and a few others - for details which test
suites are covered you can look at testing/buildbot/chromium.fyi.json and search
for "Site Isolation Linux" (note that this config file configures [with a single
entry] both the FYI bot *and* the linux_site_isolation trybot).  

Additionally content_browsertests and browser_tests are also run with and
without --site-per-process by CQ (i.e. see site_per_process_content_browsertests
step being executed by linux_chromium_rel_ng bot and configured by an entry in
testing/buildbot/chromium.linux.json).

[Lei Zhang, 2016-11-12 00:03:12]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[Lei Zhang, 2016-11-12 00:03:19]

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager_common.cc (right):

https://codereview.chromium.org/2426503002/diff/120001/chrome/browser/printin...
chrome/browser/printing/print_view_manager_common.cc:120: :
contents->GetMainFrame();
On 2016/11/08 11:13:22, Lei Zhang (slow) wrote:
> On 2016/11/02 04:50:37, nasko wrote:
> > Would this work correctly if window.print() is called from a frame that is
> > different than the focused one?
> 
> You discussed this in an earlier comment. Added a TODO for now.

window.print() doesn't go through this path. This is for user initiated printing
from the browser side.

For window.print(), the request to print comes from
PrintWebViewHelper::ScriptedPrint(), where we already know the RenderFrame that
issued the print call.

[commit-bot: I haz the power, 2016-11-12 00:03:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-11-12 00:59:38]

I'm just working on new tests now. So PTAL.

[commit-bot: I haz the power, 2016-11-12 01:23:50]

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

[commit-bot: I haz the power, 2016-11-12 01:23:51]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-11-14 20:28:52]

This is looking awesome! I have mostly nits and only one question about possible
correctness issue. I'll also suggest asking tsepez@ look over the IPC macros,
though they look good to my eyes.
Thanks a lot!

https://codereview.chromium.org/2426503002/diff/200001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.h (right):

https://codereview.chromium.org/2426503002/diff/200001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.h:92: content::RenderFrameHost*
scripted_print_rfh_;
Awesome! Thanks for removing this!

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:178: content::RenderProcessHost*
rph = web_contents()->GetRenderProcessHost();
This probably should be rfh->GetProcess() as it currently the code will get the
process of the top-level frame, not the subframe, which will be in a different
process with OOPIF.

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.h (right):

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.h:54: void
RenderFrameDeleted(content::RenderFrameHost* rfh) override;
nit: Be consistent with parameter names - render_frame_host like the other two
functions around it.

https://codereview.chromium.org/2426503002/diff/260001/components/printing/re...
File components/printing/renderer/print_web_view_helper.h (right):

https://codereview.chromium.org/2426503002/diff/260001/components/printing/re...
components/printing/renderer/print_web_view_helper.h:80: class
PrintWebViewHelper
nit: Maybe in another CL, this class looks more like PrintWebFrameHelper now
that it is an RFO. I think though that there are too many instances of WebView
substring in naming to be worth tackling right now.

https://codereview.chromium.org/2426503002/diff/260001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2426503002/diff/260001/content/public/browser...
content/public/browser/render_frame_host.h:214: // Returns true if the frame has
selection.
nit: "has a selection."

https://codereview.chromium.org/2426503002/diff/260001/content/public/rendere...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/2426503002/diff/260001/content/public/rendere...
content/public/renderer/render_frame_observer.h:124: // For printing.
nit: Can you add a bit more explanation than "for printing". If I understand it
right, this is the way we actually invoke printing as it is implemented outside
of RF and RF only serves as a conduit for the signal that we need to start a
print, right?

https://codereview.chromium.org/2426503002/diff/260001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/2426503002/diff/260001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:384: } break;
nit: break on a new line.

[Lei Zhang, 2016-11-14 23:52:47]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[Lei Zhang, 2016-11-14 23:52:48]

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.cc (right):

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.cc:178: content::RenderProcessHost*
rph = web_contents()->GetRenderProcessHost();
On 2016/11/14 20:28:52, nasko (out until Nov 17) wrote:
> This probably should be rfh->GetProcess() as it currently the code will get
the
> process of the top-level frame, not the subframe, which will be in a different
> process with OOPIF.

Done. I've made a separate patch set to specifically address this comment.

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
File chrome/browser/printing/print_view_manager.h (right):

https://codereview.chromium.org/2426503002/diff/260001/chrome/browser/printin...
chrome/browser/printing/print_view_manager.h:54: void
RenderFrameDeleted(content::RenderFrameHost* rfh) override;
On 2016/11/14 20:28:52, nasko (out until Nov 17) wrote:
> nit: Be consistent with parameter names - render_frame_host like the other two
> functions around it.

Done.

https://codereview.chromium.org/2426503002/diff/260001/components/printing/re...
File components/printing/renderer/print_web_view_helper.h (right):

https://codereview.chromium.org/2426503002/diff/260001/components/printing/re...
components/printing/renderer/print_web_view_helper.h:80: class
PrintWebViewHelper
On 2016/11/14 20:28:52, nasko (out until Nov 17) wrote:
> nit: Maybe in another CL, this class looks more like PrintWebFrameHelper now
> that it is an RFO. I think though that there are too many instances of WebView
> substring in naming to be worth tackling right now.

Yes, will do later. I don't want to rename everything in this CL.

https://codereview.chromium.org/2426503002/diff/260001/content/public/browser...
File content/public/browser/render_frame_host.h (right):

https://codereview.chromium.org/2426503002/diff/260001/content/public/browser...
content/public/browser/render_frame_host.h:214: // Returns true if the frame has
selection.
On 2016/11/14 20:28:52, nasko (out until Nov 17) wrote:
> nit: "has a selection."

Done.

https://codereview.chromium.org/2426503002/diff/260001/content/public/rendere...
File content/public/renderer/render_frame_observer.h (right):

https://codereview.chromium.org/2426503002/diff/260001/content/public/rendere...
content/public/renderer/render_frame_observer.h:124: // For printing.
On 2016/11/14 20:28:52, nasko (out until Nov 17) wrote:
> nit: Can you add a bit more explanation than "for printing". If I understand
it
> right, this is the way we actually invoke printing as it is implemented
outside
> of RF and RF only serves as a conduit for the signal that we need to start a
> print, right?

Yes, thank you for reminding me to put it in a better comment. I copied parts of
printPage() from third_party/WebKit/public/web/WebViewClient.h.

https://codereview.chromium.org/2426503002/diff/260001/ipc/ipc_message_macros.h
File ipc/ipc_message_macros.h (right):

https://codereview.chromium.org/2426503002/diff/260001/ipc/ipc_message_macros...
ipc/ipc_message_macros.h:384: } break;
On 2016/11/14 20:28:52, nasko (out until Nov 17) wrote:
> nit: break on a new line.

Done.

[nasko, 2016-11-15 01:10:08]

Awesome! LGTM. I'd leave it up to you whether tests are added in this CL or as a
follow up.

[Lei Zhang, 2016-11-15 01:35:34]

On 2016/11/15 01:10:08, nasko (out until Nov 17) wrote:
> Awesome! LGTM. I'd leave it up to you whether tests are added in this CL or as
a
> follow up.

I'm almost done with a test. I'll upload it as a separate CL first, and then we
can either include it here or land it separately.

[Lei Zhang, 2016-11-15 01:37:16]

thestig@chromium.org changed reviewers:
+ rockot@chromium.org

[Lei Zhang, 2016-11-15 01:37:17]

+rockot again to review ipc/ changes. IPC_MESSAGE_HANDLER_WITH_PARAM_DELAY_REPLY
is used in chrome/browser/printing/print_view_manager.cc - refer to patch set 7
to see my hack without the ipc/ changes.

[Lei Zhang, 2016-11-15 03:04:45]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 03:05:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-11-15 03:10:10]

Test CL is https://codereview.chromium.org/2496203003/ for anyone that's
interested.

[commit-bot: I haz the power, 2016-11-15 05:02:38]

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

[commit-bot: I haz the power, 2016-11-15 05:02:39]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-11-15 17:52:48]

ipc lgtm

[Lei Zhang, 2016-11-15 18:40:00]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-11-15 18:40:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 18:55:08]

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

[commit-bot: I haz the power, 2016-11-15 18:55:10]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Lei Zhang, 2016-11-15 19:11:01]

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

[Lei Zhang, 2016-11-15 19:11:03]

+sgurun for android_webview/

[Lei Zhang, 2016-11-15 23:13:40]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 23:14:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sgurun-gerrit only, 2016-11-15 23:19:24]

On 2016/11/15 19:11:03, Lei Zhang (not reviewing code) wrote:
> +sgurun for android_webview/

lgtm

[commit-bot: I haz the power, 2016-11-15 23:39:57]

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

[commit-bot: I haz the power, 2016-11-15 23:39:59]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Lei Zhang, 2016-11-16 01:06:48]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-11-16 01:06:49]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/2426503002/#ps340001
(title: "Better plumbing in AwPrintManager")

[commit-bot: I haz the power, 2016-11-16 01:07:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 02:07:21]

Description was changed from

==========
Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-11-16 02:07:28]

Committed patchset #17 (id:340001)

[commit-bot: I haz the power, 2016-11-16 02:09:41]

Description was changed from

==========
Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make printing work better with OOPIF.

BUG=631513
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/aaa2bba38197c0bcaa7cbcf15281ef4f9d568818
Cr-Commit-Position: refs/heads/master@{#432353}
==========

[commit-bot: I haz the power, 2016-11-16 02:09:43]

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

[sgurun-gerrit only, 2016-11-16 16:11:39]

A revert of this CL (patchset #17 id:340001) has been created in
https://codereview.chromium.org/2510753002/ by sgurun@chromium.org.

The reason for reverting is: webview printing cts test fails..

[Lei Zhang, 2016-11-17 03:25:17]

Next attempt is https://codereview.chromium.org/2508923003