Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
667143002:
[aw] Software mode tests for fullscreen API. (Closed)
|
Created:
6 years, 2 months ago by Ignacio Solla Modified:
6 years, 1 month ago Reviewers:
boliu CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@fixPowerBlockerNonMedia Project:
chromium Visibility:
Public. |
Description[aw] Software mode tests for fullscreen API.
To avoid possible deadlocks, we need to propagate
the layer type from the old container view to the
FullscreenView when entering fullscreen.
Plus a number of other improvements:
- Fix a possible crash (improbable outside tests) when
exitFullscreen is called between enterFullscreen and
enterFullscreenVideo
- Test that for fullscreen video a ContentVideoView
is created
- Remove some flakiness by waiting for the fulscreen
property in Javascript to be set
BUG=424213, 429642
Committed: https://crrev.com/06d14e5623a8e879f7e6d56c84650c0f524e2c5b
Cr-Commit-Position: refs/heads/master@{#302816}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Nits #
Total comments: 2
Patch Set 3 : Address Bo's comments #
Total comments: 4
Patch Set 4 : Capitalize "f" in TODO comment #Patch Set 5 : Rebase #Patch Set 6 : Fix possible deadlock when entering fullscreen in software mode #
Total comments: 1
Patch Set 7 : Make DisableHardware annotation apply to window #
Total comments: 4
Patch Set 8 : Move check to testing code. #Patch Set 9 : Rebase and move check for real #
Messages
Total messages: 45 (8 generated)
igsolla@chromium.org changed reviewers: + boliu@chromium.org
> - Fix a possible crash (improbable outside tests) when exitFullscreen is called between enterFullscreen and enterFullscreenVideo That's bad... Is this enterFullscreen/enterFullscreenVideo new for the full screen support for elements other than video? Feels like they shouldn't be two calls..
On 2014/10/21 16:17:36, boliu wrote: > > - Fix a possible crash (improbable outside tests) when exitFullscreen is > called between enterFullscreen and enterFullscreenVideo > > That's bad... Is this enterFullscreen/enterFullscreenVideo new for the full > screen support for elements other than video? Feels like they shouldn't be two > calls.. Thanks for letting me know your opinion about code that has already been reviewed. It is not ideal, but do you actually understand the reasons behind? If not, I feel that it is gratuitous to make such judgements calls. I doubt that revisiting this is the best use of our time given that the original change has already been reviewed throughly here: https://codereview.chromium.org/618013003/ I'd be grateful if you could help me review the new changes. Thanks.
I am reviewing this CL. It's generally a bad smell to replace an assert with an if, so I wanted to know if could be reworked to avoid that. > It is not ideal, but do you actually understand the reasons behind? I do not. Want to point me to an explanation for this? https://codereview.chromium.org/618013003/ is pretty long
Ok understood. Note that I have replaced the assert with tests. On 21 Oct 2014 19:30, <boliu@chromium.org> wrote: > I am reviewing this CL. It's generally a bad smell to replace an assert > with an > if, so I wanted to know if could be reworked to avoid that. > > It is not ideal, but do you actually understand the reasons behind? >> > > I do not. Want to point me to an explanation for this? > https://codereview.chromium.org/618013003/ is pretty long > > https://codereview.chromium.org/667143002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
asserts are better than tests :)
On 2014/10/21 18:47:10, boliu wrote: > asserts are better than tests :) I agree, but this assert is too strict. It fails with a valid combination of events that I missed when I originally added it. Ok, let me revisit https://codereview.chromium.org/618013003/ now that I'm in front of a computer again. The fact that we have two methods (enterFullscreen and enterFullscreenVideo) is a result of the content API. The description in that CL gives some background: When the browser receives the toggleFullscreenModeForTab IPC we ask the app to show the custom ViewGroup (containing a FullscreenView with the web contents in fullscreen). If later (for the video element case) a ContentVideoView is created then we add it to the custom ViewGroup. We could avoid having two methods if toggleFullscreenModeForTab would tell us whether we are going fullscreen on a video element or not, or if the ContentVideoView was created before toggleFullscreenModeForTab is invoked. None of those is the case, so this is the best we can do with the current content API. Improving the fullscreen content API in general would be good, but that's a different goal that should not block support for fullscreen for non-media elements in the WebView. If you want to revisit this decision, feel free to book some time in my calendar to discuss offline. But as I said above I do not think that that's the best use of our time. Btw, saying that things are bad before understanding the reason behind is not constructive feedback. If next time you want to know whether changes can be reworked, please say that and ask for more context :)
On 2014/10/21 19:27:53, Ignacio Solla wrote: > Btw, saying that things are bad before understanding the reason behind is not > constructive feedback. If next time you want to know whether changes can be > reworked, please say that and ask for more context :) It's my signal that I don't the direction of the CL. Existing bad code is not an excuse for making it worse, so doing so always requires more justification. Note on code quality: Chrome's philosophy is never compromise quality for to deadlines, and only ship when features are ready. Of course it's harder to keep to that on android where there are hard deadlines. But we should still try. We have fallen short in a lot of places. Media code on android is just crap all around. This is mostly due to multiple groups coming in and hacking on a feature, then left, leaving the code in a bigger mess than before. So it's not surprising you are finding difficult to work with this code. The quality of the webview graphics code I wrote is approaching a stinking pile of poo as well. But at least I have a plan to add tests and then improve it. https://codereview.chromium.org/667143002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:69: if (mCustomView == null) { This should be a content level fix then. Why is content is calling enterFullscreenVideo if nothing is currently in full screen? https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:331: return containsVideoView(mContentsClient.getCustomView()); Need to do this on the UI thread. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:342: return DOMUtils.isFullscreen(mContentViewCore.getWebContents()); I didn't find this DOMUtils method?! I assume it needs to happen on UI thread as well. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:351: private boolean containsVideoView(View view) { nit: move this next to assertContainsContentVideoView https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:397: DOMUtils.clickNode(this, mContentViewCore, CUSTOM_FULLSCREEN_CONTROL_ID); UI thread.
https://codereview.chromium.org/667143002/diff/1/android_webview/java/src/org... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/java/src/org... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:69: if (mCustomView == null) { On 2014/10/22 02:01:02, boliu wrote: > This should be a content level fix then. Why is content is calling > enterFullscreenVideo if nothing is currently in full screen? Those are all good questions and suggestions. I have filed https://code.google.com/p/chromium/issues/detail?id=425926 to investigate. In the meantime I'd like to keep this workaround. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:331: return containsVideoView(mContentsClient.getCustomView()); On 2014/10/22 02:01:02, boliu wrote: > Need to do this on the UI thread. Why? I'm happy to change it but I'd like to understand the reason first. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:342: return DOMUtils.isFullscreen(mContentViewCore.getWebContents()); On 2014/10/22 02:01:02, boliu wrote: > I didn't find this DOMUtils method?! I assume it needs to happen on UI thread as > well. To avoid conflicts, I have set the diffbase of this change against: https://codereview.chromium.org/639413002/ So crcr/639413002 needs to be landed first. That method is defined there. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:351: private boolean containsVideoView(View view) { On 2014/10/22 02:01:02, boliu wrote: > nit: move this next to assertContainsContentVideoView Done. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:397: DOMUtils.clickNode(this, mContentViewCore, CUSTOM_FULLSCREEN_CONTROL_ID); On 2014/10/22 02:01:02, boliu wrote: > UI thread. We're already dispatching the click event on the UI thread, see TouchCommon.dispatchTouchEvent that clickNode ends up calling. Do we also need to retrieve the position of the node in the UI thread? I'm happy to change it but I'd like to understand the reason first.
> Existing bad code is not an > excuse for making it worse, so doing so always requires more justification. I completely agree with this statement. But my point was that the justifications about why we have 2 methods, enterFullscreenVideo and enterFullscreen, have already been provided during the review of https://codereview.chromium.org/618013003/. Do we need to provide them again here? It seems that we both have different views about this last point, and I guess that's fine :) Anyway, if things are still not clear maybe it might be easier to sort this out in chat or quick video meeting?
On 2014/10/22 09:41:18, Ignacio Solla wrote: > > Existing bad code is not an > > excuse for making it worse, so doing so always requires more justification. > > I completely agree with this statement. But my point was that the justifications > about why we have 2 methods, enterFullscreenVideo and enterFullscreen, have > already been provided during the review of > https://codereview.chromium.org/618013003/. Do we need to provide them again > here? Yes. I didn't review that CL. Submitted code doesn't mean it's "good" or bug free. In general, don't be afraid to fix issues as you see them. > It seems that we both have different views about this last point, and I > guess that's fine :) Anyway, if things are still not clear maybe it might be > easier to sort this out in chat or quick video meeting? I talked to Daniel who reviewed that CL. So now I understand reasons now. Still really really don't like the the reasons, but fixing it is a ginormous effort. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:331: return containsVideoView(mContentsClient.getCustomView()); On 2014/10/22 09:32:16, Ignacio Solla wrote: > On 2014/10/22 02:01:02, boliu wrote: > > Need to do this on the UI thread. > > Why? I'm happy to change it but I'd like to understand the reason first. Because AwContents/ContentViewCore, and the entire view tree is single threaded and should only be accessed on the UI thread. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:342: return DOMUtils.isFullscreen(mContentViewCore.getWebContents()); On 2014/10/22 09:32:16, Ignacio Solla wrote: > On 2014/10/22 02:01:02, boliu wrote: > > I didn't find this DOMUtils method?! I assume it needs to happen on UI thread > as > > well. > > To avoid conflicts, I have set the diffbase of this change against: > https://codereview.chromium.org/639413002/ > > So crcr/639413002 needs to be landed first. That method is defined there. Ok. isFullscreen itself looks thread safe. mContentViewCore.getWebContents() is not!! https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:397: DOMUtils.clickNode(this, mContentViewCore, CUSTOM_FULLSCREEN_CONTROL_ID); On 2014/10/22 09:32:16, Ignacio Solla wrote: > On 2014/10/22 02:01:02, boliu wrote: > > UI thread. > > We're already dispatching the click event on the UI thread, see > TouchCommon.dispatchTouchEvent that clickNode ends up calling. > > Do we also need to retrieve the position of the node in the UI thread? I'm happy > to change it but I'd like to understand the reason first. Ahh crap I didn't read that far. I'm pretty sure that findViewById call in TouchCommon ain't thread safe either... But even before that, you can't use ContentViewCore in DOMUtils on the instrumentation thread. https://codereview.chromium.org/667143002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/667143002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:70: // enterFullscreenVideo will only be called after enterFullscreen, but Add a TODO and point to the bug then.
https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:331: return containsVideoView(mContentsClient.getCustomView()); On 2014/10/22 15:53:45, boliu wrote: > On 2014/10/22 09:32:16, Ignacio Solla wrote: > > On 2014/10/22 02:01:02, boliu wrote: > > > Need to do this on the UI thread. > > > > Why? I'm happy to change it but I'd like to understand the reason first. > > Because AwContents/ContentViewCore, and the entire view tree is single threaded > and should only be accessed on the UI thread. OK done. (I was thinking that read-only operations with polling in the instrumentation thread in tests were ok, but I guess it is safer to read it on the UI thread because thread-local caches in Java) https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:342: return DOMUtils.isFullscreen(mContentViewCore.getWebContents()); On 2014/10/22 15:53:45, boliu wrote: > On 2014/10/22 09:32:16, Ignacio Solla wrote: > > On 2014/10/22 02:01:02, boliu wrote: > > > I didn't find this DOMUtils method?! I assume it needs to happen on UI > thread > > as > > > well. > > > > To avoid conflicts, I have set the diffbase of this change against: > > https://codereview.chromium.org/639413002/ > > > > So crcr/639413002 needs to be landed first. That method is defined there. > > Ok. isFullscreen itself looks thread safe. mContentViewCore.getWebContents() is > not!! Ok, I'm calling mContentViewCore.getWebContents() on the UI thread now. Note that we cannot call DOMUtils.blah on the UI thread because JavaScriptUtils.executeJavaScriptAndWaitForResult must be called on the instrumentation test. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:397: DOMUtils.clickNode(this, mContentViewCore, CUSTOM_FULLSCREEN_CONTROL_ID); On 2014/10/22 15:53:45, boliu wrote: > On 2014/10/22 09:32:16, Ignacio Solla wrote: > > On 2014/10/22 02:01:02, boliu wrote: > > > UI thread. > > > > We're already dispatching the click event on the UI thread, see > > TouchCommon.dispatchTouchEvent that clickNode ends up calling. > > > > Do we also need to retrieve the position of the node in the UI thread? I'm > happy > > to change it but I'd like to understand the reason first. > > Ahh crap I didn't read that far. I'm pretty sure that findViewById call in > TouchCommon ain't thread safe either... > > But even before that, you can't use ContentViewCore in DOMUtils on the > instrumentation thread. clickNode cannot be called on the instrumentation thread because executeJavaScriptAndWaitForResult will block otherwise. You can also see that all other tests in the codebase call DOMUtils.clickNode on the instrumentation thread. Fixing the problems that you pointed out with DOMUtils.clickNode are outside the scope of this patch (but don't be afraid to fix it if you want, but understand that I don't feel that dumping this on me is reasonable. FYI, I have noticed and fixed many issues with fullscreen / video playback so in general I'm not afraid of doing that, but do understand that sometimes I also like to do more interesting work). https://codereview.chromium.org/667143002/diff/20001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/667143002/diff/20001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:70: // enterFullscreenVideo will only be called after enterFullscreen, but On 2014/10/22 15:53:45, boliu wrote: > Add a TODO and point to the bug then. Done.
https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:342: return DOMUtils.isFullscreen(mContentViewCore.getWebContents()); On 2014/10/27 11:42:06, Ignacio Solla wrote: > On 2014/10/22 15:53:45, boliu wrote: > > On 2014/10/22 09:32:16, Ignacio Solla wrote: > > > On 2014/10/22 02:01:02, boliu wrote: > > > > I didn't find this DOMUtils method?! I assume it needs to happen on UI > > thread > > > as > > > > well. > > > > > > To avoid conflicts, I have set the diffbase of this change against: > > > https://codereview.chromium.org/639413002/ > > > > > > So crcr/639413002 needs to be landed first. That method is defined there. > > > > Ok. isFullscreen itself looks thread safe. mContentViewCore.getWebContents() > is > > not!! > > Ok, I'm calling mContentViewCore.getWebContents() on the UI thread now. > > Note that we cannot call DOMUtils.blah on the UI thread because > JavaScriptUtils.executeJavaScriptAndWaitForResult must be called on the > instrumentation test. Why? I don't see it just glancing at the code. ThreadUtils.runOnUiThread calls the Runnable immediately if you are on the UI thread already, and I think everything else should just work? And maybe we should fix that in a follow up if it doesn't work. https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:397: DOMUtils.clickNode(this, mContentViewCore, CUSTOM_FULLSCREEN_CONTROL_ID); On 2014/10/27 11:42:06, Ignacio Solla wrote: > On 2014/10/22 15:53:45, boliu wrote: > > On 2014/10/22 09:32:16, Ignacio Solla wrote: > > > On 2014/10/22 02:01:02, boliu wrote: > > > > UI thread. > > > > > > We're already dispatching the click event on the UI thread, see > > > TouchCommon.dispatchTouchEvent that clickNode ends up calling. > > > > > > Do we also need to retrieve the position of the node in the UI thread? I'm > > happy > > > to change it but I'd like to understand the reason first. > > > > Ahh crap I didn't read that far. I'm pretty sure that findViewById call in > > TouchCommon ain't thread safe either... > > > > But even before that, you can't use ContentViewCore in DOMUtils on the > > instrumentation thread. > > clickNode cannot be called on the instrumentation thread because > executeJavaScriptAndWaitForResult will block otherwise. You can also see that > all other tests in the codebase call DOMUtils.clickNode on the instrumentation > thread. Fixing the problems that you pointed out with DOMUtils.clickNode are > outside the scope of this patch (but don't be afraid to fix it if you want, but > understand that I don't feel that dumping this on me is reasonable. FYI, I have > noticed and fixed many issues with fullscreen / video playback so in general I'm > not afraid of doing that, but do understand that sometimes I also like to do > more interesting work). This change has to wait for that to be fixed then. https://codereview.chromium.org/667143002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/667143002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:72: // TODO(igsolla): fix http://crbug/425926 and replace with assert. Uber nitty nit: Capitalize "f" in fix. Comments in chromium are supposed to be full sentences :p https://codereview.chromium.org/667143002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:355: } catch (Exception e) { nit: We generally add "throws" in test code instead of using try/catch. Although I have no idea what style guide says...
Filed TouchCommon issue at crbug.com/427536 Man that class is old. Basically no one touched it since it was upstreamed.
https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:342: return DOMUtils.isFullscreen(mContentViewCore.getWebContents()); On 2014/10/27 16:49:44, boliu wrote: > On 2014/10/27 11:42:06, Ignacio Solla wrote: > > On 2014/10/22 15:53:45, boliu wrote: > > > On 2014/10/22 09:32:16, Ignacio Solla wrote: > > > > On 2014/10/22 02:01:02, boliu wrote: > > > > > I didn't find this DOMUtils method?! I assume it needs to happen on UI > > > thread > > > > as > > > > > well. > > > > > > > > To avoid conflicts, I have set the diffbase of this change against: > > > > https://codereview.chromium.org/639413002/ > > > > > > > > So crcr/639413002 needs to be landed first. That method is defined there. > > > > > > Ok. isFullscreen itself looks thread safe. mContentViewCore.getWebContents() > > is > > > not!! > > > > Ok, I'm calling mContentViewCore.getWebContents() on the UI thread now. > > > > Note that we cannot call DOMUtils.blah on the UI thread because > > JavaScriptUtils.executeJavaScriptAndWaitForResult must be called on the > > instrumentation test. > > Why? I don't see it just glancing at the code. ThreadUtils.runOnUiThread calls > the Runnable immediately if you are on the UI thread already, and I think > everything else should just work? > > And maybe we should fix that in a follow up if it doesn't work. See: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... The comment reads: "Calling this from the UI thread causes it to time-out: the UI thread being blocked won't have a chance to process the JavaScript eval response)." https://codereview.chromium.org/667143002/diff/1/android_webview/javatests/sr... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:397: DOMUtils.clickNode(this, mContentViewCore, CUSTOM_FULLSCREEN_CONTROL_ID); On 2014/10/27 16:49:44, boliu wrote: > On 2014/10/27 11:42:06, Ignacio Solla wrote: > > On 2014/10/22 15:53:45, boliu wrote: > > > On 2014/10/22 09:32:16, Ignacio Solla wrote: > > > > On 2014/10/22 02:01:02, boliu wrote: > > > > > UI thread. > > > > > > > > We're already dispatching the click event on the UI thread, see > > > > TouchCommon.dispatchTouchEvent that clickNode ends up calling. > > > > > > > > Do we also need to retrieve the position of the node in the UI thread? I'm > > > happy > > > > to change it but I'd like to understand the reason first. > > > > > > Ahh crap I didn't read that far. I'm pretty sure that findViewById call in > > > TouchCommon ain't thread safe either... > > > > > > But even before that, you can't use ContentViewCore in DOMUtils on the > > > instrumentation thread. > > > > clickNode cannot be called on the instrumentation thread because > > executeJavaScriptAndWaitForResult will block otherwise. You can also see that > > all other tests in the codebase call DOMUtils.clickNode on the instrumentation > > thread. Fixing the problems that you pointed out with DOMUtils.clickNode are > > outside the scope of this patch (but don't be afraid to fix it if you want, > but > > understand that I don't feel that dumping this on me is reasonable. FYI, I > have > > noticed and fixed many issues with fullscreen / video playback so in general > I'm > > not afraid of doing that, but do understand that sometimes I also like to do > > more interesting work). > > This change has to wait for that to be fixed then. eh, this change is not changing this line of code
lgtm % nits
https://codereview.chromium.org/667143002/diff/40001/android_webview/javatest... File android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java (right): https://codereview.chromium.org/667143002/diff/40001/android_webview/javatest... android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientFullScreenTest.java:355: } catch (Exception e) { On 2014/10/27 16:49:44, boliu wrote: > nit: We generally add "throws" in test code instead of using try/catch. Although > I have no idea what style guide says... Criteria.isSatisfied does not throw, so our subclass cannot throw here.
https://codereview.chromium.org/667143002/diff/40001/android_webview/java/src... File android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java (right): https://codereview.chromium.org/667143002/diff/40001/android_webview/java/src... android_webview/java/src/org/chromium/android_webview/AwContentViewClient.java:72: // TODO(igsolla): fix http://crbug/425926 and replace with assert. On 2014/10/27 16:49:44, boliu wrote: > Uber nitty nit: Capitalize "f" in fix. Comments in chromium are supposed to be > full sentences :p Done.
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667143002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73999) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2014/10/27 17:27:25, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_gpu on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73999) > android_arm64_dbg_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) > android_chromium_gn_compile_rel on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) > android_dbg_tests_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > linux_chromium_compile_dbg_32 on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > win8_chromium_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) > win_chromium_compile_dbg on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > win_chromium_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) > win_chromium_x64_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) https://codereview.chromium.org/639413002/ needs to land first.
On 2014/10/27 17:27:25, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > mac_gpu on tryserver.chromium.gpu > (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73999) > android_arm64_dbg_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) > android_chromium_gn_compile_rel on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) > android_dbg_tests_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) > chromium_presubmit on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > linux_chromium_compile_dbg_32 on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) > win8_chromium_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) > win_chromium_compile_dbg on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) > win_chromium_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) > win_chromium_x64_rel on tryserver.chromium.win > (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) https://codereview.chromium.org/639413002/ needs to land first.
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667143002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667143002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
On 2014/11/03 11:18:07, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > android_dbg_tests_recipe on tryserver.chromium.linux > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) I had run the tests locally on a Nexus 5 and they passed. The bot runs them on a Nexus 4 and they fail there due to a deadlock. This is also reproducible locally. I have filed crbug.com/429642 to investigate.
On 2014/11/03 12:58:54, Ignacio Solla wrote: > On 2014/11/03 11:18:07, I haz the power (commit-bot) wrote: > > Try jobs failed on following builders: > > android_dbg_tests_recipe on tryserver.chromium.linux > > > (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...) > > I had run the tests locally on a Nexus 5 and they passed. The bot runs them on a > Nexus 4 and they fail there due to a deadlock. This is also reproducible > locally. I have filed crbug.com/429642 to investigate. Thanks Bo. PTAL. The tests failures were genuine, WebView apps could deadlock when entering fullscreen in software mode. The fix is simple, propagate the layer type from the WebView to the FullscreenView.
https://codereview.chromium.org/667143002/diff/100001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwContents.java (right): https://codereview.chromium.org/667143002/diff/100001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwContents.java:666: fullScreenView.setLayerType(mContainerView.getLayerType(), null); Let's do this in test classes somewhere. Say the base TestAwContentsClient. Definitely not the right thing to do in production code. It's just one of those quirks people needs to be aware of when writing instrumentation tests :(
On 2014/11/04 16:14:41, boliu wrote: > https://codereview.chromium.org/667143002/diff/100001/android_webview/java/sr... > File android_webview/java/src/org/chromium/android_webview/AwContents.java > (right): > > https://codereview.chromium.org/667143002/diff/100001/android_webview/java/sr... > android_webview/java/src/org/chromium/android_webview/AwContents.java:666: > fullScreenView.setLayerType(mContainerView.getLayerType(), null); > Let's do this in test classes somewhere. Say the base TestAwContentsClient. > Definitely not the right thing to do in production code. > > It's just one of those quirks people needs to be aware of when writing > instrumentation tests :( Thanks Bo. Just for the record, after discussing offline it is now clear to me that this is a limitation with the testing code, and in particular with the hardware mode emulation. As discussed, I have made the annotation now apply to the window. Let me know if this is what you had in mind. Thanks!
https://codereview.chromium.org/667143002/diff/120001/android_webview/test/sh... File android_webview/test/shell/AndroidManifest.xml (left): https://codereview.chromium.org/667143002/diff/120001/android_webview/test/sh... android_webview/test/shell/AndroidManifest.xml:14: android:hardwareAccelerated="true"> Hmm...have to be *very careful* here to make sure we didn't just disable hardware mode in all the tests (they would all still pass I imagine) So you * enabled hardware acceleration at application level (it's the default, no change here) * disabled it at activity level * then try to enable it at window level Honestly, I don't know if that works. I need to run right now, but I can test it later today.
https://codereview.chromium.org/667143002/diff/120001/android_webview/test/sh... File android_webview/test/shell/AndroidManifest.xml (left): https://codereview.chromium.org/667143002/diff/120001/android_webview/test/sh... android_webview/test/shell/AndroidManifest.xml:14: android:hardwareAccelerated="true"> On 2014/11/04 18:30:24, boliu wrote: > Hmm...have to be *very careful* here to make sure we didn't just disable > hardware mode in all the tests (they would all still pass I imagine) > > So you > > * enabled hardware acceleration at application level (it's the default, no > change here) > * disabled it at activity level > * then try to enable it at window level > > Honestly, I don't know if that works. I need to run right now, but I can test it > later today. Yeah this turned off hardware acceleration everywhere. You can use this test I just wrote to test it: https://codereview.chromium.org/687803009/ So now I'm confused what Window.setFlags actually does. Looks like it does nothing if the activity has hardwareAccelerated=false. It should also do nothing if activity is already hardware accelerated. So a no-op call?! Anyway, I'm ok if you just move the change in in PS6 to a test class.
https://codereview.chromium.org/667143002/diff/120001/android_webview/test/sh... File android_webview/test/shell/AndroidManifest.xml (left): https://codereview.chromium.org/667143002/diff/120001/android_webview/test/sh... android_webview/test/shell/AndroidManifest.xml:14: android:hardwareAccelerated="true"> On 2014/11/04 18:30:24, boliu wrote: > Hmm...have to be *very careful* here to make sure we didn't just disable > hardware mode in all the tests (they would all still pass I imagine) > > So you > > * enabled hardware acceleration at application level (it's the default, no > change here) > * disabled it at activity level > * then try to enable it at window level > > Honestly, I don't know if that works. I need to run right now, but I can test it > later today. Acknowledged. https://codereview.chromium.org/667143002/diff/120001/android_webview/test/sh... android_webview/test/shell/AndroidManifest.xml:14: android:hardwareAccelerated="true"> On 2014/11/04 21:53:26, boliu wrote: > On 2014/11/04 18:30:24, boliu wrote: > > Hmm...have to be *very careful* here to make sure we didn't just disable > > hardware mode in all the tests (they would all still pass I imagine) > > > > So you > > > > * enabled hardware acceleration at application level (it's the default, no > > change here) > > * disabled it at activity level > > * then try to enable it at window level > > > > Honestly, I don't know if that works. I need to run right now, but I can test > it > > later today. > > Yeah this turned off hardware acceleration everywhere. You can use this test I > just wrote to test it: https://codereview.chromium.org/687803009/ > > So now I'm confused what Window.setFlags actually does. Looks like it does > nothing if the activity has hardwareAccelerated=false. It should also do nothing > if activity is already hardware accelerated. So a no-op call?! > > Anyway, I'm ok if you just move the change in in PS6 to a test class. I have moved the change in PS6 to the testing code.
lgtm
The CQ bit was checked by igsolla@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/667143002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/06d14e5623a8e879f7e6d56c84650c0f524e2c5b Cr-Commit-Position: refs/heads/master@{#302816} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
