Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2124003004:
Force to exit full screen video upon receiving ESCAPE key event (Closed)
DescriptionForce to exit full screen video upon receiving ESCAPE key event
Currently ESCAPE key event is not handled correctly while playing
full screen video. Now exiting full screen video when receives
ESCAPE key event to match desktop behavior.
BUG=614185
Committed: https://crrev.com/a2f149f568171394c570a64d2a3d48d379ad1fd7
Cr-Commit-Position: refs/heads/master@{#407151}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Moved the ESCAPE key handling to KeyboardShortcuts #
Total comments: 2
Patch Set 3 : Kept protected scope of exitFullscreenIfShowing() API #Messages
Total messages: 15 (4 generated)
ajith.v@chromium.org changed reviewers: + tedchoc@chromium.org, yfriedman@chromium.org
PTAL!
https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1760: exitFullscreenIfShowing(); should we return true in this case?
Please check my query. https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1760: exitFullscreenIfShowing(); On 2016/07/18 21:58:09, Ted C wrote: > should we return true in this case? I also felt like that, it was just missed to address. I have another plan to handle it in ContentVideoView directly instead of handling from here. What's your opinion on it ? (Handling from targetView instead of handling from Activity)
https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1760: exitFullscreenIfShowing(); On 2016/07/20 13:38:09, AKV wrote: > On 2016/07/18 21:58:09, Ted C wrote: > > should we return true in this case? > > I also felt like that, it was just missed to address. > I have another plan to handle it in ContentVideoView directly instead of > handling from here. What's your opinion on it ? (Handling from targetView > instead of handling from Activity) I thought that initially myself as well, but that wouldn't handle the HTML5 fullscreen usecase where this option does. Test page: https://davidwalsh.name/demo/fullscreen.php But looking at this a bit more, I think we should move this handling into KeyboardShortcuts.java. That seems consistent with other keyboard handling.
PTAL! https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2124003004/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1760: exitFullscreenIfShowing(); On 2016/07/20 17:29:13, Ted C wrote: > On 2016/07/20 13:38:09, AKV wrote: > > On 2016/07/18 21:58:09, Ted C wrote: > > > should we return true in this case? > > > > I also felt like that, it was just missed to address. > > I have another plan to handle it in ContentVideoView directly instead of > > handling from here. What's your opinion on it ? (Handling from targetView > > instead of handling from Activity) > > I thought that initially myself as well, but that wouldn't handle the HTML5 > fullscreen usecase where this option does. > > Test page: > https://davidwalsh.name/demo/fullscreen.php > > But looking at this a bit more, I think we should move this handling into > KeyboardShortcuts.java. That seems consistent with other keyboard handling. Thank you Ted for the suggestion. I have moved the changes to KeyboardShortcuts.java Also I verified the issue on HTML5 Full screen URL https://davidwalsh.name/demo/fullscreen.php as well.
PTAL!
lgtm https://codereview.chromium.org/2124003004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (left): https://codereview.chromium.org/2124003004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1264: protected boolean exitFullscreenIfShowing() { protected is fine since we are in the same package
https://codereview.chromium.org/2124003004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (left): https://codereview.chromium.org/2124003004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1264: protected boolean exitFullscreenIfShowing() { On 2016/07/21 17:25:16, Ted C wrote: > protected is fine since we are in the same package Done. Thank you!
The CQ bit was checked by ajith.v@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2124003004/#ps40001 (title: "Kept protected scope of exitFullscreenIfShowing() API")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Force to exit full screen video upon receiving ESCAPE key event Currently ESCAPE key event is not handled correctly while playing full screen video. Now exiting full screen video when receives ESCAPE key event to match desktop behavior. BUG=614185 ========== to ========== Force to exit full screen video upon receiving ESCAPE key event Currently ESCAPE key event is not handled correctly while playing full screen video. Now exiting full screen video when receives ESCAPE key event to match desktop behavior. BUG=614185 Committed: https://crrev.com/a2f149f568171394c570a64d2a3d48d379ad1fd7 Cr-Commit-Position: refs/heads/master@{#407151} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a2f149f568171394c570a64d2a3d48d379ad1fd7 Cr-Commit-Position: refs/heads/master@{#407151} |
