Stub for ARC print Bridge

Stub for ARC print Bridge

All change sets include:
 - http://crrev.com/2115863002 (Chromium)
 - http://ag/1191589 (Android device/google/cheets2)
 - http://ag/1191517 (Android platform/frameworks/base)

BUG=625126
Committed: https://crrev.com/e8e78c2bf33201a83d1acc253f62a8493afac020
Cr-Commit-Position: refs/heads/master@{#408618}

[Sergey Poromov, 2016-07-01 12:29:17]

Description was changed from

==========
Stub for ARC print Bridge

BUG=625126
==========

to

==========
Stub for ARC print Bridge

All change sets include:
 - http://crrev.com/2115863002 (Chromium)
 - http://ag/1191589 (Android device/google/cheets2)
 - http://ag/1191517 (Android platform/frameworks/base)

BUG=625126
==========

[Luis Héctor Chávez, 2016-07-01 16:02:06]

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

[Luis Héctor Chávez, 2016-07-01 16:02:07]

only leaving comments on the .mojom file to avoid reviewing the non-canonical
copy in Android.

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
File components/arc/common/print.mojom (right):

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:3: // found in the LICENSE file.
// Next MinVersion: 1

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:7: interface PrintHost {
// Next method ID: 1

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:8: Print(handle file);
This and Init need explicit ordinals.

Print@0 and Init@0

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:11: interface PrintInstance {
// Next method ID: 1

[Luis Héctor Chávez, 2016-07-06 17:09:43]

missed two more .mojom-related things

https://codereview.chromium.org/2115863002/diff/40001/components/arc/common/a...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/2115863002/diff/40001/components/arc/common/a...
components/arc/common/arc_bridge.mojom:28: // Next MinVersion: 15
Next MinVersion: 16

https://codereview.chromium.org/2115863002/diff/40001/components/arc/common/a...
components/arc/common/arc_bridge.mojom:85:
OnPrintInstanceReady@121(PrintInstance instance_ptr);
[MinVersion=15]

[Sergey Poromov, 2016-07-11 14:42:06]

poromov@chromium.org changed reviewers:
+ bartfab@chromium.org

[Sergey Poromov, 2016-07-11 14:42:07]

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
File components/arc/common/print.mojom (right):

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:3: // found in the LICENSE file.
On 2016/07/01 16:02:07, Luis Héctor Chávez wrote:
> // Next MinVersion: 1

Done.

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:7: interface PrintHost {
On 2016/07/01 16:02:07, Luis Héctor Chávez wrote:
> // Next method ID: 1

Done.

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:8: Print(handle file);
On 2016/07/01 16:02:07, Luis Héctor Chávez wrote:
> This and Init need explicit ordinals.
> 
> Print@0 and Init@0

Done.

https://codereview.chromium.org/2115863002/diff/20001/components/arc/common/p...
components/arc/common/print.mojom:11: interface PrintInstance {
On 2016/07/01 16:02:07, Luis Héctor Chávez wrote:
> // Next method ID: 1

Done.

https://codereview.chromium.org/2115863002/diff/40001/components/arc/common/a...
File components/arc/common/arc_bridge.mojom (right):

https://codereview.chromium.org/2115863002/diff/40001/components/arc/common/a...
components/arc/common/arc_bridge.mojom:28: // Next MinVersion: 15
On 2016/07/06 17:09:42, Luis Héctor Chávez wrote:
> Next MinVersion: 16

Done.

https://codereview.chromium.org/2115863002/diff/40001/components/arc/common/a...
components/arc/common/arc_bridge.mojom:85:
OnPrintInstanceReady@121(PrintInstance instance_ptr);
On 2016/07/06 17:09:42, Luis Héctor Chávez wrote:
> [MinVersion=15]

Done.

[Luis Héctor Chávez, 2016-07-11 18:27:53]

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
File components/arc/print/arc_print_bridge.cc (right):

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:43: void
ArcPrintBridge::Print(mojo::ScopedHandle file) {
This function runs in the UI thread, which shouldn't be doing I/O.
Unfortunately, the I/O thread is unavailable from components/arc, so you might
have to move your code to chrome/browser/chromeos/arc, where you can access the
various BrowserThreads.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:44: LOG(ERROR) << "Print called!!! " <<
file.get().value();
Try to keep debugging statements to a minimum. If you absolutely need to keep
them, use VLOG(2)

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:61: base::FileDescriptor
fn(fd.release(), true);
This seems unnecessary. You can use |fd.get()| instead of |fn.fd|. Plus
base::FileDescriptor does not call close() when it goes out of scope.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:63: base::File fl(fn.fd);
This also feels unnecessary. Plus it will double-close the fd.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:64: LOG(ERROR) << "File length" <<
fl.GetLength();
Huh, I was not expecting this would work with pipes.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:66: base::FilePath
filePath(FILE_PATH_LITERAL("/tmp/test.pdf"));
Can you use base::CreateTemporaryFile instead? Also, is it possible to have some
RAII wrapper to ensure that the file gets deleted when this method returns?

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:71: int bytes;
size_t

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:72: while ((bytes = read(fn.fd, buf,
8192)) > 0) {
Try to model this after base::ReadFromFD. This does not handle EINTR correctly.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:77: LOG(ERROR) << "Result code: " <<
errno;
VPLOG(2)? That'll convert errno into its string representation.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:80: out.Close();
You can just do

{
  base::File out(...);
  // Write to out
}

base::File::~File() calls Close() and it should flush it before.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:81: fd.release();
This is unnecessary.

[Sergey Poromov, 2016-07-13 16:58:43]

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
File components/arc/print/arc_print_bridge.cc (right):

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:43: void
ArcPrintBridge::Print(mojo::ScopedHandle file) {
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> This function runs in the UI thread, which shouldn't be doing I/O.
> Unfortunately, the I/O thread is unavailable from components/arc, so you might
> have to move your code to chrome/browser/chromeos/arc, where you can access
the
> various BrowserThreads.

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:44: LOG(ERROR) << "Print called!!! " <<
file.get().value();
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> Try to keep debugging statements to a minimum. If you absolutely need to keep
> them, use VLOG(2)

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:61: base::FileDescriptor
fn(fd.release(), true);
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> This seems unnecessary. You can use |fd.get()| instead of |fn.fd|. Plus
> base::FileDescriptor does not call close() when it goes out of scope.

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:63: base::File fl(fn.fd);
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> This also feels unnecessary. Plus it will double-close the fd.

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:64: LOG(ERROR) << "File length" <<
fl.GetLength();
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> Huh, I was not expecting this would work with pipes.

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:66: base::FilePath
filePath(FILE_PATH_LITERAL("/tmp/test.pdf"));
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> Can you use base::CreateTemporaryFile instead? Also, is it possible to have
some
> RAII wrapper to ensure that the file gets deleted when this method returns?

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:71: int bytes;
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> size_t

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:72: while ((bytes = read(fn.fd, buf,
8192)) > 0) {
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> Try to model this after base::ReadFromFD. This does not handle EINTR
correctly.

ReadFromFD doesn't return amount of read bytes. We need it to correctly write
into a file below. Wrapped in HANDLE_EINTR.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:77: LOG(ERROR) << "Result code: " <<
errno;
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> VPLOG(2)? That'll convert errno into its string representation.

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:80: out.Close();
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> You can just do
> 
> {
>   base::File out(...);
>   // Write to out
> }
> 
> base::File::~File() calls Close() and it should flush it before.

Done.

https://codereview.chromium.org/2115863002/diff/80001/components/arc/print/ar...
components/arc/print/arc_print_bridge.cc:81: fd.release();
On 2016/07/11 18:27:53, Luis Héctor Chávez wrote:
> This is unnecessary.

Done.

[Luis Héctor Chávez, 2016-07-14 16:29:44]

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.cc (right):

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:27:
arc_bridge_service()->AddObserver(this);
arc_bridge_service()->print()->AddObserver(this);

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:61:
scoped_platform_handle.release().handle),
Ideally, make the base::ScopedFD conversion in this method:

base::ScopedFD fd(scoped_platform_handle.release().handle);

and then pass it into the base::Bind with

base::Passed(&fd)

otherwise, you'll leak the fd in some cases.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:65: void
ArcPrintBridge::SavePdf(int handle) {
This doesn't really need to be a member function. Put it in the anonymous
namespace and make it return the RAII-wrapped base::FilePath. That way you also
avoid the need to make this class RefCounted.

Doing it that way also has the additional benefit of supporting multiple
concurrent print jobs, which is not the case right now.

For the RAII-wrapped class you can do something like this:

class ScopedTempFile {
 public:
  ScopedTempFile() {
    base::CreateTemporaryFile(&file_path_);
  }
  ~ScopedTempFile() {
    base::DeleteFile(file_path_, false);
  }

  const base::FilePath& file_path() const { return file_path_; }

 private:
  base::FilePath file_path_;
  DISALLOW_COPY_AND_ASSIGN(ScopedTempFile);
};

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:66: base::ScopedFD fd(handle);
DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:70:
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
CreateTemporaryfile() creates the file to avoid races. Is FLAG_CREATE_ALWAYS
needed?

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:79: void
ArcPrintBridge::OpenPdf() {
This method should receive the RAII-wrapped base::FilePath as parameter and
delete the file once it's done printing. Otherwise you'll fill up the user's
disk with old print jobs.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:80: GURL gurl("file://" +
file_path_.value());
DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:80: GURL gurl("file://" +
file_path_.value());
Can you also use the constants in
https://cs.chromium.org/chromium/src/url/url_constants.h instead?

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.h (right):

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.h:16: public
ArcBridgeService::Observer,
You need to rebase, since this interface changed. It's now
InstanceHolder<mojom::PrintInstance>::Observer

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.h:18: public
base::RefCountedThreadSafe<ArcPrintBridge> {
base::RefCountedThreadSafe requires you to make your destructor private, making
it incompatible with unique_ptr[1]. Fortunately, you can just remove this if you
follow the rest of the suggestions I'm making.

1: https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=127

[Sergey Poromov, 2016-07-19 16:32:09]

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.cc (right):

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:27:
arc_bridge_service()->AddObserver(this);
On 2016/07/14 16:29:43, Luis Héctor Chávez wrote:
> arc_bridge_service()->print()->AddObserver(this);

Done.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:61:
scoped_platform_handle.release().handle),
On 2016/07/14 16:29:43, Luis Héctor Chávez wrote:
> Ideally, make the base::ScopedFD conversion in this method:
> 
> base::ScopedFD fd(scoped_platform_handle.release().handle);
> 
> and then pass it into the base::Bind with
> 
> base::Passed(&fd)
> 
> otherwise, you'll leak the fd in some cases.

Done.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:65: void
ArcPrintBridge::SavePdf(int handle) {
On 2016/07/14 16:29:43, Luis Héctor Chávez wrote:
> This doesn't really need to be a member function. Put it in the anonymous
> namespace and make it return the RAII-wrapped base::FilePath. That way you
also
> avoid the need to make this class RefCounted.
> 
> Doing it that way also has the additional benefit of supporting multiple
> concurrent print jobs, which is not the case right now.
> 
> For the RAII-wrapped class you can do something like this:
> 
> class ScopedTempFile {
>  public:
>   ScopedTempFile() {
>     base::CreateTemporaryFile(&file_path_);
>   }
>   ~ScopedTempFile() {
>     base::DeleteFile(file_path_, false);
>   }
> 
>   const base::FilePath& file_path() const { return file_path_; }
> 
>  private:
>   base::FilePath file_path_;
>   DISALLOW_COPY_AND_ASSIGN(ScopedTempFile);
> };

Done.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:66: base::ScopedFD fd(handle);
On 2016/07/14 16:29:44, Luis Héctor Chávez wrote:
> DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);

Done.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:70:
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
On 2016/07/14 16:29:43, Luis Héctor Chávez wrote:
> CreateTemporaryfile() creates the file to avoid races. Is FLAG_CREATE_ALWAYS
> needed?

Yes, it doesn't work without this flag.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:79: void
ArcPrintBridge::OpenPdf() {
On 2016/07/14 16:29:43, Luis Héctor Chávez wrote:
> This method should receive the RAII-wrapped base::FilePath as parameter and
> delete the file once it's done printing. Otherwise you'll fill up the user's
> disk with old print jobs.

Unfortunately we couldn't delete the file here right now - it will be not
available from new opened Chrome tab.
This implementation is just temporary and won't be exposed to users - we won't
switch from Android printing to this one while this is not fully implemented.
However, for development and testing it's better to have file here so that we
can try to do something with the data on Chrome side.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:80: GURL gurl("file://" +
file_path_.value());
On 2016/07/14 16:29:44, Luis Héctor Chávez wrote:
> DCHECK_CURRENTLY_ON(content::BrowserThread::FILE);

Done.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:80: GURL gurl("file://" +
file_path_.value());
On 2016/07/14 16:29:43, Luis Héctor Chávez wrote:
> Can you also use the constants in
> https://cs.chromium.org/chromium/src/url/url_constants.h instead?

Done.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.h (right):

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.h:16: public
ArcBridgeService::Observer,
On 2016/07/14 16:29:44, Luis Héctor Chávez wrote:
> You need to rebase, since this interface changed. It's now
> InstanceHolder<mojom::PrintInstance>::Observer

Done.

https://codereview.chromium.org/2115863002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.h:18: public
base::RefCountedThreadSafe<ArcPrintBridge> {
On 2016/07/14 16:29:44, Luis Héctor Chávez wrote:
> base::RefCountedThreadSafe requires you to make your destructor private,
making
> it incompatible with unique_ptr[1]. Fortunately, you can just remove this if
you
> follow the rest of the suggestions I'm making.
> 
> 1: https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?l=127

Done.

[Luis Héctor Chávez, 2016-07-19 16:47:43]

final nits before approval.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.cc (right):

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:30: // TODO(poromov) Delete file
after printing.
nit: Expand this comment mentioning that this is just a temporary measure to do
testing. Bonus points if you create a crbug.com for it and mention it here.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:31: //
base::DeleteFile(file_path_, false);
nit: There should not be any commented code.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:48: size_t bytes;
welp, sorry, this should have been ssize_t.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:49: while ((bytes =
HANDLE_EINTR(read(fd.get(), buf, 8192))) > 0) {
nit: s/8192/sizeof(buf)/

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.h (right):

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.h:15: class ArcPrintBridge : public
ArcService,
nit: Can you rename this to ArcPrintService?

[Sergey Poromov, 2016-07-20 15:08:15]

poromov@chromium.org changed reviewers:
+ dcheng@chromium.org

[Sergey Poromov, 2016-07-20 15:08:16]

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.cc (right):

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:30: // TODO(poromov) Delete file
after printing.
On 2016/07/19 16:47:43, Luis Héctor Chávez wrote:
> nit: Expand this comment mentioning that this is just a temporary measure to
do
> testing. Bonus points if you create a http://crbug.com for it and mention it
here.

Done.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:31: //
base::DeleteFile(file_path_, false);
On 2016/07/19 16:47:43, Luis Héctor Chávez wrote:
> nit: There should not be any commented code.

Done.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:48: size_t bytes;
On 2016/07/19 16:47:43, Luis Héctor Chávez wrote:
> welp, sorry, this should have been ssize_t.

Done.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.cc:49: while ((bytes =
HANDLE_EINTR(read(fd.get(), buf, 8192))) > 0) {
On 2016/07/19 16:47:43, Luis Héctor Chávez wrote:
> nit: s/8192/sizeof(buf)/

Done.

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_bridge.h (right):

https://codereview.chromium.org/2115863002/diff/200001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_bridge.h:15: class ArcPrintBridge : public
ArcService,
On 2016/07/19 16:47:43, Luis Héctor Chávez wrote:
> nit: Can you rename this to ArcPrintService?

Done.

[Luis Héctor Chávez, 2016-07-20 19:15:16]

lgtm

[dcheng, 2016-07-21 02:15:22]

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:48: while ((bytes =
HANDLE_EINTR(read(fd.get(), buf, sizeof(buf)))) > 0) {
I would suggest using ReadFromFd.

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:49: out.WriteAtCurrentPos(buf,
bytes);
Also, we should probably be checking for errors

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:58: GURL
gurl(std::string(url::kFileScheme) + url::kStandardSchemeSeparator +
Please use net::FilePathToFileURL

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.h (right):

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.h:25: void
Print(mojo::ScopedHandle) override;
Please name this parameter (Google style only allows omitting if the parameter
name doesn't add anything; here, it's not clear what kind of handle it is)

[Sergey Poromov, 2016-07-21 13:26:03]

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:48: while ((bytes =
HANDLE_EINTR(read(fd.get(), buf, sizeof(buf)))) > 0) {
On 2016/07/21 02:15:22, dcheng wrote:
> I would suggest using ReadFromFd.

As I understand there is no way to know how many bytes was read by ReadFromFd -
it return only bool value. Without knowing the amount we couldn't write them
into a file in next line.

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:49: out.WriteAtCurrentPos(buf,
bytes);
On 2016/07/21 02:15:22, dcheng wrote:
> Also, we should probably be checking for errors

Done.

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:58: GURL
gurl(std::string(url::kFileScheme) + url::kStandardSchemeSeparator +
On 2016/07/21 02:15:22, dcheng wrote:
> Please use net::FilePathToFileURL

Done.

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.h (right):

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.h:25: void
Print(mojo::ScopedHandle) override;
On 2016/07/21 02:15:22, dcheng wrote:
> Please name this parameter (Google style only allows omitting if the parameter
> name doesn't add anything; here, it's not clear what kind of handle it is)

Done.

[dcheng, 2016-07-25 06:36:30]

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.h (right):

https://codereview.chromium.org/2115863002/diff/220001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.h:25: void
Print(mojo::ScopedHandle) override;
On 2016/07/21 13:26:03, Sergey Poromov wrote:
> On 2016/07/21 02:15:22, dcheng wrote:
> > Please name this parameter (Google style only allows omitting if the
parameter
> > name doesn't add anything; here, it's not clear what kind of handle it is)
> 
> Done.

Nit: C++ naming convention for arg names is_like_this.

https://codereview.chromium.org/2115863002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:45:
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
Btw, can we just use base::File::FLAG_DELETE_ON_CLOSE here and get rid of the
need for ScopedTempFile? Then we can just return a base::File and use IsValid()
to check for success, right?

[Sergey Poromov, 2016-07-25 13:04:11]

https://codereview.chromium.org/2115863002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:45:
base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
On 2016/07/25 06:36:30, dcheng wrote:
> Btw, can we just use base::File::FLAG_DELETE_ON_CLOSE here and get rid of the
> need for ScopedTempFile? Then we can just return a base::File and use
IsValid()
> to check for success, right?

Looks like it's not yet possible to delete on close, as the file needs to be
reopened while opening a tab. There is a TODO few lines before. Especially as
this is a first temporary solution that won't be released to public - it's just
first step to more complex full printing support, that includes much more
interactions with printing stack.

[dcheng, 2016-07-25 13:33:01]

On 2016/07/25 13:04:11, Sergey Poromov wrote:
>
https://codereview.chromium.org/2115863002/diff/240001/chrome/browser/chromeo...
> File chrome/browser/chromeos/arc/arc_print_service.cc (right):
> 
>
https://codereview.chromium.org/2115863002/diff/240001/chrome/browser/chromeo...
> chrome/browser/chromeos/arc/arc_print_service.cc:45:
> base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE);
> On 2016/07/25 06:36:30, dcheng wrote:
> > Btw, can we just use base::File::FLAG_DELETE_ON_CLOSE here and get rid of
the
> > need for ScopedTempFile? Then we can just return a base::File and use
> IsValid()
> > to check for success, right?
> 
> Looks like it's not yet possible to delete on close, as the file needs to be
> reopened while opening a tab. There is a TODO few lines before. Especially as
> this is a first temporary solution that won't be released to public - it's
just
> first step to more complex full printing support, that includes much more
> interactions with printing stack.

Is there a design doc of how this is going to work end-from-end? That might make
this review easier.

[Sergey Poromov, 2016-07-26 14:21:21]

No, there is no design docs yet. We are still planning how to interact with
Chrome printing. But we want to land stub mojo bridge as it will anyway
required.

[dcheng, 2016-07-27 16:43:36]

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:40:
base::Optional<ScopedTempFile> SavePdf(base::ScopedFD fd) {
It doesn't seem like there's any need for ScopedTempFile atm, so let's simplify
this and just return a File object. It's easy enough to add the scoper once
we've figured out the semantics we want.

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:107: base::ScopedFD
fd(scoped_platform_handle.release().handle);
I would suggest just creating a base::File from the FD directly rather than
wrapping it in a ScopedFD. Then you can use things like ReadAtCurrentPos(),
which will make the code look a little nicer. In general, it's pretty rare to
see raw read() / write() calls, so I think it will be a bit more consistent with
the rest of Chromium as well.

[Sergey Poromov, 2016-07-27 18:24:41]

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:40:
base::Optional<ScopedTempFile> SavePdf(base::ScopedFD fd) {
On 2016/07/27 16:43:36, dcheng wrote:
> It doesn't seem like there's any need for ScopedTempFile atm, so let's
simplify
> this and just return a File object. It's easy enough to add the scoper once
> we've figured out the semantics we want.

lhchavez@ suggested it to avoid trashing /tmp after printing. Unfortunately as
there is yet no way to pass the object further, we couldn't delete it in
destructor. However, the needed API will be build in the next change and TODO
will be fixed.
I prefer not to change it back to temporary get rid of the class.

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:107: base::ScopedFD
fd(scoped_platform_handle.release().handle);
On 2016/07/27 16:43:36, dcheng wrote:
> I would suggest just creating a base::File from the FD directly rather than
> wrapping it in a ScopedFD. Then you can use things like ReadAtCurrentPos(),
> which will make the code look a little nicer. In general, it's pretty rare to
> see raw read() / write() calls, so I think it will be a bit more consistent
with
> the rest of Chromium as well.

Done.

[dcheng, 2016-07-28 01:38:09]

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:40:
base::Optional<ScopedTempFile> SavePdf(base::ScopedFD fd) {
On 2016/07/27 18:24:41, Sergey Poromov wrote:
> On 2016/07/27 16:43:36, dcheng wrote:
> > It doesn't seem like there's any need for ScopedTempFile atm, so let's
> simplify
> > this and just return a File object. It's easy enough to add the scoper once
> > we've figured out the semantics we want.
> 
> lhchavez@ suggested it to avoid trashing /tmp after printing. Unfortunately as
> there is yet no way to pass the object further, we couldn't delete it in
> destructor. However, the needed API will be build in the next change and TODO
> will be fixed.
> I prefer not to change it back to temporary get rid of the class.

It's not built out, and there's no obvious need for this abstraction. When we
figure out how we're going to use the created file, that seems like the right
time to figure out what abstraction we need.

There's already a bunch of ScopedTempFiles lying around chromium source, and I
don't really see the need for a third (and not yet implemented) one.

[Sergey Poromov, 2016-07-28 14:43:00]

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/300001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:40:
base::Optional<ScopedTempFile> SavePdf(base::ScopedFD fd) {
On 2016/07/28 01:38:09, dcheng (OOO Aug 1 - Aug 11) wrote:
> On 2016/07/27 18:24:41, Sergey Poromov wrote:
> > On 2016/07/27 16:43:36, dcheng wrote:
> > > It doesn't seem like there's any need for ScopedTempFile atm, so let's
> > simplify
> > > this and just return a File object. It's easy enough to add the scoper
once
> > > we've figured out the semantics we want.
> > 
> > lhchavez@ suggested it to avoid trashing /tmp after printing. Unfortunately
as
> > there is yet no way to pass the object further, we couldn't delete it in
> > destructor. However, the needed API will be build in the next change and
TODO
> > will be fixed.
> > I prefer not to change it back to temporary get rid of the class.
> 
> It's not built out, and there's no obvious need for this abstraction. When we
> figure out how we're going to use the created file, that seems like the right
> time to figure out what abstraction we need.
> 
> There's already a bunch of ScopedTempFiles lying around chromium source, and I
> don't really see the need for a third (and not yet implemented) one.

Done.

[Sergey Poromov, 2016-07-28 14:43:07]

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

[commit-bot: I haz the power, 2016-07-28 14:43:25]

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

[dcheng, 2016-07-29 04:11:40]

lgtm

[Sergey Poromov, 2016-07-29 11:01:29]

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

[commit-bot: I haz the power, 2016-07-29 11:01:46]

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

[Sergey Poromov, 2016-07-29 11:40:42]

The CQ bit was checked by poromov@chromium.org

[Sergey Poromov, 2016-07-29 11:40:43]

The patchset sent to the CQ was uploaded after l-g-t-m from
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2115863002/#ps360001
(title: "Remove unnecessary includes.")

[commit-bot: I haz the power, 2016-07-29 11:40:50]

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-07-29 12:09:52]

Description was changed from

==========
Stub for ARC print Bridge

All change sets include:
 - http://crrev.com/2115863002 (Chromium)
 - http://ag/1191589 (Android device/google/cheets2)
 - http://ag/1191517 (Android platform/frameworks/base)

BUG=625126
==========

to

==========
Stub for ARC print Bridge

All change sets include:
 - http://crrev.com/2115863002 (Chromium)
 - http://ag/1191589 (Android device/google/cheets2)
 - http://ag/1191517 (Android platform/frameworks/base)

BUG=625126
==========

[commit-bot: I haz the power, 2016-07-29 12:09:53]

Committed patchset #19 (id:360001)

[commit-bot: I haz the power, 2016-07-29 12:12:08]

Description was changed from

==========
Stub for ARC print Bridge

All change sets include:
 - http://crrev.com/2115863002 (Chromium)
 - http://ag/1191589 (Android device/google/cheets2)
 - http://ag/1191517 (Android platform/frameworks/base)

BUG=625126
==========

to

==========
Stub for ARC print Bridge

All change sets include:
 - http://crrev.com/2115863002 (Chromium)
 - http://ag/1191589 (Android device/google/cheets2)
 - http://ag/1191517 (Android platform/frameworks/base)

BUG=625126

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

[commit-bot: I haz the power, 2016-07-29 12:12:10]

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

[Lei Zhang, 2016-07-29 21:40:21]

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

[Lei Zhang, 2016-07-29 21:40:22]

https://codereview.chromium.org/2115863002/diff/360001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/arc_print_service.cc (right):

https://codereview.chromium.org/2115863002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:28:
base::CreateTemporaryFile(&file_path);
Check for failure?

https://codereview.chromium.org/2115863002/diff/360001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/arc_print_service.cc:38: return base::nullopt;
Can't you just return an empty FilePath to indicate failure? i.e. no need for
Optional.