Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2349663003:
Adding WebView select tests to test select element's dropdown menu (Closed)
DescriptionAdding WebView select tests to test select element's dropdown menu.
BUG=647899
Committed: https://crrev.com/21ae75b4a9109648a5fa203145f50050a28cea64
Cr-Commit-Position: refs/heads/master@{#441606}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use BaseJunitTestRunner and addressed review comments #Patch Set 3 : use BaseJUnit4ClassRunner #
Total comments: 40
Patch Set 4 : Reorganized code into Actions and Atoms.java, addressed other review comments. #
Total comments: 3
Patch Set 5 : Addressed comments from mikecase@ #
Total comments: 3
Patch Set 6 : Add todo for UseWideViewPortAction #Messages
Total messages: 50 (29 generated)
Description was changed from ========== adding select tests BUG=647899 ========== to ========== adding select tests BUG=647899 Please ignore the action mode tests part, I'll rebase and upload once cl for that test lands. ==========
aluo@chromium.org changed reviewers: + mikecase@chromium.org, yolandyan@google.com
aluo@chromium.org changed reviewers: + mikecase@google.com, timvolodine@chromium.org - mikecase@chromium.org
Adding Tim for lgtm
aluo@chromium.org changed reviewers: - timvolodine@chromium.org
Removing Tim, wrong CL.
mikecase@chromium.org changed reviewers: + mikecase@chromium.org
Some comments. Probably will have some more once action_mode stuff lands and you rebase, https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java (right): https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:17: import static org.chromium.webview_ui_test.test.DropDownListUtils.selectAction; I think we should avoid static imports as much as possible. People use them for Espresso because of the way the API is designed it reads like a sentence kinda. I dont think we should use it here. https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:37: private static final String DATA_NON_SCALED = Might be nicer to have these in html files. https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:147: private Point getSize(View v) { This function is not needed. You call this once, and then immediately call point.x and point.y Instead, you could not call this funcction and just call webView.getWidth() and webView.getHeight() I also dont think you need getScroll() function either but your choice on that one. https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:154: private Web.WebInteraction<Void> getSelectElement(String id) { This function is only used in 1 location. Probably dont need this to be its own function.
Converted to use BaseJunitTestRunner, please review, thanks. https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java (right): https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:17: import static org.chromium.webview_ui_test.test.DropDownListUtils.selectAction; On 2016/11/04 15:39:20, mikecase wrote: > I think we should avoid static imports as much as possible. People use them for > Espresso because of the way the API is designed it reads like a sentence kinda. > I dont think we should use it here. Done. https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:37: private static final String DATA_NON_SCALED = On 2016/11/04 15:39:19, mikecase wrote: > Might be nicer to have these in html files. Done. https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:147: private Point getSize(View v) { On 2016/11/04 15:39:20, mikecase wrote: > This function is not needed. You call this once, and then immediately call > point.x and point.y > Instead, you could not call this funcction and just call webView.getWidth() and > webView.getHeight() > > I also dont think you need getScroll() function either but your choice on that > one. Done. https://codereview.chromium.org/2349663003/diff/1/android_webview/tools/autom... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:154: private Web.WebInteraction<Void> getSelectElement(String id) { On 2016/11/04 15:39:19, mikecase wrote: > This function is only used in 1 location. Probably dont need this to be its own > function. Done.
Ping to see if there are any more comments, thanks.
Update CL desc with proper punctuation/capitalization and remove the "Please ignore" section https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:48: nit: remove empty lines btw these constants. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:124: * get the scroll position of the view Nit: Capitalize get https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:4: package org.chromium.webview_ui_test.test; nit: add empty line above the package line https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:23: Integer mX, mY; nit: I think you should prefer using primitives over boxed-types. Change to int https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:45: * @param view the view to act upon. never null. same as other View action comment. MAybe mention in doc view must be WebView and use @NonNull annotation if you want. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:4: package org.chromium.webview_ui_test.test; nit: add empty line above the package line https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:21: public UseWideViewPortAction() { Should this be called SetUseWideViewportAction? Also, this constructor with no args is probably unnecessary. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:43: * @param view the view to act upon. never null. Mention the view must be a WebView. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:46: public void perform(UiController uiController, View view) { maybe instead of the "never null" comment above, just use the @NonNull annotation public void perform(UiController uiController, @NonNull View view) https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/test/data/webview_scaled.html (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/test/data/webview_scaled.html:23: <option value='v2'>Value2</option></select> nit: more </select> to its own line
yolandyan@chromium.org changed reviewers: + yolandyan@chromium.org
This is great! learned quite a bit about how SimpleAtom works! https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/java/res/layout/edittext_webview.xml (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/java/res/layout/edittext_webview.xml:17: <EditText hmm, why is this needed? https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:86: onView(withId(R.id.webview)).perform(new UseWideViewPortAction()); we should follow the Espresso standard of using a utility class to provide the actual implementations of ViewActions or Atoms. So maybe create a WebViewUiActions class with a static method called useWideViewPort(). https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:93: onView(withId(R.id.webview)).perform(new ScrollByAction(w, 0)); Same as above, create scrollHorizontal() and scrollVertical() method that returns ScrollByAction. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:147: Web.WebInteraction<Void> selectElement = maybe something like this: ``` onWebView() .withElement(findElement(Locator.ID, id)) .check(webMatches(DropDownListUtils.getCurrentSelection(), is(originalItemText))) .perform(selectAction()) onView(withText(targetItemText)).inRoot(withDecorView(isEnabled())) //The drop down menu is an android view onWebView() .withElement(findElement(Locator.ID, id)) .check(webMatches(DropDownListUtils.getCurrentSelection(), is(targetItemText))) ``` https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:151: selectElement.perform(DropDownListUtils.selectAction()); I assume this is because webClick() doesn't work here? https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:152: DropDownListUtils.getSelectItem("Value2").perform(click()); Value2 shouldn't be hardcoded here, maybe make the method takes argument of text? https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/DropDownListUtils.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/DropDownListUtils.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. DropDownSelectAtoms.java https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/DropDownListUtils.java:36: public static ViewInteraction getSelectItem(String itemText) { this shouldn't be abstracted. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. maybe put this under webview_ui_test/test/util/actions/ for better organization https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:32: return allOf(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE), hmm, since these are all used on WebView, maybe set his all to `isAssignableFrom(WebView.class);` https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. similarly put this under webview_ui_test/test/util/actions/ for better organization
Description was changed from ========== adding select tests BUG=647899 Please ignore the action mode tests part, I'll rebase and upload once cl for that test lands. ========== to ========== Adding WebView select tests to test select element's dropdown menu. BUG=647899 ==========
All right, please let me know if everything looks good, thanks. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:48: On 2016/11/15 00:50:49, mikecase wrote: > nit: remove empty lines btw these constants. Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:86: onView(withId(R.id.webview)).perform(new UseWideViewPortAction()); On 2016/11/15 01:45:44, the real yoland wrote: > we should follow the Espresso standard of using a utility class to provide the > actual implementations of ViewActions or Atoms. So maybe create a > WebViewUiActions class with a static method called useWideViewPort(). Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:93: onView(withId(R.id.webview)).perform(new ScrollByAction(w, 0)); On 2016/11/15 01:45:44, the real yoland wrote: > Same as above, create scrollHorizontal() and scrollVertical() method that > returns ScrollByAction. Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:124: * get the scroll position of the view On 2016/11/15 00:50:49, mikecase wrote: > Nit: Capitalize get Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:147: Web.WebInteraction<Void> selectElement = On 2016/11/15 01:45:44, the real yoland wrote: > maybe something like this: > ``` > onWebView() > .withElement(findElement(Locator.ID, id)) > .check(webMatches(DropDownListUtils.getCurrentSelection(), > is(originalItemText))) > .perform(selectAction()) > onView(withText(targetItemText)).inRoot(withDecorView(isEnabled())) //The drop > down menu is an android view > onWebView() > .withElement(findElement(Locator.ID, id)) > .check(webMatches(DropDownListUtils.getCurrentSelection(), > is(targetItemText))) > ``` Not sure what the suggestion here? The line "onView(withText(targetItemText)).inRoot(withDecorView(isEnabled()))" isn't checking anything. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:151: selectElement.perform(DropDownListUtils.selectAction()); On 2016/11/15 01:45:44, the real yoland wrote: > I assume this is because webClick() doesn't work here? correct https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:152: DropDownListUtils.getSelectItem("Value2").perform(click()); On 2016/11/15 01:45:44, the real yoland wrote: > Value2 shouldn't be hardcoded here, maybe make the method takes argument of > text? Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/DropDownListUtils.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/DropDownListUtils.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/15 01:45:44, the real yoland wrote: > DropDownSelectAtoms.java changed to Atoms.java https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/DropDownListUtils.java:36: public static ViewInteraction getSelectItem(String itemText) { On 2016/11/15 01:45:44, the real yoland wrote: > this shouldn't be abstracted. Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/15 01:45:44, the real yoland wrote: > maybe put this under webview_ui_test/test/util/actions/ for better organization Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:4: package org.chromium.webview_ui_test.test; On 2016/11/15 00:50:49, mikecase wrote: > nit: add empty line above the package line Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:23: Integer mX, mY; On 2016/11/15 00:50:49, mikecase wrote: > nit: I think you should prefer using primitives over boxed-types. Change to int Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:32: return allOf(withEffectiveVisibility(ViewMatchers.Visibility.VISIBLE), On 2016/11/15 01:45:44, the real yoland wrote: > hmm, since these are all used on WebView, maybe set his all to > `isAssignableFrom(WebView.class);` This action will work on any view. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/ScrollByAction.java:45: * @param view the view to act upon. never null. On 2016/11/15 00:50:49, mikecase wrote: > same as other View action comment. MAybe mention in doc view must be WebView and > use @NonNull annotation if you want. Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java (right): https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/11/15 01:45:44, the real yoland wrote: > similarly put this under webview_ui_test/test/util/actions/ for better > organization Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:4: package org.chromium.webview_ui_test.test; On 2016/11/15 00:50:50, mikecase wrote: > nit: add empty line above the package line Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:21: public UseWideViewPortAction() { On 2016/11/15 00:50:49, mikecase wrote: > Should this be called SetUseWideViewportAction? > > Also, this constructor with no args is probably unnecessary. Keeping it since this is a common case, changed name though. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:43: * @param view the view to act upon. never null. On 2016/11/15 00:50:50, mikecase wrote: > Mention the view must be a WebView. Done. https://codereview.chromium.org/2349663003/diff/40001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:46: public void perform(UiController uiController, View view) { On 2016/11/15 00:50:50, mikecase wrote: > maybe instead of the "never null" comment above, just use the @NonNull > annotation > > public void perform(UiController uiController, @NonNull View view) Done.
Please let me know if there's any other comments, thanks.
The CQ bit was checked by aluo@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: This issue passed the CQ dry run.
Description was changed from ========== Adding WebView select tests to test select element's dropdown menu. BUG=647899 ========== to ========== Adding WebView select tests to test select element's dropdown menu. BUG=647899 ==========
Ping for lgtm
some nits but this looks good to me. Will give Yoland a chance to look at the Espresso (Atom/Actions) code since Im not too familiar https://codereview.chromium.org/2349663003/diff/60001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java (right): https://codereview.chromium.org/2349663003/diff/60001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:54: private static final String AFTER_VALUE = "Value2"; nit: alphabetize https://codereview.chromium.org/2349663003/diff/60001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/Atoms.java (right): https://codereview.chromium.org/2349663003/diff/60001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/Atoms.java:25: + "false, false, false, false, 0, null);" nit: Would prefer that Javascript string had proper indenting if possible https://codereview.chromium.org/2349663003/diff/60001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java (right): https://codereview.chromium.org/2349663003/diff/60001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:4: package org.chromium.webview_ui_test.test; nit: add a space above package name
The CQ bit was checked by aluo@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...
Any additional comments (Yoland?), I'd like to get this in soon, thanks!
On 2016/12/14 at 19:18:42, aluo wrote: > Any additional comments (Yoland?), I'd like to get this in soon, thanks! Sorry, will review this today!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2349663003/diff/80001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java (right): https://codereview.chromium.org/2349663003/diff/80001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/DropDownListTest.java:152: Web.WebInteraction<Void> selectElement = nit: what I meant last time for this is to change not save selectElement, rather, do `onWebView().withElement(findElement(Locator.ID, id))` every time when this element is needed. But that's just my interpretation of how espresso are meant to be like (because things might change before and after `onView(withTest)...perform(click())` for selectElement) https://codereview.chromium.org/2349663003/diff/80001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/Actions.java (right): https://codereview.chromium.org/2349663003/diff/80001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/Actions.java:61: isDescendantOfA(isAssignableFrom(View.class))); nits: this is also only used for WebView? https://codereview.chromium.org/2349663003/diff/80001/android_webview/tools/a... File android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java (right): https://codereview.chromium.org/2349663003/diff/80001/android_webview/tools/a... android_webview/tools/automated_ui_tests/javatests/src/org/chromium/webview_ui_test/test/util/UseWideViewPortAction.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. This is probably better to set it in the WebViewUiTestRule, but not urgent, maybe a TODO would be fine
I am very sorry about the delay! I will buy you icecream andrew!
The CQ bit was checked by aluo@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: This issue passed the CQ dry run.
The CQ bit was checked by aluo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yolandyan@chromium.org Link to the patchset: https://codereview.chromium.org/2349663003/#ps100001 (title: "Add todo for UseWideViewPortAction")
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by aluo@chromium.org
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 aluo@chromium.org
The CQ bit was checked by aluo@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": 100001, "attempt_start_ts": 1483602507350210,
"parent_rev": "ce2cfe770bc02341cd5bb4a5ad3f8501f9d5318b", "commit_rev":
"3631b1fc47c0907d03fc5a2ab2a0e0c37db78e4c"}
Message was sent while issue was closed.
Description was changed from ========== Adding WebView select tests to test select element's dropdown menu. BUG=647899 ========== to ========== Adding WebView select tests to test select element's dropdown menu. BUG=647899 Review-Url: https://codereview.chromium.org/2349663003 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Adding WebView select tests to test select element's dropdown menu. BUG=647899 Review-Url: https://codereview.chromium.org/2349663003 ========== to ========== Adding WebView select tests to test select element's dropdown menu. BUG=647899 Committed: https://crrev.com/21ae75b4a9109648a5fa203145f50050a28cea64 Cr-Commit-Position: refs/heads/master@{#441606} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/21ae75b4a9109648a5fa203145f50050a28cea64 Cr-Commit-Position: refs/heads/master@{#441606} |
