Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
171233002:
Cast icon should show in sync with media controls (Closed)
|
Created:
6 years, 10 months ago by cimamoglu (inactive) Modified:
6 years, 10 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, Miguel Garcia Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCast icon should show in sync with media controls
Created a listener in ContentVideoViewLegacy that interested classes can
use to listen for changes in the media controls visibility. Also changed
the touch listener to listen to the whole screen instead of just the
surface view for toggling the media controls visibility.
The corresponding downstream CL:
https://chrome-internal-review.googlesource.com/#/c/155197/
The original CL by maybelle@:
https://codereview.chromium.org/164273002/
BUG=341498
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252195
Patch Set 1 #
Total comments: 13
Patch Set 2 : Address comments #Messages
Total messages: 14 (0 generated)
LGTM with nits. Hopefully the same approach will work with the new fullscreen video controls implementation. https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:48: MediaControlsVisibilityListener listener) { nit: use one line per parameter? nit: document the parameters https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:78: public void setListener(MediaControlsVisibilityListener listener) { Is this method needed? https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:143: setOnTouchListener(new OnTouchListener() { why this change?
+bulach@ for OWNERS
https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:143: setOnTouchListener(new OnTouchListener() { I have the same question here, why this? the current behavior is inherited from the android browser. though both behavior seems fine to me
lgtm % nits (and provided others are happy too!) https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:28: private MediaControlsVisibilityListener mListener; nit: this member seem unused.. https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:45: MediaControlsVisibilityListener mListener; nit: slightly unrelated, but looks like mVideoView can be final.. (and if you remove the setListener as anton suggested, make mListener final too) https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:302: mListener = listener; looks like mListener is unused.. if it's not, it should :) it's confusing to have to have such listeners in two different layers.
https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java (right): https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:28: private MediaControlsVisibilityListener mListener; On 2014/02/19 18:41:45, bulach wrote: > nit: this member seem unused.. It is not unused, it's used to create FullScreenMediaController, inside openVideo(). https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:45: MediaControlsVisibilityListener mListener; On 2014/02/19 18:41:45, bulach wrote: > nit: slightly unrelated, but looks like mVideoView can be final.. > (and if you remove the setListener as anton suggested, make mListener final too) Done. https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:48: MediaControlsVisibilityListener listener) { On 2014/02/18 20:41:23, whywhat wrote: > nit: use one line per parameter? > nit: document the parameters Done. https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:78: public void setListener(MediaControlsVisibilityListener listener) { On 2014/02/18 20:41:23, whywhat wrote: > Is this method needed? I had it to really make sure we FullScreenMediaController has the up-to-date listener, but it seems like this is unnecessary. Deleted. https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:143: setOnTouchListener(new OnTouchListener() { On 2014/02/19 15:46:43, qinmin wrote: > I have the same question here, why this? the current behavior is inherited from > the android browser. though both behavior seems fine to me I believe it is better to be able to show/hide controls (and the media route button) through touching the whole screen, this change makes it possible. Currently, media controls can only be shown/hidden by clicking onto the video, and media route button is shown/hidden with the whole screen. https://codereview.chromium.org/171233002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentVideoViewLegacy.java:302: mListener = listener; On 2014/02/19 18:41:45, bulach wrote: > looks like mListener is unused.. if it's not, it should :) > it's confusing to have to have such listeners in two different layers. It is not unused, it's used to create FullScreenMediaController above, inside openVideo(). It's kind of confusing as you say, but it's necessary.
The CQ bit was checked by cimamoglu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/171233002/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel
Bot error seems irrelevant, put NOTRY and resubmit
The CQ bit was checked by cimamoglu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/cimamoglu@chromium.org/171233002/90001
Message was sent while issue was closed.
Change committed as 252195 |
