Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
14999010:
Allows fullscreen to be triggered with the key events used for scrolling. (Closed)
|
Created:
7 years, 7 months ago by Jinsuk Kim Modified:
7 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, cc-bugs_chromium.org, jam, apatrick_chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAllows fullscreen to be triggered with the key events used for scrolling.
This gives consistency with how fullscreen is triggered with gesture-based
scrolling. Keys to trigger fullscreen with are based on the logic implemented
in WebViewImpl::keyEventDefault()/mapKeyCodeForScroll().
BUG=240159
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206386
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments #
Total comments: 10
Patch Set 3 : Addressed comments #
Total comments: 2
Patch Set 4 : Addressed comments #
Total comments: 12
Patch Set 5 : Addressed comments #
Total comments: 10
Patch Set 6 : Addressed comments #Patch Set 7 : addressed comments #Patch Set 8 : #
Total comments: 4
Patch Set 9 : addressed comments #Patch Set 10 : #
Total comments: 10
Patch Set 11 : addressed comments #
Total comments: 2
Patch Set 12 : addressed comments #
Total comments: 2
Patch Set 13 : move the decl into the right place #
Messages
Total messages: 26 (0 generated)
Hi Ted, I know you've been working this fullscreen stuff. I'm trying to get keyboard events to trigger the mode as well. Would you take a look to see if the approach taken here seems reasonable?
Gentle reminder...
On 2013/05/20 00:10:59, jindor wrote: > Gentle reminder... Ted is out of the office. I can take a look but depending on the change it might be best to wait for him to get back.
https://codereview.chromium.org/14999010/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/1/cc/trees/layer_tree_host.cc#n... cc/trees/layer_tree_host.cc:496: top_controls_manager_weak_ptr_->ScrollBegin(); This isn't thread-safe. LayerTreeHost functions are called on the main thread but the top controls manager is owned on the impl thread. I recommend waiting until https://codereview.chromium.org/14139013/ lands and making use of LayerTreeHost::UpdateTopControlsState() instead. You may need to keep an additional copy of the latest constraints state on the main thread. https://codereview.chromium.org/14999010/diff/1/cc/trees/layer_tree_host.cc#n... cc/trees/layer_tree_host.cc:504: return top_controls_manager_weak_ptr_->controls_top_offset(); Not thread-safe either. I don't think you really need this read anyway. https://codereview.chromium.org/14999010/diff/1/content/renderer/render_widge... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/14999010/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:847: if (key == ui::VKEY_UP || key == ui::VKEY_PRIOR || key == ui::VKEY_HOME) This is pretty complex and you're likely to miss some cases... I suggest instead observing whether the scroll offset moved up or down in reaction to the key event.
Thanks for the review. I could wait till Ted comes back but your comments make sense to me, which I addressed below. Please take another look. https://codereview.chromium.org/14999010/diff/1/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/1/cc/trees/layer_tree_host.cc#n... cc/trees/layer_tree_host.cc:496: top_controls_manager_weak_ptr_->ScrollBegin(); On 2013/05/21 05:51:41, aelias wrote: > This isn't thread-safe. LayerTreeHost functions are called on the main thread > but the top controls manager is owned on the impl thread. > > I recommend waiting until https://codereview.chromium.org/14139013/ lands and > making use of LayerTreeHost::UpdateTopControlsState() instead. You may need to > keep an additional copy of the latest constraints state on the main thread. Done. Instead of keeping an additional copy I added an overloaded method of UpdateTopControlsState that preserves the constraints. Please see if it looks fine. https://codereview.chromium.org/14999010/diff/1/cc/trees/layer_tree_host.cc#n... cc/trees/layer_tree_host.cc:504: return top_controls_manager_weak_ptr_->controls_top_offset(); On 2013/05/21 05:51:41, aelias wrote: > Not thread-safe either. I don't think you really need this read anyway. Right. removed. https://codereview.chromium.org/14999010/diff/1/content/renderer/render_widge... File content/renderer/render_widget.cc (right): https://codereview.chromium.org/14999010/diff/1/content/renderer/render_widge... content/renderer/render_widget.cc:847: if (key == ui::VKEY_UP || key == ui::VKEY_PRIOR || key == ui::VKEY_HOME) On 2013/05/21 05:51:41, aelias wrote: > This is pretty complex and you're likely to miss some cases... I suggest instead > observing whether the scroll offset moved up or down in reaction to the key > event. Done. Created a new CL on WebKit side for that https://codereview.chromium.org/15735021/
I do think the behavior might be slightly odd, but this is an unlikely enough use case that I will defer to you if you think this looks ok. lgtm on my end. But getting aelias or jamesr to sign off is certainly more important than I. https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:410: void LayerTreeHost::ShowTopControls(bool show) { Same thing about moving this next to the other method (matching the ordering of the .h file). https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:411: proxy_->ImplThread()->PostTask( Similar to the other method, I would add this to the start of the method: if (!settings_.calculate_top_controls_position) return; https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.h... cc/trees/layer_tree_host.h:255: void ShowTopControls(bool show); I would move this next to UpdateTopControlsState. Any reason not to have the same signature here as you do in TopControlsManager? https://codereview.chromium.org/14999010/diff/7001/content/renderer/gpu/rende... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/14999010/diff/7001/content/renderer/gpu/rende... content/renderer/gpu/render_widget_compositor.h:52: void ShowTopControls(bool show); move next to the UpdateTopControlsState here too (same in .cc) https://codereview.chromium.org/14999010/diff/7001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/7001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2209: if (delta.height != 0.f) Hmm...this behavior isn't really consistent with how touch events are processed. In the normal touch case, the top controls will consume the portion of the scroll total before it's applied to the root layer (bringing in or hiding the top controls before the page content moves). Here it will not be tied to that and will happen at the same time. It might be kind of odd, but I don't know if there is an easier way of handling that. I'm also worried that the top controls will pop in and out if you just toggle up and down a bit. I wonder if it would be better to send these scroll deltas to the top controls manager (moving the top controls by the same amount) and have it kick off an internal timer to animate if no further scrolls are received. Granted, I don't think most people will have a keyboard attached in the android world, so this is probably ok.
Thanks for the review. aelias would you take another look? I rebased the code, which might have made the change look a bit messier. Sorry for that. https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:410: void LayerTreeHost::ShowTopControls(bool show) { On 2013/06/03 17:43:42, Ted C wrote: > Same thing about moving this next to the other method (matching the ordering of > the .h file). Done. https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.c... cc/trees/layer_tree_host.cc:411: proxy_->ImplThread()->PostTask( On 2013/06/03 17:43:42, Ted C wrote: > Similar to the other method, I would add this to the start of the method: > > if (!settings_.calculate_top_controls_position) > return; Done. https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/14999010/diff/7001/cc/trees/layer_tree_host.h... cc/trees/layer_tree_host.h:255: void ShowTopControls(bool show); On 2013/06/03 17:43:42, Ted C wrote: > I would move this next to UpdateTopControlsState. Any reason not to have the > same signature here as you do in TopControlsManager? Done. Renamed to the same signature as in TopControlsManager. https://codereview.chromium.org/14999010/diff/7001/content/renderer/gpu/rende... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/14999010/diff/7001/content/renderer/gpu/rende... content/renderer/gpu/render_widget_compositor.h:52: void ShowTopControls(bool show); On 2013/06/03 17:43:42, Ted C wrote: > move next to the UpdateTopControlsState here too (same in .cc) Done. https://codereview.chromium.org/14999010/diff/7001/content/renderer/render_vi... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/7001/content/renderer/render_vi... content/renderer/render_view_impl.cc:2209: if (delta.height != 0.f) On 2013/06/03 17:43:42, Ted C wrote: > Hmm...this behavior isn't really consistent with how touch events are processed. > > In the normal touch case, the top controls will consume the portion of the > scroll total before it's applied to the root layer (bringing in or hiding the > top controls before the page content moves). Here it will not be tied to that > and will happen at the same time. It might be kind of odd, but I don't know if > there is an easier way of handling that. I'm also worried that the top controls > will pop in and out if you just toggle up and down a bit. > > I wonder if it would be better to send these scroll deltas to the top controls > manager (moving the top controls by the same amount) and have it kick off an > internal timer to animate if no further scrolls are received. > > Granted, I don't think most people will have a keyboard attached in the android > world, so this is probably ok. Thanks for spotting it. The transition does look different from that caused by gesture. Let me try to work on that part (plus animation) in other CL after I get familiar enough with the codebase.
https://codereview.chromium.org/14999010/diff/15001/cc/input/top_controls_man... File cc/input/top_controls_manager.h (right): https://codereview.chromium.org/14999010/diff/15001/cc/input/top_controls_man... cc/input/top_controls_manager.h:51: void UpdateTopControlsStatePreservingConstraints( Let's avoid adding this method. Please cache the latest constraints passed in RenderViewImpl::UpdateTopControlsState() and pass those into the existing UpdateTopControlsState(). You can safely assume the constraints won't be changed by anything else than that IPC.
I think a reasonable place to add the cache field would be LayerTreeHost, rather than RenderViewImpl. You can add a getter that just goes to LayerTreeHost instead of crossing the thread boundary.
https://codereview.chromium.org/14999010/diff/15001/cc/input/top_controls_man... File cc/input/top_controls_manager.h (right): https://codereview.chromium.org/14999010/diff/15001/cc/input/top_controls_man... cc/input/top_controls_manager.h:51: void UpdateTopControlsStatePreservingConstraints( On 2013/06/07 19:18:17, aelias wrote: > Let's avoid adding this method. Please cache the latest constraints passed in > RenderViewImpl::UpdateTopControlsState() and pass those into the existing > UpdateTopControlsState(). You can safely assume the constraints won't be > changed by anything else than that IPC. Done.
Looking OK, nits below. Please add jamesr@ for OWNERS review when you've applied them. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1059: LOG(WARNING) << "Called before top controls constraints is set"; Please remove this log, nobody will ever pay attention to it anyway. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1062: proxy_->ImplThread()->PostTask( Please call LayerTreeHost::UpdateTopControlsState() from here instead of adding another PostTask. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:241: void UpdateTopControlsStatePreservingConstraints(bool show); Please change the arguments here to "TopControlsState current, bool animate" for consistency. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:260: Nit: extra newline https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:372: TopControlsState GetTopControlsConstraints() const { No need for this private getter, please delete it. https://codereview.chromium.org/14999010/diff/28001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/14999010/diff/28001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:52: void UpdateTopControlsStatePreservingConstraints(bool show); Change the arguments here likewise.
Thanks for the review. Added jamesr for review. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1059: LOG(WARNING) << "Called before top controls constraints is set"; On 2013/06/10 05:11:25, aelias wrote: > Please remove this log, nobody will ever pay attention to it anyway. Done. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1062: proxy_->ImplThread()->PostTask( On 2013/06/10 05:11:25, aelias wrote: > Please call LayerTreeHost::UpdateTopControlsState() from here instead of adding > another PostTask. Done. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:241: void UpdateTopControlsStatePreservingConstraints(bool show); On 2013/06/10 05:11:25, aelias wrote: > Please change the arguments here to "TopControlsState current, bool animate" for > consistency. Done. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:260: On 2013/06/10 05:11:25, aelias wrote: > Nit: extra newline Done. https://codereview.chromium.org/14999010/diff/28001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:372: TopControlsState GetTopControlsConstraints() const { On 2013/06/10 05:11:25, aelias wrote: > No need for this private getter, please delete it. Done. https://codereview.chromium.org/14999010/diff/28001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/14999010/diff/28001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:52: void UpdateTopControlsStatePreservingConstraints(bool show); On 2013/06/10 05:11:25, aelias wrote: > Change the arguments here likewise. Done.
Not quite https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1059: if (top_controls_constraints_ < 0) top_controls_constraints_ is an enum. what does "< 0" mean? that's not a valid enum value https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:372: TopControlsState top_controls_constraints_; where is this initialized? https://codereview.chromium.org/14999010/diff/38001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/14999010/diff/38001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:52: void UpdateTopControlsStatePreservingConstraints( rather than adding a new entry point, can you just keep track of the old constraints in the caller? if you do need to add a new entry point, you need to add tests for it https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... content/renderer/render_view_impl.cc:2346: if (delta.height != 0.f) the body of this if is more than one physical line, so you need {}s https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... content/renderer/render_view_impl.h:448: virtual void didScrollWithKeyboard(const WebKit::WebFloatSize& delta); scrolls from the WebKit API are always integer deltas, not float
Thanks for the review. Please take another look. https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:1059: if (top_controls_constraints_ < 0) On 2013/06/10 23:48:31, jamesr wrote: > top_controls_constraints_ is an enum. what does "< 0" mean? that's not a valid > enum value Added cc::NONE value to the enum list. https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/14999010/diff/38001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:372: TopControlsState top_controls_constraints_; On 2013/06/10 23:48:31, jamesr wrote: > where is this initialized? Good catch. Put the initialization in RenderViewImpl constructor. https://codereview.chromium.org/14999010/diff/38001/content/renderer/gpu/rend... File content/renderer/gpu/render_widget_compositor.h (right): https://codereview.chromium.org/14999010/diff/38001/content/renderer/gpu/rend... content/renderer/gpu/render_widget_compositor.h:52: void UpdateTopControlsStatePreservingConstraints( On 2013/06/10 23:48:31, jamesr wrote: > rather than adding a new entry point, can you just keep track of the old > constraints in the caller? > > if you do need to add a new entry point, you need to add tests for it Changed to keep the old constraints in RenderViewImpl. https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... content/renderer/render_view_impl.cc:2346: if (delta.height != 0.f) On 2013/06/10 23:48:31, jamesr wrote: > the body of this if is more than one physical line, so you need {}s Done. https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/38001/content/renderer/render_v... content/renderer/render_view_impl.h:448: virtual void didScrollWithKeyboard(const WebKit::WebFloatSize& delta); On 2013/06/10 23:48:31, jamesr wrote: > scrolls from the WebKit API are always integer deltas, not float Done. Updated https://codereview.chromium.org/15735021/ as well.
https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... content/renderer/render_view_impl.cc:768: top_controls_constraints_(cc::NONE), does 'NONE' make sense? the top controls have to either be visible or not visible initially, right? https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... content/renderer/render_view_impl.cc:2349: void RenderViewImpl::didScrollWithKeyboard(const WebKit::WebSize& delta) { you've declared the function in the header outside the #if defined(OS_ANDROID), so you have to provide an implementation on all platforms. either move the #if's just inside the function declaration (so it's just an empty function body on non-android) or guard the function declaration as well
https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... content/renderer/render_view_impl.cc:768: top_controls_constraints_(cc::NONE), On 2013/06/11 05:11:27, jamesr wrote: > does 'NONE' make sense? the top controls have to either be visible or not > visible initially, right? How about 'INVALID' which indicates it is not a valid value to use. https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... content/renderer/render_view_impl.cc:2349: void RenderViewImpl::didScrollWithKeyboard(const WebKit::WebSize& delta) { On 2013/06/11 05:11:27, jamesr wrote: > you've declared the function in the header outside the #if defined(OS_ANDROID), > so you have to provide an implementation on all platforms. either move the #if's > just inside the function declaration (so it's just an empty function body on > non-android) or guard the function declaration as well Moved #if's inside.
On 2013/06/11 05:23:59, jindor wrote: > https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... > content/renderer/render_view_impl.cc:768: top_controls_constraints_(cc::NONE), > On 2013/06/11 05:11:27, jamesr wrote: > > does 'NONE' make sense? the top controls have to either be visible or not > > visible initially, right? > > How about 'INVALID' which indicates it is not a valid value to use. > > https://codereview.chromium.org/14999010/diff/47002/content/renderer/render_v... > content/renderer/render_view_impl.cc:2349: void > RenderViewImpl::didScrollWithKeyboard(const WebKit::WebSize& delta) { > On 2013/06/11 05:11:27, jamesr wrote: > > you've declared the function in the header outside the #if > defined(OS_ANDROID), > > so you have to provide an implementation on all platforms. either move the > #if's > > just inside the function declaration (so it's just an empty function body on > > non-android) or guard the function declaration as well > Moved #if's inside. Care to look again?
No, it still doesn't make sense to me. If you want to set the top controls to be in a certain place, you would need to specify an actual value. If the issue here is that the code using the TopControlsState doesn't *know* where the top controls are at the time of calling then I think you have deeper design problems. https://codereview.chromium.org/14999010/diff/66001/cc/input/top_controls_sta... File cc/input/top_controls_state.h (right): https://codereview.chromium.org/14999010/diff/66001/cc/input/top_controls_sta... cc/input/top_controls_state.h:10: INVALID = 0, I still don't understand this. TopControlsState is used to define the valid positions for the top controls - either shown, hidden, or both (it doesn't matter where they are). What does another state mean? If you don't care where the top controls are, isn't BOTH the correct value?
https://codereview.chromium.org/14999010/diff/66001/cc/input/top_controls_sta... File cc/input/top_controls_state.h (right): https://codereview.chromium.org/14999010/diff/66001/cc/input/top_controls_sta... cc/input/top_controls_state.h:10: INVALID = 0, On 2013/06/13 02:06:59, jamesr wrote: > I still don't understand this. TopControlsState is used to define the valid > positions for the top controls - either shown, hidden, or both (it doesn't > matter where they are). What does another state mean? If you don't care where > the top controls are, isn't BOTH the correct value? This will clobber the existing constraints, I assume that's his concern. The value in cc::TopControlsManager can default to BOTH, so if the one in RenderViewImpl also defaults to BOTH, they'll stay in sync. https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... content/renderer/render_view_impl.cc:768: top_controls_constraints_(cc::INVALID), You don't ever set this value? Did you forget to update it in RenderViewImplAndroid::OnUpdateTopControlsState? https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... content/renderer/render_view_impl.cc:2352: compositor_->UpdateTopControlsState(top_controls_constraints_, If the field is moved to RenderViewImplAndroid, this code should also move there (you can call into it from this function). https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... content/renderer/render_view_impl.h:1365: cc::TopControlsState top_controls_constraints_; How about moving this value into RenderViewImplAndroid?
render_view_impl_android.cc seems to be the right place. Thanks. Please take another look. https://codereview.chromium.org/14999010/diff/66001/cc/input/top_controls_sta... File cc/input/top_controls_state.h (right): https://codereview.chromium.org/14999010/diff/66001/cc/input/top_controls_sta... cc/input/top_controls_state.h:10: INVALID = 0, On 2013/06/13 03:29:25, aelias wrote: > On 2013/06/13 02:06:59, jamesr wrote: > > I still don't understand this. TopControlsState is used to define the valid > > positions for the top controls - either shown, hidden, or both (it doesn't > > matter where they are). What does another state mean? If you don't care > where > > the top controls are, isn't BOTH the correct value? > > This will clobber the existing constraints, I assume that's his concern. The > value in cc::TopControlsManager can default to BOTH, so if the one in > RenderViewImpl also defaults to BOTH, they'll stay in sync. Thanks for the tip. I found that permitted_state_ is initialized to BOTH, so set the top_controls_constraints_ to BOTH too. https://codereview.chromium.org/14999010/diff/66001/cc/input/top_controls_sta... cc/input/top_controls_state.h:10: INVALID = 0, On 2013/06/13 02:06:59, jamesr wrote: > I still don't understand this. TopControlsState is used to define the valid > positions for the top controls - either shown, hidden, or both (it doesn't > matter where they are). What does another state mean? If you don't care where > the top controls are, isn't BOTH the correct value? Set to BOTH. https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... content/renderer/render_view_impl.cc:768: top_controls_constraints_(cc::INVALID), On 2013/06/13 03:29:25, aelias wrote: > You don't ever set this value? Did you forget to update it in > RenderViewImplAndroid::OnUpdateTopControlsState? Thanks for catching it. Now it is done in RenderViewImplAndroid::OnUpdateTopControlsState/UpdateTopControlsState where the constraints are updated. https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... content/renderer/render_view_impl.cc:2352: compositor_->UpdateTopControlsState(top_controls_constraints_, On 2013/06/13 03:29:25, aelias wrote: > If the field is moved to RenderViewImplAndroid, this code should also move there > (you can call into it from this function). Done. https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/66001/content/renderer/render_v... content/renderer/render_view_impl.h:1365: cc::TopControlsState top_controls_constraints_; On 2013/06/13 03:29:25, aelias wrote: > How about moving this value into RenderViewImplAndroid? The header file is shared. Kept the definition here, moved only the methods to render_view_impl_android.cc
https://codereview.chromium.org/14999010/diff/74001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/74001/content/renderer/render_v... content/renderer/render_view_impl.h:447: virtual void didScrollWithKeyboard(const WebKit::WebSize& delta); again, you've declared a function here as a member of RenderViewImpl but only provide an implementation in an android-specific location. not good! in particular, if anyone attempts to reference this function on a non-android platform (a perfectly reasonable thing to do looking at the header) they'll get a link error. either guard the declaration or provide an empty definition for non-android builds
https://codereview.chromium.org/14999010/diff/74001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/74001/content/renderer/render_v... content/renderer/render_view_impl.h:447: virtual void didScrollWithKeyboard(const WebKit::WebSize& delta); On 2013/06/13 05:25:12, jamesr wrote: > again, you've declared a function here as a member of RenderViewImpl but only > provide an implementation in an android-specific location. not good! in > particular, if anyone attempts to reference this function on a non-android > platform (a perfectly reasonable thing to do looking at the header) they'll get > a link error. > > either guard the declaration or provide an empty definition for non-android > builds Guarded the declaration.
lgtm if you move the declaration into the right place https://codereview.chromium.org/14999010/diff/79001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/79001/content/renderer/render_v... content/renderer/render_view_impl.h:448: virtual void didScrollWithKeyboard(const WebKit::WebSize& delta); please also move this down with the other #if defined(OS_ANDROID) guarded functions now that it's guarded it's really helpful and will save review cycles to spend time studying how a file is set up currently and considering if it makes sense for you to follow suit.
Thanks. Would you review https://codereview.chromium.org/15735021 as well? https://codereview.chromium.org/14999010/diff/79001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/14999010/diff/79001/content/renderer/render_v... content/renderer/render_view_impl.h:448: virtual void didScrollWithKeyboard(const WebKit::WebSize& delta); On 2013/06/13 06:03:36, jamesr wrote: > please also move this down with the other #if defined(OS_ANDROID) guarded > functions now that it's guarded > > it's really helpful and will save review cycles to spend time studying how a > file is set up currently and considering if it makes sense for you to follow > suit. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jinsukkim@chromium.org/14999010/83002
Message was sent while issue was closed.
Change committed as 206386 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
