Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2894523004:
Photo Picker dialog: Add a test. (Closed)
DescriptionPhoto Picker dialog: Add a test.
BUG=656015
Review-Url: https://codereview.chromium.org/2894523004
Cr-Commit-Position: refs/heads/master@{#474220}
Committed: https://chromium.googlesource.com/chromium/src/+/d9515a4f017409b83c75f5a743678a04dfb047b0
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add missing file #Patch Set 3 : Remove override #Patch Set 4 : Address findbugs issues #Patch Set 5 : Fix findbug issue #Patch Set 6 : Fix vector drawable error #
Total comments: 10
Patch Set 7 : Address feedback from Theresa #
Total comments: 12
Patch Set 8 : Address comments from Theresa #
Total comments: 4
Patch Set 9 : Address feedback from Ted #Messages
Total messages: 63 (46 generated)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
finnur@chromium.org changed reviewers: + twellington@chromium.org
https://codereview.chromium.org/2894523004/diff/20001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/PhotoPickerListener.java (left): https://codereview.chromium.org/2894523004/diff/20001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/PhotoPickerListener.java:45: Map<String, Long> getFilesForTesting(); I thought this hadn't gone in already, but looks like it did. Anyway -- this was how I was thinking about it initially -- but decided to optionally pass the data through the ctor instead (seemed cleaner than polluting this interface).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Second try. Now includes file that was missing in last upload. :) PTAL.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hmm... interesting. It compiled just fine on my machine before I uploaded. Anyway, troublesome override removed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
https://codereview.chromium.org/2894523004/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2894523004/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:37: boolean multiSelectionAllowed, @Nullable List<PickerBitmap> testFiles) { Rather than passing test files into the constructor, a more common pattern is to expose a static method for testing that sets a static list of files. The test can set the list of files during setup before creating the dialog. DownloadManagerUi#setProviderForTests() is a good example of this. https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java (right): https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:73: 5L, PickerBitmap.PICTURE)); mTestFiles.add(new nit: remove this comment block https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:133: CriteriaHelper.pollUiThread(new Criteria() { Where possible, it's preferable to rely on specific events rather than polling the UI thread. RecyclerView has an #addOnChildAttachStateChangeListener() that we should be able to use to detect when a child view has been attached to the RecyclerView. https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:162: CriteriaHelper.pollUiThread(new Criteria() { This is waiting for the view to be selected, correct? The other classes that use SelectableListLayout have callback helpers that wait for selection to change. The SelectionDelegate can have multiple observers, so you can create a test observer that's notified when the selection state changes. HistoryActivityTest also uses a RecyclerView.AdapterDataObserver; something like that may be useful for detecting when adapter/recyclerview contents have changed. https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:188: CriteriaHelper.pollUiThread(new Criteria() { Instead of polling the UI thread, you should be able to use a CallbackHelper. #onPickerUserAction() would notify the callback, and this method would wait for it be notified.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Patchset #7 (id:130001) has been deleted
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2894523004/diff/110001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java (right): https://codereview.chromium.org/2894523004/diff/110001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialog.java:37: boolean multiSelectionAllowed, @Nullable List<PickerBitmap> testFiles) { Good idea. Thanks! Done. https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java (right): https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:73: 5L, PickerBitmap.PICTURE)); mTestFiles.add(new Oops. :) Done. https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:133: CriteriaHelper.pollUiThread(new Criteria() { Not all of the polling turned out to be necessary; but I've converted all the other ones you pointed out to use a callback. https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:162: CriteriaHelper.pollUiThread(new Criteria() { On 2017/05/19 16:52:42, Theresa wrote: > This is waiting for the view to be selected, correct? > > The other classes that use SelectableListLayout have callback helpers that wait > for selection to change. The SelectionDelegate can have multiple observers, so > you can create a test observer that's notified when the selection state changes. > HistoryActivityTest also uses a RecyclerView.AdapterDataObserver; something like > that may be useful for detecting when adapter/recyclerview contents have > changed. > > https://cs.chromium.org/chromium/src/chrome/android/javatests/src/org/chromiu... Done. https://codereview.chromium.org/2894523004/diff/110001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:188: CriteriaHelper.pollUiThread(new Criteria() { On 2017/05/19 16:52:42, Theresa wrote: > Instead of polling the UI thread, you should be able to use a CallbackHelper. > #onPickerUserAction() would notify the callback, and this method would wait for > it be notified. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
Thank you for all of the changes! This looks really good overall, just a few more comments. Will you please add an OWNERS file to the new test directory that references chrome/android/java/src/org/chromium/chrome/browser/photo_picker/OWNERS https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java (right): https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:101: private RecyclerView recyclerView() { nit: getRecyclerView()? https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:107: ThreadUtils.runOnUiThreadBlockingNoException(new Callable<PhotoPickerDialog>() { Typically I see ThreadUtils.runOnUiThreadBlocking() rather than the NoException flavor. Is there a benefit to the NoException in this application? https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:126: public void clickView(final int position, final int expectedSelectionCount) throws Exception { nit: can this method be private? Same for the other click* methods. https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:128: final PickerBitmapViewHolder holder = I believe findbugs is complaining about this line being a deadstore. https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:138: int i = 0; nit: put the i = 0 inside the for loop since it's not needed outside the for loop. for (int i = 0; i < ...) https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:141: mTestFiles.get(position).getFilePath())) { SelectionDelegate has an #isItemSelected() method that may be helpful for asserting that the item that was just clicked is now selected.
The CQ bit was checked by finnur@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Photo Picker dialog: Add a test. BUG=656015 ========== to ========== Photo Picker dialog: Add a test. BUG=656015 ==========
finnur@chromium.org changed reviewers: + dfalcantara@chromium.org
twellington@: All addressed. PTAL. dfalcantara@: Mind giving OWNERS approval for the new OWNERS file and the test Java file? Thanks. If this looks OK to both, it would be appreciated if the latter of you (to give the green light) would check the CQ box.
Missing comments... https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java (right): https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:101: private RecyclerView recyclerView() { On 2017/05/23 15:10:29, Theresa wrote: > nit: getRecyclerView()? Done. https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:107: ThreadUtils.runOnUiThreadBlockingNoException(new Callable<PhotoPickerDialog>() { Hmm... no. Probably just copied from somewhere. https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:126: public void clickView(final int position, final int expectedSelectionCount) throws Exception { On 2017/05/23 15:10:29, Theresa wrote: > nit: can this method be private? Same for the other click* methods. Done. https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:128: final PickerBitmapViewHolder holder = On 2017/05/23 15:10:29, Theresa wrote: > I believe findbugs is complaining about this line being a deadstore. Done. https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:138: int i = 0; Yes, I moved it out because it was being used, but it no longer is. Forgot to move back. :) https://codereview.chromium.org/2894523004/diff/150001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:141: mTestFiles.get(position).getFilePath())) { Ah, right! Of course. Done.
lgtm Dan is out for the rest of the week, so we need a different javatests OWNER
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
finnur@chromium.org changed reviewers: + tedchoc@chromium.org - dfalcantara@chromium.org
finnur@chromium.org changed reviewers: + dfalcantara@chromium.org - tedchoc@chromium.org
finnur@chromium.org changed reviewers: + tedchoc@chromium.org - dfalcantara@chromium.org
Ted, do you mind doing an OWNERS check on the tests dir. If it looks OK, I would appreciate if you could flip the CQ button.
lgtm w/ a couple nits https://codereview.chromium.org/2894523004/diff/170001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java (right): https://codereview.chromium.org/2894523004/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:32: @RetryOnFailure we shouldn't add RetryOnFailure for new test classes https://codereview.chromium.org/2894523004/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:176: mDialog.dismiss(); I would wrap these in ThreadUtils.runOnUiThreadBlocking
Thanks, Ted and Theresa! All addressed, checking in. https://codereview.chromium.org/2894523004/diff/170001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java (right): https://codereview.chromium.org/2894523004/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:32: @RetryOnFailure Ah, good catch. Copy paste error. Removed. https://codereview.chromium.org/2894523004/diff/170001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/photo_picker/PhotoPickerDialogTest.java:176: mDialog.dismiss(); On 2017/05/23 20:57:39, Ted C wrote: > I would wrap these in ThreadUtils.runOnUiThreadBlocking Done.
The CQ bit was checked by finnur@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2894523004/#ps190001 (title: "Address feedback from Ted")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by finnur@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 190001, "attempt_start_ts": 1495613610649670,
"parent_rev": "5ed8ccaec77a06d6610d40e1ea7aefa8e364eb2b", "commit_rev":
"d9515a4f017409b83c75f5a743678a04dfb047b0"}
Message was sent while issue was closed.
Description was changed from ========== Photo Picker dialog: Add a test. BUG=656015 ========== to ========== Photo Picker dialog: Add a test. BUG=656015 Review-Url: https://codereview.chromium.org/2894523004 Cr-Commit-Position: refs/heads/master@{#474220} Committed: https://chromium.googlesource.com/chromium/src/+/d9515a4f017409b83c75f5a74367... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:190001) as https://chromium.googlesource.com/chromium/src/+/d9515a4f017409b83c75f5a74367... |
