Hide location bar on Javascript-initiated scroll.

Hide location bar on Javascript-initiated scroll.

Many sites use window.scrollTo(0,1) and other offsets as a means of 
hiding the location bar on mobile. In fact, Android Browser and 
iOS support hiding the location bar on any Javascript-initiated 
scroll. 

See also corresponding WebKit bugs: 
https://bugs.webkit.org/show_bug.cgi?id=107027 

This is a patch based on on jknottten's work:
https://chromiumcodereview.appspot.com/11967015/

TEST=Test that URL bar is hidden when pressing on the various test buttons in http://jsbin.com/eruxon/5 

BUG=165317
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201303

[Michael van Ouwerkerk, 2013-04-15 16:56:41]

Hi guys, could you please review this change? Thanks!

[Ted C, 2013-04-16 02:05:58]

lgtm w/ a few style nits for the androidy side of things

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager.cc
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager...
cc/input/top_controls_manager.cc:94: 
remove extra line

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager...
cc/input/top_controls_manager.cc:173: controls_top_offset_ ==
-top_controls_height_)
if I recall, I think you need braces if the conditional is more than one line

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager...
cc/input/top_controls_manager.cc:216: client_->DidChangeTopControlsPosition();
can remove this because it is called in Setup

https://codereview.chromium.org/14139013/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl_android.cc (right):

https://codereview.chromium.org/14139013/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl_android.cc:25: DCHECK(compositor_);
Probably should remove the DCHECK as this was crashing above

[Michael van Ouwerkerk, 2013-04-16 15:02:47]

Thanks for the quick review Ted!

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager.cc
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager...
cc/input/top_controls_manager.cc:94: 
On 2013/04/16 02:05:58, Ted C wrote:
> remove extra line

Done.

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager...
cc/input/top_controls_manager.cc:173: controls_top_offset_ ==
-top_controls_height_)
On 2013/04/16 02:05:58, Ted C wrote:
> if I recall, I think you need braces if the conditional is more than one line

Done.

https://codereview.chromium.org/14139013/diff/1/cc/input/top_controls_manager...
cc/input/top_controls_manager.cc:216: client_->DidChangeTopControlsPosition();
On 2013/04/16 02:05:58, Ted C wrote:
> can remove this because it is called in Setup

Done.

[palmer, 2013-04-16 17:12:42]

https://codereview.chromium.org/14139013/diff/18001/cc/input/top_controls_man...
File cc/input/top_controls_manager.h (right):

https://codereview.chromium.org/14139013/diff/18001/cc/input/top_controls_man...
cc/input/top_controls_manager.h:59: void ShowTopControls(bool show);
Should this (and its implementation) be #ifdef'd for Android?

https://codereview.chromium.org/14139013/diff/18001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/14139013/diff/18001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:233: void ShowTopControls(bool show);
#ifdef for Android?

https://codereview.chromium.org/14139013/diff/18001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_delegate.h (right):

https://codereview.chromium.org/14139013/diff/18001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_delegate.h:347: virtual void
DidProgrammaticallyScroll(const gfx::Point& scroll_point) {}
Again, #ifdef? Throughout for all non-.../android* files.

[aelias_OOO_until_Jul13, 2013-04-16 22:16:17]

We want to avoid #ifdef at least within cc/ so that Linux-based unit tests can
exercise this code.

[palmer, 2013-04-16 22:22:34]

IPC security LGTM.

[Michael van Ouwerkerk, 2013-04-17 13:52:09]

joth/joi: could you please check for OWNERS approval in content and components?
aelias: OWNERS for cc?
jam: could you please check for content?

Thanks!

[Jói, 2013-04-17 13:56:23]

LGTM for content/public/browser. joth@ is a better reviewer for the Java bits.

[jam, 2013-04-17 16:24:58]

https://codereview.chromium.org/14139013/diff/23001/components/web_contents_d...
File components/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://codereview.chromium.org/14139013/diff/23001/components/web_contents_d...
components/web_contents_delegate_android/web_contents_delegate_android.h:95: 
nit: here and above, do not add blank spaces between overridden methods per
convention

https://codereview.chromium.org/14139013/diff/23001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/14139013/diff/23001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1270: host->Send(new
ViewMsg_ShowTopControls(host->GetRoutingID(), show));
it's simpler to just do

web_contents_->Send(new FooMsg(web_contents_->GetRoutingID())

https://codereview.chromium.org/14139013/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/14139013/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1082:
delegate_->DidProgrammaticallyScroll(scroll_point);
do not dispatch a message in RVHImpl just to call WebContentsImpl. just dispatch
it directly in WCImpl

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
content/common/view_messages.h:1370:
IPC_MESSAGE_ROUTED1(ViewMsg_ShowTopControls, bool /* show */)
i'm not familiar what top controls are?

is this a ui feature of the android browser?

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
content/common/view_messages.h:1517: gfx::Point /* scroll point */)
nit: convention is that the comment holds the parameter name in the dispatching
method, i.e. scroll_point

[aelias_OOO_until_Jul13, 2013-04-17 18:58:17]

The new ViewMsg and TopControlsManager method looks redundant with
UpdateTopControlsState.  Why do you need a new one?

[Ted C, 2013-04-17 20:18:00]

On 2013/04/17 18:58:17, aelias wrote:
> The new ViewMsg and TopControlsManager method looks redundant with
> UpdateTopControlsState.  Why do you need a new one?

UpdateTopControlsState determines whether the top controls can be scrolled
off/on at all (i.e. if the site has broken https then we disable scrolling off
the top controls).

The new message is simply to animate the controls on or off but still allow them
to be controlled by user gesture.

[joth, 2013-04-17 20:34:30]

cc: mkosiba FYI - new didProgrammaticalllyScroll call from JS ->
WebContentsDelegate.

java files lgtm.

You probably want to update the CL description: the webkit bug link looks
obsolete now.

[Michael van Ouwerkerk, 2013-04-18 18:11:38]

Thanks for the review! Please take another look.

https://codereview.chromium.org/14139013/diff/23001/components/web_contents_d...
File components/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://codereview.chromium.org/14139013/diff/23001/components/web_contents_d...
components/web_contents_delegate_android/web_contents_delegate_android.h:95: 
On 2013/04/17 16:24:59, jam wrote:
> nit: here and above, do not add blank spaces between overridden methods per
> convention

Done.

https://codereview.chromium.org/14139013/diff/23001/content/browser/android/c...
File content/browser/android/content_view_core_impl.cc (right):

https://codereview.chromium.org/14139013/diff/23001/content/browser/android/c...
content/browser/android/content_view_core_impl.cc:1270: host->Send(new
ViewMsg_ShowTopControls(host->GetRoutingID(), show));
On 2013/04/17 16:24:59, jam wrote:
> it's simpler to just do
> 
> web_contents_->Send(new FooMsg(web_contents_->GetRoutingID())

Done.

https://codereview.chromium.org/14139013/diff/23001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/14139013/diff/23001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:1082:
delegate_->DidProgrammaticallyScroll(scroll_point);
On 2013/04/17 16:24:59, jam wrote:
> do not dispatch a message in RVHImpl just to call WebContentsImpl. just
dispatch
> it directly in WCImpl

Oh nice! Done.

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
content/common/view_messages.h:1370:
IPC_MESSAGE_ROUTED1(ViewMsg_ShowTopControls, bool /* show */)
On 2013/04/17 16:24:59, jam wrote:
> i'm not familiar what top controls are?
> 
> is this a ui feature of the android browser?

It's the piece of browser ui at the top that includes the url bar, tab switching
button, menu button, etc. It gets hidden in fullscreen mode.

Related code: cc/input/top_controls_manager.h

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
content/common/view_messages.h:1517: gfx::Point /* scroll point */)
On 2013/04/17 16:24:59, jam wrote:
> nit: convention is that the comment holds the parameter name in the
dispatching
> method, i.e. scroll_point

Done.

[Michael van Ouwerkerk, 2013-04-19 13:21:34]

James, Alexandre seems to be too busy / on vacation. Would you mind taking a
look for cc/ ?

[Michael van Ouwerkerk, 2013-04-19 13:25:47]

John, I think I have addressed your comments. Could you please take another look
for content/ ? Thanks!

[jam, 2013-04-19 18:30:21]

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/14139013/diff/23001/content/common/view_messa...
content/common/view_messages.h:1370:
IPC_MESSAGE_ROUTED1(ViewMsg_ShowTopControls, bool /* show */)
On 2013/04/18 18:11:38, Michael van Ouwerkerk wrote:
> On 2013/04/17 16:24:59, jam wrote:
> > i'm not familiar what top controls are?
> > 
> > is this a ui feature of the android browser?
> 
> It's the piece of browser ui at the top that includes the url bar, tab
switching
> button, menu button, etc. It gets hidden in fullscreen mode.
> 
> Related code: cc/input/top_controls_manager.h

I see. it seems like a layering violation that this code (anything related to
top controls) is in content, since content just knows about rendering a view.
anything related to the UI of an app is the responsibility of the embedder.

[jamesr, 2013-04-19 19:28:31]

cc/ and content/renderer lgtm.

Where's the logic that shows the URL bar after it's been hidden and the user
does a down gesture?

[palmer, 2013-04-19 20:50:02]

> Where's the logic that shows the URL bar after it's been hidden and the user
> does a down gesture?

I don't know :) but thankfully I *do* see that good behavior when I use the
jsbin demo.

Ironically, maps.google.com appears not to use the scrollTo trick. (I am afraid
of it capturing and swallowing the down gesture, and so wanted to test it.)

[Ted C, 2013-04-19 21:04:01]

On 2013/04/19 19:28:31, jamesr wrote:
> cc/ and content/renderer lgtm.
> 
> Where's the logic that shows the URL bar after it's been hidden and the user
> does a down gesture?

That is still handled through the normal flow in LayerTreeHostImpl::ScrollBy. 
This just animates it off but doesn't force it into the off position.  If it
were to start animating off and the user started a scroll, the animation would
be cancelled and it would continue to follow the finger.

There are two methods in top controls manager:
1.) ShowTopControls -- Hides/shows the top controls as if the user had done it.
2.) UpdateTopControlsState -- Forces the top controls to be permanently
hidden/shown (or removes those restrictions).

This should just be using #1

[jamesr, 2013-04-19 21:05:10]

On 2013/04/19 21:04:01, Ted C wrote:
> On 2013/04/19 19:28:31, jamesr wrote:
> > cc/ and content/renderer lgtm.
> > 
> > Where's the logic that shows the URL bar after it's been hidden and the user
> > does a down gesture?
> 
> That is still handled through the normal flow in LayerTreeHostImpl::ScrollBy. 
> This just animates it off but doesn't force it into the off position.  If it
> were to start animating off and the user started a scroll, the animation would
> be cancelled and it would continue to follow the finger.

That can't work - if the page eats the touch events or if the element can't
scroll ScrollBy() will do nothing but we still need to show the URL bar.

> 
> There are two methods in top controls manager:
> 1.) ShowTopControls -- Hides/shows the top controls as if the user had done
it.
> 2.) UpdateTopControlsState -- Forces the top controls to be permanently
> hidden/shown (or removes those restrictions).
> 
> This should just be using #1

[Ted C, 2013-04-19 21:34:48]

On 2013/04/19 21:05:10, jamesr wrote:
> On 2013/04/19 21:04:01, Ted C wrote:
> > On 2013/04/19 19:28:31, jamesr wrote:
> > > cc/ and content/renderer lgtm.
> > > 
> > > Where's the logic that shows the URL bar after it's been hidden and the
user
> > > does a down gesture?
> > 
> > That is still handled through the normal flow in
LayerTreeHostImpl::ScrollBy. 
> > This just animates it off but doesn't force it into the off position.  If it
> > were to start animating off and the user started a scroll, the animation
would
> > be cancelled and it would continue to follow the finger.
> 
> That can't work - if the page eats the touch events or if the element can't
> scroll ScrollBy() will do nothing but we still need to show the URL bar.

This is why we have the escape hatch that we always show the top controls on
navigation (always accessible via the hardware back button on android), when you
exit/return to the app or by initiating side swipe.
> 
> > 
> > There are two methods in top controls manager:
> > 1.) ShowTopControls -- Hides/shows the top controls as if the user had done
> it.
> > 2.) UpdateTopControlsState -- Forces the top controls to be permanently
> > hidden/shown (or removes those restrictions).
> > 
> > This should just be using #1

[jamesr, 2013-04-19 22:03:31]

That's not what the security team agreed to and would be a serious regression
from the Android browser behavior since it would let a page completely spoof UI
in an inescapable way.  Navigations can be spoofed perfectly if the URL bar is
hidden.

Sorry, this is not lgtm without the browser side piece.  This patch landing by
itself will result in an insecure browser.

[Ted C, 2013-04-19 22:07:50]

On 2013/04/19 22:03:31, jamesr wrote:
> That's not what the security team agreed to and would be a serious regression
> from the Android browser behavior since it would let a page completely spoof
UI
> in an inescapable way.  Navigations can be spoofed perfectly if the URL bar is
> hidden.
> 
> Sorry, this is not lgtm without the browser side piece.  This patch landing by
> itself will result in an insecure browser.

To the best of my knowledge, the swipe down from the top is for the fullscreen
API only, which is in place already.

I wasn't at the security review for this, so I can't comment on what was
discussed and what was accepted to be ok.

[palmer, 2013-04-19 22:09:05]

> This is why we have the escape hatch that we always show the top controls on
> navigation (always accessible via the hardware back button on android), when
you
> exit/return to the app or by initiating side swipe.

I don't remember agreeing to that for Chrome on Android; I thought I agreed to
it for Chrome on iOS because we are limited in what we can do there. But, to be
honest, I got a bit lost after re-hashing the design decisions multiple times on
both platforms. :) On Android, we have the true HTML 5 Fullscreen API available
to us, so we don't need to rely on this hackery as our only fullscreen
mechanism. Sites that want true full screen access should use that real API.

Undiscoverable UI gestures, like side-swipe, don't exist and therefore provide
no security guarantee.

Showing the Omnibox on (normal, in-page) navigation is not enough, because once
yo've visited an attacking page that fakes the Onibox, the attacking page can
falsify the Omnibox and let you think you're navigating to other sites normally.

Thus the only real UI available to recover the Omnibox is the "hardware" back
button, but I'm not sure users will expect/know it to have this security
guarantee. Perhaps they will. It'd be good to add a note to this effect on
http://support.google.com/chrome/bin/answer.py?hl=en&answer=95440 and
http://support.google.com/chrome/bin/answer.py?hl=en&answer=95617.

[jamesr, 2013-04-19 22:42:43]

On this page:

http://webstuff.nfshost.com/scrollto/scrollcapture.html

in android browser the URL bar is available when swiping down even though the
page consumes all touches.

[aelias_OOO_until_Jul13, 2013-04-20 04:23:34]

On 2013/04/19 18:30:21, jam wrote:
> I see. it seems like a layering violation that this code (anything related to
> top controls) is in content, since content just knows about rendering a view.
> anything related to the UI of an app is the responsibility of the embedder.

Top-controls animation logic needs to be handled by the render-process impl
thread of CC for several reasons -- it's tightly bound to root-layer scrolling,
and affects rendering of scrollbars and fixed-position elements.  So this
message needs to go from the embedder to the render-process instance of CC, and
RenderViewImpl is simply a passthrough.  What would be a more appropriate place
to put the message?  content/common/cc_messages.h doesn't seem quite right
either...

[jamesr, 2013-04-22 21:45:29]

On 2013/04/20 04:23:34, aelias wrote:
> On 2013/04/19 18:30:21, jam wrote:
> > I see. it seems like a layering violation that this code (anything related
to
> > top controls) is in content, since content just knows about rendering a
view.
> > anything related to the UI of an app is the responsibility of the embedder.
> 
> Top-controls animation logic needs to be handled by the render-process impl
> thread of CC for several reasons -- it's tightly bound to root-layer
scrolling,
> and affects rendering of scrollbars and fixed-position elements.  So this
> message needs to go from the embedder to the render-process instance of CC,
and
> RenderViewImpl is simply a passthrough.  What would be a more appropriate
place
> to put the message?  content/common/cc_messages.h doesn't seem quite right
> either...


I think jam@'s concern is different from the one you are responding to.  jam@ is
pointing out that content/ shouldn't have to deal with browser UI concepts
directly.  You're saying that the logic should be in the render process.  Both
can be true - we can have the logic in the chrome/ layer in the renderer process
that deals with the top controls and sends chrome/ IPC messages to the browser
process to do positioning.

[aelias_OOO_until_Jul13, 2013-04-22 21:58:11]

Sounds good, let's move the ViewMsg_ShowTopControls and
ViewMsg_UpdateTopControlsState to chrome/.  chrome/common/render_messages.h
looks appropriate.

I also propose merging those two messages as follows:

namespace cc {
enum TopControlsState { SHOWN, HIDDEN, BOTH };
}

namespace chrome {
IPC_MESSAGE_ROUTED3(ChromeViewMsg_UpdateTopControlsState,
    TopControlsState /* constraints */
    TopControlsState /* current */
    bool /* animate */)
}

"constraints" represents where the top controls are allowed to go, and "current"
specifies where it should animate to as soon as the message is received (if
"BOTH", then leave it where it already was).  The receiver should DCHECK the
constraints and current are consistent.  "animate" specifies whether it should
animate gradually there or pop instantly.

[Michael van Ouwerkerk, 2013-05-02 13:46:32]

Agreed on merging ShowTopControls into UpdateTopControlsState and using an enum
for its arguments. Done.

Moving the IPC message from content/common/view_messages.h to
chrome/common/render_messages.h would introduce a dependency from
content/renderer/render_view_impl.cc to chrome/ which seems a dependency in the
wrong direction, so I didn't do that.

Please take another look.

On 2013/04/22 21:58:11, aelias wrote:
> Sounds good, let's move the ViewMsg_ShowTopControls and
> ViewMsg_UpdateTopControlsState to chrome/.  chrome/common/render_messages.h
> looks appropriate.
> 
> I also propose merging those two messages as follows:
> 
> namespace cc {
> enum TopControlsState { SHOWN, HIDDEN, BOTH };
> }
> 
> namespace chrome {
> IPC_MESSAGE_ROUTED3(ChromeViewMsg_UpdateTopControlsState,
>     TopControlsState /* constraints */
>     TopControlsState /* current */
>     bool /* animate */)
> }
> 
> "constraints" represents where the top controls are allowed to go, and
"current"
> specifies where it should animate to as soon as the message is received (if
> "BOTH", then leave it where it already was).  The receiver should DCHECK the
> constraints and current are consistent.  "animate" specifies whether it should
> animate gradually there or pop instantly.

[jamesr1, 2013-05-02 17:10:33]

On May 2, 2013 6:46 AM, <mvanouwerkerk@chromium.org> wrote:
>
> Agreed on merging ShowTopControls into UpdateTopControlsState and using
an enum
> for its arguments. Done.
>
> Moving the IPC message from content/common/view_messages.h to
> chrome/common/render_messages.h would introduce a dependency from
> content/renderer/render_view_impl.cc to chrome/ which seems a dependency
in the
> wrong direction, so I didn't do that.

You will have to add public content api to get from render_view_impl to
chrome/

>
> Please take another look.
>
> On 2013/04/22 21:58:11, aelias wrote:
>>
>> Sounds good, let's move the ViewMsg_ShowTopControls and
>> ViewMsg_UpdateTopControlsState to chrome/.
 chrome/common/render_messages.h
>> looks appropriate.
>
>
>> I also propose merging those two messages as follows:
>
>
>> namespace cc {
>> enum TopControlsState { SHOWN, HIDDEN, BOTH };
>> }
>
>
>> namespace chrome {
>> IPC_MESSAGE_ROUTED3(ChromeViewMsg_UpdateTopControlsState,
>>      TopControlsState /* constraints */
>>      TopControlsState /* current */
>>      bool /* animate */)
>> }
>
>
>> "constraints" represents where the top controls are allowed to go, and
>
> "current"
>>
>> specifies where it should animate to as soon as the message is received
(if
>> "BOTH", then leave it where it already was).  The receiver should DCHECK
the
>> constraints and current are consistent.  "animate" specifies whether it
should
>> animate gradually there or pop instantly.
>
>
>
> https://codereview.chromium.org/14139013/

[aelias_OOO_until_Jul13, 2013-05-02 18:44:22]

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:62: DCHECK(current != HIDDEN);
Nit, formulate it as:

DCHECK(!(constraints == SHOWN && current == HIDDEN));
DCHECK(!(constraints == HIDDEN && current == SHOWN));

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:70: if (constraints == BOTH && current == BOTH)
return;
Style: return on next line

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:87: client_->DidChangeTopControlsPosition();
Please track the old value of controls_top_offset_ and only call this if it
changed.

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_sta...
File cc/input/top_controls_state_list.h (right):

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_sta...
cc/input/top_controls_state_list.h:12: // These values are defined with macros
so that a Java class can be generated
I understand the problem you're addressing here but it's not really a concern of
CC, and AFAIK this is not an accepted pattern in Chromium code.  Please define
the enum normally and have matching constants on the Java side, and add some
comments warning about the other class that should be modified in sync.  (This
enum shouldn't often change anyway.)

https://codereview.chromium.org/14139013/diff/71001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/14139013/diff/71001/content/common/view_messa...
content/common/view_messages.h:1427: gfx::Point /* scroll_point */)
Let's change this to gfx::Vector2d (and likewise everywhere else in this patch),
that's the new conventional type for scroll offsets in Chromium code.

[Michael van Ouwerkerk, 2013-05-03 16:10:17]

Thanks for the quick reviews!

I have a plan for moving the IPC message to chrome/ and avoid plumbing through
the content layer in the browser process. I just wanted to confirm that this
approach makes sense to you:

A signal that the top controls state must be updated is sent over JNI
chrome/browser/android/tab_android.cc recives the JNI call and uses WebContents
to send the IPC message
chrome/common/render_messages.h defines the IPC message
chrome/renderer/chrome_render_view_observer.cc receives the IPC message, it has
a RenderView which has a RenderWidgetCompositor
content/renderer/gpu/render_widget_compositor.h
cc/trees/layer_tree_host
cc/input/top_controls_manager

Just to make clear for anyone following this feature: this will not land before
M28 is branched, so it will not be in.

I'm working on a flag for enabling the feature. Is it ok to include that in this
patch or would it be better as a followup?

After that I'll land the Chrome on Android patch that actually contains the
logic for when the top controls must be hidden.

[palmer, 2013-05-03 17:55:53]

That sounds fine to me, but I'll defer to other people who know more.

Putting the flag in this CL, or in another CL — either way is fine by me.

[jamesr, 2013-05-03 17:57:34]

On 2013/05/03 16:10:17, Michael van Ouwerkerk wrote:
> Thanks for the quick reviews!
> 
> I have a plan for moving the IPC message to chrome/ and avoid plumbing through
> the content layer in the browser process. I just wanted to confirm that this
> approach makes sense to you:
> 
> A signal that the top controls state must be updated is sent over JNI
> chrome/browser/android/tab_android.cc recives the JNI call and uses
WebContents
> to send the IPC message
> chrome/common/render_messages.h defines the IPC message
> chrome/renderer/chrome_render_view_observer.cc receives the IPC message, it
has
> a RenderView which has a RenderWidgetCompositor

(there must be some content/renderer/public/ API here, right?)

> content/renderer/gpu/render_widget_compositor.h
> cc/trees/layer_tree_host
> cc/input/top_controls_manager

Looks good.

> 
> Just to make clear for anyone following this feature: this will not land
before
> M28 is branched, so it will not be in.
> 
> I'm working on a flag for enabling the feature. Is it ok to include that in
this
> patch or would it be better as a followup?

I think it'd be fine to either do that as part of this patch or land the parts
of this patch that aren't related to the IPCs, potentially as multiple patches,
and land the IPC bits on their own.  The latter would take a bit more setup time
but would probably overall land faster.

> 
> After that I'll land the Chrome on Android patch that actually contains the
> logic for when the top controls must be hidden.

[Michael van Ouwerkerk, 2013-05-08 16:21:15]

Thanks for the reviews! Please take another look.

ChromeViewMsg_UpdateTopControlsState is now sent from a class in Chrome on
Android, that's why you won't see it being sent in this patch anymore.

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
File cc/input/top_controls_manager.cc (right):

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:62: DCHECK(current != HIDDEN);
On 2013/05/02 18:44:23, aelias wrote:
> Nit, formulate it as:
> 
> DCHECK(!(constraints == SHOWN && current == HIDDEN));
> DCHECK(!(constraints == HIDDEN && current == SHOWN));

Done.

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:70: if (constraints == BOTH && current == BOTH)
return;
On 2013/05/02 18:44:23, aelias wrote:
> Style: return on next line

Done.

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:87: client_->DidChangeTopControlsPosition();
On 2013/05/02 18:44:23, aelias wrote:
> Please track the old value of controls_top_offset_ and only call this if it
> changed.

Done.

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_sta...
File cc/input/top_controls_state_list.h (right):

https://codereview.chromium.org/14139013/diff/71001/cc/input/top_controls_sta...
cc/input/top_controls_state_list.h:12: // These values are defined with macros
so that a Java class can be generated
Well, there is precedent in net/ chrome/ and content/:
net/android/java/NetError.template
chrome/android/java/ToolbarModelSecurityLevel.template
content/public/android/java/src/org/chromium/content/common/ResultCodes.template

Marcus Bulach tells me this is the way forward, and we expect to use this
pattern more in the future. Because of this, I'd prefer to keep this part as is.

On 2013/05/02 18:44:23, aelias wrote:
> I understand the problem you're addressing here but it's not really a concern
of
> CC, and AFAIK this is not an accepted pattern in Chromium code.  Please define
> the enum normally and have matching constants on the Java side, and add some
> comments warning about the other class that should be modified in sync.  (This
> enum shouldn't often change anyway.)

https://codereview.chromium.org/14139013/diff/71001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/14139013/diff/71001/content/common/view_messa...
content/common/view_messages.h:1427: gfx::Point /* scroll_point */)
On 2013/05/02 18:44:23, aelias wrote:
> Let's change this to gfx::Vector2d (and likewise everywhere else in this
patch),
> that's the new conventional type for scroll offsets in Chromium code.

Done.

[Michael van Ouwerkerk, 2013-05-08 16:46:24]

I noticed I should leave some version of the old code path that uses
ViewMsg_UpdateTopControlsState. This is currently used by ContentViewCore(Impl)
and called from Chrome on Android. I'll need to land a patch in Chrome on
Android to start calling the new code path before I can remove the old one. I'll
update this patch to restore the old code path.

[Michael van Ouwerkerk, 2013-05-08 17:26:43]

On 2013/05/08 16:46:24, Michael van Ouwerkerk wrote:
> I noticed I should leave some version of the old code path that uses
> ViewMsg_UpdateTopControlsState.

This is now done. Please take another look.

[jamesr, 2013-05-08 17:31:15]

https://codereview.chromium.org/14139013/diff/85001/content/public/renderer/r...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/14139013/diff/85001/content/public/renderer/r...
content/public/renderer/render_view.h:10: #include
"cc/input/top_controls_state.h"
NAK, don't include cc types or headers in public content API.  You just want a
number here, right?

https://codereview.chromium.org/14139013/diff/85001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/14139013/diff/85001/content/renderer/render_w...
content/renderer/render_widget.cc:1609: routing_id_,
gfx::Vector2d(scrollPoint.x, scrollPoint.y)));
bad alignment

[jam, 2013-05-09 21:08:57]

I thought the top controls stuff was going to move out of content?

[Michael van Ouwerkerk, 2013-05-13 13:27:18]

https://codereview.chromium.org/14139013/diff/85001/content/public/renderer/r...
File content/public/renderer/render_view.h (right):

https://codereview.chromium.org/14139013/diff/85001/content/public/renderer/r...
content/public/renderer/render_view.h:10: #include
"cc/input/top_controls_state.h"
On 2013/05/08 17:31:16, jamesr wrote:
> NAK, don't include cc types or headers in public content API.  You just want a
> number here, right?

As discussed on chat, I've introduced a mirror enum in the content namespace to
avoid this dependency on cc/ and added some boilerplate to help keep them in
sync.

https://codereview.chromium.org/14139013/diff/85001/content/renderer/render_w...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/14139013/diff/85001/content/renderer/render_w...
content/renderer/render_widget.cc:1609: routing_id_,
gfx::Vector2d(scrollPoint.x, scrollPoint.y)));
On 2013/05/08 17:31:16, jamesr wrote:
> bad alignment

Done.

[Michael van Ouwerkerk, 2013-05-13 13:43:21]

On 2013/05/09 21:08:57, jam wrote:
> I thought the top controls stuff was going to move out of content?

I've moved the pre-existing message from content/common/view_messages.h to
chrome/common/render_messages.h

Message receiving is now handled in
chrome/renderer/chrome_render_view_observer.h instead of
content/renderer/render_view_impl.h

The new message is sent from Chrome on Android (TabAndroidImpl) instead of
content/browser/android/content_view_core_impl.h

But eventually I need to plumb down into the compositor from chrome/renderer/.
It seems this should go via content/renderer/ or not? I described my intent of
doing this in comment #32. I had hoped that I wouldn't need to change that
approach after it had been given the ok.

As a followup I will remove the UpdateTopControls path from ContentViewCore. But
first I must update Chrome on Android to stop calling that, and use the path
defined in this patch.

[Michael van Ouwerkerk, 2013-05-14 09:47:20]

James, Alexandre, John, could you please take another look? I think I have
addressed all your comments with either code changes or a reply in this thread.

[aelias_OOO_until_Jul13, 2013-05-14 20:57:52]

lgtm functionality-wise.

(Though it looks like non-Android trybots are red, you probably need to fix
something there.)

[jamesr, 2013-05-15 18:14:17]

lgtm

https://codereview.chromium.org/14139013/diff/107001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/14139013/diff/107001/content/renderer/render_...
content/renderer/render_view_impl.h:733: virtual cc::TopControlsState
ContentToCcTopControlsState(
this definitely doesn't need to be a virtual member function - it doesn't have
overrides, it doesn't depend on member state, and it's only called from one
file. Can't this just be a free function inside render_view_impl_android.cc ?

[Michael van Ouwerkerk, 2013-05-15 18:52:11]

Hi joi, could you please take one more look for content/public/ ? There have
been some changes since you gave the lgtm. Thanks!

[Jói, 2013-05-15 19:24:44]

LGTM for //content/public.

[Nico, 2013-05-15 19:47:45]

https://codereview.chromium.org/14139013/diff/107001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/14139013/diff/107001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2270:
'../content/public/common/top_controls_state_list.h',
Why is a file from content in the file list of chrome_browser.gypi?

[Michael van Ouwerkerk, 2013-05-15 20:23:50]

https://codereview.chromium.org/14139013/diff/107001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/14139013/diff/107001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2270:
'../content/public/common/top_controls_state_list.h',
On 2013/05/15 19:47:46, Nico wrote:
> Why is a file from content in the file list of chrome_browser.gypi?

Is there a more appropriate way of using this file for generating
org.chromium.chrome.browser.TopControlsState.java?

[Nico, 2013-05-15 21:02:06]

https://codereview.chromium.org/14139013/diff/107001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/14139013/diff/107001/chrome/chrome_browser.gy...
chrome/chrome_browser.gypi:2270:
'../content/public/common/top_controls_state_list.h',
On 2013/05/15 20:23:50, Michael van Ouwerkerk wrote:
> On 2013/05/15 19:47:46, Nico wrote:
> > Why is a file from content in the file list of chrome_browser.gypi?
> 
> Is there a more appropriate way of using this file for generating
> org.chromium.chrome.browser.TopControlsState.java?

I'm not familiar with the java generation code, but files in content/ should
live in a gyp file in content. Why is the template file in content but the
java_cpp_template invocation for it in chrome?

[Michael van Ouwerkerk, 2013-05-16 16:25:59]

Nico: You're right it was odd. Seems like a leftover from previous revisions.
I've moved the TopControlsState.java generation code to content/ where the C++
code lives. There is precedent for this in ResultCodes.template

https://codereview.chromium.org/14139013/diff/107001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/14139013/diff/107001/content/renderer/render_...
content/renderer/render_view_impl.h:733: virtual cc::TopControlsState
ContentToCcTopControlsState(
On 2013/05/15 18:14:18, jamesr wrote:
> this definitely doesn't need to be a virtual member function - it doesn't have
> overrides, it doesn't depend on member state, and it's only called from one
> file. Can't this just be a free function inside render_view_impl_android.cc ?

Done.

[Nico, 2013-05-16 17:33:33]

chrome/ lgtm

[commit-bot: I haz the power, 2013-05-16 18:06:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/14139013/11...

[Nico, 2013-05-16 18:15:28]

On 2013/05/16 18:06:47, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/mvanouwerkerk%40chromium.org/14139013/...

(Don't you need some content OWNER to look at this again after the file moved?)

[commit-bot: I haz the power, 2013-05-16 19:20:25]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-17 09:21:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/14139013/11...

[commit-bot: I haz the power, 2013-05-17 10:00:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/14139013/13...

[commit-bot: I haz the power, 2013-05-17 11:10:05]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Jói, 2013-05-17 18:06:47]

Patch set #14 LGTM

[commit-bot: I haz the power, 2013-05-17 18:30:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/14139013/13...

[commit-bot: I haz the power, 2013-05-17 19:41:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-05-20 11:20:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/14139013/14...

[commit-bot: I haz the power, 2013-05-20 12:27:37]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Michael van Ouwerkerk, 2013-05-20 13:46:32]

This issue has been marked private, does anyone know why? I think that's why the
commit queue is failing. If there is no good reason for this issue to be
private, I'll uncheck that and try submitting it.

[commit-bot: I haz the power, 2013-05-20 14:53:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/14139013/14...

[commit-bot: I haz the power, 2013-05-20 15:01:58]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[brettw, 2013-05-20 23:19:38]

just some random style nits, lgtm

https://codereview.chromium.org/14139013/diff/145001/content/public/common/to...
File content/public/common/top_controls_state.h (right):

https://codereview.chromium.org/14139013/diff/145001/content/public/common/to...
content/public/common/top_controls_state.h:8: namespace content {
Need a blank line after this.

https://codereview.chromium.org/14139013/diff/145001/content/public/common/to...
content/public/common/top_controls_state.h:14: }
And a blank line before this, with a "  // namespace content" at the end of this
line.

https://codereview.chromium.org/14139013/diff/145001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/14139013/diff/145001/content/renderer/render_...
content/renderer/render_widget.cc:1614: const WebKit::WebPoint& scrollPoint) {
Use google-style for arg names

[Michael van Ouwerkerk, 2013-05-21 10:02:13]

Thanks Brett!

https://codereview.chromium.org/14139013/diff/145001/content/public/common/to...
File content/public/common/top_controls_state.h (right):

https://codereview.chromium.org/14139013/diff/145001/content/public/common/to...
content/public/common/top_controls_state.h:8: namespace content {
On 2013/05/20 23:19:38, brettw wrote:
> Need a blank line after this.

Done.

https://codereview.chromium.org/14139013/diff/145001/content/public/common/to...
content/public/common/top_controls_state.h:14: }
On 2013/05/20 23:19:38, brettw wrote:
> And a blank line before this, with a "  // namespace content" at the end of
this
> line.

Done.

https://codereview.chromium.org/14139013/diff/145001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/14139013/diff/145001/content/renderer/render_...
content/renderer/render_widget.cc:1614: const WebKit::WebPoint& scrollPoint) {
On 2013/05/20 23:19:38, brettw wrote:
> Use google-style for arg names 

Done.

[commit-bot: I haz the power, 2013-05-21 10:26:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvanouwerkerk@chromium.org/14139013/13...

[commit-bot: I haz the power, 2013-05-21 14:00:21]

Change committed as 201303