Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2825493005:
Enable smooth transition when show/hide media remoting interstitial. (Closed)
|
Created:
3 years, 8 months ago by xjz Modified:
3 years, 7 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. |
DescriptionEnable smooth transition when show/hide media remoting interstitial.
BUG=712479
Review-Url: https://codereview.chromium.org/2825493005
Cr-Commit-Position: refs/heads/master@{#465712}
Committed: https://chromium.googlesource.com/chromium/src/+/82722b0100d7e57d75b52fb1be9cb742f5cb91ba
Patch Set 1 #Patch Set 2 : Rebase only. #
Total comments: 8
Patch Set 3 : Addressed comments. #
Total comments: 2
Patch Set 4 : Fix nits. #
Total comments: 2
Messages
Total messages: 28 (18 generated)
The CQ bit was checked by xjz@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...
Description was changed from ========== Enable smooth transition when show/hide media remoting interstitial. BUG=712479 ========== to ========== Enable smooth transition when show/hide media remoting interstitial. BUG=712479 ==========
xjz@chromium.org changed reviewers: + kinuko@chromium.org, mlamouri@chromium.org
kinuko: RS on third_party/WebKit/Source/core/css/mediaControls.css please. Thanks! mlamouri: As explained in the crbug page, transition doesn't work if display property and opacity property change together. So a timer is introduced to allow one change stable before the second change. PTAL. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xjz@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...
rs/lgtm for css change, one nit comment for others https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h (right): https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:47: void ToggleInterstitialTimerFired(TimerBase*); If this one's not overrides please don't interleave.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp (right): https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:49: if (is_shown_ && toggle_insterstitial_timer_.IsActive()) Why do you check if the timer is active? Actually, do we expect this to be called when the interstitial is showing? https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:77: exit_button_->OnShown(); Why are we not being consistent with onShown and onHidden? https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h (right): https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:50: bool is_shown_; nit: initialise the bool here. Also, add a comment explaining what it actually means. Because we are transitioning, visibility isn't really a boolean so does it include "transitions" or not? Maybe it should be named `is_visible_or_transitioning_`?
Thanks for reviewing! Addressed comments. PTAL again. Thanks! https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp (right): https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:49: if (is_shown_ && toggle_insterstitial_timer_.IsActive()) On 2017/04/19 16:17:19, mlamouri wrote: > Why do you check if the timer is active? Actually, do we expect this to be > called when the interstitial is showing? Done. Yes, the timer could be active. For example, start remoting immediately after stop remoting. But you are right, we should not expect this to be called when |is_shown| is true. Add DCHECK. https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.cpp:77: exit_button_->OnShown(); On 2017/04/19 16:17:19, mlamouri wrote: > Why are we not being consistent with onShown and onHidden? The initial thought was call onHidden() first to stop listening the mouse events when remoting stops. And call OnShown() after the interstial is stably shown. But after a second thought, I agree that it should be OK to make the calls consistent, which is more readable and easier for maintenance. https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h (right): https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:47: void ToggleInterstitialTimerFired(TimerBase*); On 2017/04/19 06:13:43, kinuko wrote: > If this one's not overrides please don't interleave. Done. https://codereview.chromium.org/2825493005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/shadow/MediaRemotingInterstitial.h:50: bool is_shown_; On 2017/04/19 16:17:19, mlamouri wrote: > nit: initialise the bool here. Also, add a comment explaining what it actually > means. Because we are transitioning, visibility isn't really a boolean so does > it include "transitions" or not? > > Maybe it should be named `is_visible_or_transitioning_`? Done. Added comments. This boolean indicates whether we should show the interstitial. It could be in transition or after.
The CQ bit was checked by xjz@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...
lgtm https://codereview.chromium.org/2825493005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2825493005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:511: DCHECK_EQ(media_remoting_status_, MediaRemotingStatus::kStarted); nit: swap the two parameters -- Also, I can't remember if MediaRemotingStatus is an enum class but if that's the case, some compilers might not accept this and you will have to do DCHECK(foo == bar)
Thanks so much for the quick reviewing! https://codereview.chromium.org/2825493005/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2825493005/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:511: DCHECK_EQ(media_remoting_status_, MediaRemotingStatus::kStarted); On 2017/04/19 17:58:46, mlamouri wrote: > nit: swap the two parameters -- Also, I can't remember if MediaRemotingStatus is > an enum class but if that's the case, some compilers might not accept this and > you will have to do DCHECK(foo == bar) Done. Yes, MediaRemotingStatus is an enum class. Changed to DCHECK(foo == bar) now.
The CQ bit was checked by xjz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/2825493005/#ps60001 (title: "Fix nits.")
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": 1492625312468640,
"parent_rev": "aa7abc610aa948bad8abfb0feff94c9a13a6c903", "commit_rev":
"4449124a3f83aad54eef3c860381fcb6265c7861"}
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492625312468640,
"parent_rev": "ae43980829da6a1c385bed0be8fd4d2969b30795", "commit_rev":
"82722b0100d7e57d75b52fb1be9cb742f5cb91ba"}
Message was sent while issue was closed.
Description was changed from ========== Enable smooth transition when show/hide media remoting interstitial. BUG=712479 ========== to ========== Enable smooth transition when show/hide media remoting interstitial. BUG=712479 Review-Url: https://codereview.chromium.org/2825493005 Cr-Commit-Position: refs/heads/master@{#465712} Committed: https://chromium.googlesource.com/chromium/src/+/82722b0100d7e57d75b52fb1be9c... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/82722b0100d7e57d75b52fb1be9c...
Message was sent while issue was closed.
avayvod@chromium.org changed reviewers: + avayvod@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2825493005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2825493005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:521: MediaRemotingStopped(); This line seems to be either out of order or obsolete as MediaRemotingStopped will now early return due to the added check, won't it?
Message was sent while issue was closed.
https://codereview.chromium.org/2825493005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLVideoElement.cpp (right): https://codereview.chromium.org/2825493005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLVideoElement.cpp:521: MediaRemotingStopped(); On 2017/05/18 16:31:36, whywhat wrote: > This line seems to be either out of order or obsolete as MediaRemotingStopped > will now early return due to the added check, won't it? Yes, you are right. This call does nothing. With a second thought, I think the MediaRemotingStopped() call here and the added early return can be removed. Will upload a quick fix soon. Thanks for catching this! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
