Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218623003:
Remove |ContentViewCore.getViewAndroidDelegate()| (Closed)
|
Created:
4 years, 4 months ago by Jinsuk Kim Modified:
4 years, 4 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, jam, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, darin-cc_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove |ContentViewCore.getViewAndroidDelegate()|
Removed the method since it is prone to misuse and now in use for
test classes only. The tests now define their own ViewAndroidDelegate
instance rather than the one defined internally but inaccessible
due to the removal. This does not make difference from the tests'
perspective because it is associated with the same
container view to acquire/remove anchor views.
BUG=617750
Committed: https://crrev.com/5ad16b4b8be2fb8d3890a984f043c843f3f536e8
Cr-Commit-Position: refs/heads/master@{#411257}
Patch Set 1 #
Total comments: 4
Patch Set 2 : comments #
Total comments: 4
Patch Set 3 : comments #
Messages
Total messages: 32 (18 generated)
The CQ bit was checked by jinsukkim@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 ========== Remove |ContentViewCore.getViewAndroidDelegate()| Removed the method since it is prone to misuse and now in use for test classes only. The tests now define their own ViewAndroidDelegate instance rather than the one defined internally but became inaccessible due to the removal. This does not make any difference from the tests' perspective - it is used to acquire/remove anchor views against the same container view. BUG=617750 ========== to ========== Remove |ContentViewCore.getViewAndroidDelegate()| Removed the method since it is prone to misuse and now in use for test classes only. The tests now define their own ViewAndroidDelegate instance rather than the one defined internally but inaccessible due to the removal. This does not make difference from the tests' perspective because it is associated with the same container view to acquire/remove anchor views. BUG=617750 ==========
jinsukkim@chromium.org changed reviewers: + boliu@chromium.org, nyquist@chromium.org, sievers@chromium.org
Removing one TODO at a time.. nyquist@chromium.org: Please review changes in chrome/ boliu@chromium.org: Please review changes in android_webview/ sievers@chromium.org: Please review changes in content/
https://codereview.chromium.org/2218623003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java (left): https://codereview.chromium.org/2218623003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:27: @UiThreadTest why remove UiThreadTest? a lot of these tests are no longer thread safe if you remove that
https://codereview.chromium.org/2218623003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java (left): https://codereview.chromium.org/2218623003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:27: @UiThreadTest On 2016/08/05 15:51:21, boliu wrote: > why remove UiThreadTest? a lot of these tests are no longer thread safe if you > remove that That being said, these tests also don't really need a browser context i.e. they could be java unit tests instead (I mean they basically already are... except that they spin up a browser for no good reason). But that can probably be spun off into a different task for someone else - unless it's straightforward?
chrome/android lgtm
The CQ bit was checked by jinsukkim@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.
https://codereview.chromium.org/2218623003/diff/1/android_webview/javatests/s... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java (left): https://codereview.chromium.org/2218623003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:27: @UiThreadTest On 2016/08/05 15:51:21, boliu wrote: > why remove UiThreadTest? a lot of these tests are no longer thread safe if you > remove that Got it. Put it back. https://codereview.chromium.org/2218623003/diff/1/android_webview/javatests/s... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:27: @UiThreadTest On 2016/08/05 18:07:22, sievers wrote: > On 2016/08/05 15:51:21, boliu wrote: > > why remove UiThreadTest? a lot of these tests are no longer thread safe if you > > remove that > > That being said, these tests also don't really need a browser context i.e. they > could be java unit tests instead (I mean they basically already are... except > that they spin up a browser for no good reason). > > But that can probably be spun off into a different task for someone else - > unless it's straightforward? The tests don't bring up the whole browser; it is an instrument test that just provides an activity which is necessary for the tests to create views/layouts. I went as far as removing AwTestContainerView/TestAwContentsClient in the new patch - they are not necessary for what's being tested. You reminded me of the ContentViewCoreViewAndroidDelegateTest that indeed brings up browser unnecessarily. I deleted the class, moved the remaining tests to this class as there's not much left (only 2 simple tests) in that class any more. Though the 2 tests are not strictly related to WebView delegate, I hope that's fine. Will write a new test for ViewAndroidDelegate under ui/ if necessity for more tests arise for the class.
lgtm % comment nits https://codereview.chromium.org/2218623003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java (right): https://codereview.chromium.org/2218623003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:80: // Verify that no anchor view is transferred between mContainerViews don't change comment https://codereview.chromium.org/2218623003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:160: // Replace mContainerView ditto
lgtm
The CQ bit was checked by jinsukkim@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2218623003/#ps40001 (title: "comments")
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 jinsukkim@chromium.org
https://codereview.chromium.org/2218623003/diff/20001/android_webview/javates... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java (right): https://codereview.chromium.org/2218623003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:80: // Verify that no anchor view is transferred between mContainerViews On 2016/08/08 05:36:55, boliu wrote: > don't change comment Done. https://codereview.chromium.org/2218623003/diff/20001/android_webview/javates... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsAnchorViewTest.java:160: // Replace mContainerView On 2016/08/08 05:36:55, boliu wrote: > ditto Done.
The CQ bit was checked by jinsukkim@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 commit-bot@chromium.org
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 jinsukkim@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove |ContentViewCore.getViewAndroidDelegate()| Removed the method since it is prone to misuse and now in use for test classes only. The tests now define their own ViewAndroidDelegate instance rather than the one defined internally but inaccessible due to the removal. This does not make difference from the tests' perspective because it is associated with the same container view to acquire/remove anchor views. BUG=617750 ========== to ========== Remove |ContentViewCore.getViewAndroidDelegate()| Removed the method since it is prone to misuse and now in use for test classes only. The tests now define their own ViewAndroidDelegate instance rather than the one defined internally but inaccessible due to the removal. This does not make difference from the tests' perspective because it is associated with the same container view to acquire/remove anchor views. BUG=617750 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove |ContentViewCore.getViewAndroidDelegate()| Removed the method since it is prone to misuse and now in use for test classes only. The tests now define their own ViewAndroidDelegate instance rather than the one defined internally but inaccessible due to the removal. This does not make difference from the tests' perspective because it is associated with the same container view to acquire/remove anchor views. BUG=617750 ========== to ========== Remove |ContentViewCore.getViewAndroidDelegate()| Removed the method since it is prone to misuse and now in use for test classes only. The tests now define their own ViewAndroidDelegate instance rather than the one defined internally but inaccessible due to the removal. This does not make difference from the tests' perspective because it is associated with the same container view to acquire/remove anchor views. BUG=617750 Committed: https://crrev.com/5ad16b4b8be2fb8d3890a984f043c843f3f536e8 Cr-Commit-Position: refs/heads/master@{#411257} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5ad16b4b8be2fb8d3890a984f043c843f3f536e8 Cr-Commit-Position: refs/heads/master@{#411257} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
