Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2898553002:
[Media,Remoting] Remove the disable remoting button on Android (Closed)
|
Created:
3 years, 7 months ago by whywhat Modified:
3 years, 6 months ago CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, rwlbuis, nessy, Srirama Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Media,Remoting] Remove the disable remoting button on Android
The --new-remote-playback-pipeline flag will stay disabled on desktop by default.
We want to reuse the media remoting interstitial with the new remote playback pipeline instead of the old fake video frame with text generated using GL.
The interstitial currently has a button on it that says "Play on both devices" that turns remoting off. This CL allows the button to be removed.
BUG=517102
TEST=initiate remoting with --new-remote-playback-pipeline and check the button is not there
Review-Url: https://codereview.chromium.org/2898553002
Cr-Commit-Position: refs/heads/master@{#475363}
Committed: https://chromium.googlesource.com/chromium/src/+/cd64baef7c36f9763e0d152ca3a3723077b80a7f
Patch Set 1 #Patch Set 2 : Don't show the exit button on Android only for now #Patch Set 3 : Use Android CSS to disable the button #
Total comments: 4
Patch Set 4 : Removed the style check for event listeners #Messages
Total messages: 41 (18 generated)
avayvod@chromium.org changed reviewers: + mlamouri@chromium.org, xjz@chromium.org
PTaL
The CQ bit was checked by avayvod@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: 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_...)
Can you please hold off this change? We are not quite ready yet. We need to figure out where to move this button first. :)
On 2017/05/22 at 20:49:41, xjz wrote: > Can you please hold off this change? We are not quite ready yet. We need to figure out where to move this button first. :) Hm, I could either make it conditional so it's not shown on Android or make it work on Android (right now it doesn't update the remoting state if the video element as it doesn't stop flinging). WDYT?
On 2017/05/22 21:35:57, whywhat wrote: > On 2017/05/22 at 20:49:41, xjz wrote: > > Can you please hold off this change? We are not quite ready yet. We need to > figure out where to move this button first. :) > > Hm, I could either make it conditional so it's not shown on Android or make it > work on Android (right now it doesn't update the remoting state if the video > element as it doesn't stop flinging). WDYT? Making it conditional for now sgtm.
Don't show the exit button on Android only for now
The CQ bit was checked by avayvod@chromium.org to run a CQ dry run
PTAL I didn't want to introduce a separate runtime feature flag and it seems that noone uses OS_ANDROID in WebKit.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Media,Remoting] Remove the disable remoting button. BUG=517102 TEST=initiate remoting and check the button is not there ========== to ========== [Media,Remoting] Remove the disable remoting button on Android The --new-remote-playback-pipeline flag will stay disabled on desktop by default. BUG=517102 TEST=initiate remoting with --new-remote-playback-pipeline and check the button is not there ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
avayvod@, I am not sure what this is trying to do: can you explain in the CL description why this button is being removed and also how this is related to the new pipeline?
Description was changed from ========== [Media,Remoting] Remove the disable remoting button on Android The --new-remote-playback-pipeline flag will stay disabled on desktop by default. BUG=517102 TEST=initiate remoting with --new-remote-playback-pipeline and check the button is not there ========== to ========== [Media,Remoting] Remove the disable remoting button on Android The --new-remote-playback-pipeline flag will stay disabled on desktop by default. We want to reuse the media remoting interstitial with the new remote playback pipeline instead of the old fake video frame with text generated using GL. The interstitial currently has a button on it that says "Play on both devices" that turns remoting off. This CL allows the button to be removed. BUG=517102 TEST=initiate remoting with --new-remote-playback-pipeline and check the button is not there ==========
On 2017/05/24 at 16:42:05, mlamouri wrote: > avayvod@, I am not sure what this is trying to do: can you explain in the CL description why this button is being removed and also how this is related to the new pipeline? Done. To think of it, I should probably rather have a bool that RemotePlayback would set when causing the interstitial to be shown. However, there's a discussion about removing the button on all platforms so I didn't want to overengineer it :)
lgtm
Use Android CSS to disable the button
avayvod@chromium.org changed reviewers: + rune@opera.com
rune@, PTaL at mediacontrolsandroid.css, thanks!
https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp (right): https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:68: if (style->Display() == EDisplay::kNone) Have you tested on this? I am not sure whether this works. IIUC, the showing/hiding of the button is decided by the previous RemoveInlineStyleProperty()/SetInlineStyleProperty() call: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/shad... OnShown()/OnHidden() here are not to show/hide the button, but to register/de-register the event handler.
https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp (right): https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:68: if (style->Display() == EDisplay::kNone) On 2017/05/25 at 19:02:18, xjz wrote: > Have you tested on this? I am not sure whether this works. IIUC, the showing/hiding of the button is decided by the previous RemoveInlineStyleProperty()/SetInlineStyleProperty() call: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/shad... The hiding of the button happens via the mediacontrolsandroid.css that affects the button only. The one you pointed to affects the whole interstitial, AFAIK. > > OnShown()/OnHidden() here are not to show/hide the button, but to register/de-register the event handler. Yes, my intention here is not to register/unregister the event handler in case the button is not visible. The names became a bit confusing though. It now mean more like "OnInterstitialShown" rather than "OnExitButtonShown" :) Mounir suggested that the event handlers won't get called for display:none elements anyway, but still, registering them felt like a waste.
https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp (right): https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:68: if (style->Display() == EDisplay::kNone) On 2017/05/25 20:26:29, whywhat wrote: > On 2017/05/25 at 19:02:18, xjz wrote: > > Have you tested on this? I am not sure whether this works. IIUC, the > showing/hiding of the button is decided by the previous > RemoveInlineStyleProperty()/SetInlineStyleProperty() call: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/shad... > > The hiding of the button happens via the mediacontrolsandroid.css that affects > the button only. The one you pointed to affects the whole interstitial, AFAIK. > > > > > OnShown()/OnHidden() here are not to show/hide the button, but to > register/de-register the event handler. > > Yes, my intention here is not to register/unregister the event handler in case > the button is not visible. > The names became a bit confusing though. It now mean more like > "OnInterstitialShown" rather than "OnExitButtonShown" :) > > Mounir suggested that the event handlers won't get called for display:none > elements anyway, but still, registering them felt like a waste. Yes, you are correct. When showing/hiding the interstitial, only the display property of the whole interstitial (MediaRemotingInterstitial) is changed, not the separate elements. I still feel a little confusing to do the check here. Maybe we can do the check in MediaRemotingInterstitial::Show()/Hide(). If the Display property of the |exit_button_| is kNone (indicating that the button is not visible), don't call this OnShown()/OnHidden(). WDYT?
avayvod@chromium.org changed reviewers: + chrishtr@chromium.org - rune@opera.com
-rune +chrishtr for mediacontrolsandroid.css PTaL
rune@opera.com changed reviewers: + rune@opera.com
https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp (right): https://codereview.chromium.org/2898553002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingElements.cpp:68: if (style->Display() == EDisplay::kNone) On 2017/05/25 19:02:18, xjz wrote: > Have you tested on this? I am not sure whether this works. IIUC, the > showing/hiding of the button is decided by the previous > RemoveInlineStyleProperty()/SetInlineStyleProperty() call: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/shad... > > OnShown()/OnHidden() here are not to show/hide the button, but to > register/de-register the event handler. Querying the computed style requires that the computed style is up-to-date. I don't think it is at this point because RemoveInlineStyleProperty() will cause the computed style to be updated asynchronously. It could be an idea to add/remove the event listener on MediaRemotingExitButtonElement::AttachLayoutTree/DetachLayoutTree instead.
I wouldn't mind having the events there all the time as it would be a no-op when the element is `display:none`. I have no strong opinions and if xjz@ or rune@ have different preferences, it's still lgtm :)
Removed the style check for event listeners
I'm all for simplification. rune@ PTAL, thanks!
Having the event listeners lgtm too. :)
rs lgtm
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2898553002/#ps60001 (title: "Removed the style check for event listeners")
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": 60001, "attempt_start_ts": 1496075242638320,
"parent_rev": "1a1cb7b513eb2ed111b66e47989dc5b515c912ed", "commit_rev":
"cd64baef7c36f9763e0d152ca3a3723077b80a7f"}
Message was sent while issue was closed.
Description was changed from ========== [Media,Remoting] Remove the disable remoting button on Android The --new-remote-playback-pipeline flag will stay disabled on desktop by default. We want to reuse the media remoting interstitial with the new remote playback pipeline instead of the old fake video frame with text generated using GL. The interstitial currently has a button on it that says "Play on both devices" that turns remoting off. This CL allows the button to be removed. BUG=517102 TEST=initiate remoting with --new-remote-playback-pipeline and check the button is not there ========== to ========== [Media,Remoting] Remove the disable remoting button on Android The --new-remote-playback-pipeline flag will stay disabled on desktop by default. We want to reuse the media remoting interstitial with the new remote playback pipeline instead of the old fake video frame with text generated using GL. The interstitial currently has a button on it that says "Play on both devices" that turns remoting off. This CL allows the button to be removed. BUG=517102 TEST=initiate remoting with --new-remote-playback-pipeline and check the button is not there Review-Url: https://codereview.chromium.org/2898553002 Cr-Commit-Position: refs/heads/master@{#475363} Committed: https://chromium.googlesource.com/chromium/src/+/cd64baef7c36f9763e0d152ca3a3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/cd64baef7c36f9763e0d152ca3a3... |
