Refactor Android printing code to make it more testable.

Refactor Android printing code to make it more testable.

* Move printing logic from Tab to TabBase (i.e. to upstream), and
  also in the relevant files tab_android.*, TabPrinter.java.
* Remove obsolete Android printing feature detection code.
* Move PrintingControllerFactory logic into PrintingControllerImpl.
* Create a new PrintingControllerFactory, so the clients have a
ligher weight creation process (5-6 lines to 1).
* Instead of depending on Context to create a PrintManager, depend
  on an interface, namely PrintManagerDelegate.
* Remove setErrorText (move the logic inside factory).
* Remove the hardcoded default file name (use Printable#getTitle)
  instead.

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

[Vitaly Buka (NO REVIEWS), 2013-11-18 19:30:46]

On 2013/11/18 17:47:49, cimamoglu1 wrote:

lgtm printing/

[cimamoglu1, 2013-11-19 15:15:45]

Added bulach@ for *tab* files' ownership.

[bulach, 2013-11-19 15:32:22]

lgtm once whywhat is happy :) just nits below:

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java
(right):

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:17:
* This class doesn't have any lifetime expectations with regards to Tab, since
we keep a week
nit: s/week/weak/

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:32:
TabBase tab = mTab.get();
nit: indent

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:39:
TabBase tab = mTab.get();
nit: indent

https://codereview.chromium.org/63483007/diff/50001/chrome/browser/android/ta...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/63483007/diff/50001/chrome/browser/android/ta...
chrome/browser/android/tab_android.cc:403: 
nit: remove extra \n

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:115:
ThreadUtils.assertOnUiThread();
nit: indent this block

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File
printing/android/java/src/org/chromium/printing/SystemPrintManagerDelegate.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/SystemPrintManagerDelegate.java:15:
private final PrintManager mPrintManager;
indentation

[whywhat, 2013-11-19 15:39:32]

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:325: /** Prints
the current page. */
nit: Should be the multiline comment, add a @return statement as well.

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java
(right):

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:15:
* Wraps printing related functionality of a Tab object.
s/Tab/TabBase
nit: maybe even {@link TabBase} ?

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:17:
* This class doesn't have any lifetime expectations with regards to Tab, since
we keep a week
ditto
s/week/weak

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:32:
TabBase tab = mTab.get();
indent

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:39:
TabBase tab = mTab.get();
indent

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File printing/android/java/src/org/chromium/printing/PrintingContext.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingContext.java:40:
mController = PrintingControllerImpl.getInstance();
Are you going to override the controller for testing?

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:105:
* The controller is a singleton, since there can be only one printing action at
any time.
Is this going to be true with the window.print() support?
Also, why do we need such a strange singleton pattern with create() and
instance() ?

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:118:
sInstance = new PrintingControllerImpl(printManager, errorText);
also, Java indent is 4 spaces so indent this line two spaces more from within
the outside block

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File
printing/android/java/src/org/chromium/printing/SystemPrintManagerDelegate.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/SystemPrintManagerDelegate.java:14:
public class SystemPrintManagerDelegate implements PrintManagerDelegate {
Why System? What other implementation do you expect? Could it be
PrintManagerDelegateImpl?

[cimamoglu1, 2013-11-19 17:40:32]

PTAL?

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:325: /** Prints
the current page. */
On 2013/11/19 15:39:32, whywhat wrote:
> nit: Should be the multiline comment, add a @return statement as well.

Done.

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
File
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java
(right):

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:15:
* Wraps printing related functionality of a Tab object.
On 2013/11/19 15:39:32, whywhat wrote:
> s/Tab/TabBase
> nit: maybe even {@link TabBase} ? 

Done.

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:17:
* This class doesn't have any lifetime expectations with regards to Tab, since
we keep a week
On 2013/11/19 15:32:22, bulach wrote:
> nit: s/week/weak/

Done.

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:32:
TabBase tab = mTab.get();
On 2013/11/19 15:39:32, whywhat wrote:
> indent

Done.

https://codereview.chromium.org/63483007/diff/50001/chrome/android/java/src/o...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:39:
TabBase tab = mTab.get();
On 2013/11/19 15:39:32, whywhat wrote:
> indent

Done.

https://codereview.chromium.org/63483007/diff/50001/chrome/browser/android/ta...
File chrome/browser/android/tab_android.cc (right):

https://codereview.chromium.org/63483007/diff/50001/chrome/browser/android/ta...
chrome/browser/android/tab_android.cc:403: 
On 2013/11/19 15:32:22, bulach wrote:
> nit: remove extra \n

Done.

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File printing/android/java/src/org/chromium/printing/PrintingContext.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingContext.java:40:
mController = PrintingControllerImpl.getInstance();
On 2013/11/19 15:39:32, whywhat wrote:
> Are you going to override the controller for testing?

Yes. Also, if need be, I plan to add a method (just for testing) like
setController.

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:105:
* The controller is a singleton, since there can be only one printing action at
any time.
On 2013/11/19 15:39:32, whywhat wrote:
> Is this going to be true with the window.print() support?
> Also, why do we need such a strange singleton pattern with create() and
> instance() ?

We don't "need", but this is nicer approach:
For creation, we need the print manager and error text, but afterwards we don't
need them to get the created instance.
We would need to pass dummy values. In any case, the passed values would be
useless. With this approach, create does what it needs to, and getInstance does
what it needs to do.

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:115:
ThreadUtils.assertOnUiThread();
On 2013/11/19 15:32:22, bulach wrote:
> nit: indent this block

Done.

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:118:
sInstance = new PrintingControllerImpl(printManager, errorText);
On 2013/11/19 15:39:32, whywhat wrote:
> also, Java indent is 4 spaces so indent this line two spaces more from within
> the outside block 

Done.

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
File
printing/android/java/src/org/chromium/printing/SystemPrintManagerDelegate.java
(right):

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/SystemPrintManagerDelegate.java:14:
public class SystemPrintManagerDelegate implements PrintManagerDelegate {
On 2013/11/19 15:39:32, whywhat wrote:
> Why System? What other implementation do you expect? Could it be
> PrintManagerDelegateImpl?

Done.

https://codereview.chromium.org/63483007/diff/50001/printing/android/java/src...
printing/android/java/src/org/chromium/printing/SystemPrintManagerDelegate.java:15:
private final PrintManager mPrintManager;
On 2013/11/19 15:32:22, bulach wrote:
> indentation

Done.

[nyquist, 2013-11-20 07:44:37]

This seemed like a cleanup-type CL, so added a few nits. Given you already have
LGTMs from the OWNERS needed, feel free to ignore or punt to a future CL (if you
do punt them, there's a few more things I'd like to comment on for these files
though).

In general, for someone not used to printing, the PrintingContext, PrintManager,
PrintManagerDelegate, PrintController, etc., are a bit confusing names. Good
thing they at least have comments.

Anyway, looking forward to the tests!

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:327: * @return
True if the printing process is started successfully, false otherwise.
Nit: @return whether the printing process is started successfully.

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/printing/PrintingControllerFactory.java
(right):

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/PrintingControllerFactory.java:8:
import org.chromium.printing.PrintManagerDelegate;
Unused import

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/PrintingControllerFactory.java:19:
public static PrintingController create(Activity activity) {
This should only require Context, instead of Activity (also, remove import).
Both getResources() and getSystemService(...) are defined in that parent class.

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java
(right):

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:23:
private static String sDefaultTitle;
Nit: Static defines before member fields.

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:25:
public TabPrinter(final TabBase tab) {
Nit: unnecessary final

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:36:
if (tab == null) return false;
Nit: return tab != null && tab.print(); ?

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
File
printing/android/java/src/org/chromium/printing/PrintManagerDelegateImpl.java
(right):

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintManagerDelegateImpl.java:14:
public class PrintManagerDelegateImpl implements PrintManagerDelegate {
How about SystemPrintManagerDelegate? That way you could have pretty test-class
names such as FakePrintManagerDelegate or MockPrintManagerDelegate.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
File printing/android/java/src/org/chromium/printing/PrintingContext.java
(right):

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:19: public
class PrintingContext implements PrintingContextInterface {
Nit: Using 'Interface' as a suffix to a Java interface should probably be
avoided. Rename to DefaultPrintingContext implements PrintingContext? Or even
PrintingContextImpl for the implementation.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:21: private
static final String TAG = "PrintingContext";
Nit: I don't think this constant is ever used?

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:30: private
static final SparseArray<PrintingContext> sPrintingContextMap =
Nit: This is a constant so it should use constant naming --
^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:40:
mController = PrintingControllerImpl.getInstance();
This seems a bit strange. Would it be possible for native to somehow pass it in
as an argument? I realize that might be hard, since the native code lives in
src/printing. Just a question...

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:124: //
AskUserForSettings callback) twice.  See PrintingControllerImpl#onFinish
Nit: Use {@link ...}

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
File printing/android/java/src/org/chromium/printing/PrintingController.java
(right):

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingController.java:12:
public interface PrintingController {
This name is a little bit confusing when compared with the
"PrintManagerDelegate" as 'manager' and 'controller' seem quite similar. I
haven't come up with a better name yet though.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java
(right):

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:9:
import android.annotation.SuppressLint;
I belive the following imports are unused:
import android.content.Context;
import android.os.CancellationSignal.OnCancelListener;
import android.print.PrintAttributes.MediaSize;
import android.print.PrintAttributes.Resolution;
import android.print.PrintDocumentAdapter.LayoutResultCallback;
import android.print.PrintDocumentAdapter.WriteResultCallback;
import android.util.Log;
import java.util.Arrays;
import java.util.List;

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:47:
private String mErrorMessage;
Nit: should be final

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:49:
private final PrintManagerDelegate mPrintManager;
Nit: Reorder all final members to be above member fields.
same goes for the private statics. So for private fields, something like this:
private static final
private static
private final
private 

That makes it much clearer what can change in the objects lifetime.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:97:
private PrintingControllerImpl(final PrintManagerDelegate printManager, String
errorText) {
Nit: Unecessary final

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:107:
* @param printManager
If you don't describe it, remove from comment?

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:113:
public static PrintingController create(final PrintManagerDelegate printManager,
Nit: Unnecessary final.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:341:
private void closeFileDescriptor(int fd) {
Nit: While you are at it... this method and the two below should be static to
signal to the reader that they do not change instance state.

[cimamoglu1, 2013-11-20 14:40:34]

Thanks for interesting & thorough comments, Tommy!

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/TabBase.java (right):

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/TabBase.java:327: * @return
True if the printing process is started successfully, false otherwise.
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: @return whether the printing process is started successfully.

Done.

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/printing/PrintingControllerFactory.java
(right):

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/PrintingControllerFactory.java:8:
import org.chromium.printing.PrintManagerDelegate;
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Unused import

Done.

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/PrintingControllerFactory.java:19:
public static PrintingController create(Activity activity) {
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> This should only require Context, instead of Activity (also, remove import).
> Both getResources() and getSystemService(...) are defined in that parent
class.

Unfortunately, this does require Activity. yfriedman@ made a similar comment
earlier, and I did change to Context (and passed with getApplicationContext) but
it caused problems. see
http://developer.android.com/reference/android/print/PrintManager.html or
crbug.com/320123

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
File
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java
(right):

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:23:
private static String sDefaultTitle;
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: Static defines before member fields.

Done.

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:25:
public TabPrinter(final TabBase tab) {
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: unnecessary final

Done.

https://codereview.chromium.org/63483007/diff/370001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/printing/TabPrinter.java:36:
if (tab == null) return false;
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: return tab != null && tab.print(); ?

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
File
printing/android/java/src/org/chromium/printing/PrintManagerDelegateImpl.java
(right):

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintManagerDelegateImpl.java:14:
public class PrintManagerDelegateImpl implements PrintManagerDelegate {
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> How about SystemPrintManagerDelegate? That way you could have pretty
test-class
> names such as FakePrintManagerDelegate or MockPrintManagerDelegate.

It was SystemPrintManagerDelegate in the previous set of comments, but avayvod@
suggested this way = ) I'll keep it this way.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
File printing/android/java/src/org/chromium/printing/PrintingContext.java
(right):

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:19: public
class PrintingContext implements PrintingContextInterface {
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: Using 'Interface' as a suffix to a Java interface should probably be
> avoided. Rename to DefaultPrintingContext implements PrintingContext? Or even
> PrintingContextImpl for the implementation.

I understand having "Interface" suffix is not super desirable, but I think it's
better to match the native naming here
(printing_context.cc/printing_context_android.cc), and keep it as
PrintingContext.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:21: private
static final String TAG = "PrintingContext";
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: I don't think this constant is ever used?

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:30: private
static final SparseArray<PrintingContext> sPrintingContextMap =
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: This is a constant so it should use constant naming --
> ^[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:40:
mController = PrintingControllerImpl.getInstance();
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> This seems a bit strange. Would it be possible for native to somehow pass it
in
> as an argument? I realize that might be hard, since the native code lives in
> src/printing. Just a question...

As you said, it would be quite hard to do. I'll add a method later, something
like setPrintingController for easier testing.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingContext.java:124: //
AskUserForSettings callback) twice.  See PrintingControllerImpl#onFinish
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: Use {@link ...}

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java
(right):

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:9:
import android.annotation.SuppressLint;
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> I belive the following imports are unused:
> import android.content.Context;
> import android.os.CancellationSignal.OnCancelListener;
> import android.print.PrintAttributes.MediaSize;
> import android.print.PrintAttributes.Resolution;
> import android.print.PrintDocumentAdapter.LayoutResultCallback;
> import android.print.PrintDocumentAdapter.WriteResultCallback;
> import android.util.Log;
> import java.util.Arrays;
> import java.util.List;

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:47:
private String mErrorMessage;
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: should be final

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:49:
private final PrintManagerDelegate mPrintManager;
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: Reorder all final members to be above member fields.
> same goes for the private statics. So for private fields, something like this:
> private static final
> private static
> private final
> private 
> 
> That makes it much clearer what can change in the objects lifetime.

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:97:
private PrintingControllerImpl(final PrintManagerDelegate printManager, String
errorText) {
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: Unecessary final

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:107:
* @param printManager
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> If you don't describe it, remove from comment?

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:113:
public static PrintingController create(final PrintManagerDelegate printManager,
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: Unnecessary final.

Done.

https://codereview.chromium.org/63483007/diff/370001/printing/android/java/sr...
printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:341:
private void closeFileDescriptor(int fd) {
On 2013/11/20 07:44:38, nyquist (GMT 11.4-11.18) wrote:
> Nit: While you are at it... this method and the two below should be static to
> signal to the reader that they do not change instance state.

Done.

[whywhat, 2013-11-20 14:42:04]

lgtm

[commit-bot: I haz the power, 2013-11-20 14:42:44]

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

[commit-bot: I haz the power, 2013-11-20 14:57:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-11-20 15:01:13]

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

[commit-bot: I haz the power, 2013-11-20 16:52:23]

Change committed as 236256

[cjhopman, 2013-11-20 20:54:02]

On 2013/11/20 16:52:23, I haz the power (commit-bot) wrote:
> Change committed as 236256

Is this code untested? I think PrintingControllerFactory.create throws a
NoClassDefFoundError on pre-K builds.