Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2810813004:
Hide fullscreen rotation jank (Closed)
|
Created:
3 years, 8 months ago by steimel Modified:
3 years, 5 months ago Reviewers:
aelias_OOO_until_Jul13, boliu, mlamouri (slow - plz ping), David Trainor- moved to gerrit, liberato (no reviews please), Khushal CC:
chromium-reviews, darin-cc_chromium.org, jam, nona+watch_chromium.org, shuchen+watch_chromium.org, James Su, yusukes+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionHide fullscreen rotation jank on Android
In order to hide the jank that is shown during fullscreen rotations, evict the
frame and show a black screen while in fullscreen when the frame size doesn't
match the physical backing size. This change also clears the thumbnail cache of
the tab when entering fullscreen so that the thumbnail isn't shown when the
frame is evicted.
BUG=710243
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2810813004
Cr-Commit-Position: refs/heads/master@{#486080}
Committed: https://chromium.googlesource.com/chromium/src/+/88f9d604d77b922af3117d72a2433a67ccdf1a46
Patch Set 1 #
Total comments: 1
Patch Set 2 : Show a black frame during fullscreen transitions to hide jank #
Total comments: 6
Patch Set 3 : Rebase #Patch Set 4 : Build error fixes #
Total comments: 8
Patch Set 5 : Evict frame instead of showing a black frame #
Total comments: 6
Patch Set 6 : make overlay_video_mode not affect background transparency #
Total comments: 8
Patch Set 7 : Refactor jank logic. Make RWHVA a WebContentsObserver to observe fullscreen state #
Total comments: 22
Patch Set 8 : Refactor to encapsulate logic into single function using enum of fullscreen states #Patch Set 9 : Refer to static layer as thumbnail layer within content. Rebase #
Total comments: 18
Patch Set 10 : Make OnFullscreenStateChanged a function at the RWVHB level. cr feedback #
Total comments: 8
Patch Set 11 : Added as feature (temporarily enabled-by-default for testing CQ failures). Other cr feedback #
Total comments: 1
Patch Set 12 : Assume true/false when updating fullscreen state #
Total comments: 32
Patch Set 13 : Remove physical_backing_resized param and other cr feedback #
Total comments: 30
Patch Set 14 : Clear thumbnail cache instead of hiding static layer. Break out results of changing fullscreen stat… #
Total comments: 8
Patch Set 15 : Pull logic out into testable helper class. Disable FullscreenActivity tests #Patch Set 16 : Remove awaiting_resize_ and instead SetIsFullscreen in FSController's UpdateSize #Patch Set 17 : BUILD file fix attempt #Patch Set 18 : Add CONTENT_EXPORT to fix build issue #Patch Set 19 : Re-add feature flag #
Total comments: 10
Patch Set 20 : Pull out everything except what's needed to solve fullscreen rotation jank #
Total comments: 6
Patch Set 21 : Remove clearThumbnailPlaceholder fn #Messages
Total messages: 159 (84 generated)
khushalsagar@chromium.org changed reviewers: + khushalsagar@chromium.org
https://codereview.chromium.org/2810813004/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:1140: // In fullscreen, prevent background from flashing white. Like we were discussing this will still miss a few cases. 1) This call only happens when you get a new frame. So till the time there is no update from the renderer, you won't see any change. 2) For the transition into video, you'll still see the stale frame. Except the area not drawn will be black. May be the better thing to do is to draw black instead of the stale frame when we go fullscreen in the browser, until we wait for the renderer's corresponding frame.
On second thought, I think just using the bg fill would be easier first and could look better. But doing it correctly still requires more plumbing. 1) We need to know if the renderer frame was actually for fullscreen video. I think you can plumb that from here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Fullscreen... into the LTH and then to the browser in CompositorFrameMetadata. 2) Plumb the browser's fullscreen video state into RWHVA. This was already being done here: https://codereview.chromium.org/2302023003/patch/40001/50001 3) If there is a mismatch between the current renderer frame and browser state, force the bg color to black in UpdateBgColor in RWHVA which will get this to the browser compositor through SceneLayer::GetBackgroundColor. You would also need to set the transparency on the browser compositor based on this color instead of the current way (https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...). There is already a hack to prevent white flashes for out of fullscreen transitions here that tries to do something similar: https://cs.chromium.org/chromium/src/content/browser/renderer_host/compositor.... I think this might take care of both transitions better. Can you give this a shot?
Description was changed from ========== Prevent white flashes during fullscreen transition and during rotation while in fullscreen BUG=710243 ========== to ========== Prevent white flashes during fullscreen transition and during rotation while in fullscreen BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by steimel@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 ========== Prevent white flashes during fullscreen transition and during rotation while in fullscreen BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== In order to hide the jank that is shown during fullscreen transitions, show a black frame during transitions. This includes transitioning into and out of fullscreen as well as rotations while in fullscreen mode. BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by steimel@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by steimel@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_...)
I took a look at the cc part. Still checking out the code in RWHVA. I don't think we can do this by sticking a black frame. You'll need to evict the bad frame. https://codereview.chromium.org/2810813004/diff/20001/cc/trees/proxy.h File cc/trees/proxy.h (right): https://codereview.chromium.org/2810813004/diff/20001/cc/trees/proxy.h#newcode46 cc/trees/proxy.h:46: virtual void SetIsFullscreen(bool is_fullscreen) = 0; Don't pipe this though proxy. The fullscreen state needs to be a frame thing so it needs to happen with a commit. Similar to |background_color_|, push it in LayerTreeHost::PushLayerTreePropertiesTo. And on the impl side, it needs to be on LayerTreeImpl instead of the host. In the metadata, put the value on the active tree, since that represents the state for the CompositorFrame that the metadata is accompanying. https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FullscreenController.cpp:270: web_view_base_->LayerTreeView()->SetIsFullscreen(false); Why not do this in DidExitFullScreen?
https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:580: prev_physical_backing_size_ = view_.GetPhysicalBackingSize(); I'm a bit confused about saving the previous fullscreen and size state here. We just want to make sure that for fullscreen transitions, the physical backing size matches the frame size right? Would it be sufficient to do: bool IsInFullscreenTransition() const { if (content_view_core_->IsFullscreen() || current_frame_is_fullscreen_) { return (content_view_core_->IsFullscreen() != current_frame_is_fullscreen_) || (view_.GetPhysicalBackingSize() != current_surface_size_); } } The current_frame stuff stored based on the CompositorFrameMetadata from the last frame. // If either the browser or renderer's frame is in fullscreen state, fallback to a black frame when sizes mismatch. } https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1241: view_.GetPhysicalBackingSize() != metadata.device_viewport_size; Shouldn't we be comparing this with the |current_surface_size_|? https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1246: bool controls_will_transition = Could you describe what the behaviour is for top controls during the transition? I think we go into this persistent fullscreen state that hides the toolbar android view in fullscreen. But I'm not totally clear on how that affects the composited toolbar. And what happens in the out of fullscreen transition.
Thanks for the feedback. Regarding evicting the frame vs showing a black frame, the reason I went with showing the black frame is that just evicting the frame will show the thumbnail layer if there is one, and the easiest way I found to hide that thumbnail layer is by showing a black frame, but I agree that evicting the frame *and* hiding the thumbnail layer would be cleaner if I could find a clean way to do it. we should discuss this in more detail offline. https://codereview.chromium.org/2810813004/diff/20001/cc/trees/proxy.h File cc/trees/proxy.h (right): https://codereview.chromium.org/2810813004/diff/20001/cc/trees/proxy.h#newcode46 cc/trees/proxy.h:46: virtual void SetIsFullscreen(bool is_fullscreen) = 0; On 2017/05/24 02:12:23, Khushal wrote: > Don't pipe this though proxy. The fullscreen state needs to be a frame thing so > it needs to happen with a commit. Similar to |background_color_|, push it in > LayerTreeHost::PushLayerTreePropertiesTo. And on the impl side, it needs to be > on LayerTreeImpl instead of the host. > > In the metadata, put the value on the active tree, since that represents the > state for the CompositorFrame that the metadata is accompanying. Ah gotcha, thanks for the insight and I'll make that change in the next patch I upload. I was following what I saw with visibility https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FullscreenController.cpp:270: web_view_base_->LayerTreeView()->SetIsFullscreen(false); On 2017/05/24 02:12:24, Khushal wrote: > Why not do this in DidExitFullScreen? To me it seemed to make more sense that is_fullscreen becomes false when state_ becomes kInitial as opposed to kNeedsScrollAndStateRestore. WDYT? https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:580: prev_physical_backing_size_ = view_.GetPhysicalBackingSize(); On 2017/05/24 05:14:22, Khushal wrote: > I'm a bit confused about saving the previous fullscreen and size state here. We > just want to make sure that for fullscreen transitions, the physical backing > size matches the frame size right? Would it be sufficient to do: > > bool IsInFullscreenTransition() const { > if (content_view_core_->IsFullscreen() || current_frame_is_fullscreen_) { > return (content_view_core_->IsFullscreen() != > current_frame_is_fullscreen_) > || (view_.GetPhysicalBackingSize() != current_surface_size_); > } > } > > The current_frame stuff stored based on the CompositorFrameMetadata from the > last frame. > > // If either the browser or renderer's frame is in fullscreen state, fallback to > a black frame when sizes mismatch. > > } Hmm I may be able to simplify some of the logic, but the issue with this is that it doesn't take into account the top_controls stuff, and we'll need a bool that keeps track of whether or not we're still in a fullscreen transition since coming out of one we'll hit a point where both content_view_core_->IsFullscreen() and current_frame_is_fullscreen_ are false, but we're still blocking frames for other reasons. But we'll only want to check those reasons when we're mid-transition if that makes sense. We can discuss this more offline. I'm also planning to cover this logic on the document I'm working on https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1241: view_.GetPhysicalBackingSize() != metadata.device_viewport_size; On 2017/05/24 05:14:23, Khushal wrote: > Shouldn't we be comparing this with the |current_surface_size_|? Well, view_.GetPhysicalBackingSize is the "correct" physical size we're looking for, and metadata.device_viewport_size is the physical size of the incoming frame. current_surface_size_ is the size of the last good frame we received. Unless you mean that I would compare what the new frame *would* set current_surface_size_ to with view_.GetPhysicalBackingSize, which could potentially work but I haven't checked it out. https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1246: bool controls_will_transition = On 2017/05/24 05:14:23, Khushal wrote: > Could you describe what the behaviour is for top controls during the transition? > I think we go into this persistent fullscreen state that hides the toolbar > android view in fullscreen. But I'm not totally clear on how that affects the > composited toolbar. And what happens in the out of fullscreen transition. So this particular check is something that's used when exiting fullscreen. There is jank that happens as the top controls expands from shown_ratio == 0 to shown_ratio == 1. This is normally hidden via the top_controls_transitioning condition above. However, there are times where the exiting fullscreen transition gets to a "good" state before the top controls begin their expansion. This means that the fullscreen_transition will be marked done and we'll be showing frames and therefore miss the chance to hide that jank. I don't have a full understanding of the details, but content_view_core_->GetViewSize seems to be aware that it's about to lose the height space from the controls before they start coming down which is why this check is true when the controls are coming down but haven't started yet. I did just realize that for completeness I should probably check that shown_ratio == 0 and that DoBrowserControlsShrinkBlinkSize() == true (I should also check that bool for the transitioning conditions).
Let me know when you remove the parts related to top controls transitioning. https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FullscreenController.cpp:270: web_view_base_->LayerTreeView()->SetIsFullscreen(false); On 2017/05/24 19:32:58, steimel wrote: > On 2017/05/24 02:12:24, Khushal wrote: > > Why not do this in DidExitFullScreen? > > To me it seemed to make more sense that is_fullscreen becomes false when state_ > becomes kInitial as opposed to kNeedsScrollAndStateRestore. WDYT? The kNeedsScrollAndStateRestore is needed for changes that can happen only after layout. Just setting the fullscreen state on the LayerTreeHost doesn't need to wait for that. https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1241: view_.GetPhysicalBackingSize() != metadata.device_viewport_size; On 2017/05/24 19:32:58, steimel wrote: > On 2017/05/24 05:14:23, Khushal wrote: > > Shouldn't we be comparing this with the |current_surface_size_|? > > Well, view_.GetPhysicalBackingSize is the "correct" physical size we're looking > for, and metadata.device_viewport_size is the physical size of the incoming > frame. current_surface_size_ is the size of the last good frame we received. > Unless you mean that I would compare what the new frame *would* set > current_surface_size_ to with view_.GetPhysicalBackingSize, which could > potentially work but I haven't checked it out. Yup, my bad for not stating that clearly. But you should compare it to the size of the root render pass, which is the size of the incoming frame.
https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:565: view_.GetPhysicalBackingSize() != prev_physical_backing_size_) { This should compare the physical backing size with the size of the current frame. You can call the function that is dealing with state transitions here. Btw, I noticed that we don't update current_surface_size_ to empty on evicting a frame. That should be fixed. https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:568: fullscreen_transitioning_ = true; I think I follow this better now. So for the case where you are transitioning out of fullscreen, you can have a case where the fullscreen state on the browser and renderer frame match, but the sizes still mismatch. And we want that to also be black? I would suggest adding an enum to make the states clearer. enum class FullscreenState { // The browser is not fullscreen and the renderer frame is consistent with this state. kNotFullscreen, // The browser has transitioned to fullscreen state but the renderer frame has not transitioned, i.e., either // the renderer frame is not fullscreen or the frame size does not match the expected size. kEnteringFullscreen, // The browser is in fullscreen state and the renderer frame is consistent with expected state. kFullscreen, // The browser has exited fullscreen state but the renderer frame has not transitioned. kExitingFullscreen, } And instead of using WasResized, plumb the correct fullscreen state from WebContentsImpl here. Just add it RWHVA and cast the current RWHV in WebContentsImpl to propagate it. You can keep the logic for updating and validating the state transitions in one function, have it evict the current frame if necessary, and call it from all places where you expect the state to change. https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1219: view_.GetPhysicalBackingSize() != metadata.device_viewport_size; I think this should be the same as |current_surface_size_|. Can you verify?
https://codereview.chromium.org/2810813004/diff/100001/chrome/browser/android... File chrome/browser/android/compositor/compositor_view.cc (right): https://codereview.chromium.org/2810813004/diff/100001/chrome/browser/android... chrome/browser/android/compositor/compositor_view.cc:190: compositor_->SetHasTransparentBackground(SkColorGetA(color) == SK_AlphaTRANSPARENT); Can you also remove the hack in CompositorImpl?
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
I understand this is still WIP but I left a couple of comments below. https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:563: // is completed. We should probably have a content or cc Feature in order to turn this on/off and have this behind an experiment. https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1208: was_fullscreen_ = !was_fullscreen_; Here and below, it is fairly confusing that a method called `CheckFor...` ends up. https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1284: } For my culture, why do we have to repeat the logic here? Also, would it make sense to have this in a different method as the logic is fairly the same as above?
Here's the latest. There's an issue where occasionally a rotate will look janky (infrequently enough to make it hard to debug, frequently enough to not be ignorable :)). I spent about 3 hours trying to figure it out so I could get it into this patch but realized it wasn't going to happen today, so I'm uploading this so you can check it out while I debug. Thanks for the input! https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/FullscreenController.cpp (right): https://codereview.chromium.org/2810813004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/web/FullscreenController.cpp:270: web_view_base_->LayerTreeView()->SetIsFullscreen(false); On 2017/05/25 03:33:40, Khushal wrote: > On 2017/05/24 19:32:58, steimel wrote: > > On 2017/05/24 02:12:24, Khushal wrote: > > > Why not do this in DidExitFullScreen? > > > > To me it seemed to make more sense that is_fullscreen becomes false when > state_ > > becomes kInitial as opposed to kNeedsScrollAndStateRestore. WDYT? > > The kNeedsScrollAndStateRestore is needed for changes that can happen only after > layout. Just setting the fullscreen state on the LayerTreeHost doesn't need to > wait for that. Done. https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1241: view_.GetPhysicalBackingSize() != metadata.device_viewport_size; On 2017/05/25 03:33:40, Khushal wrote: > On 2017/05/24 19:32:58, steimel wrote: > > On 2017/05/24 05:14:23, Khushal wrote: > > > Shouldn't we be comparing this with the |current_surface_size_|? > > > > Well, view_.GetPhysicalBackingSize is the "correct" physical size we're > looking > > for, and metadata.device_viewport_size is the physical size of the incoming > > frame. current_surface_size_ is the size of the last good frame we received. > > Unless you mean that I would compare what the new frame *would* set > > current_surface_size_ to with view_.GetPhysicalBackingSize, which could > > potentially work but I haven't checked it out. > > Yup, my bad for not stating that clearly. But you should compare it to the size > of the root render pass, which is the size of the incoming frame. Done. https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:565: view_.GetPhysicalBackingSize() != prev_physical_backing_size_) { On 2017/05/26 21:46:18, Khushal wrote: > This should compare the physical backing size with the size of the current > frame. You can call the function that is dealing with state transitions here. > > Btw, I noticed that we don't update current_surface_size_ to empty on evicting a > frame. That should be fixed. Done. https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:568: fullscreen_transitioning_ = true; On 2017/05/26 21:46:18, Khushal wrote: > I think I follow this better now. So for the case where you are transitioning > out of fullscreen, you can have a case where the fullscreen state on the browser > and renderer frame match, but the sizes still mismatch. And we want that to also > be black? > > I would suggest adding an enum to make the states clearer. > > enum class FullscreenState { > // The browser is not fullscreen and the renderer frame is consistent with > this state. > kNotFullscreen, > // The browser has transitioned to fullscreen state but the renderer frame has > not transitioned, i.e., either > // the renderer frame is not fullscreen or the frame size does not match the > expected size. > kEnteringFullscreen, > // The browser is in fullscreen state and the renderer frame is consistent > with expected state. > kFullscreen, > // The browser has exited fullscreen state but the renderer frame has not > transitioned. > kExitingFullscreen, > } > > And instead of using WasResized, plumb the correct fullscreen state from > WebContentsImpl here. Just add it RWHVA and cast the current RWHV in > WebContentsImpl to propagate it. > > You can keep the logic for updating and validating the state transitions in one > function, have it evict the current frame if necessary, and call it from all > places where you expect the state to change. The logic has been revamped (albeit without the addition of the enum you suggested). My hope is that it's been simplified enough to not need it, but let me know if you still think the enum would make a measurable difference in readability. https://codereview.chromium.org/2810813004/diff/80001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1219: view_.GetPhysicalBackingSize() != metadata.device_viewport_size; On 2017/05/26 21:46:18, Khushal wrote: > I think this should be the same as |current_surface_size_|. Can you verify? Done. https://codereview.chromium.org/2810813004/diff/100001/chrome/browser/android... File chrome/browser/android/compositor/compositor_view.cc (right): https://codereview.chromium.org/2810813004/diff/100001/chrome/browser/android... chrome/browser/android/compositor/compositor_view.cc:190: compositor_->SetHasTransparentBackground(SkColorGetA(color) == SK_AlphaTRANSPARENT); On 2017/05/31 20:26:51, Khushal wrote: > Can you also remove the hack in CompositorImpl? Done. https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:563: // is completed. On 2017/06/05 12:57:44, mlamouri wrote: > We should probably have a content or cc Feature in order to turn this on/off and > have this behind an experiment. Good idea. I don't know much about hiding features behind flags but maybe we can discuss offline https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1208: was_fullscreen_ = !was_fullscreen_; On 2017/06/05 12:57:44, mlamouri wrote: > Here and below, it is fairly confusing that a method called `CheckFor...` ends > up. I'm assuming you mean "ends up modifying state". I didn't really know how to name this to correctly imply what it was doing without accidentally implying that it doesn't change state (outside of the fact that it's not const). Fortunately, in my cleanup of the logic this function doesn't modify state anymore, so it's const and hopefully clearer :). https://codereview.chromium.org/2810813004/diff/100001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1284: } On 2017/06/05 12:57:44, mlamouri wrote: > For my culture, why do we have to repeat the logic here? > > Also, would it make sense to have this in a different method as the logic is > fairly the same as above? I have fixed this in my cleanup. It was separate here to make it easier to trace separately from the other frame eviction events while I was debugging an issue.
https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:590: SetBackgroundColor(transparent ? SK_ColorBLACK : SK_ColorWHITE); Could you leave a TODO here to set the color based on the color of the renderer frame. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:569: fullscreen_transitioning_ = true; The reason why I suggested the enum was so its easy to distinguish which transition we are handling, and that they are happening in the expected order. Right now we set this bool for the transition in and out of fullscreen and for size changes during fullscreen. And its hard to follow what initiated a transition change. The logic for the size change jank during fullscreen still needs to know what the current fullscreen state is. So the triggers/conditions that define jank also end up being in multiple places. I think you can have one function you call from every place where the state could have changed (fullscreen toggling, resize, new frame). Update the fullscreen state there accordingly and evict the frame if you have to. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1212: awaiting_resize_; When do we hit this case? Where layer sizes and fullscreen states match but we still need to wait for a resize? https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2084: bool is_fullscreen = content_view_core_->GetWebContents()->IsFullscreen(); Shouldn't be able to include WebContents here but its transitively coming from ContentViewCoreImpl. Woops. I'd suggest use host_->delegate()->IsFullscreenForTab. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:74: public content::WebContentsObserver, You can not use WebContentsObserver here. renderer_host can not know about web_contents. https://cs.chromium.org/chromium/src/content/browser/renderer_host/DEPS. I think plumbing it to the current RWHV from WebContentsImpl will be better. Something like: static_cast<RenderWidgetHostViewAndroid*>(GetRenderWidgetHostViewAndroid())->DidToggleFullscreenModeForTab. https://codereview.chromium.org/2810813004/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2810813004/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:178: boolean canUseStaticLayer(); nit: Its unclear what a static layer is. Static/live layer are very Chrome layer terms. May be canShowThumbnailPlaceholder would read better.
mlamouri@chromium.org changed reviewers: + liberato@chromium.org
+liberato@ https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:355: bool HasFullscreenTransitionJank(const cc::CompositorFrame& frame) const; nit: that name is quite confusing as we don't know if there is a jank. Maybe rename to something like "ShouldHideFullscreenTransition"? https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:356: void HideFullscreenTransitionJank(); nit: maybe drop "jank"? https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:357: void EndFullscreenTransition(); Can we re-use this for non-fullscreen transitions? Like simple portrait<->landscape transitions?
sorry for the delay. i wanted to try this locally, but i can't get it to compile due to ToT changes. will wait for the next rebase. thanks -fl https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:569: fullscreen_transitioning_ = true; On 2017/06/07 05:36:01, Khushal wrote: > The reason why I suggested the enum was so its easy to distinguish which > transition we are handling, and that they are happening in the expected order. > Right now we set this bool for the transition in and out of fullscreen and for > size changes during fullscreen. And its hard to follow what initiated a > transition change. > > The logic for the size change jank during fullscreen still needs to know what > the current fullscreen state is. So the triggers/conditions that define jank > also end up being in multiple places. > > I think you can have one function you call from every place where the state > could have changed (fullscreen toggling, resize, new frame). Update the > fullscreen state there accordingly and evict the frame if you have to. +1. it took me several reads to notice that |fullscreen_transitioning_| has two meanings. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1202: if (fullscreen_transitioning_) { if (!fullscreen_transitioning_) return false; one might also want to "if (!fullscreen && !transitioning)", so that "transitioning" can just mean "entering / exiting fullscreen". any size mismatch in fullscreen mode is cause to hide the display. then one could avoid setting or checking |transitioning| in OnPhysicalBackingSizeChanged.
Here's another refactor :). Next patch will have a rebase https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:590: SetBackgroundColor(transparent ? SK_ColorBLACK : SK_ColorWHITE); On 2017/06/07 05:36:01, Khushal wrote: > Could you leave a TODO here to set the color based on the color of the renderer > frame. Done. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:569: fullscreen_transitioning_ = true; On 2017/06/13 22:06:42, liberato wrote: > On 2017/06/07 05:36:01, Khushal wrote: > > The reason why I suggested the enum was so its easy to distinguish which > > transition we are handling, and that they are happening in the expected order. > > Right now we set this bool for the transition in and out of fullscreen and for > > size changes during fullscreen. And its hard to follow what initiated a > > transition change. > > > > The logic for the size change jank during fullscreen still needs to know what > > the current fullscreen state is. So the triggers/conditions that define jank > > also end up being in multiple places. > > > > I think you can have one function you call from every place where the state > > could have changed (fullscreen toggling, resize, new frame). Update the > > fullscreen state there accordingly and evict the frame if you have to. > > +1. it took me several reads to notice that |fullscreen_transitioning_| has two > meanings. Done. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1202: if (fullscreen_transitioning_) { On 2017/06/13 22:06:42, liberato wrote: > if (!fullscreen_transitioning_) > return false; > > one might also want to "if (!fullscreen && !transitioning)", so that > "transitioning" can just mean "entering / exiting fullscreen". any size > mismatch in fullscreen mode is cause to hide the display. then one could avoid > setting or checking |transitioning| in OnPhysicalBackingSizeChanged. This has been completely refactored now :) https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1212: awaiting_resize_; On 2017/06/07 05:36:01, Khushal wrote: > When do we hit this case? Where layer sizes and fullscreen states match but we > still need to wait for a resize? Discussed offline https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2084: bool is_fullscreen = content_view_core_->GetWebContents()->IsFullscreen(); On 2017/06/07 05:36:01, Khushal wrote: > Shouldn't be able to include WebContents here but its transitively coming from > ContentViewCoreImpl. Woops. > > I'd suggest use host_->delegate()->IsFullscreenForTab. Refactored to store what we get when WebContents tells us fullscreen state has changed. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:74: public content::WebContentsObserver, On 2017/06/07 05:36:01, Khushal wrote: > You can not use WebContentsObserver here. renderer_host can not know about > web_contents. > https://cs.chromium.org/chromium/src/content/browser/renderer_host/DEPS. > > I think plumbing it to the current RWHV from WebContentsImpl will be better. > Something like: > static_cast<RenderWidgetHostViewAndroid*>(GetRenderWidgetHostViewAndroid())->DidToggleFullscreenModeForTab. Done. https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:355: bool HasFullscreenTransitionJank(const cc::CompositorFrame& frame) const; On 2017/06/12 13:10:07, mlamouri wrote: > nit: that name is quite confusing as we don't know if there is a jank. Maybe > rename to something like "ShouldHideFullscreenTransition"? Refactoring replaced all three of these functions with a single function containing all the logic https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:356: void HideFullscreenTransitionJank(); On 2017/06/12 13:10:07, mlamouri wrote: > nit: maybe drop "jank"? Same as above https://codereview.chromium.org/2810813004/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:357: void EndFullscreenTransition(); On 2017/06/12 13:10:07, mlamouri wrote: > Can we re-use this for non-fullscreen transitions? Like simple > portrait<->landscape transitions? The new function should be re-useable https://codereview.chromium.org/2810813004/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2810813004/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:178: boolean canUseStaticLayer(); On 2017/06/07 05:36:01, Khushal wrote: > nit: Its unclear what a static layer is. Static/live layer are very Chrome layer > terms. May be canShowThumbnailPlaceholder would read better. Ack'd. Not included in the patch I'm uploading now, but I will change that in the next patch. Do you want that to be the name across the whole pipeline, or just when I'm outside of the Chrome layer?
rebased https://codereview.chromium.org/2810813004/diff/120001/content/public/android... File content/public/android/java/src/org/chromium/content_public/browser/WebContents.java (right): https://codereview.chromium.org/2810813004/diff/120001/content/public/android... content/public/android/java/src/org/chromium/content_public/browser/WebContents.java:178: boolean canUseStaticLayer(); On 2017/06/13 23:15:44, steimel wrote: > On 2017/06/07 05:36:01, Khushal wrote: > > nit: Its unclear what a static layer is. Static/live layer are very Chrome > layer > > terms. May be canShowThumbnailPlaceholder would read better. > > Ack'd. Not included in the patch I'm uploading now, but I will change that in > the next patch. Do you want that to be the name across the whole pipeline, or > just when I'm outside of the Chrome layer? Done.
The CQ bit was checked by steimel@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_...)
https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:571: EvictFrameIfNecessary(false); nit: UpdateFullscreenState? https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1208: if (mismatched_fullscreen_states) { Should this be a DCHECK that web_contents_is_fullscreen_? Because the renderer doesn't transition until the browser Acks the request. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: awaiting_resize_ = !physical_backing_resized; This should also be a DCHECK I think. You should definitely see a resize *after* the transition is requested. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1216: if (mismatched_fullscreen_states) { Same DCHECKs here but flipped. Browser transitions out of fullscreen first and you should see a resize for sure. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1224: fullscreen_state_ = FullscreenState::kFullscreenRotation; The only reason we needed the extra state was to use in CanShowThumbnail right? I'd prefer just evicting the frame on any size mismatch while in fullscreen and not showing the thumbnail at all in fullscreen state. But this is good too. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:241: bool can_show_thumbnail_placeholder() const { nit: not a trivial accessor. CanShowThumbnailPlaceholder and define in .cc? https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:360: enum class FullscreenState { nit: Generally this would be at the beginning of the private declarations. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:448: bool awaiting_resize_ = false; nit: fullscreen_transition_awaiting_resize_? And could you add a comment documenting the use? There are 2 things to note: 1) We are assuming that every transition to/from fullscreen will be followed by a change in view's physical backing size (which is basically the android Surface size). 2) And while we could simply take a mismatch in fullscreen states as the trigger for a transition, and assume the transition to have finished once the fullscreen states and sizes match, we have cases where we reach this consistent state before the physical backing size changes. Which is why we defer assuming the transition to be completed until a resize event is received.
i've done some testing locally, including some of the upcoming overlay changes. the transitions do look better. i even tried some really javascript-heavy test sites (https://jsbin.com/nexuxareli , courtesy of watk@). we get a longer black frame in those extreme cases, but it's really taking blink that long to get around to doing a relayout. lgtm. thanks -fl https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1200: if (physical_backing_resized) { nit: no need for {}
https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:571: EvictFrameIfNecessary(false); On 2017/06/14 19:43:14, Khushal wrote: > nit: UpdateFullscreenState? Done. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1200: if (physical_backing_resized) { On 2017/06/14 21:01:50, liberato wrote: > nit: no need for {} Done. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1208: if (mismatched_fullscreen_states) { On 2017/06/14 19:43:14, Khushal wrote: > Should this be a DCHECK that web_contents_is_fullscreen_? Because the renderer > doesn't transition until the browser Acks the request. Done. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: awaiting_resize_ = !physical_backing_resized; On 2017/06/14 19:43:14, Khushal wrote: > This should also be a DCHECK I think. You should definitely see a resize *after* > the transition is requested. Done. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1216: if (mismatched_fullscreen_states) { On 2017/06/14 19:43:14, Khushal wrote: > Same DCHECKs here but flipped. Browser transitions out of fullscreen first and > you should see a resize for sure. Done. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1224: fullscreen_state_ = FullscreenState::kFullscreenRotation; On 2017/06/14 19:43:14, Khushal wrote: > The only reason we needed the extra state was to use in CanShowThumbnail right? > I'd prefer just evicting the frame on any size mismatch while in fullscreen and > not showing the thumbnail at all in fullscreen state. But this is good too. Acknowledged. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:241: bool can_show_thumbnail_placeholder() const { On 2017/06/14 19:43:14, Khushal wrote: > nit: not a trivial accessor. CanShowThumbnailPlaceholder and define in .cc? Done. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:360: enum class FullscreenState { On 2017/06/14 19:43:14, Khushal wrote: > nit: Generally this would be at the beginning of the private declarations. Done. https://codereview.chromium.org/2810813004/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:448: bool awaiting_resize_ = false; On 2017/06/14 19:43:14, Khushal wrote: > nit: fullscreen_transition_awaiting_resize_? > > And could you add a comment documenting the use? There are 2 things to note: > > 1) We are assuming that every transition to/from fullscreen will be followed by > a change in view's physical backing size (which is basically the android Surface > size). > > 2) And while we could simply take a mismatch in fullscreen states as the trigger > for a transition, and assume the transition to have finished once the fullscreen > states and sizes match, we have cases where we reach this consistent state > before the physical backing size changes. Which is why we defer assuming the > transition to be completed until a resize event is received. Done.
The CQ bit was checked by steimel@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Could you add a feature in content/public/common/content_features.cc that will gate this change? Make it DISABLED_BY_DEFAULT for now and in a follow-up CL you can mark it ENABLED_BY_DEFAULT (so if there are regression, we can easily revert the latter instead of the full feature). https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:315: kFullscreenRotation, Do we not care about rotation when not in fullscreen?
One last question in WebContentsImpl. Looks great other than that! https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1228: case FullscreenState::kExitingFullscreen: nit: Would it be better if we handled the 3 transitions here separately. So kEntering goes to kFullscreen and kExiting goes to kNotFullscreen? And you can DCHECK on the |web_content_is_fullscreen_| to see that its expected. https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:267: void OnFullscreenStateChanged(bool entered_fullscreen) override; nit: Put this with the rest of the RWHV overrides. https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:315: kFullscreenRotation, On 2017/06/15 13:23:41, mlamouri wrote: > Do we not care about rotation when not in fullscreen? In those case we just use the frame we have and use the page background color to cover the gutter areas. That approach looks better for the not fullscreen case. https://codereview.chromium.org/2810813004/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2810813004/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2059: RenderWidgetHostViewBase* rwhvb = Don't we need to call this after delegate_->ExitFullscreenModeForTab on the next line? Since that is what will actually change the state reported in IsFullscreenForCurrentTab?
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1228: case FullscreenState::kExitingFullscreen: On 2017/06/15 18:42:55, Khushal wrote: > nit: Would it be better if we handled the 3 transitions here separately. So > kEntering goes to kFullscreen and kExiting goes to kNotFullscreen? And you can > DCHECK on the |web_content_is_fullscreen_| to see that its expected. Discussed offline. Holding off on updating the transition logic for now https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:267: void OnFullscreenStateChanged(bool entered_fullscreen) override; On 2017/06/15 18:42:55, Khushal wrote: > nit: Put this with the rest of the RWHV overrides. Done. https://codereview.chromium.org/2810813004/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2810813004/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2059: RenderWidgetHostViewBase* rwhvb = On 2017/06/15 18:42:56, Khushal wrote: > Don't we need to call this after delegate_->ExitFullscreenModeForTab on the next > line? Since that is what will actually change the state reported in > IsFullscreenForCurrentTab? Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm. Thanks! https://codereview.chromium.org/2810813004/diff/200001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/200001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1205: fullscreen_transition_awaiting_resize_ = false; I'm still a bit unsure about whether this is the best way to do this. Essentially we want to know whether the Surface for the compositor has been resized to what it should be based on the current fullscreen state. May be a better alternate would be to have an API using ViewAndroidDelegate that queries for exactly that. I'll let content/chrome folks suggest on that. Also, Bo can verify whether this this will work with WebView.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by steimel@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by steimel@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by steimel@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.
steimel@chromium.org changed reviewers: + aelias@chromium.org, boliu@chromium.org, dtrainor@chromium.org
PTAL
https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:914: public boolean canUseStaticLayer() { This whole canUseStaticLayer thing seems to be unused. Looks like a legacy of an obsolete patchset, please delete it. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:602: // TODO(steimel): Set the color based on the color of the renderer frame. This is done in OnFrameMetadataUpdated right now. Please refactor this so that there's a unified logic that calls SetBackgroundColor by considering all the current state instead of having these different subsystems fight each other for authority over it. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1227: if (mismatched_sizes) "else". I don't think ExitingFullscreen being clobbered by FullscreenRotation is ever desired. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1247: EvictDelegatedFrame(); This only needs to happen in SubmitCompositorFrame(), right? Please move this out of here and at the end of SubmitCompositorFrame(), explicitly do "if (IsInFullscreenTransition()) EvictDelegatedFrame();". https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1248: UpdateBackgroundColor(SK_ColorBLACK); Please change the main UpdateBackgroundColor call in OnFrameMetadataUpdated to take fullscreen state into account, instead of calling it a second time here. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1265: current_frame_is_fullscreen_ = frame.metadata.is_fullscreen; Please move this to OnFrameMetadataUpdated. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1298: UpdateFullscreenState(false); Please move this call to OnFrameMetadataUpdated. In this spot, do "if (IsInFullscreenTransition()) EvictDelegatedFrame();" instead. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2002: bool RenderWidgetHostViewAndroid::CanShowThumbnailPlaceholder() const { I'd prefer this to be called IsInFullscreenTransition(). https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2139: UpdateFullscreenState(true); Simpler to remove "physical_backing_resized" argument and instead do "fullscreen_transition_awaiting_resize_ = false; UpdateFullscreenState(); https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:445: // While we could simply take a mismatch in fullscreen states as the trigger I think there's a bug on the renderer side then. CC is misreporting a frame as being fullscreen in its metadata, when its size is still the old non-fullscreen size. Please find a way to defer the fullscreen metadata setting in the renderer instead of working around the problem here.
The CQ bit was checked by steimel@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...
Thanks for the feedback :). Haven't gone through all the comments yet, but I went through the quicker ones and will get to the others next week https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:914: public boolean canUseStaticLayer() { On 2017/06/16 22:46:40, aelias wrote: > This whole canUseStaticLayer thing seems to be unused. Looks like a legacy of > an obsolete patchset, please delete it. It's actually used to plumb that bool out to ContentLayer to tell it to not show the static/thumbnail layer when we're trying to show a black frame: https://codereview.chromium.org/2810813004/diff/220001/chrome/browser/android... https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1227: if (mismatched_sizes) On 2017/06/16 22:46:40, aelias wrote: > "else". I don't think ExitingFullscreen being clobbered by FullscreenRotation > is ever desired. Good point. done. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1247: EvictDelegatedFrame(); On 2017/06/16 22:46:40, aelias wrote: > This only needs to happen in SubmitCompositorFrame(), right? Please move this > out of here and at the end of SubmitCompositorFrame(), explicitly do "if > (IsInFullscreenTransition()) EvictDelegatedFrame();". No, this needs to happen regardless of where UpdateFullscreenState is called, since we don't want to wait until the next frame to start showing a black screen https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1248: UpdateBackgroundColor(SK_ColorBLACK); On 2017/06/16 22:46:41, aelias wrote: > Please change the main UpdateBackgroundColor call in OnFrameMetadataUpdated to > take fullscreen state into account, instead of calling it a second time here. If we're not coming from a SubmitCompositorFrame call, we won't have an OnFrameMetadataUpdated call AFAIK, and as before, we'll want this to happen before we receive the next frame. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1265: current_frame_is_fullscreen_ = frame.metadata.is_fullscreen; On 2017/06/16 22:46:40, aelias wrote: > Please move this to OnFrameMetadataUpdated. Done. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1298: UpdateFullscreenState(false); On 2017/06/16 22:46:40, aelias wrote: > Please move this call to OnFrameMetadataUpdated. In this spot, do "if > (IsInFullscreenTransition()) EvictDelegatedFrame();" instead. Done with first part. Second part discussed above https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2002: bool RenderWidgetHostViewAndroid::CanShowThumbnailPlaceholder() const { On 2017/06/16 22:46:41, aelias wrote: > I'd prefer this to be called IsInFullscreenTransition(). It's a bit tricky, since yes, CanShowThumbnailPlaceholder == IsInFullscreenTransition. This is used specifically by the WebContentsAndroid to ask whether or not it can show the thumbnail layer. If we rename this to IsInFullscreenTransition, then it's the WebContentsAndroid "deciding" that we can't show the thumbnail because we're in a fullscreen transition. If we leave the name as is, then it's the RWHVA "deciding" that we can't show the thumbnail because we're in a fullscreen transition, and the WebContentsAndroid just gets the end result. I'm fine with either name, but I figured I'd let you know why first before you decide. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2139: UpdateFullscreenState(true); On 2017/06/16 22:46:40, aelias wrote: > Simpler to remove "physical_backing_resized" argument and instead do > "fullscreen_transition_awaiting_resize_ = false; UpdateFullscreenState(); Done.
https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:914: public boolean canUseStaticLayer() { On 2017/06/16 at 23:24:12, steimel wrote: > On 2017/06/16 22:46:40, aelias wrote: > > This whole canUseStaticLayer thing seems to be unused. Looks like a legacy of > > an obsolete patchset, please delete it. > > It's actually used to plumb that bool out to ContentLayer to tell it to not show the static/thumbnail layer when we're trying to show a black frame: https://codereview.chromium.org/2810813004/diff/220001/chrome/browser/android... OK, I guess live_layer_draws is false because it was detached. How about calling ThumbnailCache::Remove() when entering fullscreen instead? It won't repopulate it from the disk cache because it's the primary tab. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1247: EvictDelegatedFrame(); On 2017/06/16 at 23:24:12, steimel wrote: > On 2017/06/16 22:46:40, aelias wrote: > > This only needs to happen in SubmitCompositorFrame(), right? Please move this > > out of here and at the end of SubmitCompositorFrame(), explicitly do "if > > (IsInFullscreenTransition()) EvictDelegatedFrame();". > > No, this needs to happen regardless of where UpdateFullscreenState is called, since we don't want to wait until the next frame to start showing a black screen I'd still rather it be broken out and called in multiple places because this meaty substantive result of the method is easy to miss when mixed in with all the other logic, and I think it makes more sense for UpdateFullscreenState to just passively update the state machine. Also, is it needed to call it on physical backing size change? https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1248: UpdateBackgroundColor(SK_ColorBLACK); On 2017/06/16 at 23:24:12, steimel wrote: > On 2017/06/16 22:46:41, aelias wrote: > > Please change the main UpdateBackgroundColor call in OnFrameMetadataUpdated to > > take fullscreen state into account, instead of calling it a second time here. > > If we're not coming from a SubmitCompositorFrame call, we won't have an OnFrameMetadataUpdated call AFAIK, and as before, we'll want this to happen before we receive the next frame. Is transparency equally good as black for your purposes? is_transparent is used exclusively for fullscreen anyway. I might suggest setting that argument to true if the fullscreen state is in transition. Alternatively, if SkColor_BLACK is better than SkColor_TRANSPARENT during the transition, then always set both to black. Either way I'd rather not have two different background color paths both to solve fullscreen transition cases. https://codereview.chromium.org/2810813004/diff/240001/cc/output/compositor_f... File cc/output/compositor_frame_metadata.h (right): https://codereview.chromium.org/2810813004/diff/240001/cc/output/compositor_f... cc/output/compositor_frame_metadata.h:51: bool is_fullscreen = false; What about using root_pass->has_transparent_background instead of introducing this? Transparent CC root surfaces pretty much == fullscreen. Maybe it would also work better for your size mismatch issue?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1248: UpdateBackgroundColor(SK_ColorBLACK); On 2017/06/17 01:18:34, aelias wrote: > On 2017/06/16 at 23:24:12, steimel wrote: > > On 2017/06/16 22:46:41, aelias wrote: > > > Please change the main UpdateBackgroundColor call in OnFrameMetadataUpdated > to > > > take fullscreen state into account, instead of calling it a second time > here. > > > > If we're not coming from a SubmitCompositorFrame call, we won't have an > OnFrameMetadataUpdated call AFAIK, and as before, we'll want this to happen > before we receive the next frame. > > Is transparency equally good as black for your purposes? is_transparent is used > exclusively for fullscreen anyway. I might suggest setting that argument to > true if the fullscreen state is in transition. > > Alternatively, if SkColor_BLACK is better than SkColor_TRANSPARENT during the > transition, then always set both to black. Either way I'd rather not have two > different background color paths both to solve fullscreen transition cases. aside: i'm decoupling uses of is_transparent_background / SkColor_TRANSPARENT from fullscreen as part of switching away from ContentVideoView. once this CL lands, i think that black will be preferable anyway.
https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1247: EvictDelegatedFrame(); On 2017/06/17 01:18:34, aelias wrote: > On 2017/06/16 at 23:24:12, steimel wrote: > > On 2017/06/16 22:46:40, aelias wrote: > > > This only needs to happen in SubmitCompositorFrame(), right? Please move > this > > > out of here and at the end of SubmitCompositorFrame(), explicitly do "if > > > (IsInFullscreenTransition()) EvictDelegatedFrame();". > > > > No, this needs to happen regardless of where UpdateFullscreenState is called, > since we don't want to wait until the next frame to start showing a black screen > > I'd still rather it be broken out and called in multiple places because this > meaty substantive result of the method is easy to miss when mixed in with all > the other logic, and I think it makes more sense for UpdateFullscreenState to > just passively update the state machine. Also, is it needed to call it on > physical backing size change? The physical backing size change is the main trigger for handling rotations. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1248: UpdateBackgroundColor(SK_ColorBLACK); On 2017/06/17 01:18:34, aelias wrote: > On 2017/06/16 at 23:24:12, steimel wrote: > > On 2017/06/16 22:46:41, aelias wrote: > > > Please change the main UpdateBackgroundColor call in OnFrameMetadataUpdated > to > > > take fullscreen state into account, instead of calling it a second time > here. > > > > If we're not coming from a SubmitCompositorFrame call, we won't have an > OnFrameMetadataUpdated call AFAIK, and as before, we'll want this to happen > before we receive the next frame. > > Is transparency equally good as black for your purposes? is_transparent is used > exclusively for fullscreen anyway. I might suggest setting that argument to > true if the fullscreen state is in transition. Transparency is only used when the video is being rendered to a seperate SurfaceView. This doesn't capture the cases where a non-video element is fullscreen-ed that doesn't go through this path, which I think is used by pages for custom video controls. > > Alternatively, if SkColor_BLACK is better than SkColor_TRANSPARENT during the > transition, then always set both to black. Either way I'd rather not have two > different background color paths both to solve fullscreen transition cases. We can not always set the color to black right now since in the separate SurfaceView case, transparency is needed for the browser compositor to draw transparent quads to make only the controls visible. May be liberato@'s work will change that? https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:445: // While we could simply take a mismatch in fullscreen states as the trigger On 2017/06/16 22:46:41, aelias wrote: > I think there's a bug on the renderer side then. CC is misreporting a frame as > being fullscreen in its metadata, when its size is still the old non-fullscreen > size. Please find a way to defer the fullscreen metadata setting in the > renderer instead of working around the problem here. We use the state transitions in FullscreenController.h to set the fullscreen state for a frame coming from the renderer. Are we doing something incorrect there? If the state reported in the CompositorFrame is correct then the scenario where I expect waiting for a resize to help is the case where the renderer transitions to fullscreen and sends a frame with the previous size before the android Surface is resized.
https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1248: UpdateBackgroundColor(SK_ColorBLACK); On 2017/06/19 at 18:46:34, Khushal wrote: > We can not always set the color to black right now since in the separate SurfaceView case, transparency is needed for the browser compositor to draw transparent quads to make only the controls visible. May be liberato@'s work will change that? I'd be interested in hearing whether or not we see any flickers if we try transparency instead of black in the cases meant to be fixed by this patch. If we don't, maybe we could always use transparency for video use cases. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:445: // While we could simply take a mismatch in fullscreen states as the trigger On 2017/06/19 at 18:46:34, Khushal wrote: > On 2017/06/16 22:46:41, aelias wrote: > > I think there's a bug on the renderer side then. CC is misreporting a frame as > > being fullscreen in its metadata, when its size is still the old non-fullscreen > > size. Please find a way to defer the fullscreen metadata setting in the > > renderer instead of working around the problem here. > > We use the state transitions in FullscreenController.h to set the fullscreen state for a frame coming from the renderer. Are we doing something incorrect there? > > If the state reported in the CompositorFrame is correct then the scenario where I expect waiting for a resize to help is the case where the renderer transitions to fullscreen and sends a frame with the previous size before the android Surface is resized. I'm not sure precisely what is going wrong, but it seems wrong in principle for CC to report a frame with the non-fullscreen size as being fullscreen. Instead of waiting on the browser side for the size to change, I suggest something on the renderer should be waiting for the size to change before flipping is_fullscreen. Or, what I would find even nicer is for is_fullscreen not be introduced at all, for example if you use another signal like the transparency status of the surface, or if you find a better way to obtain the expected final size on the browser and then wait for all sizes to converge to it.
https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1248: UpdateBackgroundColor(SK_ColorBLACK); On 2017/06/19 18:46:34, Khushal wrote: > On 2017/06/17 01:18:34, aelias wrote: > > On 2017/06/16 at 23:24:12, steimel wrote: > > > On 2017/06/16 22:46:41, aelias wrote: > > > > Please change the main UpdateBackgroundColor call in > OnFrameMetadataUpdated > > to > > > > take fullscreen state into account, instead of calling it a second time > > here. > > > > > > If we're not coming from a SubmitCompositorFrame call, we won't have an > > OnFrameMetadataUpdated call AFAIK, and as before, we'll want this to happen > > before we receive the next frame. > > > > Is transparency equally good as black for your purposes? is_transparent is > used > > exclusively for fullscreen anyway. I might suggest setting that argument to > > true if the fullscreen state is in transition. > > Transparency is only used when the video is being rendered to a seperate > SurfaceView. This doesn't capture the cases where a non-video element is > fullscreen-ed that doesn't go through this path, which I think is used by pages > for custom video controls. > > > > > Alternatively, if SkColor_BLACK is better than SkColor_TRANSPARENT during the > > transition, then always set both to black. Either way I'd rather not have two > > different background color paths both to solve fullscreen transition cases. > > We can not always set the color to black right now since in the separate > SurfaceView case, transparency is needed for the browser compositor to draw > transparent quads to make only the controls visible. May be liberato@'s work > will change that? re "transparency is needed... controls visible". my work won't change how that part works. however, i don't believe that's related to the layer tree background, either. the LT background just helps to make a nice orientation transition, else the re-oriented (by android) SurfaceView clips against the black gutter until blink does a relayout. the part that lets the controls show over the video is this: creating a ContentVideoView starts a chain of events that causes CompostorSurfaceManager to notify android not to ignore the alpha channel in the compositor's output. it's shortly before CompositorView fiddles with has_transparent_background, if i remember. either way, the browser compositor will draw a transparent quad that exactly covers the SurfaceView through which one can see the video (if android doesn't ignore alpha!). that's done by the overlay processer, i think, in the browser compositor. the work i'm involved in decouples "using overlay" from "using fullscreen". it makes the transparent gutter trick somewhat less appealing, since i'd like to use it for custom video controls too. i'm not sure that we need the transparent gutter thing at all once this CL lands. we might just be able to go back to a black gutter, all the way from blink to here. at least, it looks pretty good in my local testing on slower hardware with this CL applied.
On 2017/06/19 20:22:29, liberato wrote: > https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_android.cc:1248: > UpdateBackgroundColor(SK_ColorBLACK); > On 2017/06/19 18:46:34, Khushal wrote: > > On 2017/06/17 01:18:34, aelias wrote: > > > On 2017/06/16 at 23:24:12, steimel wrote: > > > > On 2017/06/16 22:46:41, aelias wrote: > > > > > Please change the main UpdateBackgroundColor call in > > OnFrameMetadataUpdated > > > to > > > > > take fullscreen state into account, instead of calling it a second time > > > here. > > > > > > > > If we're not coming from a SubmitCompositorFrame call, we won't have an > > > OnFrameMetadataUpdated call AFAIK, and as before, we'll want this to happen > > > before we receive the next frame. > > > > > > Is transparency equally good as black for your purposes? is_transparent is > > used > > > exclusively for fullscreen anyway. I might suggest setting that argument to > > > true if the fullscreen state is in transition. > > > > Transparency is only used when the video is being rendered to a seperate > > SurfaceView. This doesn't capture the cases where a non-video element is > > fullscreen-ed that doesn't go through this path, which I think is used by > pages > > for custom video controls. > > > > > > > > Alternatively, if SkColor_BLACK is better than SkColor_TRANSPARENT during > the > > > transition, then always set both to black. Either way I'd rather not have > two > > > different background color paths both to solve fullscreen transition cases. > > > > We can not always set the color to black right now since in the separate > > SurfaceView case, transparency is needed for the browser compositor to draw > > transparent quads to make only the controls visible. May be liberato@'s work > > will change that? > > re "transparency is needed... controls visible". my work won't change how that > part works. > > however, i don't believe that's related to the layer tree background, either. > the LT background just helps to make a nice orientation transition, else the > re-oriented (by android) SurfaceView clips against the black gutter until blink > does a relayout. > > the part that lets the controls show over the video is this: creating a > ContentVideoView starts a chain of events that causes CompostorSurfaceManager > to notify android not to ignore the alpha channel in the compositor's output. > it's shortly before CompositorView fiddles with has_transparent_background, if i > remember. Its not exactly related to the Layer tree background now but with this change is making it so. The |has_transparent_background| is what tells the browser compositor to not append any quads for the background color. The fact that it happens with ContentVideoView would cause white flashes in out of fullscreen transition since this bit is flipped before we have the correct frame from the renderer. It would take the current transparent frame and use the default white color. There is a hack there that just delays setting the color to white and temporarily uses black when transparency is reset to avoid this. This change is also removing the |has_transparent_background| fiddling from ContentVideoView and setting it based on the renderer frame background color. But I guess that part is unnecessary, because evicting the frames during the transitions should take care of that. In which case using black in fullscreen throughout the stack should work. > > either way, the browser compositor will draw a transparent quad that exactly > covers the SurfaceView through which one can see the video (if android doesn't > ignore alpha!). that's done by the overlay processer, i think, in the browser > compositor. > > the work i'm involved in decouples "using overlay" from "using fullscreen". it > makes the transparent gutter trick somewhat less appealing, since i'd like to > use it for custom video controls too. > > i'm not sure that we need the transparent gutter thing at all once this CL > lands. we might just be able to go back to a black gutter, all the way from > blink to here. at least, it looks pretty good in my local testing on slower > hardware with this CL applied.
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; this seems way too brittle. how do you know enter/exit fullscreen will always come before the resize? https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && also, how do you know entering/existing fullscreen will cause a resize? it's possible in theory that the webcontents stay the same size (no window borders etc). for rotation, in the pathological case case, what if you have a square phone? I don't think there's any guarantee that the fullscreen state converges either. overall, I think this code is too brittle, it's adding assumptions/dependencies between a whole bunch of largely independent signals, with no tests. no one is going to understand this in a year or two, and it'll probably just get removed https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1241: EvictDelegatedFrame(); need a comment why this is desirable https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:409: virtual void OnFullscreenStateChanged(bool is_fullscreen) {} how is this different from the is_fullscreen thing on line 434? https://codereview.chromium.org/2810813004/diff/240001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/public/common/... content/public/common/content_features.cc:115: "HideFullscreenTransitionJank", base::FEATURE_ENABLED_BY_DEFAULT}; is this necessary? are you planning on running an experiment or something?
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/20 at 20:54:44, boliu wrote: > this seems way too brittle. how do you know enter/exit fullscreen will always come before the resize? Agreed, I gave a lot of feedback/suggestions about this above. https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && On 2017/06/20 at 20:54:44, boliu wrote: > also, how do you know entering/existing fullscreen will cause a resize? it's possible in theory that the webcontents stay the same size (no window borders etc). > > for rotation, in the pathological case case, what if you have a square phone? I'm comfortable assuming that fullscreen and rotation transitions will always cause resize. These concerns are too theoretical -- let's not overconstrain the solution space unnecessarily, because this problem is difficult enough as it is. There have been thousands of models of Android devices and all of them to the best of my knowledge have a mandatory notification shade and are not perfectly square, and we probably have a fair amount of code implicitly relying on these properties already.
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/20 20:54:44, boliu wrote: > this seems way too brittle. how do you know enter/exit fullscreen will always > come before the resize? You're right, this is a brittle assumption. We need a better way to detect when the fullscreen transition on the browser actually completes. Having android resize the surface is async and we currently notify the renderer that the transition is complete before that happens and we start getting frames with the element fullscreen-ed using the old surface size which causes videos to jump around during these transitions. So either notifying the renderer needs to wait on all state transitions on the browser to finish or we need an API that the code can call here to know what the fullscreen size is supposed to be and not mark the transition complete until both the browser and renderer converge to that size (which is what I think aelias@ was suggesting). https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && On 2017/06/20 20:54:44, boliu wrote: > also, how do you know entering/existing fullscreen will cause a resize? it's > possible in theory that the webcontents stay the same size (no window borders > etc). Yup, need a more explicit way to handle the resize. > > for rotation, in the pathological case case, what if you have a square phone? The primary cases where the rotation transition looks bad is where there is a mismatch in portrait vs landscape sizes and the video suddenly jumps from a corner on the screen to fullscreen. In the case where these sizes match, not entering the transition should look fine IMO. > > I don't think there's any guarantee that the fullscreen state converges either. What would be the case where the fullscreen states don't converge? > > overall, I think this code is too brittle, it's adding assumptions/dependencies > between a whole bunch of largely independent signals, with no tests. no one is > going to understand this in a year or two, and it'll probably just get removed
https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2810813004/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:914: public boolean canUseStaticLayer() { On 2017/06/17 01:18:34, aelias wrote: > On 2017/06/16 at 23:24:12, steimel wrote: > > On 2017/06/16 22:46:40, aelias wrote: > > > This whole canUseStaticLayer thing seems to be unused. Looks like a legacy > of > > > an obsolete patchset, please delete it. > > > > It's actually used to plumb that bool out to ContentLayer to tell it to not > show the static/thumbnail layer when we're trying to show a black frame: > https://codereview.chromium.org/2810813004/diff/220001/chrome/browser/android... > > OK, I guess live_layer_draws is false because it was detached. How about > calling ThumbnailCache::Remove() when entering fullscreen instead? It won't > repopulate it from the disk cache because it's the primary tab. Done and will be included in next patch https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:2002: bool RenderWidgetHostViewAndroid::CanShowThumbnailPlaceholder() const { On 2017/06/16 23:24:12, steimel wrote: > On 2017/06/16 22:46:41, aelias wrote: > > I'd prefer this to be called IsInFullscreenTransition(). > > It's a bit tricky, since yes, CanShowThumbnailPlaceholder == > IsInFullscreenTransition. This is used specifically by the WebContentsAndroid to > ask whether or not it can show the thumbnail layer. If we rename this to > IsInFullscreenTransition, then it's the WebContentsAndroid "deciding" that we > can't show the thumbnail because we're in a fullscreen transition. If we leave > the name as is, then it's the RWHVA "deciding" that we can't show the thumbnail > because we're in a fullscreen transition, and the WebContentsAndroid just gets > the end result. > > I'm fine with either name, but I figured I'd let you know why first before you > decide. This is method getting removed in the next patch https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && On 2017/06/20 21:15:14, aelias wrote: > On 2017/06/20 at 20:54:44, boliu wrote: > > also, how do you know entering/existing fullscreen will cause a resize? it's > possible in theory that the webcontents stay the same size (no window borders > etc). > > > > for rotation, in the pathological case case, what if you have a square phone? > > I'm comfortable assuming that fullscreen and rotation transitions will always > cause resize. These concerns are too theoretical -- let's not overconstrain the > solution space unnecessarily, because this problem is difficult enough as it is. > There have been thousands of models of Android devices and all of them to the > best of my knowledge have a mandatory notification shade and are not perfectly > square, and we probably have a fair amount of code implicitly relying on these > properties already. A lot of good points here, though regarding the square phone case: this will (correctly) not trigger since we won't need to block any frames (since they'll already be the right size)
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && On 2017/06/20 21:15:14, aelias wrote: > On 2017/06/20 at 20:54:44, boliu wrote: > > also, how do you know entering/existing fullscreen will cause a resize? it's > possible in theory that the webcontents stay the same size (no window borders > etc). > > > > for rotation, in the pathological case case, what if you have a square phone? > > I'm comfortable assuming that fullscreen and rotation transitions will always > cause resize. These concerns are too theoretical -- let's not overconstrain the > solution space unnecessarily, because this problem is difficult enough as it is. > There have been thousands of models of Android devices and all of them to the > best of my knowledge have a mandatory notification shade and are not perfectly > square, and we probably have a fair amount of code implicitly relying on these > properties already. Sure square phone is not realistic. But I'm not sure about notification shade thing. How many of the existing assumptions would completely break rendering if it was wrong? On 2017/06/20 21:26:35, Khushal wrote: > On 2017/06/20 20:54:44, boliu wrote: > > also, how do you know entering/existing fullscreen will cause a resize? it's > > possible in theory that the webcontents stay the same size (no window borders > > etc). > > Yup, need a more explicit way to handle the resize. > > > > > for rotation, in the pathological case case, what if you have a square phone? > > The primary cases where the rotation transition looks bad is where there is a > mismatch in portrait vs landscape sizes and the video suddenly jumps from a > corner on the screen to fullscreen. In the case where these sizes match, not > entering the transition should look fine IMO. > > > > > I don't think there's any guarantee that the fullscreen state converges > either. > > What would be the case where the fullscreen states don't converge? Glanced at the cc change. I don't think setting fullscreen actually guarantees a frame will be produced. It's all side effects of other code.. > > > > > overall, I think this code is too brittle, it's adding > assumptions/dependencies > > between a whole bunch of largely independent signals, with no tests. no one is > > going to understand this in a year or two, and it'll probably just get removed >
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/20 21:15:14, aelias wrote: > On 2017/06/20 at 20:54:44, boliu wrote: > > this seems way too brittle. how do you know enter/exit fullscreen will always > come before the resize? > > Agreed, I gave a lot of feedback/suggestions about this above. How do you feel about an API via ViewAndroidDelegate to know what fullscreen size to expect and using that here instead? https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && On 2017/06/20 21:33:49, boliu wrote: > On 2017/06/20 21:15:14, aelias wrote: > > On 2017/06/20 at 20:54:44, boliu wrote: > > > also, how do you know entering/existing fullscreen will cause a resize? it's > > possible in theory that the webcontents stay the same size (no window borders > > etc). > > > > > > for rotation, in the pathological case case, what if you have a square > phone? > > > > I'm comfortable assuming that fullscreen and rotation transitions will always > > cause resize. These concerns are too theoretical -- let's not overconstrain > the > > solution space unnecessarily, because this problem is difficult enough as it > is. > > There have been thousands of models of Android devices and all of them to the > > best of my knowledge have a mandatory notification shade and are not perfectly > > square, and we probably have a fair amount of code implicitly relying on these > > properties already. > > Sure square phone is not realistic. But I'm not sure about notification shade > thing. How many of the existing assumptions would completely break rendering if > it was wrong? > > On 2017/06/20 21:26:35, Khushal wrote: > > On 2017/06/20 20:54:44, boliu wrote: > > > also, how do you know entering/existing fullscreen will cause a resize? it's > > > possible in theory that the webcontents stay the same size (no window > borders > > > etc). > > > > Yup, need a more explicit way to handle the resize. > > > > > > > > for rotation, in the pathological case case, what if you have a square > phone? > > > > The primary cases where the rotation transition looks bad is where there is a > > mismatch in portrait vs landscape sizes and the video suddenly jumps from a > > corner on the screen to fullscreen. In the case where these sizes match, not > > entering the transition should look fine IMO. > > > > > > > > I don't think there's any guarantee that the fullscreen state converges > > either. > > > > What would be the case where the fullscreen states don't converge? > > Glanced at the cc change. I don't think setting fullscreen actually guarantees a > frame will be produced. It's all side effects of other code.. The place where that bit is being set doesn't require a commit because its done where the renderer is granted fullscreen so the bit will come whenever the first fullscreen frame is produced. Its similar to the background color change. > > > > > > > > > overall, I think this code is too brittle, it's adding > > assumptions/dependencies > > > between a whole bunch of largely independent signals, with no tests. no one > is > > > going to understand this in a year or two, and it'll probably just get > removed > > >
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/20 21:44:03, Khushal wrote: > On 2017/06/20 21:15:14, aelias wrote: > > On 2017/06/20 at 20:54:44, boliu wrote: > > > this seems way too brittle. how do you know enter/exit fullscreen will > always > > come before the resize? > > > > Agreed, I gave a lot of feedback/suggestions about this above. > > How do you feel about an API via ViewAndroidDelegate to know what fullscreen > size to expect and using that here instead? You can get the display size directly through the ui::display api. No need to go through ViewAndroidDelegate. But I'm more concerned about if that's actually the fullscreen size. This is android.. any kind of weirdness is possible. Also in webview, the fullscreen size is controlled directly by the app, and apps have no obligations to actually make it fullscreen (although I guess this code is only really relevant to chrome I guess). Also not sure how this plays with picture in picture, mlamouri@ can comment on that. We probably can order these events though. The full screen signal goes from renderer to browser, then browser requests fullscreen, which leads to a corresponding resize. So if we set this pending fullscreen state before we request fullscreen, then the resize is guaranteed to come after. It's just this code is definitely not written that way. Although I suppose the problem then might be the reverse, that an empty frame is swapped before the fullscreen transition starts? https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && On 2017/06/20 21:44:03, Khushal wrote: > On 2017/06/20 21:33:49, boliu wrote: > > On 2017/06/20 21:15:14, aelias wrote: > > > On 2017/06/20 at 20:54:44, boliu wrote: > > > > also, how do you know entering/existing fullscreen will cause a resize? > it's > > > possible in theory that the webcontents stay the same size (no window > borders > > > etc). > > > > > > > > for rotation, in the pathological case case, what if you have a square > > phone? > > > > > > I'm comfortable assuming that fullscreen and rotation transitions will > always > > > cause resize. These concerns are too theoretical -- let's not overconstrain > > the > > > solution space unnecessarily, because this problem is difficult enough as it > > is. > > > There have been thousands of models of Android devices and all of them to > the > > > best of my knowledge have a mandatory notification shade and are not > perfectly > > > square, and we probably have a fair amount of code implicitly relying on > these > > > properties already. > > > > Sure square phone is not realistic. But I'm not sure about notification shade > > thing. How many of the existing assumptions would completely break rendering > if > > it was wrong? > > > > On 2017/06/20 21:26:35, Khushal wrote: > > > On 2017/06/20 20:54:44, boliu wrote: > > > > also, how do you know entering/existing fullscreen will cause a resize? > it's > > > > possible in theory that the webcontents stay the same size (no window > > borders > > > > etc). > > > > > > Yup, need a more explicit way to handle the resize. > > > > > > > > > > > for rotation, in the pathological case case, what if you have a square > > phone? > > > > > > The primary cases where the rotation transition looks bad is where there is > a > > > mismatch in portrait vs landscape sizes and the video suddenly jumps from a > > > corner on the screen to fullscreen. In the case where these sizes match, not > > > entering the transition should look fine IMO. > > > > > > > > > > > I don't think there's any guarantee that the fullscreen state converges > > > either. > > > > > > What would be the case where the fullscreen states don't converge? > > > > Glanced at the cc change. I don't think setting fullscreen actually guarantees > a > > frame will be produced. It's all side effects of other code.. > > The place where that bit is being set doesn't require a commit because its done > where the renderer is granted fullscreen so the bit will come whenever the first > fullscreen frame is produced. Its similar to the background color change. Err... that makes me think background color is wrong too, not the other way around. Btw, if you want something to go through renderer compositor, but not necessarily trigger swap or any additional work that's otherwise not needed, you want SwapPromise. Looks like this might be a good fit (assuming if you don't take the transparent background advice) > > > > > > > > > > > > > > overall, I think this code is too brittle, it's adding > > > assumptions/dependencies > > > > between a whole bunch of largely independent signals, with no tests. no > one > > is > > > > going to understand this in a year or two, and it'll probably just get > > removed > > > > > >
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/20 22:07:51, boliu wrote: > On 2017/06/20 21:44:03, Khushal wrote: > > On 2017/06/20 21:15:14, aelias wrote: > > > On 2017/06/20 at 20:54:44, boliu wrote: > > > > this seems way too brittle. how do you know enter/exit fullscreen will > > always > > > come before the resize? > > > > > > Agreed, I gave a lot of feedback/suggestions about this above. > > > > How do you feel about an API via ViewAndroidDelegate to know what fullscreen > > size to expect and using that here instead? > > You can get the display size directly through the ui::display api. No need to go > through ViewAndroidDelegate. > > But I'm more concerned about if that's actually the fullscreen size. This is > android.. any kind of weirdness is possible. Also in webview, the fullscreen > size is controlled directly by the app, and apps have no obligations to actually > make it fullscreen (although I guess this code is only really relevant to chrome > I guess). Also not sure how this plays with picture in picture, mlamouri@ can > comment on that. WebView was what I was most unsure about. Is a resize guaranteed there? For Chrome whichever of these 2 you think will be a better assumption to make in this code would be fine. > > We probably can order these events though. The full screen signal goes from > renderer to browser, then browser requests fullscreen, which leads to a > corresponding resize. So if we set this pending fullscreen state before we > request fullscreen, then the resize is guaranteed to come after. It's just this > code is definitely not written that way. Although I suppose the problem then > might be the reverse, that an empty frame is swapped before the fullscreen > transition starts? Sure, the pending state can be set before the transition is initiated (in WebContentsImpl). The reason it works currently is because the resize always comes asynchronously from android. Though WebContentsImpl uses IsFullscreenForTab after requesting the transition to know if it was granted. Is there a case where that request could fail? The empty frame doesn't get committed/swapped until the next frame is requested by the Display. That behavior won't change. https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1229: if (!mismatched_fullscreen_states && !mismatched_sizes && On 2017/06/20 22:07:51, boliu wrote: > On 2017/06/20 21:44:03, Khushal wrote: > > On 2017/06/20 21:33:49, boliu wrote: > > > On 2017/06/20 21:15:14, aelias wrote: > > > > On 2017/06/20 at 20:54:44, boliu wrote: > > > > > also, how do you know entering/existing fullscreen will cause a resize? > > it's > > > > possible in theory that the webcontents stay the same size (no window > > borders > > > > etc). > > > > > > > > > > for rotation, in the pathological case case, what if you have a square > > > phone? > > > > > > > > I'm comfortable assuming that fullscreen and rotation transitions will > > always > > > > cause resize. These concerns are too theoretical -- let's not > overconstrain > > > the > > > > solution space unnecessarily, because this problem is difficult enough as > it > > > is. > > > > There have been thousands of models of Android devices and all of them to > > the > > > > best of my knowledge have a mandatory notification shade and are not > > perfectly > > > > square, and we probably have a fair amount of code implicitly relying on > > these > > > > properties already. > > > > > > Sure square phone is not realistic. But I'm not sure about notification > shade > > > thing. How many of the existing assumptions would completely break rendering > > if > > > it was wrong? > > > > > > On 2017/06/20 21:26:35, Khushal wrote: > > > > On 2017/06/20 20:54:44, boliu wrote: > > > > > also, how do you know entering/existing fullscreen will cause a resize? > > it's > > > > > possible in theory that the webcontents stay the same size (no window > > > borders > > > > > etc). > > > > > > > > Yup, need a more explicit way to handle the resize. > > > > > > > > > > > > > > for rotation, in the pathological case case, what if you have a square > > > phone? > > > > > > > > The primary cases where the rotation transition looks bad is where there > is > > a > > > > mismatch in portrait vs landscape sizes and the video suddenly jumps from > a > > > > corner on the screen to fullscreen. In the case where these sizes match, > not > > > > entering the transition should look fine IMO. > > > > > > > > > > > > > > I don't think there's any guarantee that the fullscreen state converges > > > > either. > > > > > > > > What would be the case where the fullscreen states don't converge? > > > > > > Glanced at the cc change. I don't think setting fullscreen actually > guarantees > > a > > > frame will be produced. It's all side effects of other code.. > > > > The place where that bit is being set doesn't require a commit because its > done > > where the renderer is granted fullscreen so the bit will come whenever the > first > > fullscreen frame is produced. Its similar to the background color change. > > Err... that makes me think background color is wrong too, not the other way > around. Hmmm, may be it should be asking for a commit... > > Btw, if you want something to go through renderer compositor, but not > necessarily trigger swap or any additional work that's otherwise not needed, you > want SwapPromise. Looks like this might be a good fit (assuming if you don't > take the transparent background advice) I wanted this state to be bundled with the frame so we can use it to decide whether to use a frame on the browser or not so the metadata was the best place to put it. SwapPromise wouldn't give that. The reason why the transparent bg wouldn't work was because blink uses that only for the path where a separate SurfaceView is used for videos and we wanted a signal to capture all fullscreen cases. But since the fullscreen size is different on Chrome, do we just want to use that to decide that the renderer frame is or is not fullscreen and evict frames if there is a mismatch? > > > > > > > > > > > > > > > > > > > > overall, I think this code is too brittle, it's adding > > > > assumptions/dependencies > > > > > between a whole bunch of largely independent signals, with no tests. no > > one > > > is > > > > > going to understand this in a year or two, and it'll probably just get > > > removed > > > > > > > > > > https://codereview.chromium.org/2810813004/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2038: rwhvb->OnFullscreenStateChanged(true); I think we were earlier using IsFullscreenModeForTab here but that was breaking in the FullscreenActivity case. Presumably because the transition in that case is async so IsFullscreenForCurrentTab returns false. I feel like we should fix that and be using IsFullscreenForCurrentTab consistently. Because otherwise the callback dispatched to observers below would have incorrect state.
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/20 23:25:05, Khushal wrote: > WebView was what I was most unsure about. Is a resize guaranteed there? Nope. (It's more likely the app doesn't implement fullscreen, in which case the request to go fullscreen will just be ignored). But webview doesn't have a browser compositor, and EvictDelegatedFrame here does nothing on webview, so it's kind of moot anyway. > > For Chrome whichever of these 2 you think will be a better assumption to make in > this code would be fine. Hmm? > Sure, the pending state can be set before the transition is initiated (in > WebContentsImpl). The reason it works currently is because the resize always > comes asynchronously from android. Though WebContentsImpl uses > IsFullscreenForTab after requesting the transition to know if it was granted. Is > there a case where that request could fail? Webview: yes. Chrome: dunno? > The empty frame doesn't get committed/swapped until the next frame is requested > by the Display. That behavior won't change. Wait, I don't get that part. Display as in cc::Display, right? Chrome is has a push model, frames are pushed from rendere to display. Do you mean display can stops sending begin frames? Does that actually happen during fullscreen transition?
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/20 23:53:51, boliu wrote: > On 2017/06/20 23:25:05, Khushal wrote: > > WebView was what I was most unsure about. Is a resize guaranteed there? > > Nope. (It's more likely the app doesn't implement fullscreen, in which case the > request to go fullscreen will just be ignored). But webview doesn't have a > browser compositor, and EvictDelegatedFrame here does nothing on webview, so > it's kind of moot anyway. Oh right. WebView doesn't get any frames at all here. It has its own frame sink. > > > > > For Chrome whichever of these 2 you think will be a better assumption to make > in > > this code would be fine. > > Hmm? I meant between assuming that a resize will follow, or assuming the screen size should match the expected fullscreen size. > > > Sure, the pending state can be set before the transition is initiated (in > > WebContentsImpl). The reason it works currently is because the resize always > > comes asynchronously from android. Though WebContentsImpl uses > > IsFullscreenForTab after requesting the transition to know if it was granted. > Is > > there a case where that request could fail? > > Webview: yes. Chrome: dunno? > > > The empty frame doesn't get committed/swapped until the next frame is > requested > > by the Display. That behavior won't change. > > Wait, I don't get that part. Display as in cc::Display, right? Chrome is has a > push model, frames are pushed from rendere to display. Do you mean display can > stops sending begin frames? Does that actually happen during fullscreen > transition? I meant that the frame update will happen on a different task, whether you evict the frame before initiating the transition or after. The frame eviction will have to go through a browser compositor commit since it will need a new CompositorFrame which does not embed the Surface for the renderer's frame.
https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1210: fullscreen_transition_awaiting_resize_ = true; On 2017/06/21 00:08:25, Khushal wrote: > > > > > > For Chrome whichever of these 2 you think will be a better assumption to > make > > in > > > this code would be fine. > > > > Hmm? > > I meant between assuming that a resize will follow, or assuming the screen size > should match the expected fullscreen size. I'd prefer neither. But resize is probably safer of the two. I think on older versions of android, there was no way to actually query the display size, and ui::display just returns the size minus the borders which presumably would disappear in fullscreen.
The CQ bit was checked by steimel@chromium.org to run a CQ dry run
https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:602: // TODO(steimel): Set the color based on the color of the renderer frame. On 2017/06/16 22:46:40, aelias wrote: > This is done in OnFrameMetadataUpdated right now. Please refactor this so that > there's a unified logic that calls SetBackgroundColor by considering all the > current state instead of having these different subsystems fight each other for > authority over it. Done. https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1247: EvictDelegatedFrame(); On 2017/06/17 01:18:34, aelias wrote: > On 2017/06/16 at 23:24:12, steimel wrote: > > On 2017/06/16 22:46:40, aelias wrote: > > > This only needs to happen in SubmitCompositorFrame(), right? Please move > this > > > out of here and at the end of SubmitCompositorFrame(), explicitly do "if > > > (IsInFullscreenTransition()) EvictDelegatedFrame();". > > > > No, this needs to happen regardless of where UpdateFullscreenState is called, > since we don't want to wait until the next frame to start showing a black screen > > I'd still rather it be broken out and called in multiple places because this > meaty substantive result of the method is easy to miss when mixed in with all > the other logic, and I think it makes more sense for UpdateFullscreenState to > just passively update the state machine. Also, is it needed to call it on > physical backing size change? Done. https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1241: EvictDelegatedFrame(); On 2017/06/20 20:54:44, boliu wrote: > need a comment why this is desirable Done. https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:409: virtual void OnFullscreenStateChanged(bool is_fullscreen) {} On 2017/06/20 20:54:44, boliu wrote: > how is this different from the is_fullscreen thing on line 434? Right now, the is_fullscreen stuff is used in RenderWidgetHostViewAura to keep track of when it's initialized as fullscreen. This here is an event handler to give a way for WebContents to notify us of fullscreen changes. I could potentially re-use the is_fullscreen_ bool to store the result (as opposed to web_contents_is_fullscreen_), but I'm not sure of the side-effects and don't want to make one bool do two things even if for two different subclasses. https://codereview.chromium.org/2810813004/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2038: rwhvb->OnFullscreenStateChanged(true); On 2017/06/20 23:25:05, Khushal wrote: > I think we were earlier using IsFullscreenModeForTab here but that was breaking > in the FullscreenActivity case. Presumably because the transition in that case > is async so IsFullscreenForCurrentTab returns false. > > I feel like we should fix that and be using IsFullscreenForCurrentTab > consistently. Because otherwise the callback dispatched to observers below would > have incorrect state. Yeah, I think some of the issues with the FullscreenActivity feature (it was enabled and then disabled after seeing issues) stem from this problem. I emailed the owner of that but they're OOO for now. https://codereview.chromium.org/2810813004/diff/240001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/public/common/... content/public/common/content_features.cc:115: "HideFullscreenTransitionJank", base::FEATURE_ENABLED_BY_DEFAULT}; On 2017/06/20 20:54:44, boliu wrote: > is this necessary? are you planning on running an experiment or something? Good question. This was requested by mlamouri@, but has now been removed in the latest patch, since there are cleanup changes in this CL that would cause regressions if the feature was disabled. If some people still feel that this change necessitates a feature flag I can re-add this.
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
Thanks for the fixes. https://codereview.chromium.org/2810813004/diff/260001/chrome/browser/android... File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc (right): https://codereview.chromium.org/2810813004/diff/260001/chrome/browser/android... chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:90: const JavaParamRef<jobject>& jobj, Now that you no longer need to touch these files, please remove the "git cl format" changes from the patch to unclutter the code review and git blame history. https://codereview.chromium.org/2810813004/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:456: bool fullscreen_transition_awaiting_resize_ = false; Ping again when you make progress on removing this and/or is_fullscreen, which are the hard open question. We don't have an exact idea how best to fix it and the only thing me and boliu@ agree on is that they should and somehow can be done more cleanly. I suggest trying out different options out of the menu we've offered until you find one that works reliably (it's hard for me to know just by theorycrafting about it). Let us know if it would help to have a short VC about it if you're stuck or wondering if a given solution would be approved of (afternoon times work better for me).
didn't look at everything, but looks like that giant state machine is still there, without tests.. https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:409: virtual void OnFullscreenStateChanged(bool is_fullscreen) {} On 2017/06/22 01:32:11, steimel wrote: > On 2017/06/20 20:54:44, boliu wrote: > > how is this different from the is_fullscreen thing on line 434? > > Right now, the is_fullscreen stuff is used in RenderWidgetHostViewAura to keep > track of when it's initialized as fullscreen. This here is an event handler to > give a way for WebContents to notify us of fullscreen changes. I could > potentially re-use the is_fullscreen_ bool to store the result (as opposed to > web_contents_is_fullscreen_), but I'm not sure of the side-effects and don't > want to make one bool do two things even if for two different subclasses. My problem is that this is just too confusing. Having two "fullscreen" things in one class that are not referring to the same thing. Looks like is_fullscreen_ is only used by aura. RenderWidgetHostViewEventHandler is an aura-only class too, so maybe it can use RWHVAura directly. So then is_fullscreen_ can be moved into RWHVAura? Separate CL though Also I concur with the other comment, should use RWHDelegate::IsFullscreenForCurrentTab instead of adding this.
It will be hard to test if you have this logic in RWHVA. Probably move this to a helper class with a client interface for evicting frames so you can unit test that class. https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:409: virtual void OnFullscreenStateChanged(bool is_fullscreen) {} On 2017/06/23 01:51:42, boliu wrote: > On 2017/06/22 01:32:11, steimel wrote: > > On 2017/06/20 20:54:44, boliu wrote: > > > how is this different from the is_fullscreen thing on line 434? > > > > Right now, the is_fullscreen stuff is used in RenderWidgetHostViewAura to keep > > track of when it's initialized as fullscreen. This here is an event handler to > > give a way for WebContents to notify us of fullscreen changes. I could > > potentially re-use the is_fullscreen_ bool to store the result (as opposed to > > web_contents_is_fullscreen_), but I'm not sure of the side-effects and don't > > want to make one bool do two things even if for two different subclasses. > > My problem is that this is just too confusing. Having two "fullscreen" things in > one class that are not referring to the same thing. > > Looks like is_fullscreen_ is only used by aura. RenderWidgetHostViewEventHandler > is an aura-only class too, so maybe it can use RWHVAura directly. So then > is_fullscreen_ can be moved into RWHVAura? Separate CL though > > Also I concur with the other comment, should use > RWHDelegate::IsFullscreenForCurrentTab instead of adding this. Agreed. I think the function will still be needed to notify the view when the fullscreen state changes but the current state can be queried using RWHDelegate method. https://codereview.chromium.org/2810813004/diff/260001/chrome/browser/android... File chrome/browser/android/compositor/compositor_view.cc (right): https://codereview.chromium.org/2810813004/diff/260001/chrome/browser/android... chrome/browser/android/compositor/compositor_view.cc:188: root_layer_->SetBackgroundColor(color); Another thing, this root_layer_ doesn't need to be a solid color layer now. It can be a simple layer. https://codereview.chromium.org/2810813004/diff/260001/content/public/browser... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/2810813004/diff/260001/content/public/browser... content/public/browser/android/compositor.h:74: // Sets the background color of the SceneLayer. nit: There is no scene layer here. "color of the compositor" is fine too.
PTAL. I've pulled out the logic into a separate class and added unit tests. I'm not the best namer so let me know if you have a better name for this class :). https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_base.h (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/render... content/browser/renderer_host/render_widget_host_view_base.h:409: virtual void OnFullscreenStateChanged(bool is_fullscreen) {} On 2017/06/23 03:09:50, Khushal wrote: > On 2017/06/23 01:51:42, boliu wrote: > > On 2017/06/22 01:32:11, steimel wrote: > > > On 2017/06/20 20:54:44, boliu wrote: > > > > how is this different from the is_fullscreen thing on line 434? > > > > > > Right now, the is_fullscreen stuff is used in RenderWidgetHostViewAura to > keep > > > track of when it's initialized as fullscreen. This here is an event handler > to > > > give a way for WebContents to notify us of fullscreen changes. I could > > > potentially re-use the is_fullscreen_ bool to store the result (as opposed > to > > > web_contents_is_fullscreen_), but I'm not sure of the side-effects and don't > > > want to make one bool do two things even if for two different subclasses. > > > > My problem is that this is just too confusing. Having two "fullscreen" things > in > > one class that are not referring to the same thing. > > > > Looks like is_fullscreen_ is only used by aura. > RenderWidgetHostViewEventHandler > > is an aura-only class too, so maybe it can use RWHVAura directly. So then > > is_fullscreen_ can be moved into RWHVAura? Separate CL though > > > > Also I concur with the other comment, should use > > RWHDelegate::IsFullscreenForCurrentTab instead of adding this. > > Agreed. I think the function will still be needed to notify the view when the > fullscreen state changes but the current state can be queried using RWHDelegate > method. Changed to not have is_fullscreen param, but as Khushal mentioned this function is still necessary for RWHVA to get notified of changes to fullscreen state https://codereview.chromium.org/2810813004/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2810813004/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2038: rwhvb->OnFullscreenStateChanged(true); On 2017/06/22 01:32:11, steimel wrote: > On 2017/06/20 23:25:05, Khushal wrote: > > I think we were earlier using IsFullscreenModeForTab here but that was > breaking > > in the FullscreenActivity case. Presumably because the transition in that case > > is async so IsFullscreenForCurrentTab returns false. > > > > I feel like we should fix that and be using IsFullscreenForCurrentTab > > consistently. Because otherwise the callback dispatched to observers below > would > > have incorrect state. > > Yeah, I think some of the issues with the FullscreenActivity feature (it was > enabled and then disabled after seeing issues) stem from this problem. I emailed > the owner of that but they're OOO for now. After discussing with Mounir, I'm removing the hack of assuming true and instead disabling the FullscreenWebContentsActivity tests that were failing https://codereview.chromium.org/2810813004/diff/260001/chrome/browser/android... File chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc (right): https://codereview.chromium.org/2810813004/diff/260001/chrome/browser/android... chrome/browser/android/compositor/scene_layer/tab_list_scene_layer.cc:90: const JavaParamRef<jobject>& jobj, On 2017/06/22 02:52:05, aelias wrote: > Now that you no longer need to touch these files, please remove the "git cl > format" changes from the patch to unclutter the code review and git blame > history. Done. https://codereview.chromium.org/2810813004/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:456: bool fullscreen_transition_awaiting_resize_ = false; On 2017/06/22 02:52:06, aelias wrote: > Ping again when you make progress on removing this and/or is_fullscreen, which > are the hard open question. > > We don't have an exact idea how best to fix it and the only thing me and boliu@ > agree on is that they should and somehow can be done more cleanly. I suggest > trying out different options out of the menu we've offered until you find one > that works reliably (it's hard for me to know just by theorycrafting about it). > Let us know if it would help to have a short VC about it if you're stuck or > wondering if a given solution would be approved of (afternoon times work better > for me). According to Khushal, the discussion on crbug.com/736092 has led to the conclusion that we should keep both the fullscreen metadata and resizing checks. Let me know if I've misinterpreted https://codereview.chromium.org/2810813004/diff/260001/content/public/browser... File content/public/browser/android/compositor.h (right): https://codereview.chromium.org/2810813004/diff/260001/content/public/browser... content/public/browser/android/compositor.h:74: // Sets the background color of the SceneLayer. On 2017/06/23 03:09:51, Khushal wrote: > nit: There is no scene layer here. "color of the compositor" is fine too. Done.
The CQ bit was checked by steimel@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2810813004/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/2810813004/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.h:456: bool fullscreen_transition_awaiting_resize_ = false; On 2017/06/29 at 00:38:51, steimel wrote: > On 2017/06/22 02:52:06, aelias wrote: > > Ping again when you make progress on removing this and/or is_fullscreen, which > > are the hard open question. > > > > We don't have an exact idea how best to fix it and the only thing me and boliu@ > > agree on is that they should and somehow can be done more cleanly. I suggest > > trying out different options out of the menu we've offered until you find one > > that works reliably (it's hard for me to know just by theorycrafting about it). > > Let us know if it would help to have a short VC about it if you're stuck or > > wondering if a given solution would be approved of (afternoon times work better > > for me). > > According to Khushal, the discussion on crbug.com/736092 has led to the conclusion that we should keep both the fullscreen metadata and resizing checks. Let me know if I've misinterpreted The initial concerns expressed by me and Bo didn't have to do with Viz. The discussion on crbug.com/736092 concluded that there are no additional constraints related to Viz, but it didn't clear them up. There's a complexity problem in that you're combining awaiting_resize, is_fullscreen, and frame size to arrive at a decision. Basically you have three low-signals you're combining to arrive at a high-quality one. That means this code depends on subtleties of many different faraway subsystems all at once -- it is brittle and hard to understand, and the contract that outside code is supposed to abide by in the future to interact with it is not clearly defined. There has to be a simpler way to achieve your goal. Please improve the quality (i.e. timing) of one or more of the signals at its source to remove at least one of the three.
The CQ bit was checked by steimel@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...
Okay, I removed awaiting_resize_ by essentially moving the resizing assumption inside of FullscreenController.cpp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
As discussed offline with steimel@, I'm a bit worried about landing this without a Feature flag as it means we would need to hard revert the change if it happens to regress something (can't use Finch). Now that there is a separate FullscreenJankDetector, how hard would it be to have the behaviour behind a flag? I understand that there is logic leaving outside of FullscreenJankDetector but how much of this would need to leave behind the flag?
The CQ bit was checked by steimel@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_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by steimel@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...
PTAL :) So the fact that we extracted the logic into a helper class doesn't make it any harder or easier to have the behavior itself behind a flag (it would just live in a different spot than before). I would also need to re-add the old behaviors I removed in chrome/browser/android/compositor/compositor_view.cc and content/browser/renderer_host/compositor_impl_android.cc and hide those with the flag as well with a TODO to remove once we remove the flag. It's not a big effort or anything, but it does add some future cleanup to do. Should I go ahead and add the flag back in? Regarding the discussion on making Entering/Exiting fullscreen not depend on size changes: we still need the size check, as without it we see some jank before the size updates to take the bottom control bar disappearing into account.
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 steimel@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...
I went ahead and added the feature flag back. Let me know if there's anything else that needs to be done before this CL is good to go. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm :) https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1208: // fullscreen transition hiding is working correctly. I would rephrase the TODO to say that we should remove the flag when the feature has launched and is stable. https://codereview.chromium.org/2810813004/diff/360001/content/public/common/... File content/public/common/content_features.cc (right): https://codereview.chromium.org/2810813004/diff/360001/content/public/common/... content/public/common/content_features.cc:126: "HideFullscreenTransitionJank", base::FEATURE_ENABLED_BY_DEFAULT}; You might want to make it DISABLED_BY_DEFAULT as I don't think we can launch this in M61. I think turning it on by default after branch would be a safer idea. Meanwhile, we can turn it on in Canary/Dev using Finch.
Ok, spent a few hours this morning learning how the sequence of events for fullscreen works, with help from Khushal. I think this can be *significantly* simplified. I think there are two problems you are trying to solve at the same time: 1) browser ack fullscreen too early, and blink can produce a frame of "fullscreen" content, but not at the final fullscreen size 2) general resize problem, ie resize is (more or less) synchronous on the UI thread, but getting a frame with new size is asynchronous and slow, so what to render while there is a size mismatch Note if you only want to handle the rotation case (only case mentioned in the bug), then you don't need to solve 1). The correct way to solve 1) is to make sure TabWebContentsDelegateAndroid.isFullscreenForTabOrPending does *not* return true until the corresponding resize has happened. TabWebContentsDelegateAndroid is in a much better position to decide whether resize has already happened (or whether resize will happen at all). Eg, looks like FullscreenHtmlApiHandler.enterFullscreen already has a layout listener to specifically handle this. 2) is always a trade off of course. So you would like the policy (at least during fullscreen) to handle mismatched size is to just drop the frame and render background color. I wonder it's ok to use that policy for all mismatch cases, and not just limit it to fullscreen. If not, having chrome tell content the policy is fine as well. Does that make sense? I think that solves everything, and much simpler. Sorry this is basically asking you to rewrite the whole thing, but I really think the maintenance burden for the current form is too high. And I guess most of the inline comments are also irrelevant. https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... File content/browser/renderer_host/fullscreen_jank_detector.cc (right): https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... content/browser/renderer_host/fullscreen_jank_detector.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... content/browser/renderer_host/fullscreen_jank_detector.cc:24: frame_size_ = frame_size; this frame size may not be the "final" size, even if frame_is_fullscreen is in its final state. that's because browser side acks the fullscreen request *before* the actual resize has happened (ie backing size changed here). So: * renderer request fullscreen * browser receives request, sends ack to renderer, and at the same time, request fullscreen then these two things races: a) renderer receives ack, and pushes a frame fullscreen set, but potentially wrong size b) actual resize happens if a) happens first, then you can still get an unwanted bad frame in here I am aware that right now you have code in blink to wait for the resize before setting fullscreen, so this can't actually happen. But I don't think that's acceptable blink code, since there is definitely code that specifically handles fullscreen that does *not* involve a resize. https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... content/browser/renderer_host/fullscreen_jank_detector.cc:54: case FullscreenState::kExitingFullscreen: looks like these three states can just be merged? https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... File content/browser/renderer_host/fullscreen_jank_detector.h (right): https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... content/browser/renderer_host/fullscreen_jank_detector.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... content/browser/renderer_host/fullscreen_jank_detector.h:17: class CONTENT_EXPORT FullscreenJankDetector { I get really picky about the word "jank", because it's pretty ambiguous outside of people working on perf, but for those who do, jank is definitely the wrong word here. how about FullscreenTransitionStateMachine? https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... content/browser/renderer_host/fullscreen_jank_detector.h:19: FullscreenJankDetector(){}; space after (), and no semicolon at the end https://codereview.chromium.org/2810813004/diff/360001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/2810813004/diff/360001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:2039: RenderWidgetHostViewBase* rwhvb = chaining these two calls through web contents does not make sense, this could be talking to the entirely wrong render widget RenderFrameHostImpl::OnToggleFullscreen should call things on its widget directly using render_view_host_->GetWidget()->GetView() https://codereview.chromium.org/2810813004/diff/360001/ui/android/view_android.h File ui/android/view_android.h (right): https://codereview.chromium.org/2810813004/diff/360001/ui/android/view_androi... ui/android/view_android.h:133: void ClearThumbnailPlaceholder(); this has nothing to do with ViewAndroid tree and feels like a layering violation too, since thumbnail placeholder is an entirely chrome/ concept, so lower layers like content/ or ui/ should not know about it. shouldn't chrome layer know to do this already, whenever it enters fullscreen? If that doesn't work, then need to name this to "what happened" rather than "do this", maybe something like DidEvictFrameForFullscreenTransition
On 2017/07/05 17:39:49, boliu wrote: > Ok, spent a few hours this morning learning how the sequence of events for > fullscreen works, with help from Khushal. I think this can be *significantly* > simplified. > > I think there are two problems you are trying to solve at the same time: > 1) browser ack fullscreen too early, and blink can produce a frame of > "fullscreen" content, but not at the final fullscreen size > 2) general resize problem, ie resize is (more or less) synchronous on the UI > thread, but getting a frame with new size is asynchronous and slow, so what to > render while there is a size mismatch > > Note if you only want to handle the rotation case (only case mentioned in the > bug), then you don't need to solve 1). > > The correct way to solve 1) is to make sure > TabWebContentsDelegateAndroid.isFullscreenForTabOrPending does *not* return true > until the corresponding resize has happened. TabWebContentsDelegateAndroid is in > a much better position to decide whether resize has already happened (or whether > resize will happen at all). Eg, looks like > FullscreenHtmlApiHandler.enterFullscreen already has a layout listener to > specifically handle this. > > 2) is always a trade off of course. So you would like the policy (at least > during fullscreen) to handle mismatched size is to just drop the frame and > render background color. I wonder it's ok to use that policy for all mismatch > cases, and not just limit it to fullscreen. If not, having chrome tell content > the policy is fine as well. > > Does that make sense? I think that solves everything, and much simpler. Sorry > this is basically asking you to rewrite the whole thing, but I really think the > maintenance burden for the current form is too high. And I guess most of the > inline comments are also irrelevant. > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > File content/browser/renderer_host/fullscreen_jank_detector.cc (right): > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > content/browser/renderer_host/fullscreen_jank_detector.cc:1: // Copyright (c) > 2017 The Chromium Authors. All rights reserved. > no (c) > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > content/browser/renderer_host/fullscreen_jank_detector.cc:24: frame_size_ = > frame_size; > this frame size may not be the "final" size, even if frame_is_fullscreen is in > its final state. > > that's because browser side acks the fullscreen request *before* the actual > resize has happened (ie backing size changed here). So: > * renderer request fullscreen > * browser receives request, sends ack to renderer, and at the same time, request > fullscreen > then these two things races: > a) renderer receives ack, and pushes a frame fullscreen set, but potentially > wrong size > b) actual resize happens > > if a) happens first, then you can still get an unwanted bad frame in here > > I am aware that right now you have code in blink to wait for the resize before > setting fullscreen, so this can't actually happen. But I don't think that's > acceptable blink code, since there is definitely code that specifically handles > fullscreen that does *not* involve a resize. > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > content/browser/renderer_host/fullscreen_jank_detector.cc:54: case > FullscreenState::kExitingFullscreen: > looks like these three states can just be merged? > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > File content/browser/renderer_host/fullscreen_jank_detector.h (right): > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > content/browser/renderer_host/fullscreen_jank_detector.h:1: // Copyright (c) > 2017 The Chromium Authors. All rights reserved. > no (c) > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > content/browser/renderer_host/fullscreen_jank_detector.h:17: class > CONTENT_EXPORT FullscreenJankDetector { > I get really picky about the word "jank", because it's pretty ambiguous outside > of people working on perf, but for those who do, jank is definitely the wrong > word here. > > how about FullscreenTransitionStateMachine? > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/render... > content/browser/renderer_host/fullscreen_jank_detector.h:19: > FullscreenJankDetector(){}; > space after (), and no semicolon at the end > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/web_co... > File content/browser/web_contents/web_contents_impl.cc (right): > > https://codereview.chromium.org/2810813004/diff/360001/content/browser/web_co... > content/browser/web_contents/web_contents_impl.cc:2039: > RenderWidgetHostViewBase* rwhvb = > chaining these two calls through web contents does not make sense, this could be > talking to the entirely wrong render widget > > RenderFrameHostImpl::OnToggleFullscreen should call things on its widget > directly using render_view_host_->GetWidget()->GetView() > > https://codereview.chromium.org/2810813004/diff/360001/ui/android/view_android.h > File ui/android/view_android.h (right): > > https://codereview.chromium.org/2810813004/diff/360001/ui/android/view_androi... > ui/android/view_android.h:133: void ClearThumbnailPlaceholder(); > this has nothing to do with ViewAndroid tree > > and feels like a layering violation too, since thumbnail placeholder is an > entirely chrome/ concept, so lower layers like content/ or ui/ should not know > about it. > > shouldn't chrome layer know to do this already, whenever it enters fullscreen? > If that doesn't work, then need to name this to "what happened" rather than "do > this", maybe something like DidEvictFrameForFullscreenTransition i did some experimentation the other day: https://docs.google.com/a/chromium.org/document/d/1sWDe0UlP7zQkPgI1oRsa3vWyij... TL;DR: +1 on boliu@'s "browser currently acks too early". chrome has all the info it needs to avoid the resize messages that cause all the trouble. thanks -fl
On 2017/07/05 17:52:25, liberato wrote: > i did some experimentation the other day: > https://docs.google.com/a/chromium.org/document/d/1sWDe0UlP7zQkPgI1oRsa3vWyij... > > TL;DR: +1 on boliu@'s "browser currently acks too early". chrome has all the > info it needs to avoid the resize messages that cause all the trouble. Also.. the money quote for why content code acks as early as it can. It's to deal with resize potentially not happening! https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_...
The CQ bit was checked by steimel@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...
PTAL. Thanks for all the comments and research. For this CL, I'm now just solving the fullscreen rotation case (and not entering/exiting) since that is the simpler and less controversial (hopefully) case to solve. The entering/exiting cases will be tackled in a separate CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
content lgtm you still need someone to look over chrome. Also please format your CL description better: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
very nice. -fl
https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1205: current_surface_size_ != view_.GetPhysicalBackingSize()) { Could this be behind a feature flag?
https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1205: current_surface_size_ != view_.GetPhysicalBackingSize()) { On 2017/07/12 10:36:16, mlamouri (slow - plz ping) wrote: > Could this be behind a feature flag? not necessarily against it, but why? and why only this part of the CL?
chrome/ lgtm % mlamouri's nit
This will still have some effect in the entering fullscreen transition since it will trigger a frame eviction immediately after the resize (just not as early as the previous patch), but not the exiting transition. I hope that does not look odd or inconsistent. https://codereview.chromium.org/2810813004/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2810813004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2553: public void clearThumbnailPlaceholder() { You don't need this public function. Use the native function directly in toggleFullscreenMode?
https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1205: current_surface_size_ != view_.GetPhysicalBackingSize()) { On 2017/07/12 at 17:08:01, boliu wrote: > On 2017/07/12 10:36:16, mlamouri (slow - plz ping) wrote: > > Could this be behind a feature flag? > > not necessarily against it, but why? and why only this part of the CL? In general, I'm a bit worried about introducing this change so close to the branch and after feature freeze. I talked offline with steimel@ and we decided to land this as is at it doesn't require more l-g-t-m and decide next week before branch if we want to turn the feature off in M61 or not.
Description was changed from ========== In order to hide the jank that is shown during fullscreen transitions, show a black frame during transitions. This includes transitioning into and out of fullscreen as well as rotations while in fullscreen mode. BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== In order to hide the jank that is shown during fullscreen rotations, evict the frame and show a black screen while in fullscreen when the frame size doesn't match the physical backing size. This change also clears the thumbnail cache of the tab when entering fullscreen so that the thumbnail isn't shown when the frame is evicted. BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== In order to hide the jank that is shown during fullscreen rotations, evict the frame and show a black screen while in fullscreen when the frame size doesn't match the physical backing size. This change also clears the thumbnail cache of the tab when entering fullscreen so that the thumbnail isn't shown when the frame is evicted. BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Hide fullscreen rotation jank on Android In order to hide the jank that is shown during fullscreen rotations, evict the frame and show a black screen while in fullscreen when the frame size doesn't match the physical backing size. This change also clears the thumbnail cache of the tab when entering fullscreen so that the thumbnail isn't shown when the frame is evicted. BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by steimel@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...
Yes, this does temporarily show a black screen while entering fullscreen, but it seems to look fine to me in the sites that I've tested (cnn, youtube, vimeo, espn) https://codereview.chromium.org/2810813004/diff/380001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2810813004/diff/380001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:2553: public void clearThumbnailPlaceholder() { On 2017/07/12 17:15:39, Khushal wrote: > You don't need this public function. Use the native function directly in > toggleFullscreenMode? Done.
lgtm.
https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1205: current_surface_size_ != view_.GetPhysicalBackingSize()) { On 2017/07/12 17:29:54, mlamouri (slow - plz ping) wrote: > On 2017/07/12 at 17:08:01, boliu wrote: > > On 2017/07/12 10:36:16, mlamouri (slow - plz ping) wrote: > > > Could this be behind a feature flag? > > > > not necessarily against it, but why? and why only this part of the CL? > > In general, I'm a bit worried about introducing this change so close to the > branch and after feature freeze. I talked offline with steimel@ and we decided > to land this as is at it doesn't require more l-g-t-m and decide next week > before branch if we want to turn the feature off in M61 or not. Does turn off feature flag involve a CL? because you could just as easily revert this whole thing, now that it's much smaller. I just have a general wariness for making conditionals more complicated, and for obsolete switches and feature flags lying around.
On 2017/07/12 at 20:16:16, boliu wrote: > https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_android.cc:1205: current_surface_size_ != view_.GetPhysicalBackingSize()) { > On 2017/07/12 17:29:54, mlamouri (slow - plz ping) wrote: > > On 2017/07/12 at 17:08:01, boliu wrote: > > > On 2017/07/12 10:36:16, mlamouri (slow - plz ping) wrote: > > > > Could this be behind a feature flag? > > > > > > not necessarily against it, but why? and why only this part of the CL? > > > > In general, I'm a bit worried about introducing this change so close to the > > branch and after feature freeze. I talked offline with steimel@ and we decided > > to land this as is at it doesn't require more l-g-t-m and decide next week > > before branch if we want to turn the feature off in M61 or not. > > Does turn off feature flag involve a CL? because you could just as easily revert this whole thing, now that it's much smaller. > > I just have a general wariness for making conditionals more complicated, and for obsolete switches and feature flags lying around. Feature flags can be interacted with via Finch while reverting requires spinning a new build.
On 2017/07/12 20:21:17, mlamouri (slow - plz ping) wrote: > On 2017/07/12 at 20:16:16, boliu wrote: > > > https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... > > File content/browser/renderer_host/render_widget_host_view_android.cc (right): > > > > > https://codereview.chromium.org/2810813004/diff/380001/content/browser/render... > > content/browser/renderer_host/render_widget_host_view_android.cc:1205: > current_surface_size_ != view_.GetPhysicalBackingSize()) { > > On 2017/07/12 17:29:54, mlamouri (slow - plz ping) wrote: > > > On 2017/07/12 at 17:08:01, boliu wrote: > > > > On 2017/07/12 10:36:16, mlamouri (slow - plz ping) wrote: > > > > > Could this be behind a feature flag? > > > > > > > > not necessarily against it, but why? and why only this part of the CL? > > > > > > In general, I'm a bit worried about introducing this change so close to the > > > branch and after feature freeze. I talked offline with steimel@ and we > decided > > > to land this as is at it doesn't require more l-g-t-m and decide next week > > > before branch if we want to turn the feature off in M61 or not. > > > > Does turn off feature flag involve a CL? because you could just as easily > revert this whole thing, now that it's much smaller. > > > > I just have a general wariness for making conditionals more complicated, and > for obsolete switches and feature flags lying around. > > Feature flags can be interacted with via Finch while reverting requires spinning > a new build. Ok then. Just remember the remove it when done
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 steimel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, mlamouri@chromium.org, boliu@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/2810813004/#ps400001 (title: "Remove clearThumbnailPlaceholder fn")
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": 400001, "attempt_start_ts": 1499892654740340,
"parent_rev": "7e67623bd0d6d4c8b4857c9ca481138bda67ae63", "commit_rev":
"88f9d604d77b922af3117d72a2433a67ccdf1a46"}
Message was sent while issue was closed.
Description was changed from ========== Hide fullscreen rotation jank on Android In order to hide the jank that is shown during fullscreen rotations, evict the frame and show a black screen while in fullscreen when the frame size doesn't match the physical backing size. This change also clears the thumbnail cache of the tab when entering fullscreen so that the thumbnail isn't shown when the frame is evicted. BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Hide fullscreen rotation jank on Android In order to hide the jank that is shown during fullscreen rotations, evict the frame and show a black screen while in fullscreen when the frame size doesn't match the physical backing size. This change also clears the thumbnail cache of the tab when entering fullscreen so that the thumbnail isn't shown when the frame is evicted. BUG=710243 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2810813004 Cr-Commit-Position: refs/heads/master@{#486080} Committed: https://chromium.googlesource.com/chromium/src/+/88f9d604d77b922af3117d72a243... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001) as https://chromium.googlesource.com/chromium/src/+/88f9d604d77b922af3117d72a243...
Message was sent while issue was closed.
On 2017/07/12 20:57:07, commit-bot: I haz the power wrote: > Committed patchset #21 (id:400001) as > https://chromium.googlesource.com/chromium/src/+/88f9d604d77b922af3117d72a243... fwiw, I don't see a feature flag..
Message was sent while issue was closed.
To clarify, I discussed with Mounir offline and we ended up on: "Land without the flag and we will decide next week if we want one, either to turn it off or if we think we might need to"
Message was sent while issue was closed.
On 2017/07/12 21:01:40, steimel wrote: > To clarify, I discussed with Mounir offline and we ended up on: > > "Land without the flag and we will decide next week if we want one, either to > turn it off or if we think we might need to" oh, misread earlier comment |
