Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2566223005:
Media Remoting: Update remoting interstitial when status changes. (Closed)
|
Created:
4 years ago by xjz Modified:
4 years ago Reviewers:
miu CC:
chromium-reviews, feature-media-reviews_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org, apacible+watch_chromium.org, chromoting-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMedia Remoting: Update remoting interstitial when status changes.
This CL let RemotingRendererController control updating/showing
interstitial. The interstitial will be updated when remoting starts
successfully, or fails, or stops.
BUG=643964
Committed: https://crrev.com/af5c859fb2708358eb042fe62a8e5d5e66b264ae
Cr-Commit-Position: refs/heads/master@{#440355}
Patch Set 1 #Patch Set 2 : Removed unused declare. #
Total comments: 2
Patch Set 3 : Draw interstitial on media thread. #Patch Set 4 : Show unprocessed background image when exiting remoting. #
Total comments: 38
Patch Set 5 : Addressed comments. #
Total comments: 12
Patch Set 6 : Addressed comments. #
Total comments: 2
Patch Set 7 : Addressed comments. #Patch Set 8 : Rebased only. #
Messages
Total messages: 23 (10 generated)
xjz@chromium.org changed reviewers: + miu@chromium.org
PTAL Most codes are originally from: https://codereview.chromium.org/2538853002. Thanks!
https://codereview.chromium.org/2566223005/diff/20001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.cc (right): https://codereview.chromium.org/2566223005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.cc:45: base::Bind(&RemotingInterstitialUI::ShowInterstitialOnSink, As discussed face-to-face, consider: base::Bind(&base::SingleThreadTaskRunner::PostTask, media_task_runner_, FROM_HERE, base::Bind(&RemoteRendererImpl::ShowInterstitialOnSink, weak_factory_.GetWeakPtr(), video_renderer_sink));
Addressed comment. PTAL. https://codereview.chromium.org/2566223005/diff/20001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.cc (right): https://codereview.chromium.org/2566223005/diff/20001/media/remoting/remote_r... media/remoting/remote_renderer_impl.cc:45: base::Bind(&RemotingInterstitialUI::ShowInterstitialOnSink, On 2016/12/13 22:39:09, miu wrote: > As discussed face-to-face, consider: > > base::Bind(&base::SingleThreadTaskRunner::PostTask, media_task_runner_, > FROM_HERE, > base::Bind(&RemoteRendererImpl::ShowInterstitialOnSink, > weak_factory_.GetWeakPtr(), video_renderer_sink)); Done. Use static function instead, as this doesn't work.
Comments on PS4: https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.cc (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remote_r... media/remoting/remote_renderer_impl.cc:56: // Post task on main thread to unregister message receiver. Above, this, you should add a main thread task to "unset" the callback: main_task_runner_->PostTask( FROM_HERE, base::Bind(&RemotingRendererController::SetShowInterstitialCallback, remoting_renderer_controller_, RemotingRendererController::ShowInterstitialCallback())); https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:33: gfx::Size scaled_size = ScaleSizeToFitWithinTarget(image_size, canvas_size); Don't need this (see later comment). https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:42: void DrawInterstitial(SkCanvas& canvas, style: Pass by pointer, not by reference. (and it should be the 3rd argument, since it's an in-out argument) https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:44: bool is_remoting_successful) { nit: Instead of the boolean, just pass the "type" enum. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:99: scoped_refptr<VideoFrame> GetInterstitial(const SkBitmap& image, naming nit: How about RenderInterstitialFrame()? https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:102: SkBitmap black_background; naming nit: This isn't "black background" by the end of this function. So, how about "canvas_bitmap" or "result_bitmap"? https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:110: background_image = ResizeImage(image, canvas_size); Looks like you should call ComputeLetterboxRegion() first, then pass centered_rect.size() as the 2nd argument to ResizeImage() here. Then, in ResizeImage(), you don't need to call ScaleSizeToFitWithinTarget(). https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:120: const gfx::Size image_size = Given the suggestions above, this and the following 9 LOC can just become: canvas.writePixels(processed_image, centered_rect.x(), centered_rect.y()); https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:133: DrawInterstitial(canvas, canvas_size, naming: Instead of DrawInterstitial(), how about RenderCastMessage()? https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:152: void ShowRemotingInterstitial(VideoRendererSink* video_renderer_sink, naming nit: "Show" infers the interstitial can also be hidden, but that's not how this stuff works. How about PaintRemotingInterstitial()? https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:9: #include "ui/gfx/geometry/size.h" nit: Since gfx::Size only appears as a const-ref in this header file, just forward-declare it instead: namespace gfx { class Size; } https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:15: enum RemotingInterstitialType { naming: Let's name these so that they describe which "phase" of UX we're in. Suggestions: NONE --> BETWEEN_SESSIONS (i.e., before or after a session) SUCCESS --> IN_SESSION FAIL --> ENCRYPTED_MEDIA_FATAL_ERROR As for the 3rd one: When do we show that? I think we had intended it for when encrypted media fails to remote. The controller doesn't seem to set the interstitial appropriately when that happens. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:27: void ShowRemotingInterstitial(VideoRendererSink* video_renderer_sink, style nit: Please place the video_renderer_sink "output" argument last (order of args is always inputs, then in-outs, then outputs). https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:51: } Looks like you should call UpdateInterstitial() here (at the end of this method). https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:57: if (remoting_source_->state() == How about just calling UpdateInterstitial() unconditionally (i.e., for all state changes), and let the code in UpdateInterstitial() decide whether anything actually changed before running the callback? https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:262: show_interstitial_cb_.Run(SkBitmap(), pipeline_metadata_.natural_size, Hmm...It looks like you don't need this here (if you follow suggestions above about just always calling UpdateInterstitial() on state changes). If you're concerned about the very last frame painted before the RemoteRendererImpl is destroyed, the RemoteRendererImpl should probably paint a black frame from it's destructor. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:264: show_interstitial_cb_.Reset(); Why do we reset the callback? What if we start remoting again, and then there's no callback? Should be safe to just leave it. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:274: DCHECK(!cb.is_null()); As mentioned above, let RemotingRendererImpl set a null callback to "unset" it. This allows the "view" to explicitly disconnect from the "controller." https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.h:59: void UpdateInterstitial(); Seems like this should be a private method.
Patchset #5 (id:80001) has been deleted
Addressed comments. PTAL. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remote_r... File media/remoting/remote_renderer_impl.cc (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remote_r... media/remoting/remote_renderer_impl.cc:56: // Post task on main thread to unregister message receiver. On 2016/12/20 00:16:16, miu wrote: > Above, this, you should add a main thread task to "unset" the callback: > > main_task_runner_->PostTask( > FROM_HERE, > base::Bind(&RemotingRendererController::SetShowInterstitialCallback, > remoting_renderer_controller_, > RemotingRendererController::ShowInterstitialCallback())); Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:33: gfx::Size scaled_size = ScaleSizeToFitWithinTarget(image_size, canvas_size); On 2016/12/20 00:16:17, miu wrote: > Don't need this (see later comment). Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:42: void DrawInterstitial(SkCanvas& canvas, On 2016/12/20 00:16:17, miu wrote: > style: Pass by pointer, not by reference. (and it should be the 3rd argument, > since it's an in-out argument) Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:44: bool is_remoting_successful) { On 2016/12/20 00:16:16, miu wrote: > nit: Instead of the boolean, just pass the "type" enum. Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:99: scoped_refptr<VideoFrame> GetInterstitial(const SkBitmap& image, On 2016/12/20 00:16:17, miu wrote: > naming nit: How about RenderInterstitialFrame()? Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:102: SkBitmap black_background; On 2016/12/20 00:16:17, miu wrote: > naming nit: This isn't "black background" by the end of this function. So, how > about "canvas_bitmap" or "result_bitmap"? Done. Renamed it as "canvas_bitmap". https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:110: background_image = ResizeImage(image, canvas_size); On 2016/12/20 00:16:16, miu wrote: > Looks like you should call ComputeLetterboxRegion() first, then pass > centered_rect.size() as the 2nd argument to ResizeImage() here. Then, in > ResizeImage(), you don't need to call ScaleSizeToFitWithinTarget(). Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:120: const gfx::Size image_size = On 2016/12/20 00:16:16, miu wrote: > Given the suggestions above, this and the following 9 LOC can just become: > > canvas.writePixels(processed_image, centered_rect.x(), centered_rect.y()); Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:133: DrawInterstitial(canvas, canvas_size, On 2016/12/20 00:16:17, miu wrote: > naming: Instead of DrawInterstitial(), how about RenderCastMessage()? Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.cc:152: void ShowRemotingInterstitial(VideoRendererSink* video_renderer_sink, On 2016/12/20 00:16:17, miu wrote: > naming nit: "Show" infers the interstitial can also be hidden, but that's not > how this stuff works. How about PaintRemotingInterstitial()? Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:9: #include "ui/gfx/geometry/size.h" On 2016/12/20 00:16:17, miu wrote: > nit: Since gfx::Size only appears as a const-ref in this header file, just > forward-declare it instead: > > namespace gfx { > class Size; > } Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:15: enum RemotingInterstitialType { On 2016/12/20 00:16:17, miu wrote: > naming: Let's name these so that they describe which "phase" of UX we're in. > Suggestions: > > NONE --> BETWEEN_SESSIONS (i.e., before or after a session) > SUCCESS --> IN_SESSION > FAIL --> ENCRYPTED_MEDIA_FATAL_ERROR > > As for the 3rd one: When do we show that? I think we had intended it for when > encrypted media fails to remote. The controller doesn't seem to set the > interstitial appropriately when that happens. Done. As chatted face to face, the 3rd one is set when remoting state changes to SESSION_PERMANENTLY_STOPPED. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_interstitial_ui.h:27: void ShowRemotingInterstitial(VideoRendererSink* video_renderer_sink, On 2016/12/20 00:16:17, miu wrote: > style nit: Please place the video_renderer_sink "output" argument last (order of > args is always inputs, then in-outs, then outputs). Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:51: } On 2016/12/20 00:16:17, miu wrote: > Looks like you should call UpdateInterstitial() here (at the end of this > method). As chatted face to face, there is no need to call it here, since it will be called when either RemoteRendererImpl is created or conditions change during remoting session. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:57: if (remoting_source_->state() == On 2016/12/20 00:16:17, miu wrote: > How about just calling UpdateInterstitial() unconditionally (i.e., for all state > changes), and let the code in UpdateInterstitial() decide whether anything > actually changed before running the callback? Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:262: show_interstitial_cb_.Run(SkBitmap(), pipeline_metadata_.natural_size, On 2016/12/20 00:16:17, miu wrote: > Hmm...It looks like you don't need this here (if you follow suggestions above > about just always calling UpdateInterstitial() on state changes). > > If you're concerned about the very last frame painted before the > RemoteRendererImpl is destroyed, the RemoteRendererImpl should probably paint a > black frame from it's destructor. Done. Now let RemoteRendererImpl destructor handles painting the last frame. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:264: show_interstitial_cb_.Reset(); On 2016/12/20 00:16:17, miu wrote: > Why do we reset the callback? What if we start remoting again, and then there's > no callback? Should be safe to just leave it. When we start remoting again, a new RemoteRendererImpl will be created and a new callback will be set. As suggested, now let RemoteRendererImpl reset the callback. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.cc:274: DCHECK(!cb.is_null()); On 2016/12/20 00:16:17, miu wrote: > As mentioned above, let RemotingRendererImpl set a null callback to "unset" it. > This allows the "view" to explicitly disconnect from the "controller." Done. https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... File media/remoting/remoting_renderer_controller.h (right): https://codereview.chromium.org/2566223005/diff/60001/media/remoting/remoting... media/remoting/remoting_renderer_controller.h:59: void UpdateInterstitial(); On 2016/12/20 00:16:17, miu wrote: > Seems like this should be a private method. Done.
Just a few small things left. Comments on PS5: https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_interstitial_ui.cc:114: background_image = ResizeImage(image, centered_rect.size()); |background_image| is an extra variable that's not needed, and eliminating it would simplify this code: SkBitmap processed_image = ResizeImage(image, centered_rect.size()); if (type != RemotingInterstitialType::BETWEEN_SESSIONS) { color_utils::HSL shift = {-1, 0, 0.2}; // Make monochromatic. processed_image = SkBitmapOperations::CreateHSLShiftedBitmap(processed_image, shift); } canvas.writePixels(processed_image, centered_rect.x(), centered_rect.y()); https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_interstitial_ui.h:24: // Draw remoting interstitial on |background_image| and show it. nit: First sentence should be something like "Paint an interstitial frame and send it to the given VideoRendererSink." https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:287: case SESSION_STARTING: IMHO, we should show the "IN_SESSION" interstitial when in the SESSION_STARTING state. https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:289: return; I think you meant "break" instead of "return" here, so that we would paint the "BETWEEN_SESSIONS" interstitial for these cases.
Addressed comments. PTAL https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_interstitial_ui.cc (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_interstitial_ui.cc:114: background_image = ResizeImage(image, centered_rect.size()); On 2016/12/20 21:21:04, miu wrote: > |background_image| is an extra variable that's not needed, and eliminating it > would simplify this code: > > SkBitmap processed_image = ResizeImage(image, centered_rect.size()); > if (type != RemotingInterstitialType::BETWEEN_SESSIONS) { > color_utils::HSL shift = {-1, 0, 0.2}; // Make monochromatic. > processed_image = > SkBitmapOperations::CreateHSLShiftedBitmap(processed_image, shift); > } > canvas.writePixels(processed_image, centered_rect.x(), centered_rect.y()); Done. https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_interstitial_ui.h (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_interstitial_ui.h:24: // Draw remoting interstitial on |background_image| and show it. On 2016/12/20 21:21:04, miu wrote: > nit: First sentence should be something like "Paint an interstitial frame and > send it to the given VideoRendererSink." Done. https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:287: case SESSION_STARTING: On 2016/12/20 21:21:04, miu wrote: > IMHO, we should show the "IN_SESSION" interstitial when in the SESSION_STARTING > state. Why? We only need (and can only) show interstitial when RemoteRendererImpl is created, which can not be in "SESSION_STARTING" state. https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:289: return; On 2016/12/20 21:21:04, miu wrote: > I think you meant "break" instead of "return" here, so that we would paint the > "BETWEEN_SESSIONS" interstitial for these cases. I mean "return" because we don't need to paint interstitial on other cases. The "BETWEEN_SESSIONS" interstitial will only be painted by RemoteRendererImpl when it is destructed. Is there any case that RemoteRendererImpl destructor is not called whe n remoting session stops?
Comments on PS6: https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:287: case SESSION_STARTING: On 2016/12/20 22:15:37, xjz wrote: > On 2016/12/20 21:21:04, miu wrote: > > IMHO, we should show the "IN_SESSION" interstitial when in the > SESSION_STARTING > > state. > > Why? We only need (and can only) show interstitial when RemoteRendererImpl is > created, which can not be in "SESSION_STARTING" state. tl;dr: Don't bake assumptions into your code when you don't have to. We should try not to write code that makes assumptions about what code elsewhere will or will not do. Code outside of this method will change, and so we should try to make sure this method always does what it was intended to do. As I see it, this method's purpose is to decide whether the interstitial *should* be updated, and then which one should be showing, and then run the callback. It does not know anything about why it was called, nor what the effects of running the callback will be. It should do the same thing no matter what those reasons/effects are. At the most narrow scoping, we are looking at just this switch statement: Its purpose is to map one or more RemotingSource states into a RemotingInterstitialType. The mapping from "SESSION_STARTING" to "show the BETWEEN_SESSIONS interstitial" is incorrect if you consider that and nothing else. None of this seems to matter now, but it is important in the long-run in order to effectively manage large software systems. No one can hold perfectly detailed knowledge of the entire codebase in their head at once; and so any given developer's effectiveness is inversely proportional to the number of baked-in assumptions in the existing code. For example, what if in the future somebody (maybe not either of us) comes along and re-works the whole interstitial UI? They want to make it so that interstitials are rendered a bit earlier to give users a "snappier" response when remoting is engaged. It would be nice if they could depend on the UpdateInterstitial() method to work correctly regardless of the fact that the conditions under which it is used have changed. With the code as you have it now, they would encounter a bug and have to search through the codebase to eventually find out that UpdateInterstitial() was choosing "BETWEEN_SESSIONS." Then, they will assume the last developer had a good reason to map "SESSION_STARTING" to "BETWEEN_SESSIONS." They really want to change it, but will worry about whether changing it will break something else; and it will take them time to convince themselves that it is actually safe to change it to "IN_SESSION". Make sense? ;-) https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:289: return; On 2016/12/20 22:15:37, xjz wrote: > On 2016/12/20 21:21:04, miu wrote: > > I think you meant "break" instead of "return" here, so that we would paint the > > "BETWEEN_SESSIONS" interstitial for these cases. > > I mean "return" because we don't need to paint interstitial on other cases. The > "BETWEEN_SESSIONS" interstitial will only be painted by RemoteRendererImpl when > it is destructed. This is another flavor of the prior comment: Don't assume what the callback will or won't do. Just call it with the right value for the current situation. It may have no effect, but it's not up to UpdateInterstitial() to have to know that. > Is there any case that RemoteRendererImpl destructor is not > called whe n remoting session stops? Why should it matter to the code in UpdateInterstitial() what some other class's destructor does? ;-) https://codereview.chromium.org/2566223005/diff/120001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:294: DCHECK(type != RemotingInterstitialType::BETWEEN_SESSIONS); Per above comments, please remove this.
Thanks for reviewing! Addressed comments. PTAL. https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:287: case SESSION_STARTING: On 2016/12/20 23:15:49, miu wrote: > On 2016/12/20 22:15:37, xjz wrote: > > On 2016/12/20 21:21:04, miu wrote: > > > IMHO, we should show the "IN_SESSION" interstitial when in the > > SESSION_STARTING > > > state. > > > > Why? We only need (and can only) show interstitial when RemoteRendererImpl is > > created, which can not be in "SESSION_STARTING" state. > > tl;dr: Don't bake assumptions into your code when you don't have to. > > We should try not to write code that makes assumptions about what code elsewhere > will or will not do. Code outside of this method will change, and so we should > try to make sure this method always does what it was intended to do. As I see > it, this method's purpose is to decide whether the interstitial *should* be > updated, and then which one should be showing, and then run the callback. It > does not know anything about why it was called, nor what the effects of running > the callback will be. It should do the same thing no matter what those > reasons/effects are. > > At the most narrow scoping, we are looking at just this switch statement: Its > purpose is to map one or more RemotingSource states into a > RemotingInterstitialType. The mapping from "SESSION_STARTING" to "show the > BETWEEN_SESSIONS interstitial" is incorrect if you consider that and nothing > else. > > None of this seems to matter now, but it is important in the long-run in order > to effectively manage large software systems. No one can hold perfectly detailed > knowledge of the entire codebase in their head at once; and so any given > developer's effectiveness is inversely proportional to the number of baked-in > assumptions in the existing code. > > For example, what if in the future somebody (maybe not either of us) comes along > and re-works the whole interstitial UI? They want to make it so that > interstitials are rendered a bit earlier to give users a "snappier" response > when remoting is engaged. It would be nice if they could depend on the > UpdateInterstitial() method to work correctly regardless of the fact that the > conditions under which it is used have changed. With the code as you have it > now, they would encounter a bug and have to search through the codebase to > eventually find out that UpdateInterstitial() was choosing "BETWEEN_SESSIONS." > Then, they will assume the last developer had a good reason to map > "SESSION_STARTING" to "BETWEEN_SESSIONS." They really want to change it, but > will worry about whether changing it will break something else; and it will take > them time to convince themselves that it is actually safe to change it to > "IN_SESSION". > > Make sense? ;-) Thanks for the explanation. Learned a lot from your comments. :) As chatted face to face, still map the "SESSION_STARTING" to the "BETWEEN_SESSIONS" interstitial, as it is the transient state between two sessions. https://codereview.chromium.org/2566223005/diff/100001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:289: return; On 2016/12/20 23:15:49, miu wrote: > On 2016/12/20 22:15:37, xjz wrote: > > On 2016/12/20 21:21:04, miu wrote: > > > I think you meant "break" instead of "return" here, so that we would paint > the > > > "BETWEEN_SESSIONS" interstitial for these cases. > > > > I mean "return" because we don't need to paint interstitial on other cases. > The > > "BETWEEN_SESSIONS" interstitial will only be painted by RemoteRendererImpl > when > > it is destructed. > > This is another flavor of the prior comment: Don't assume what the callback will > or won't do. Just call it with the right value for the current situation. It may > have no effect, but it's not up to UpdateInterstitial() to have to know that. > Done. Thanks for the explanation. > > Is there any case that RemoteRendererImpl destructor is not > > called whe n remoting session stops? > > Why should it matter to the code in UpdateInterstitial() what some other class's > destructor does? ;-) I see. Thanks. :) https://codereview.chromium.org/2566223005/diff/120001/media/remoting/remotin... File media/remoting/remoting_renderer_controller.cc (right): https://codereview.chromium.org/2566223005/diff/120001/media/remoting/remotin... media/remoting/remoting_renderer_controller.cc:294: DCHECK(type != RemotingInterstitialType::BETWEEN_SESSIONS); On 2016/12/20 23:15:50, miu wrote: > Per above comments, please remove this. Done.
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...
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 miu@chromium.org
lgtm
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": 160001, "attempt_start_ts": 1482374145293480,
"parent_rev": "48e4b2b7d346b733e65950bbd68cf5410b9e4495", "commit_rev":
"466210b333690d00d46be2289e74eae85d2880c8"}
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Update remoting interstitial when status changes. This CL let RemotingRendererController control updating/showing interstitial. The interstitial will be updated when remoting starts successfully, or fails, or stops. BUG=643964 ========== to ========== Media Remoting: Update remoting interstitial when status changes. This CL let RemotingRendererController control updating/showing interstitial. The interstitial will be updated when remoting starts successfully, or fails, or stops. BUG=643964 Review-Url: https://codereview.chromium.org/2566223005 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Media Remoting: Update remoting interstitial when status changes. This CL let RemotingRendererController control updating/showing interstitial. The interstitial will be updated when remoting starts successfully, or fails, or stops. BUG=643964 Review-Url: https://codereview.chromium.org/2566223005 ========== to ========== Media Remoting: Update remoting interstitial when status changes. This CL let RemotingRendererController control updating/showing interstitial. The interstitial will be updated when remoting starts successfully, or fails, or stops. BUG=643964 Committed: https://crrev.com/af5c859fb2708358eb042fe62a8e5d5e66b264ae Cr-Commit-Position: refs/heads/master@{#440355} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/af5c859fb2708358eb042fe62a8e5d5e66b264ae Cr-Commit-Position: refs/heads/master@{#440355} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
