Adds PrintingContext implementation stub for Android.

Adds PrintingContext implementation stub for Android.

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

[Lei Zhang, 2013-08-13 23:26:37]

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
File chrome/browser/printing/print_view_manager_basic.cc (right):

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
chrome/browser/printing/print_view_manager_basic.cc:18: file_descriptor_(-1,
false)
The FileDescriptor() version of the ctor does this already.

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
chrome/browser/printing/print_view_manager_basic.h:26: void
SetFileDescriptor(const base::FileDescriptor& file_descriptor);
nit: add a blank line after this.

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
chrome/browser/printing/print_view_manager_basic.h:28: base::FileDescriptor
GetFileDescriptor();
const method

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:186: temp_file_fd->auto_close
= false;
You probably want this set to true? If not, can you explain why it needs to be
false?

https://codereview.chromium.org/23116003/diff/2001/chrome/browser/printing/pr...
chrome/browser/printing/printing_message_filter.cc:209: #endif
Are you going to handle the TempFileForPrintingWritten msg on Android at all? If
yes, add:

#elif defined(OS_ANDROID)
NOTIMPLEMENTED();

https://codereview.chromium.org/23116003/diff/2001/chrome/common/print_messag...
File chrome/common/print_messages.h (right):

https://codereview.chromium.org/23116003/diff/2001/chrome/common/print_messag...
chrome/common/print_messages.h:373: #if defined(USE_X11) || defined(OS_ANDROID)
BTW, can you change USE_X11 to OS_CHROMEOS?

https://codereview.chromium.org/23116003/diff/2001/chrome/common/print_messag...
chrome/common/print_messages.h:377: int /* render_view_id */, // Used only by
Android.
We can use the RenderView id on CrOS as well. Just leave a TODO(thestig) in
printing_message_filter.cc.

https://codereview.chromium.org/23116003/diff/2001/chrome/renderer/printing/p...
File chrome/renderer/printing/print_web_view_helper_linux.cc (left):

https://codereview.chromium.org/23116003/diff/2001/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper_linux.cc:8: #include
"base/logging.h"
Undo - logging.h also contain DCHECK(), NOTREACHED(), etc.

https://codereview.chromium.org/23116003/diff/2001/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper_linux.cc:45:
print_preview_context_.GetPrintCanvasSize(),
one arg per line is acceptable, no need to change it.

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

https://codereview.chromium.org/23116003/diff/2001/chrome/renderer/printing/p...
chrome/renderer/printing/print_web_view_helper_linux.cc:111: return false;
bad indent

https://codereview.chromium.org/23116003/diff/2001/printing/printed_document.cc
File printing/printed_document.cc (right):

https://codereview.chromium.org/23116003/diff/2001/printing/printed_document....
printing/printed_document.cc:228: #if defined(OS_POSIX) && (defined(USE_AURA) ||
defined(OS_ANDROID))
Can you make the #if statement match the comment?
#if (defined(OS_POSIX) && (defined(USE_AURA)) || defined(OS_ANDROID) ?

https://codereview.chromium.org/23116003/diff/2001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/23116003/diff/2001/printing/printing.gyp#newc...
printing/printing.gyp:233: 'printing_context_android.h',
No printing_context_android.cc? How does that work?

https://codereview.chromium.org/23116003/diff/2001/printing/printing_context_...
File printing/printing_context_android.h (right):

https://codereview.chromium.org/23116003/diff/2001/printing/printing_context_...
printing/printing_context_android.h:8: #include <jni.h>
nit: in Chromium's interpretation of the Google C++ style guide, there's a blank
line to separate C and C++ headers.

https://codereview.chromium.org/23116003/diff/2001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/23116003/diff/2001/skia/skia_library.gypi#new...
skia/skia_library.gypi:23: ['OS=="ios" or (OS=="android" and enable_printing ==
0)', {
Maybe just: 'OS=="ios" or enabling_printing==0' ?

[whywhat, 2013-08-13 23:38:48]

Thanks for the quick review!

https://codereview.chromium.org/23116003/diff/2001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/23116003/diff/2001/printing/printing.gyp#newc...
printing/printing.gyp:233: 'printing_context_android.h',
On 2013/08/13 23:26:37, Lei Zhang wrote:
> No printing_context_android.cc? How does that work?

We figured that printing will not be compiled for Android without the private
repo (e.g. TestShell doesn't use printing). So we have a header in the public
repo but the .cc file is in the private repo. This is already used for
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
for example.

Otherwise we'd have to provide a dummy implementation and some run-time hook
(like with MediaPlayerManager,
https://code.google.com/p/chromium/codesearch#chromium/src/media/base/android...)
to override the dummy implementation if the private repo is present.

Unfortunately, GYP can't replace .cc file based on whether we have the private
repo or not AFAIK.

[cimamoglu1, 2013-08-14 16:10:36]

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
File chrome/browser/printing/print_view_manager_basic.cc (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
chrome/browser/printing/print_view_manager_basic.cc:18: file_descriptor_(-1,
false)
On 2013/08/13 23:26:37, Lei Zhang wrote:
> The FileDescriptor() version of the ctor does this already.

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
chrome/browser/printing/print_view_manager_basic.h:26: void
SetFileDescriptor(const base::FileDescriptor& file_descriptor);
On 2013/08/13 23:26:37, Lei Zhang wrote:
> nit: add a blank line after this.

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
chrome/browser/printing/print_view_manager_basic.h:28: base::FileDescriptor
GetFileDescriptor();
On 2013/08/13 23:26:37, Lei Zhang wrote:
> const method

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
File chrome/browser/printing/printing_message_filter.cc (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.cc:186: temp_file_fd->auto_close
= false;
On 2013/08/13 23:26:37, Lei Zhang wrote:
> You probably want this set to true? If not, can you explain why it needs to be
> false?

The file descriptor is created in & passed from Android side, and it will handle
the closing.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/browser/prin...
chrome/browser/printing/printing_message_filter.cc:209: #endif
On 2013/08/13 23:26:37, Lei Zhang wrote:
> Are you going to handle the TempFileForPrintingWritten msg on Android at all?
If
> yes, add:
> 
> #elif defined(OS_ANDROID)
> NOTIMPLEMENTED();

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/common/print...
File chrome/common/print_messages.h (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/common/print...
chrome/common/print_messages.h:373: #if defined(USE_X11) || defined(OS_ANDROID)
On 2013/08/13 23:26:37, Lei Zhang wrote:
> BTW, can you change USE_X11 to OS_CHROMEOS?

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/common/print...
chrome/common/print_messages.h:377: int /* render_view_id */, // Used only by
Android.
On 2013/08/13 23:26:37, Lei Zhang wrote:
> We can use the RenderView id on CrOS as well. Just leave a TODO(thestig) in
> printing_message_filter.cc.

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/renderer/pri...
File chrome/renderer/printing/print_web_view_helper_linux.cc (left):

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/renderer/pri...
chrome/renderer/printing/print_web_view_helper_linux.cc:8: #include
"base/logging.h"
On 2013/08/13 23:26:37, Lei Zhang wrote:
> Undo - logging.h also contain DCHECK(), NOTREACHED(), etc.

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/renderer/pri...
chrome/renderer/printing/print_web_view_helper_linux.cc:45:
print_preview_context_.GetPrintCanvasSize(),
On 2013/08/13 23:26:37, Lei Zhang wrote:
> one arg per line is acceptable, no need to change it.

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/renderer/pri...
File chrome/renderer/printing/print_web_view_helper_linux.cc (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/chrome/renderer/pri...
chrome/renderer/printing/print_web_view_helper_linux.cc:111: return false;
On 2013/08/13 23:26:37, Lei Zhang wrote:
> bad indent

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/printing/printed_do...
File printing/printed_document.cc (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/printing/printed_do...
printing/printed_document.cc:228: #if defined(OS_POSIX) && (defined(USE_AURA) ||
defined(OS_ANDROID))
On 2013/08/13 23:26:37, Lei Zhang wrote:
> Can you make the #if statement match the comment?
> #if (defined(OS_POSIX) && (defined(USE_AURA)) || defined(OS_ANDROID) ?

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/printing/printing_c...
File printing/printing_context_android.h (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/printing/printing_c...
printing/printing_context_android.h:8: #include <jni.h>
On 2013/08/13 23:26:37, Lei Zhang wrote:
> nit: in Chromium's interpretation of the Google C++ style guide, there's a
blank
> line to separate C and C++ headers.

Done.

https://chromiumcodereview.appspot.com/23116003/diff/2001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://chromiumcodereview.appspot.com/23116003/diff/2001/skia/skia_library.g...
skia/skia_library.gypi:23: ['OS=="ios" or (OS=="android" and enable_printing ==
0)', {
On 2013/08/13 23:26:37, Lei Zhang wrote:
> Maybe just: 'OS=="ios" or enabling_printing==0' ?

Done.

[Vitaly Buka (NO REVIEWS), 2013-08-14 16:23:42]

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper_android.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper_android.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
// Copyright 2013 The Chromium Authors. All rights reserved.

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper_android.cc:7: #include
"chrome/renderer/printing/print_web_view_helper_linux.cc"
why do you need print_web_view_helper_android.cc
Can you just compile  "chrome/renderer/printing/print_web_view_helper_linux.cc"
?

https://codereview.chromium.org/23116003/diff/18001/printing/printing_context...
File printing/printing_context_android.h (right):

https://codereview.chromium.org/23116003/diff/18001/printing/printing_context...
printing/printing_context_android.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
// Copyright 2013 The Chromium Authors. All rights reserved.

[whywhat, 2013-08-14 16:28:22]

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper_android.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper_android.cc:7: #include
"chrome/renderer/printing/print_web_view_helper_linux.cc"
On 2013/08/14 16:23:42, Vitaly Buka wrote:
> why do you need print_web_view_helper_android.cc
> Can you just compile 
"chrome/renderer/printing/print_web_view_helper_linux.cc"
> ?

_linux.cc files are excluded from Android builds (see
build/filename_rules.gypi).
Including the file locally didn't help so Cihat ended up with this workaround.

[Lei Zhang, 2013-08-14 18:49:52]

https://codereview.chromium.org/23116003/diff/18001/chrome/browser/printing/p...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:190: temp_file_fd->fd =
print_view_manager->GetFileDescriptor().fd;
I don't know what the exact lifetime of
|PrintViewManagerBasic::file_descriptor_| is, but you probably should set it to
-1 at some point, here or in OnTempFileForPrintingWritten(), so the same file
descriptor doesn't somehow accidentally get reused.

For instance, if the renderer is misbehaving and asks for a FD twice, you can
deny it on the second attempt because you know on the browser side that you have
already given it the FD.

https://codereview.chromium.org/23116003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:213: NOTIMPLEMENTED();
So can you tell me what's going to happen with this? Is the Clank build going to
do something different here, or are you going to add more code here?

[cimamoglu1, 2013-08-14 20:38:09]

https://codereview.chromium.org/23116003/diff/18001/chrome/browser/printing/p...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:190: temp_file_fd->fd =
print_view_manager->GetFileDescriptor().fd;
On 2013/08/14 18:49:52, Lei Zhang wrote:
> I don't know what the exact lifetime of
> |PrintViewManagerBasic::file_descriptor_| is, but you probably should set it
to
> -1 at some point, here or in OnTempFileForPrintingWritten(), so the same file
> descriptor doesn't somehow accidentally get reused.
> 
> For instance, if the renderer is misbehaving and asks for a FD twice, you can
> deny it on the second attempt because you know on the browser side that you
have
> already given it the FD.

Done.

https://codereview.chromium.org/23116003/diff/18001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:213: NOTIMPLEMENTED();
On 2013/08/14 18:49:52, Lei Zhang wrote:
> So can you tell me what's going to happen with this? Is the Clank build going
to
> do something different here, or are you going to add more code here?

I was planning to add more code in the future. Your comment above actually made
me do now = )

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper_android.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper_android.cc:1: // Copyright (c)
2013 The Chromium Authors. All rights reserved.
On 2013/08/14 16:23:42, Vitaly Buka wrote:
> // Copyright 2013 The Chromium Authors. All rights reserved.

Done.

https://codereview.chromium.org/23116003/diff/18001/printing/printing_context...
File printing/printing_context_android.h (right):

https://codereview.chromium.org/23116003/diff/18001/printing/printing_context...
printing/printing_context_android.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2013/08/14 16:23:42, Vitaly Buka wrote:
> // Copyright 2013 The Chromium Authors. All rights reserved.

Done.

[Lei Zhang, 2013-08-14 20:55:46]

+tsepez for chrome/common/print_messages.h. Do you remember why we used a
sequence number instead of just using the render view id in
http://crrev.com/71244 ?

[Tom Sepez, 2013-08-14 21:07:10]

On 2013/08/14 20:55:46, Lei Zhang wrote:
> +tsepez for chrome/common/print_messages.h. Do you remember why we used a
> sequence number instead of just using the render view id in
> http://crrev.com/71244 ?

Might have something to do with a single render view spawning multiple prints? 
Messages LGTM otherwise.

[Lei Zhang, 2013-08-15 01:25:22]

On 2013/08/14 21:07:10, Tom Sepez wrote:
> On 2013/08/14 20:55:46, Lei Zhang wrote:
> > +tsepez for chrome/common/print_messages.h. Do you remember why we used a
> > sequence number instead of just using the render view id in
> > http://crrev.com/71244 ?
> 
> Might have something to do with a single render view spawning multiple prints?

> Messages LGTM otherwise.

Every tab seems to have its own render view id. If I just keep opening new NTP
pages, they are still part of the same renderer process, but they have 1, 3, 5
... as their render view ids, and the ids don't get reused based on my limited
testing.

[Lei Zhang, 2013-08-15 01:31:05]

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:189: temp_file_fd->auto_close
= false;
So after you hand off the fd to the renderer, and the renderer crashes, aren't
you leaking a fd? Since you are not auto closing, you probably need to do a
little more work to prevent fd leaks.

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:218:
print_view_manager->SetFileDescriptor(base::FileDescriptor(-1, false));
Don't you need to do something here to tell the upper layers the renderer is
done writing and it is time to close the fd and send the result out to the
printer?

[tomhudson, 2013-08-15 13:04:35]

Skia gypi changes LGTM

[cimamoglu1, 2013-08-15 18:36:13]

I may have found a nasty bug that affects us (and very probably Chrome OS). I
already talked to Lei & Vitaly about it, but I'm writing here in more detail:

The issue
PrintJobWorker::OnNewPage() is being called infinitely through recursion

Why?
This line (
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...
) would always return false, triggering a 500ms delayed invocation of the
function itself.

The line would always return false because PrintedDocument::SetPage is not
called for Chrome OS or us (since it's only called through
PrintViewManagerBase::OnDidPrintPage and this message is not sent for Chrome OS,
as you can see in
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/pr...)

The actual printing would still work because a print dialog is created for
Chrome OS at
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...
and that dialog still gets the PDF, and does whatever it needs to do with it.

How does it affect us?
1) It's really bad to have a method calling itself through infinite recursion
(for performance, memory & general stability)
2) PrintJobWorker::OnDocumentDone() and hence PrintingContext::DocumentDone()
are not called.

[whywhat, 2013-08-15 20:19:56]

Some nits I've got while going through the change. Feel free to ignore it if it
conflicts with other reviewers previous comments that I may have missed.

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_view_manager_basic.h:26: void
SetFileDescriptor(const base::FileDescriptor& file_descriptor);
these methods could actually be set_file_descriptor() and file_descriptor() and
even be inlined here since it's just accessors for the private member of the
class

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_view_manager_basic.h:36: // The file descriptor
the PDF of the page will be written.
s/written/written into/

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/printing_message_filter.cc:187: temp_file_fd->fd =
print_view_manager->GetFileDescriptor().fd;
Check that GetFileDescritor() is not NULL?

https://codereview.chromium.org/23116003/diff/1/chrome/renderer/printing/prin...
File chrome/renderer/printing/print_web_view_helper_linux.cc (right):

https://codereview.chromium.org/23116003/diff/1/chrome/renderer/printing/prin...
chrome/renderer/printing/print_web_view_helper_linux.cc:43:
PrintPageInternal(page_params, print_preview_context_.GetPrintCanvasSize(),
Return this back to the next line

https://codereview.chromium.org/23116003/diff/1/chrome/renderer/printing/prin...
chrome/renderer/printing/print_web_view_helper_linux.cc:111: return false;
Fix indent here.

[whywhat, 2013-08-15 22:41:42]

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.h:26: void
SetFileDescriptor(const base::FileDescriptor& file_descriptor);
It's currently unclear how we will set this. We'll need to bind the
PrintingContext with PrintViewManager somehow. Any thoughts on how to do this?

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:189: temp_file_fd->auto_close
= false;
On 2013/08/15 01:31:05, Lei Zhang wrote:
> So after you hand off the fd to the renderer, and the renderer crashes, aren't
> you leaking a fd? Since you are not auto closing, you probably need to do a
> little more work to prevent fd leaks.

How is the browser notified when the renderer crashed? We need to handle this
case and notify the owner of the FD.

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:218:
print_view_manager->SetFileDescriptor(base::FileDescriptor(-1, false));
On 2013/08/15 01:31:05, Lei Zhang wrote:
> Don't you need to do something here to tell the upper layers the renderer is
> done writing and it is time to close the fd and send the result out to the
> printer?

We could use the fd as the key here. When fd is created, we remember it on the
Java side in a singleton mapping the fd to the fd owner.
From this method we'd call a static Java method via JNI in PrintingContext that
would find the fd owner and notify it.

[Lei Zhang, 2013-08-15 23:18:01]

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:189: temp_file_fd->auto_close
= false;
On 2013/08/15 22:41:43, whywhat wrote:
> On 2013/08/15 01:31:05, Lei Zhang wrote:
> > So after you hand off the fd to the renderer, and the renderer crashes,
aren't
> > you leaking a fd? Since you are not auto closing, you probably need to do a
> > little more work to prevent fd leaks.
> 
> How is the browser notified when the renderer crashed? We need to handle this
> case and notify the owner of the FD.

PrintViewManagerBase -> content::WebContentsObserver ->
content::WebContentsObserverRenderProcessGone(). You probably want to notify the
upper layer and set the fd to -1 in PrintViewManagerBase.

https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:218:
print_view_manager->SetFileDescriptor(base::FileDescriptor(-1, false));
On 2013/08/15 22:41:43, whywhat wrote:
> On 2013/08/15 01:31:05, Lei Zhang wrote:
> > Don't you need to do something here to tell the upper layers the renderer is
> > done writing and it is time to close the fd and send the result out to the
> > printer?
> 
> We could use the fd as the key here. When fd is created, we remember it on the
> Java side in a singleton mapping the fd to the fd owner.
> From this method we'd call a static Java method via JNI in PrintingContext
that
> would find the fd owner and notify it.

I'm just trying to understand if that's something you can do from here, where
the renderer is explicitly telling you, "hey I'm done" versus if you need the
rest of the printing system to chew though it and get to
PrintingContext::DocumentDone() somehow.

I mentioned to Cihat the idea of just calling some dummy function here. How the
dummy function is implemented in Clank is up to you. To me, it feels better to
handle it here. The fd comes in at the chrome/ layer via
PrintViewManagerBasic::SetFileDescriptor() somehow, and you can reply at the
chrome/ layer via the dummy function.

[whywhat, 2013-08-16 00:58:28]

On 2013/08/15 23:18:01, Lei Zhang wrote:
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> File chrome/browser/printing/printing_message_filter.cc (right):
> 
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> chrome/browser/printing/printing_message_filter.cc:189:
temp_file_fd->auto_close
> = false;
> On 2013/08/15 22:41:43, whywhat wrote:
> > On 2013/08/15 01:31:05, Lei Zhang wrote:
> > > So after you hand off the fd to the renderer, and the renderer crashes,
> aren't
> > > you leaking a fd? Since you are not auto closing, you probably need to do
a
> > > little more work to prevent fd leaks.
> > 
> > How is the browser notified when the renderer crashed? We need to handle
this
> > case and notify the owner of the FD.
> 
> PrintViewManagerBase -> content::WebContentsObserver ->
> content::WebContentsObserverRenderProcessGone(). You probably want to notify
the
> upper layer and set the fd to -1 in PrintViewManagerBase.
> 
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> chrome/browser/printing/printing_message_filter.cc:218:
> print_view_manager->SetFileDescriptor(base::FileDescriptor(-1, false));
> On 2013/08/15 22:41:43, whywhat wrote:
> > On 2013/08/15 01:31:05, Lei Zhang wrote:
> > > Don't you need to do something here to tell the upper layers the renderer
is
> > > done writing and it is time to close the fd and send the result out to the
> > > printer?
> > 
> > We could use the fd as the key here. When fd is created, we remember it on
the
> > Java side in a singleton mapping the fd to the fd owner.
> > From this method we'd call a static Java method via JNI in PrintingContext
> that
> > would find the fd owner and notify it.
> 
> I'm just trying to understand if that's something you can do from here, where
> the renderer is explicitly telling you, "hey I'm done" versus if you need the
> rest of the printing system to chew though it and get to
> PrintingContext::DocumentDone() somehow.
> 
> I mentioned to Cihat the idea of just calling some dummy function here. How
the
> dummy function is implemented in Clank is up to you. To me, it feels better to
> handle it here. The fd comes in at the chrome/ layer via
> PrintViewManagerBasic::SetFileDescriptor() somehow, and you can reply at the
> chrome/ layer via the dummy function.

I agree.
The question that remains is how to pass the fd to PrintViewManagerBasic. We'll
receive it only when handling the AskUserForSettings request and we only have a
pointer to PrintingContextAndroid from Java at this moment. Ideally, we'd set it
in one of the browser callbacks called when AskUserForSettings result sent back
from Java to the renderer. What do you think the best place to do it?

[Lei Zhang, 2013-08-16 01:03:15]

On 2013/08/16 00:58:28, whywhat wrote:
> On 2013/08/15 23:18:01, Lei Zhang wrote:
> >
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> > File chrome/browser/printing/printing_message_filter.cc (right):
> > 
> >
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> > chrome/browser/printing/printing_message_filter.cc:189:
> temp_file_fd->auto_close
> > = false;
> > On 2013/08/15 22:41:43, whywhat wrote:
> > > On 2013/08/15 01:31:05, Lei Zhang wrote:
> > > > So after you hand off the fd to the renderer, and the renderer crashes,
> > aren't
> > > > you leaking a fd? Since you are not auto closing, you probably need to
do
> a
> > > > little more work to prevent fd leaks.
> > > 
> > > How is the browser notified when the renderer crashed? We need to handle
> this
> > > case and notify the owner of the FD.
> > 
> > PrintViewManagerBase -> content::WebContentsObserver ->
> > content::WebContentsObserverRenderProcessGone(). You probably want to notify
> the
> > upper layer and set the fd to -1 in PrintViewManagerBase.
> > 
> >
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> > chrome/browser/printing/printing_message_filter.cc:218:
> > print_view_manager->SetFileDescriptor(base::FileDescriptor(-1, false));
> > On 2013/08/15 22:41:43, whywhat wrote:
> > > On 2013/08/15 01:31:05, Lei Zhang wrote:
> > > > Don't you need to do something here to tell the upper layers the
renderer
> is
> > > > done writing and it is time to close the fd and send the result out to
the
> > > > printer?
> > > 
> > > We could use the fd as the key here. When fd is created, we remember it on
> the
> > > Java side in a singleton mapping the fd to the fd owner.
> > > From this method we'd call a static Java method via JNI in PrintingContext
> > that
> > > would find the fd owner and notify it.
> > 
> > I'm just trying to understand if that's something you can do from here,
where
> > the renderer is explicitly telling you, "hey I'm done" versus if you need
the
> > rest of the printing system to chew though it and get to
> > PrintingContext::DocumentDone() somehow.
> > 
> > I mentioned to Cihat the idea of just calling some dummy function here. How
> the
> > dummy function is implemented in Clank is up to you. To me, it feels better
to
> > handle it here. The fd comes in at the chrome/ layer via
> > PrintViewManagerBasic::SetFileDescriptor() somehow, and you can reply at the
> > chrome/ layer via the dummy function.
> 
> I agree.
> The question that remains is how to pass the fd to PrintViewManagerBasic.
We'll
> receive it only when handling the AskUserForSettings request and we only have
a
> pointer to PrintingContextAndroid from Java at this moment. Ideally, we'd set
it
> in one of the browser callbacks called when AskUserForSettings result sent
back
> from Java to the renderer. What do you think the best place to do it?

See my last comment in https://codereview.chromium.org/22491003/ for my idea WRT
AskUserForSettings().

But I thought you guys were going to set it when
PrintViewManagerBase::PrintNow() gets called?

[whywhat, 2013-08-16 01:24:55]

On 2013/08/16 01:03:15, Lei Zhang wrote:
> On 2013/08/16 00:58:28, whywhat wrote:
> > On 2013/08/15 23:18:01, Lei Zhang wrote:
> > >
> >
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> > > File chrome/browser/printing/printing_message_filter.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> > > chrome/browser/printing/printing_message_filter.cc:189:
> > temp_file_fd->auto_close
> > > = false;
> > > On 2013/08/15 22:41:43, whywhat wrote:
> > > > On 2013/08/15 01:31:05, Lei Zhang wrote:
> > > > > So after you hand off the fd to the renderer, and the renderer
crashes,
> > > aren't
> > > > > you leaking a fd? Since you are not auto closing, you probably need to
> do
> > a
> > > > > little more work to prevent fd leaks.
> > > > 
> > > > How is the browser notified when the renderer crashed? We need to handle
> > this
> > > > case and notify the owner of the FD.
> > > 
> > > PrintViewManagerBase -> content::WebContentsObserver ->
> > > content::WebContentsObserverRenderProcessGone(). You probably want to
notify
> > the
> > > upper layer and set the fd to -1 in PrintViewManagerBase.
> > > 
> > >
> >
>
https://codereview.chromium.org/23116003/diff/39001/chrome/browser/printing/p...
> > > chrome/browser/printing/printing_message_filter.cc:218:
> > > print_view_manager->SetFileDescriptor(base::FileDescriptor(-1, false));
> > > On 2013/08/15 22:41:43, whywhat wrote:
> > > > On 2013/08/15 01:31:05, Lei Zhang wrote:
> > > > > Don't you need to do something here to tell the upper layers the
> renderer
> > is
> > > > > done writing and it is time to close the fd and send the result out to
> the
> > > > > printer?
> > > > 
> > > > We could use the fd as the key here. When fd is created, we remember it
on
> > the
> > > > Java side in a singleton mapping the fd to the fd owner.
> > > > From this method we'd call a static Java method via JNI in
PrintingContext
> > > that
> > > > would find the fd owner and notify it.
> > > 
> > > I'm just trying to understand if that's something you can do from here,
> where
> > > the renderer is explicitly telling you, "hey I'm done" versus if you need
> the
> > > rest of the printing system to chew though it and get to
> > > PrintingContext::DocumentDone() somehow.
> > > 
> > > I mentioned to Cihat the idea of just calling some dummy function here.
How
> > the
> > > dummy function is implemented in Clank is up to you. To me, it feels
better
> to
> > > handle it here. The fd comes in at the chrome/ layer via
> > > PrintViewManagerBasic::SetFileDescriptor() somehow, and you can reply at
the
> > > chrome/ layer via the dummy function.
> > 
> > I agree.
> > The question that remains is how to pass the fd to PrintViewManagerBasic.
> We'll
> > receive it only when handling the AskUserForSettings request and we only
have
> a
> > pointer to PrintingContextAndroid from Java at this moment. Ideally, we'd
set
> it
> > in one of the browser callbacks called when AskUserForSettings result sent
> back
> > from Java to the renderer. What do you think the best place to do it?
> 
> See my last comment in https://codereview.chromium.org/22491003/ for my idea
WRT
> AskUserForSettings().

I think that might work! Hopefully Cihat will be able to try it tomorrow :)

> 
> But I thought you guys were going to set it when
> PrintViewManagerBase::PrintNow() gets called?

[Lei Zhang, 2013-08-16 02:29:11]

On 2013/08/15 18:36:13, cimamoglu1 wrote:
> I may have found a nasty bug that affects us (and very probably Chrome OS). I
> already talked to Lei & Vitaly about it, but I'm writing here in more detail:
> 
> The issue
> PrintJobWorker::OnNewPage() is being called infinitely through recursion
> 
> Why?
> This line (
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...
> ) would always return false, triggering a 500ms delayed invocation of the
> function itself.
> 
> The line would always return false because PrintedDocument::SetPage is not
> called for Chrome OS or us (since it's only called through
> PrintViewManagerBase::OnDidPrintPage and this message is not sent for Chrome
OS,
> as you can see in
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/pr...)
> 
> The actual printing would still work because a print dialog is created for
> Chrome OS at
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...
> and that dialog still gets the PDF, and does whatever it needs to do with it.
> 
> How does it affect us?
> 1) It's really bad to have a method calling itself through infinite recursion
> (for performance, memory & general stability)
> 2) PrintJobWorker::OnDocumentDone() and hence PrintingContext::DocumentDone()
> are not called.

I build on Linux with GYP_DEFINES="chromeos=1" to look at the CrOS printing
behavior. (It's been a while!)

In PrintWebViewHelper::PrintPages(), PrintHostMsg_DidGetPrintedPagesCount only
gets sent for the non-ChromeOS printing workflow. Since Android is doing
CrOS-style printing, it shouldn't send PrintHostMsg_DidGetPrintedPagesCount
either. Thus you should be skipping out on a lot of the "full" workflow, thus
you should never reach this infinite loop, and you should also never reach
PrintingContext::DocumentDone() by design.

[cimamoglu1, 2013-08-16 17:20:06]

On 2013/08/16 02:29:11, Lei Zhang wrote:
> On 2013/08/15 18:36:13, cimamoglu1 wrote:
> > I may have found a nasty bug that affects us (and very probably Chrome OS).
I
> > already talked to Lei & Vitaly about it, but I'm writing here in more
detail:
> > 
> > The issue
> > PrintJobWorker::OnNewPage() is being called infinitely through recursion
> > 
> > Why?
> > This line (
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...
> > ) would always return false, triggering a 500ms delayed invocation of the
> > function itself.
> > 
> > The line would always return false because PrintedDocument::SetPage is not
> > called for Chrome OS or us (since it's only called through
> > PrintViewManagerBase::OnDidPrintPage and this message is not sent for Chrome
> OS,
> > as you can see in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/pr...)
> > 
> > The actual printing would still work because a print dialog is created for
> > Chrome OS at
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pri...
> > and that dialog still gets the PDF, and does whatever it needs to do with
it.
> > 
> > How does it affect us?
> > 1) It's really bad to have a method calling itself through infinite
recursion
> > (for performance, memory & general stability)
> > 2) PrintJobWorker::OnDocumentDone() and hence
PrintingContext::DocumentDone()
> > are not called.
> 
> I build on Linux with GYP_DEFINES="chromeos=1" to look at the CrOS printing
> behavior. (It's been a while!)
> 
> In PrintWebViewHelper::PrintPages(), PrintHostMsg_DidGetPrintedPagesCount only
> gets sent for the non-ChromeOS printing workflow. Since Android is doing
> CrOS-style printing, it shouldn't send PrintHostMsg_DidGetPrintedPagesCount
> either. Thus you should be skipping out on a lot of the "full" workflow, thus
> you should never reach this infinite loop, and you should also never reach
> PrintingContext::DocumentDone() by design.

Sorry for the wrong Chrome OS alarm, but it did at least help us catch the
issue. Thanks!

[cimamoglu1, 2013-08-16 17:20:37]

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_view_manager_basic.h:26: void
SetFileDescriptor(const base::FileDescriptor& file_descriptor);
On 2013/08/15 20:19:56, whywhat wrote:
> these methods could actually be set_file_descriptor() and file_descriptor()
and
> even be inlined here since it's just accessors for the private member of the
> class

Done.

https://codereview.chromium.org/23116003/diff/1/chrome/browser/printing/print...
chrome/browser/printing/print_view_manager_basic.h:36: // The file descriptor
the PDF of the page will be written.
On 2013/08/15 20:19:56, whywhat wrote:
> s/written/written into/

Done.

https://codereview.chromium.org/23116003/diff/1/chrome/renderer/printing/prin...
File chrome/renderer/printing/print_web_view_helper_linux.cc (right):

https://codereview.chromium.org/23116003/diff/1/chrome/renderer/printing/prin...
chrome/renderer/printing/print_web_view_helper_linux.cc:43:
PrintPageInternal(page_params, print_preview_context_.GetPrintCanvasSize(),
On 2013/08/15 20:19:56, whywhat wrote:
> Return this back to the next line

Done.

https://codereview.chromium.org/23116003/diff/1/chrome/renderer/printing/prin...
chrome/renderer/printing/print_web_view_helper_linux.cc:111: return false;
On 2013/08/15 20:19:56, whywhat wrote:
> Fix indent here.

Done.

[Vitaly Buka (NO REVIEWS), 2013-08-16 17:27:45]

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper_android.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper_android.cc:7: #include
"chrome/renderer/printing/print_web_view_helper_linux.cc"
Probably better solution is to rename print_web_view_helper_linux.cc and use
condition in gyp file

On 2013/08/14 16:28:22, whywhat wrote:
> On 2013/08/14 16:23:42, Vitaly Buka wrote:
> > why do you need print_web_view_helper_android.cc
> > Can you just compile 
> "chrome/renderer/printing/print_web_view_helper_linux.cc"
> > ?
> 
> _linux.cc files are excluded from Android builds (see
> build/filename_rules.gypi).
> Including the file locally didn't help so Cihat ended up with this workaround.

[whywhat, 2013-08-16 23:31:36]

Some additional nits.

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper_android.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper_android.cc:7: #include
"chrome/renderer/printing/print_web_view_helper_linux.cc"
On 2013/08/16 17:27:47, Vitaly Buka wrote:
> Probably better solution is to rename print_web_view_helper_linux.cc and use
> condition in gyp file

Any suggestions for the name?
print_web_view_helper_{common|shared|opensource}.cc ?

> 
> On 2013/08/14 16:28:22, whywhat wrote:
> > On 2013/08/14 16:23:42, Vitaly Buka wrote:
> > > why do you need print_web_view_helper_android.cc
> > > Can you just compile 
> > "chrome/renderer/printing/print_web_view_helper_linux.cc"
> > > ?
> > 
> > _linux.cc files are excluded from Android builds (see
> > build/filename_rules.gypi).
> > Including the file locally didn't help so Cihat ended up with this
workaround.
>

https://codereview.chromium.org/23116003/diff/64001/chrome/android/testshell/...
File chrome/android/testshell/testshell_stubs.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/android/testshell/...
chrome/android/testshell/testshell_stubs.cc:5: #include <string>
nit: I don't think this header is needed. The type is used by reference and the
prototype of the method is already using std::string& in
printing/printing_context.h

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/android/ta...
File chrome/browser/android/tab_android_test_stubs.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/android/ta...
chrome/browser/android/tab_android_test_stubs.cc:8: #include <string>
nit: Ditto

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_base.h (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_base.h:60: virtual void
OnPrintingFailed(int cookie);
Maybe we should instead observe NOTIFICATION_PRINT_JOB_RELEASED?

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.cc:44: return handled ? true :
PrintViewManagerBase::OnMessageReceived(message);
nit: I find something like

if (handled)
  return true;
return PrintViewManagerBase::OnMessageReceived(message);

slightly more readable.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.h:31: base::FileDescriptor
get_file_descriptor() const { return file_descriptor_; }
nit: s/get_file_descriptor/file_descriptor

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
File printing/printing_context_android.h (right):

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:17: class PRINTING_EXPORT
PrintingContextAndroid : public PrintingContext {
nit: class-level comment?

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:22: // Called when writing a page to a PDF
operation for a particular file
nit: Called when the page is successfully written to a PDF using the file
descriptor specified, or when the printing operation failed.

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:26: // Called from Java, when printing
settings from the user is ready or the
nit: s/is/are/

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:54: // The callback from AskUserForSettings
to be called upon the reply from Java.
nit: s/upon the reply from Java/when the settings are ready on the Java side/ ?

[Lei Zhang, 2013-08-18 09:56:25]

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_base.h (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_base.h:60: virtual void
OnPrintingFailed(int cookie);
On 2013/08/16 23:31:36, whywhat wrote:
> Maybe we should instead observe NOTIFICATION_PRINT_JOB_RELEASED?

I think other people are trying to kill Notifications altogether, so try not to
add new ones.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.cc:25: void
PrintViewManagerBasic::RenderProcessGone(base::TerminationStatus status) {
I think any RenderViewHost that prints will have a PrintViewManager. Even after
the printing has finished, when the tab closes, this may get called. If this
happens, please make sure that doesn't cause any weird side effects.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.cc:44: return handled ? true :
PrintViewManagerBase::OnMessageReceived(message);
On 2013/08/16 23:31:36, whywhat wrote:
> nit: I find something like
> 
> if (handled)
>   return true;
> return PrintViewManagerBase::OnMessageReceived(message);
> 
> slightly more readable.

I think I suggested the ternary operator in the first place. I don't have a
strong preference.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.h:46: virtual void
OnPrintingFailed(int cookie);
OVERRIDE

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:194: temp_file_fd->fd =
file_descriptor.fd;
nit: initialize |temp_file_fd->fd| before auto_close, so this line sits closer
to line 191, or if you change get_file_descriptor() to file_descriptor() and it
fits on one line...

[cimamoglu1, 2013-08-19 13:47:26]

Thanks for all the comments. I tried to address them (most were minor issues
this time), and hopefully I will be able to submit this today.

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
File chrome/renderer/printing/print_web_view_helper_android.cc (right):

https://codereview.chromium.org/23116003/diff/18001/chrome/renderer/printing/...
chrome/renderer/printing/print_web_view_helper_android.cc:7: #include
"chrome/renderer/printing/print_web_view_helper_linux.cc"
If there were a semantically meaningful name to cover all Android+Linux+Chrome
OS+Posix-Mac, I'd love to choose it. But I believe this is the easiest and
cleanest solution since I can't find such name unfortunately.
On 2013/08/16 23:31:36, whywhat wrote:
> On 2013/08/16 17:27:47, Vitaly Buka wrote:
> > Probably better solution is to rename print_web_view_helper_linux.cc and use
> > condition in gyp file
> 
> Any suggestions for the name?
> print_web_view_helper_{common|shared|opensource}.cc ?
> 
> > 
> > On 2013/08/14 16:28:22, whywhat wrote:
> > > On 2013/08/14 16:23:42, Vitaly Buka wrote:
> > > > why do you need print_web_view_helper_android.cc
> > > > Can you just compile 
> > > "chrome/renderer/printing/print_web_view_helper_linux.cc"
> > > > ?
> > > 
> > > _linux.cc files are excluded from Android builds (see
> > > build/filename_rules.gypi).
> > > Including the file locally didn't help so Cihat ended up with this
> workaround.
> > 
>

https://codereview.chromium.org/23116003/diff/64001/chrome/android/testshell/...
File chrome/android/testshell/testshell_stubs.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/android/testshell/...
chrome/android/testshell/testshell_stubs.cc:5: #include <string>
On 2013/08/16 23:31:36, whywhat wrote:
> nit: I don't think this header is needed. The type is used by reference and
the
> prototype of the method is already using std::string& in
> printing/printing_context.h

Done.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/android/ta...
File chrome/browser/android/tab_android_test_stubs.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/android/ta...
chrome/browser/android/tab_android_test_stubs.cc:8: #include <string>
On 2013/08/16 23:31:36, whywhat wrote:
> nit: Ditto

Done.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.h (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.h:31: base::FileDescriptor
get_file_descriptor() const { return file_descriptor_; }
On 2013/08/16 23:31:36, whywhat wrote:
> nit: s/get_file_descriptor/file_descriptor

Done.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.h:46: virtual void
OnPrintingFailed(int cookie);
On 2013/08/18 09:56:26, Lei Zhang wrote:
> OVERRIDE

Done.

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/printing_message_filter.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/printing_message_filter.cc:194: temp_file_fd->fd =
file_descriptor.fd;
On 2013/08/18 09:56:26, Lei Zhang wrote:
> nit: initialize |temp_file_fd->fd| before auto_close, so this line sits closer
> to line 191, or if you change get_file_descriptor() to file_descriptor() and
it
> fits on one line...

Done.

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
File printing/printing_context_android.h (right):

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:17: class PRINTING_EXPORT
PrintingContextAndroid : public PrintingContext {
On 2013/08/16 23:31:36, whywhat wrote:
> nit: class-level comment?

Done.

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:22: // Called when writing a page to a PDF
operation for a particular file
On 2013/08/16 23:31:36, whywhat wrote:
> nit: Called when the page is successfully written to a PDF using the file
> descriptor specified, or when the printing operation failed.

Done.

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:26: // Called from Java, when printing
settings from the user is ready or the
On 2013/08/16 23:31:36, whywhat wrote:
> nit: s/is/are/

Done.

https://codereview.chromium.org/23116003/diff/64001/printing/printing_context...
printing/printing_context_android.h:54: // The callback from AskUserForSettings
to be called upon the reply from Java.
On 2013/08/16 23:31:36, whywhat wrote:
> nit: s/upon the reply from Java/when the settings are ready on the Java side/
?

Done.

[Vitaly Buka (NO REVIEWS), 2013-08-19 16:14:18]

lgtm

https://codereview.chromium.org/23116003/diff/89001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.cc (right):

https://codereview.chromium.org/23116003/diff/89001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.cc:28:
PrintViewManagerBase::RenderProcessGone(status);
class PrintViewManagerAndroid : public PrintViewManagerBasic

print_view_manager_android.h + print_view_manager_android.cc ?

https://codereview.chromium.org/23116003/diff/89001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.cc:44: return handled ? true :
PrintViewManagerBase::OnMessageReceived(message);
 return handled || PrintViewManagerBase::OnMessageReceived(message);

https://codereview.chromium.org/23116003/diff/89001/chrome/common/print_messa...
File chrome/common/print_messages.h (right):

https://codereview.chromium.org/23116003/diff/89001/chrome/common/print_messa...
chrome/common/print_messages.h:373: #if defined(OS_CHROMEOS) ||
defined(OS_ANDROID)
If we define this messages for all platforms it should simplify some code.

[Lei Zhang, 2013-08-19 21:02:14]

lgtm

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
File chrome/browser/printing/print_view_manager_basic.cc (right):

https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
chrome/browser/printing/print_view_manager_basic.cc:25: void
PrintViewManagerBasic::RenderProcessGone(base::TerminationStatus status) {
On 2013/08/18 09:56:26, Lei Zhang wrote:
> I think any RenderViewHost that prints will have a PrintViewManager. Even
after
> the printing has finished, when the tab closes, this may get called. If this
> happens, please make sure that doesn't cause any weird side effects.

I didn't get a response to this, but I tested this and you'll get calls here for
every renderer that goes away. Assuming PdfWritingDone() can handle this, then
ok.

https://codereview.chromium.org/23116003/diff/89001/printing/printing.gyp
File printing/printing.gyp (right):

https://codereview.chromium.org/23116003/diff/89001/printing/printing.gyp#new...
printing/printing.gyp:234: 'printing_context_android.h',
Shouldn't you be able to just put this in the sources list with everything else?
Shouldn't build/filename_rules.gypi filter this out for non-Android builds?

[whywhat, 2013-08-19 21:41:50]

On 2013/08/19 21:02:14, Lei Zhang wrote:
> lgtm
> 
>
https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
> File chrome/browser/printing/print_view_manager_basic.cc (right):
> 
>
https://codereview.chromium.org/23116003/diff/64001/chrome/browser/printing/p...
> chrome/browser/printing/print_view_manager_basic.cc:25: void
> PrintViewManagerBasic::RenderProcessGone(base::TerminationStatus status) {
> On 2013/08/18 09:56:26, Lei Zhang wrote:
> > I think any RenderViewHost that prints will have a PrintViewManager. Even
> after
> > the printing has finished, when the tab closes, this may get called. If this
> > happens, please make sure that doesn't cause any weird side effects.
> 
> I didn't get a response to this, but I tested this and you'll get calls here
for
> every renderer that goes away. Assuming PdfWritingDone() can handle this, then
> ok.
> 
> https://codereview.chromium.org/23116003/diff/89001/printing/printing.gyp
> File printing/printing.gyp (right):
> 
>
https://codereview.chromium.org/23116003/diff/89001/printing/printing.gyp#new...
> printing/printing.gyp:234: 'printing_context_android.h',
> Shouldn't you be able to just put this in the sources list with everything
else?
> Shouldn't build/filename_rules.gypi filter this out for non-Android builds?

I'll try to submit TBR from https://codereview.chromium.org/22999026

[commit-bot: I haz the power, 2013-08-19 21:46:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/23116003/89001

[whywhat, 2013-08-19 23:34:15]

Submitted with https://codereview.chromium.org/22999026
Decided not to merge it today, so should be able to follow up with the nits
left.

Also, adding Parisa and Jorge to review the IPC message change.

[Jorge Lucangeli Obes, 2013-08-19 23:47:56]

On 2013/08/19 23:34:15, whywhat wrote:
> Submitted with https://codereview.chromium.org/22999026
> Decided not to merge it today, so should be able to follow up with the nits
> left.
> 
> Also, adding Parisa and Jorge to review the IPC message change.

Hasn't Tom reviewed the IPC message changes? Or do you mean a change over what
Tom has reviewed?

[whywhat, 2013-08-19 23:50:13]

On 2013/08/19 23:47:56, Jorge Lucangeli Obes wrote:
> On 2013/08/19 23:34:15, whywhat wrote:
> > Submitted with https://codereview.chromium.org/22999026
> > Decided not to merge it today, so should be able to follow up with the nits
> > left.
> > 
> > Also, adding Parisa and Jorge to review the IPC message change.
> 
> Hasn't Tom reviewed the IPC message changes? Or do you mean a change over what
> Tom has reviewed?

Indeed. I guess I've triggered a false alarm then :) My apologies.
It wasn't clear to me Tom was reviewing this from security team's point of view.

[Jorge Lucangeli Obes, 2013-08-19 23:54:44]

On 2013/08/19 23:50:13, whywhat wrote:
> On 2013/08/19 23:47:56, Jorge Lucangeli Obes wrote:
> > On 2013/08/19 23:34:15, whywhat wrote:
> > > Submitted with https://codereview.chromium.org/22999026
> > > Decided not to merge it today, so should be able to follow up with the
nits
> > > left.
> > > 
> > > Also, adding Parisa and Jorge to review the IPC message change.
> > 
> > Hasn't Tom reviewed the IPC message changes? Or do you mean a change over
what
> > Tom has reviewed?
> 
> Indeed. I guess I've triggered a false alarm then :) My apologies.
> It wasn't clear to me Tom was reviewing this from security team's point of
view.

It's confusing to reply to an "or" question with "Indeed" ;-) but from the next
sentence I take it that the changes you refer to where the ones reviewed by Tom.

However, I was still surprised by asking for an IPC review after submitting the
change. Security IPC reviews need to happen before submitting the CL (which
happened in this case, but by accident).

[commit-bot: I haz the power, 2013-08-20 11:40:40]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...