Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
85693005:
Android: refactors printing code & adds a test (Closed)
|
Created:
7 years ago by cimamoglu (inactive) Modified:
7 years ago CC:
chromium-reviews, Miguel Garcia, nyquist Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAndroid: refactors printing code & adds a test
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238776
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address Marcus & Anton comments, findbugs issues #
Total comments: 14
Patch Set 3 : Address final comments #Patch Set 4 : Fix findbugs errors #Patch Set 5 : Findbugs fixes #Patch Set 6 : Don't run the test if the device doesn't have KitKat #
Messages
Total messages: 19 (0 generated)
lgtm +Tommy since I don't have enough Java expertise (esp. for Chromium Test Shell tests) https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:96: new PrintDocumentAdapterWrapper.LayoutResultCallbackWrapper(null) { I think it's cleaner to have an interface with two implementations, one for the test, another one that takes the real callback for the actual code. https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java (right): https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:152: return mPages == null ? null : mPages.clone(); This is fixed by another CL, right?
On 2013/11/25 21:17:00, whywhat wrote: > lgtm > > +Tommy since I don't have enough Java expertise (esp. for Chromium Test Shell > tests) > > https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... > File > chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java > (right): > > https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... > chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:96: > new PrintDocumentAdapterWrapper.LayoutResultCallbackWrapper(null) { > I think it's cleaner to have an interface with two implementations, one for the > test, another one that takes the real callback for the actual code. > > https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... > File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java > (right): > > https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... > printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:152: > return mPages == null ? null : mPages.clone(); > This is fixed by another CL, right? Also, fix the errors found by trybots :)
Marcus, could you take a look?
sorry, I can't quite get my head around the adapters and wrappers... I think it may be possible to have a class FooCallbackWrapper that derives from Callback, take a Callback in its constructor and just passes it through... test code would then inject its own version, and production code would simply create one that delegates to the real Callback implementors... no need to test for null and have the "Internal" methods in an abstract base class... would that work? https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:50: public void testNormalPrintingFlow() throws Throwable { it'd be nice to add some cancellation / restart flow as well.. https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:124: runTestOnUiThread(new Runnable() { shouldn't this be one of ThreadUtils.runOnUiThread* instead? it looks like it's more common.. https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:137: runTestOnUiThread( new Runnable() { nit: remove space before "new" https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... File printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java (right): https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:34: if (mCallback != null) mCallback.onLayoutFinished(info, changed); is this null check just for the test? it looks like "real" code would always call this, right? in that case, remove all null checks, and let the test pass an empty anonymous implementation of the callback.. ditto for class below.. would it be possible to have this class deriving from LayoutResultCallback itself? this would probably remove the *Internal() methods below, no? so tests would inject a version of this passing a special LayoutResultCallback to be able to inspect, and production code would just pass the actual implementation... would that be possible?
On 2013/11/27 17:45:04, bulach wrote: > sorry, I can't quite get my head around the adapters and wrappers... > I think it may be possible to have a class FooCallbackWrapper that derives from > Callback, take a Callback in its constructor and just passes it through... > test code would then inject its own version, and production code would simply > create one that delegates to the real Callback implementors... no need to test > for null and have the "Internal" methods in an abstract base class... > would that work? > > https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... > File > chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java > (right): > > https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... > chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:50: > public void testNormalPrintingFlow() throws Throwable { > it'd be nice to add some cancellation / restart flow as well.. > > https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... > chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:124: > runTestOnUiThread(new Runnable() { > shouldn't this be one of ThreadUtils.runOnUiThread* instead? it looks like it's > more common.. > > https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... > chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:137: > runTestOnUiThread( new Runnable() { > nit: remove space before "new" > > https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... > File > printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java > (right): > > https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... > printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:34: > if (mCallback != null) mCallback.onLayoutFinished(info, changed); > is this null check just for the test? it looks like "real" code would always > call this, right? > in that case, remove all null checks, and let the test pass an empty anonymous > implementation of the callback.. ditto for class below.. > > > would it be possible to have this class deriving from LayoutResultCallback > itself? this would probably remove the *Internal() methods below, no? > > so tests would inject a version of this passing a special LayoutResultCallback > to be able to inspect, and production code would just pass the actual > implementation... would that be possible? The problem is that the real callbacks classes don't have public constructors so it's not possible to instantiate an object of any derived class.
thanks for the clarification! I should've read (and remembered what cihat told me last week). my bad!! I have some other suggestions below though, let me know if that would work. https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... File printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java (right): https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:21: * {@link PrintDocumentAdapter#WriteResultCallback} don't have public constructors. This makes duh, I should've read this.. :) https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:26: public abstract class PrintDocumentAdapterWrapper extends PrintDocumentAdapter { so would it be possible to make the "Impl" class a pure implementation of an interface defined here then? something like: PrintDocumentAdapterWrapper defines an internal interface like: interface PrintingController { onLayout() / onWrite() } taking the "CallbackWrappers" defined here. it would contain a member mChromePrinter that is injected during constructor. PrintingControllerImpl.java doesn't derive from anything, but rather, implements ChromePrinter. what I'm trying to get to is: - all communication with _android_ is purely delegation via this class.. as you both pointed out, we can't change its interfaces. - all communication with _chrome_ is done via clearly defined interfaces that can be properly tested without bringing up the whole world.. since the code is ours, we can create better abstractions / layers. would that work?
Addressed comments. One issue regarding bulach@ 's request: I did refactor as you suggested, but because of PrintingControllerImpl#startPrint()'s need for PrintDocumentAdapter (actually PrintManager's), I needed to make some more changes. Ran the test and made sure it works. PTAL? https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:50: public void testNormalPrintingFlow() throws Throwable { On 2013/11/27 17:45:04, bulach wrote: > it'd be nice to add some cancellation / restart flow as well.. I would rather keep this CL no bigger than it already is. I promise I'll add more tests here =) https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:96: new PrintDocumentAdapterWrapper.LayoutResultCallbackWrapper(null) { On 2013/11/25 21:17:01, whywhat wrote: > I think it's cleaner to have an interface with two implementations, one for the > test, another one that takes the real callback for the actual code. Done. https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:124: runTestOnUiThread(new Runnable() { On 2013/11/27 17:45:04, bulach wrote: > shouldn't this be one of ThreadUtils.runOnUiThread* instead? it looks like it's > more common.. Actually, runTestOnUiThread seems slightly more common in codesearch =) Still, there are three alternatives, and I asked in the group about which one is the preferred way among UiUtils.runOnUiThread vs ThreadUtils.runOnUiThread vs InstrumentationTestCase.runTestOnUiThread . For now, I'd rather keep it this way. https://codereview.chromium.org/85693005/diff/1/chrome/android/javatests/src/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:137: runTestOnUiThread( new Runnable() { On 2013/11/27 17:45:04, bulach wrote: > nit: remove space before "new" Done. https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... File printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java (right): https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:21: * {@link PrintDocumentAdapter#WriteResultCallback} don't have public constructors. This makes On 2013/11/27 19:26:04, bulach wrote: > duh, I should've read this.. :) Done. https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:26: public abstract class PrintDocumentAdapterWrapper extends PrintDocumentAdapter { On 2013/11/27 19:26:04, bulach wrote: > so would it be possible to make the "Impl" class a pure implementation of an > interface defined here then? > > something like: > > PrintDocumentAdapterWrapper defines an internal interface like: > interface PrintingController { onLayout() / onWrite() } taking the > "CallbackWrappers" defined here. > it would contain a member mChromePrinter that is injected during constructor. > > PrintingControllerImpl.java doesn't derive from anything, but rather, implements > ChromePrinter. > > > > what I'm trying to get to is: > - all communication with _android_ is purely delegation via this class.. as you > both pointed out, we can't change its interfaces. > > - all communication with _chrome_ is done via clearly defined interfaces that > can be properly tested without bringing up the whole world.. since the code is > ours, we can create better abstractions / layers. > > would that work? Done. https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:34: if (mCallback != null) mCallback.onLayoutFinished(info, changed); On 2013/11/27 17:45:04, bulach wrote: > is this null check just for the test? it looks like "real" code would always > call this, right? > in that case, remove all null checks, and let the test pass an empty anonymous > implementation of the callback.. ditto for class below.. > > > would it be possible to have this class deriving from LayoutResultCallback > itself? this would probably remove the *Internal() methods below, no? > > so tests would inject a version of this passing a special LayoutResultCallback > to be able to inspect, and production code would just pass the actual > implementation... would that be possible? Nope, as avayvod@ explained LayoutResultCallback has a hidden constructor so it cannot be subclassed or initialized. https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... File printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java (right): https://codereview.chromium.org/85693005/diff/1/printing/android/java/src/org... printing/android/java/src/org/chromium/printing/PrintingControllerImpl.java:152: return mPages == null ? null : mPages.clone(); On 2013/11/25 21:17:01, whywhat wrote: > This is fixed by another CL, right? Yep.
lgtm, neat, thanks! this will hopefully allow us to test those pathological cases you mentioned a while ago... just some small suggestions below, go ahead once whywhat is happy.. https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:23: import org.chromium.printing.PrintDocumentAdapterWrapper.LayoutResultCallbackWrapper; nit: I think we normally don't import inner classes, just the outer one, i.e: import org.chromium.printing.PrintDocumentAdapterWrapper; ....implements PrintDocumentAdapterWrapper.LayoutResultCallbackWrapper { } ... https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:76: * order. nit: order, in the UI thread. https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:84: final PrintManagerDelegate MockPrintManagerDelegate = new PrintManagerDelegate() { nit: s/MockPrint.../mockPrint.../ https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:98: printDocumentStart(printingController); nit: instead of "printDocument...Start,Layout,Write,Finish", how about: call...Start,Layout,Write,Finish...OnUiThread ? so then the test here would look more like the description above: ... callStartOnUiThread(); .... callLayoutOnUiThread(); ... callWriteOnUiThread(); ... callFinishOnUiThread(); ... // Check PDF is valid. ...
LGTM with a few nits and "what Marcus said" (c) Do you think the chromiumtestshell_instrumentation_tests failure is due to new files being added but not compiled by the trybot? C 160s Main [FAIL] org.chromium.printing.PrintingControllerTest#testNormalPrintingFlow: C 160s Main java.lang.NoClassDefFoundError: org.chromium.printing.PrintDocumentAdapterWrapper C 160s Main at org.chromium.printing.PrintingControllerTest.testNormalPrintingFlow(PrintingControllerTest.java:92) C 160s Main at java.lang.reflect.Method.invokeNative(Native Method) C 160s Main at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) C 160s Main at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) C 160s Main at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) C 160s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:169) C 160s Main at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:154) C 160s Main at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:545) C 160s Main at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1551) https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:149: tempFile.delete(); nit: Maybe you could use TestFileUtil.deleteFile(path) to delete the file, that is already whitelisted in findbugs.txt so you don't have to whitelist your test. https://codereview.chromium.org/85693005/diff/20001/printing/android/java/src... File printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java (right): https://codereview.chromium.org/85693005/diff/20001/printing/android/java/src... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:60: mCallback = callback; nit: assert callback != null? https://codereview.chromium.org/85693005/diff/20001/printing/android/java/src... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:82: mCallback = callback; nit: ditto
https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... File chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java (right): https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:23: import org.chromium.printing.PrintDocumentAdapterWrapper.LayoutResultCallbackWrapper; On 2013/11/29 14:20:54, bulach wrote: > nit: I think we normally don't import inner classes, just the outer one, i.e: > import org.chromium.printing.PrintDocumentAdapterWrapper; > > ....implements PrintDocumentAdapterWrapper.LayoutResultCallbackWrapper { > } > ... > Done. https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:76: * order. On 2013/11/29 14:20:54, bulach wrote: > nit: order, in the UI thread. Done. https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:84: final PrintManagerDelegate MockPrintManagerDelegate = new PrintManagerDelegate() { On 2013/11/29 14:20:54, bulach wrote: > nit: s/MockPrint.../mockPrint.../ Done. https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:98: printDocumentStart(printingController); On 2013/11/29 14:20:54, bulach wrote: > nit: instead of "printDocument...Start,Layout,Write,Finish", how about: > call...Start,Layout,Write,Finish...OnUiThread ? > > so then the test here would look more like the description above: > > ... > callStartOnUiThread(); > .... > callLayoutOnUiThread(); > ... > callWriteOnUiThread(); > ... > callFinishOnUiThread(); > ... > // Check PDF is valid. > ... Done. https://codereview.chromium.org/85693005/diff/20001/chrome/android/javatests/... chrome/android/javatests/src/org/chromium/chrome/browser/printing/PrintingControllerTest.java:149: tempFile.delete(); On 2013/11/29 14:46:14, whywhat wrote: > nit: Maybe you could use TestFileUtil.deleteFile(path) to delete the file, that > is already whitelisted in findbugs.txt so you don't have to whitelist your test. Done. https://codereview.chromium.org/85693005/diff/20001/printing/android/java/src... File printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java (right): https://codereview.chromium.org/85693005/diff/20001/printing/android/java/src... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:60: mCallback = callback; On 2013/11/29 14:46:14, whywhat wrote: > nit: assert callback != null? Done. https://codereview.chromium.org/85693005/diff/20001/printing/android/java/src... printing/android/java/src/org/chromium/printing/PrintDocumentAdapterWrapper.java:82: mCallback = callback; On 2013/11/29 14:46:14, whywhat wrote: > nit: ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/85693005/40001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/85693005/50001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/85693005/70001
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...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/85693005/90001
Message was sent while issue was closed.
Change committed as 238776 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
