Hide location bar on WebKit programmatic 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

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

BUG=165317

[John Knottenbelt, 2013-01-16 18:05:36]

Ted, please could you take a look. If you think that this CL is ready for wider
review, please could you suggest some reviewers?

[enne (OOO), 2013-01-16 18:12:08]

Whoa, plumbing.

Can't you determine via a change during commit in LayerImpl::scrollOffset that a
scroll has happened from Javascript?

[John Knottenbelt, 2013-01-17 18:39:17]

Hi enne, thanks for taking a look. I'm new to this code, so, please, any
suggestions on how to do this in a better way are very much appreciated!

One difficulty is that window.scrollTo(0,1) should hide the URL bar even if the
scroll position is already at (0,1). So looking for a change in scroll offsets
won't be a complete solution.

Another difficulty is that in WebKit, there are multiple sources of what appear
to be Javascript initiated scrolls from Chromium's point of view, although
perhaps there is a way of telling them apart I'm missing. For example, when the
page has finished loading WebKit will try to restore the page offset and scroll
position. In this case, we shouldn't hide the URL bar.

Another way in which the scrollOffset can be set is when you touch on a link and
the zoomed link disambiguater popup shows. 

I tried an experiment along the lines of what you suggested above:

void LayerImpl::setScrollOffset(gfx::Vector2d scrollOffset)
{
    if (m_layerTreeImpl && m_layerTreeImpl->root_scroll_layer() == this &&
        m_layerTreeImpl->currently_scrolling_layer() != this) {
         LOG(ERROR) << "LayerImpl(" << this << ")::setScrollOffset("
                    << scrollOffset.x() << ", " << scrollOffset.y() 
                    << ")";
    }


This hides some of the scrolls due to the user, but not all. If I scroll quickly
or change direction I see the message. I think this is to be expected because
there will be some lag between the layer tree impl stopping scrolling and the
scroll events coming back from WebKit.

On 2013/01/16 18:12:08, enne wrote:
> Whoa, plumbing.
> 
> Can't you determine via a change during commit in LayerImpl::scrollOffset that
a
> scroll has happened from Javascript?

[John Knottenbelt, 2013-01-22 18:36:48]

In https://bugs.webkit.org/show_bug.cgi?id=107027, JamesR that I should plumb
the scroll event through to WebWidgetClient or WebViewClient rather than through
WebLayerTreeHost. 

I can't seem to figure out how to get from the RenderViewImpl, which implements
WebWidgetClient/WebViewClient to the cc::LayerTreeHostImpl that owns the
TopControls. Do you have any ideas on how this might be done?

Thanks

On 2013/01/17 18:39:17, John Knottenbelt wrote:
> Hi enne, thanks for taking a look. I'm new to this code, so, please, any
> suggestions on how to do this in a better way are very much appreciated!
> 
> One difficulty is that window.scrollTo(0,1) should hide the URL bar even if
the
> scroll position is already at (0,1). So looking for a change in scroll offsets
> won't be a complete solution.
> 
> Another difficulty is that in WebKit, there are multiple sources of what
appear
> to be Javascript initiated scrolls from Chromium's point of view, although
> perhaps there is a way of telling them apart I'm missing. For example, when
the
> page has finished loading WebKit will try to restore the page offset and
scroll
> position. In this case, we shouldn't hide the URL bar.
> 
> Another way in which the scrollOffset can be set is when you touch on a link
and
> the zoomed link disambiguater popup shows. 
> 
> I tried an experiment along the lines of what you suggested above:
> 
> void LayerImpl::setScrollOffset(gfx::Vector2d scrollOffset)
> {
>     if (m_layerTreeImpl && m_layerTreeImpl->root_scroll_layer() == this &&
>         m_layerTreeImpl->currently_scrolling_layer() != this) {
>          LOG(ERROR) << "LayerImpl(" << this << ")::setScrollOffset("
>                     << scrollOffset.x() << ", " << scrollOffset.y() 
>                     << ")";
>     }
> 
> 
> This hides some of the scrolls due to the user, but not all. If I scroll
quickly
> or change direction I see the message. I think this is to be expected because
> there will be some lag between the layer tree impl stopping scrolling and the
> scroll events coming back from WebKit.
> 
> On 2013/01/16 18:12:08, enne wrote:
> > Whoa, plumbing.
> > 
> > Can't you determine via a change during commit in LayerImpl::scrollOffset
that
> a
> > scroll has happened from Javascript?

[enne (OOO), 2013-01-22 19:01:27]

I'm not exactly sure what jamesr was suggesting.

At the very least, http://code.google.com/p/chromium/issues/detail?id=156175
should help you get to the LayerTreeHost without going through WebLayerTreeView.

However, this sort of patch makes me wonder if the TopControlsManager is
misplaced by being in cc on the LayerTreeHostImpl.  Maybe cc should be sending
more scroll-related information out rather than adding more information being
sent in.

[John Knottenbelt, 2013-01-24 18:18:28]

Ted, is this the correct way of hiding the top controls? Should I be plumbing
through to the browser process instead? It seems that it would be nice to have
all the code responsible for hiding and showing the bar in one place.

[Ted C, 2013-01-24 22:01:00]

It seems like the right place to have it hide the top controls (i.e. in the
renderer by the top controls manager), and piping it back through the browser
process seems unnecessary to me.

Granted, I have no idea how you should get the signal from webkit to the top
controls manager.

When talking (James and I) about communications from LayerTreeHost ->
LayerTreeHostImpl -> TopControlsManager, we initially though of just having a
weak ptr in LayerTreeHost that would allow you to push messages to it on the
compositor thread without plumbing all the messages through the proxies.

As for getting the messages from Webkit, I think the two options are an
additional interface on LayerTreeHost that has some Webkit corollary, which is
what this change appears to have.  The other is to pipe it to render_widget and
have it dispatch it to LayerTreeHost. I don't know if James's ownership change
has landed that would allow this, but it's on the radar for sure.

Sadly, I probably won't be too much help in giving direction on how to get the
message from Webkit.

https://chromiumcodereview.appspot.com/11967015/diff/13001/cc/top_controls_ma...
File cc/top_controls_manager.cc (right):

https://chromiumcodereview.appspot.com/11967015/diff/13001/cc/top_controls_ma...
cc/top_controls_manager.cc:55: ScrollBy(gfx::Vector2dF(0,
top_controls_height_));
hmm...so any javascript scroll will attempt to hide the top controls?  This
should definitely do the trick, but I thought it was only supposed to be limited
to scroll(0,0) or scroll(0,1), but I guess maybe that is what you were saying in
the recent meeting where all types of things hide the top controls.

https://chromiumcodereview.appspot.com/11967015/diff/13001/cc/top_controls_ma...
cc/top_controls_manager.cc:56: StartAnimationIfNecessary();
The start animation if necessary shouldn't be required as you should also be
hiding it entirely.

If you want it to always hide the controls but animate it, you should be able to
replace both of these lines with this:
SetupAnimation(HIDING_CONTROLS)

[John Knottenbelt, 2013-01-30 16:08:45]

https://codereview.chromium.org/11967015/diff/13001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11967015/diff/13001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:55: ScrollBy(gfx::Vector2dF(0,
top_controls_height_));
We can restrict it here to just 0,0 and 0,1 if we decide that this is what we
really want, but I think hiding on any programmatic scroll matches existing
mobile browsers behaviour more closely. 

On 2013/01/24 22:01:01, Ted C wrote:
> hmm...so any javascript scroll will attempt to hide the top controls?  This
> should definitely do the trick, but I thought it was only supposed to be
limited
> to scroll(0,0) or scroll(0,1), but I guess maybe that is what you were saying
in
> the recent meeting where all types of things hide the top controls.

https://codereview.chromium.org/11967015/diff/13001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:56: StartAnimationIfNecessary();
On 2013/01/24 22:01:01, Ted C wrote:
> The start animation if necessary shouldn't be required as you should also be
> hiding it entirely.
> 
> If you want it to always hide the controls but animate it, you should be able
to
> replace both of these lines with this:
> SetupAnimation(HIDING_CONTROLS)

Done.

[jamesr, 2013-03-04 21:44:03]

Out of curiosity, where's the code to show the URL bar when the user does a
pan-down gesture?

https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:65: // We're in a user scroll.
This seems kind of iffy - who's responsible for deciding if the top controls
hide or not?  Is it cc:: or the embedder (i.e. content) or WebKit or what?  I
think we need to decide on what the source of truth is and not second-guess it.

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

https://codereview.chromium.org/11967015/diff/27001/content/renderer/render_w...
content/renderer/render_widget.cc:1485: if (!compositor_)
Do you expect this case to be hit?  Why?

[John Knottenbelt, 2013-03-05 16:41:37]

On 2013/03/04 21:44:03, jamesr wrote:
> Out of curiosity, where's the code to show the URL bar when the user does a
> pan-down gesture?

Thanks for review!

See inline how the offsets managed by the TopControlsManager in cc can determine
the position of the URL bar and content view as the user scrolls the page.

If the page is consuming the scroll (e.g. like maps.google.com), we do not, by
design, want to show the URL bar on pan down gesture, as this would interfere
with the user's map browsing experience. 

When WebKit requests the URL bar to be hidden for the first time, the plan is to
show an on screen bubble, in the same way as will happen for fullscreen API. See
http://crbug.com/176206 for more details.

> 
> https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.cc
> File cc/top_controls_manager.cc (right):
> 
>
https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.c...
> cc/top_controls_manager.cc:65: // We're in a user scroll.
> This seems kind of iffy - who's responsible for deciding if the top controls
> hide or not?  Is it cc:: or the embedder (i.e. content) or WebKit or what?  I
> think we need to decide on what the source of truth is and not second-guess
it.
> 
>
https://codereview.chromium.org/11967015/diff/27001/content/renderer/render_w...
> File content/renderer/render_widget.cc (right):
> 
>
https://codereview.chromium.org/11967015/diff/27001/content/renderer/render_w...
> content/renderer/render_widget.cc:1485: if (!compositor_)
> Do you expect this case to be hit?  Why?

[John Knottenbelt, 2013-03-05 16:41:47]

https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:65: // We're in a user scroll.
The ultimate authority on whether the top controls should be showing is the java
FullscreenManager in the browser process. 

When the user scrolls, the TopControlsManager in cc is updating 
controls_top_offset_ and content_top_offset_. These values are plumbed through
to the browser process java FullscreenManager where they are used to translate
the top controls view (omnibox) and the content view. These values may be
overridden by the FullscreenManager under certain circumstances, for example,
the top controls will stay on screen while the page is loading, regardless of
whether the user is scrolling.

The question that the if statement is trying to address is, "what should happen
to the top controls if the user is scrolling at the same time as WebKit?" It
seems that the scroll that happened last will determine the content scrollY, so
I think, for consistency, it should also determine the position of the top
controls. This means that the if statement (line 66, 67) should be removed.

On 2013/03/04 21:44:03, jamesr wrote:
> This seems kind of iffy - who's responsible for deciding if the top controls
> hide or not?  Is it cc:: or the embedder (i.e. content) or WebKit or what?  I
> think we need to decide on what the source of truth is and not second-guess
it.

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

https://codereview.chromium.org/11967015/diff/27001/content/renderer/render_w...
content/renderer/render_widget.cc:1485: if (!compositor_)
On 2013/03/04 21:44:03, jamesr wrote:
> Do you expect this case to be hit?  Why?

It seems that all method invocations on compositor_ in this file are guarded by
a NULL check.

I would expect that for Chrome on Android, compositor_ would be non-null, as the
call to hideTopControls originates from a WebKit scroll, and I believe that the
compositor_ should already have been set up by then.

For platforms that are not use accelerated compositing, or have it disabled due
to a switch, the compositor_ will be NULL. hideTopControls() method is public on
RenderWidget, so it could still be called from WebKit.

[jamesr, 2013-03-05 18:38:16]

On 2013/03/05 16:41:37, John Knottenbelt wrote:
> On 2013/03/04 21:44:03, jamesr wrote:
> > Out of curiosity, where's the code to show the URL bar when the user does a
> > pan-down gesture?
> 
> Thanks for review!
> 
> See inline how the offsets managed by the TopControlsManager in cc can
determine
> the position of the URL bar and content view as the user scrolls the page.
> 
> If the page is consuming the scroll (e.g. like http://maps.google.com), we do
not, by
> design, want to show the URL bar on pan down gesture, as this would interfere
> with the user's map browsing experience. 

Hmm - this is not consistent with the security team's feedback or Android
Browser's behavior.  If the page consuming the touch suppressed showing the URL
bar, it'd be trivial to trap the user in a situation where they could never get
access to the URL bar.

[John Knottenbelt, 2013-03-11 17:23:06]

https://codereview.chromium.org/11967015/diff/27001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11967015/diff/27001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:518: m_proxy->implThread()->postTask(
Posting the task to the TopControlsManager in this way has the disadvantage that
the request to hide the top controls could arrive before the Commit() arising
from Javascript adjusting the offset of the scroll layer.

An alternative is to follow the same pattern as setPageScaleFactorAndLimits and
call setNeedsCommit. This could be picked up in
LayerTreeHost::finishCommitOnImplThread. This should be the same commit that
applies the change in scroll offset from Javascript.

What do you think? Should I implement this alternative flow?

[palmer, 2013-03-11 21:57:52]

With this change, the Java FullscreenManager will get invoked, thus explaining
to the user how to get out of fullscreen mode, right?

[John Knottenbelt, 2013-03-12 11:13:40]

On 2013/03/11 21:57:52, Chris P. wrote:
> With this change, the Java FullscreenManager will get invoked, thus explaining
> to the user how to get out of fullscreen mode, right?

Yes, that plumbing isn't here yet, but that is the intention. We will need to
send a separate message to the FullscreenManager to indicate that the site
hidden the URL bar via the scrollTo API.

[palmer, 2013-03-12 19:44:57]

> Yes, that plumbing isn't here yet, but that is the intention. We will need to
> send a separate message to the FullscreenManager to indicate that the site
> hidden the URL bar via the scrollTo API.

Ok. Could you please CC me on that CL too, when it comes? Thanks.

[jamesr, 2013-03-14 19:38:34]

https://codereview.chromium.org/11967015/diff/27001/cc/layer_tree_host.cc
File cc/layer_tree_host.cc (right):

https://codereview.chromium.org/11967015/diff/27001/cc/layer_tree_host.cc#new...
cc/layer_tree_host.cc:518: m_proxy->implThread()->postTask(
On 2013/03/11 17:23:06, John Knottenbelt wrote:
> Posting the task to the TopControlsManager in this way has the disadvantage
that
> the request to hide the top controls could arrive before the Commit() arising
> from Javascript adjusting the offset of the scroll layer.

I believe it always will arrive before the Commit since both are run as tasks
posted from the main thread to the impl (compositor) thread and tasks are run in
FIFO order.

> 
> An alternative is to follow the same pattern as setPageScaleFactorAndLimits
and
> call setNeedsCommit. This could be picked up in
> LayerTreeHost::finishCommitOnImplThread. This should be the same commit that
> applies the change in scroll offset from Javascript.
> 
> What do you think? Should I implement this alternative flow?

Do you need the top-controls hiding to be evaluated at the same time as the
scroll offset?  If so then yes, I think communicating this information along
with the commit is the right thing to do.  I'm not sure why the two need to be
evaluated together, though.

https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.cc
File cc/top_controls_manager.cc (right):

https://codereview.chromium.org/11967015/diff/27001/cc/top_controls_manager.c...
cc/top_controls_manager.cc:65: // We're in a user scroll.
On 2013/03/05 16:41:48, John Knottenbelt wrote:
> The ultimate authority on whether the top controls should be showing is the
java
> FullscreenManager in the browser process. 
> 
> When the user scrolls, the TopControlsManager in cc is updating 
> controls_top_offset_ and content_top_offset_. These values are plumbed through
> to the browser process java FullscreenManager where they are used to translate
> the top controls view (omnibox) and the content view. These values may be
> overridden by the FullscreenManager under certain circumstances, for example,
> the top controls will stay on screen while the page is loading, regardless of
> whether the user is scrolling.
> 
> The question that the if statement is trying to address is, "what should
happen
> to the top controls if the user is scrolling at the same time as WebKit?" It
> seems that the scroll that happened last will determine the content scrollY,
so
> I think, for consistency, it should also determine the position of the top
> controls. This means that the if statement (line 66, 67) should be removed.

Yeah, I think this if should be removed.

> 
> On 2013/03/04 21:44:03, jamesr wrote:
> > This seems kind of iffy - who's responsible for deciding if the top controls
> > hide or not?  Is it cc:: or the embedder (i.e. content) or WebKit or what? 
I
> > think we need to decide on what the source of truth is and not second-guess
> it.
>

[John Knottenbelt, 2013-03-25 15:29:47]

In-person code review from Ted and Michael.

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

https://codereview.chromium.org/11967015/diff/46001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:70: void
TopControlsManager::ShowTopControls(bool show) {
c++ indenting instead of java style

https://codereview.chromium.org/11967015/diff/46001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/11967015/diff/46001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:975: {
To previous line.

https://codereview.chromium.org/11967015/diff/46001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:976: proxy_->ImplThread()->PostTask(
Add  if (!settings_.calculate_top_controls_position)
  967     return;	  967     return;

https://codereview.chromium.org/11967015/diff/46001/components/web_contents_d...
File
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java
(right):

https://codereview.chromium.org/11967015/diff/46001/components/web_contents_d...
components/web_contents_delegate_android/android/java/src/org/chromium/components/web_contents_delegate_android/WebContentsDelegateAndroid.java:120:
public void requestShowTopControls(ContentViewCore contentViewCore, boolean
show) {
Add a JavaDoc and remove contentViewCore.

https://codereview.chromium.org/11967015/diff/46001/content/renderer/render_v...
File content/renderer/render_view_impl_android.cc (right):

https://codereview.chromium.org/11967015/diff/46001/content/renderer/render_v...
content/renderer/render_view_impl_android.cc:23: if (compositor_) {
remove these braces...whoever added them above should be ashamed

[Ted C, 2013-03-25 16:57:35]

lgtm w/ small nit  -- thanks for fixing my bracing shame :-D

https://chromiumcodereview.appspot.com/11967015/diff/49002/content/public/bro...
File content/public/browser/web_contents_delegate.h (right):

https://chromiumcodereview.appspot.com/11967015/diff/49002/content/public/bro...
content/public/browser/web_contents_delegate.h:347: virtual void
RequestShowTopControls(WebContents* web_contents, bool show);
looks like all void functions have their empty default implementations
inlined...I would assume do that for consistency, but I can't speak for the
owners whether they want to keep it this way.

[palmer, 2013-03-25 21:16:12]

IPC security LGTM.

My comments about renaming the messages might be off-base; feel free to ignore
me if necessary.

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

https://codereview.chromium.org/11967015/diff/55001/content/common/view_messa...
content/common/view_messages.h:1505: // hidden the URL bar programmatically by
means of a scroll.
"has hidden or shown"

NIT/ASTRONAUTISM: The name |RequestShowTopControls| isn't really distinct enough
from |ShowTopControls| above.  The names alone don't tell me that the two
messages differ in what direction the messages go, which seems important to me.
Consider giving these two types of IPC better names, if possible?

[aelias_OOO_until_Jul13, 2013-03-25 22:35:35]

I'm told that we got approval from security team to not show the warning bubble
in the scrollTo(0, 1) case.  So there's no reason to have the
RequestShowTopControls message at all.  Please delete that plumbing and just do
the call directly from renderer to renderer.

[aelias_OOO_until_Jul13, 2013-03-25 22:35:44]

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

https://codereview.chromium.org/11967015/diff/55001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:72: if (controls_top_offset_ == 0)
Please add these two early outs to SetupAnimation instead.

https://codereview.chromium.org/11967015/diff/55001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:80: }
Looks like you're missing a client_->setNeedsRedraw() here.  The other callers
of SetupAnimation all do it, and it's needed to reliably kick off the animation.
 I recommend adding setNeedsRedraw to the end of SetupAnimation and removing it
from the other callsites.

[Ted C, 2013-03-25 22:44:33]

The plumbing exists because the browser manages whether the top controls
can hide due to various states (ime shown, page is loading, HTTPS
certificate is broken).  Notifying the browser allows it to decide whether
it should hide the controls, delay the hiding until hiding is enabled
again, or discard the request if its not valid.

If the render talks directly to the renderer then we'll need to track when
to process the request.  Also, we'd need to clear the request on
navigation.  It seems cleaner to keep all the state management in the
browser to avoid these syncing annoyances.

- Ted
On Mar 25, 2013 10:35 PM, <aelias@chromium.org> wrote:

>
> https://codereview.chromium.**org/11967015/diff/55001/cc/**
>
input/top_controls_manager.cc<https://codereview.chromium.org/11967015/diff/55001/cc/input/top_controls_manager.cc>
> File cc/input/top_controls_manager.**cc (right):
>
> https://codereview.chromium.**org/11967015/diff/55001/cc/**
>
input/top_controls_manager.cc#**newcode72<https://codereview.chromium.org/11967015/diff/55001/cc/input/top_controls_manager.cc#newcode72>
> cc/input/top_controls_manager.**cc:72: if (controls_top_offset_ == 0)
> Please add these two early outs to SetupAnimation instead.
>
> https://codereview.chromium.**org/11967015/diff/55001/cc/**
>
input/top_controls_manager.cc#**newcode80<https://codereview.chromium.org/11967015/diff/55001/cc/input/top_controls_manager.cc#newcode80>
> cc/input/top_controls_manager.**cc:80: }
> Looks like you're missing a client_->setNeedsRedraw() here.  The other
> callers of SetupAnimation all do it, and it's needed to reliably kick
> off the animation.  I recommend adding setNeedsRedraw to the end of
> SetupAnimation and removing it from the other callsites.
>
>
https://codereview.chromium.**org/11967015/<https://codereview.chromium.org/1...
>

[aelias_OOO_until_Jul13, 2013-03-25 22:51:09]

OK, that seems reasonable.  We can leave the message and just change the name:

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

https://codereview.chromium.org/11967015/diff/55001/content/common/view_messa...
content/common/view_messages.h:1505: // hidden the URL bar programmatically by
means of a scroll.
How about renaming this to ViewHostMsg_DidProgrammaticallyScroll?  We could call
this "DidProgrammaticallyScroll" all the way from WebKit to the browser.

[John Knottenbelt, 2013-03-26 14:49:17]

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

https://codereview.chromium.org/11967015/diff/55001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:72: if (controls_top_offset_ == 0)
On 2013/03/25 22:35:44, aelias wrote:
> Please add these two early outs to SetupAnimation instead.

Done.

https://codereview.chromium.org/11967015/diff/55001/cc/input/top_controls_man...
cc/input/top_controls_manager.cc:80: }
On 2013/03/25 22:35:44, aelias wrote:
> Looks like you're missing a client_->setNeedsRedraw() here.  The other callers
> of SetupAnimation all do it, and it's needed to reliably kick off the
animation.
>  I recommend adding setNeedsRedraw to the end of SetupAnimation and removing
it
> from the other callsites.

Done.

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

https://codereview.chromium.org/11967015/diff/55001/content/common/view_messa...
content/common/view_messages.h:1505: // hidden the URL bar programmatically by
means of a scroll.
On 2013/03/25 22:51:09, aelias wrote:
> How about renaming this to ViewHostMsg_DidProgrammaticallyScroll?  We could
call
> this "DidProgrammaticallyScroll" all the way from WebKit to the browser.

Done.

[Ted C, 2013-03-26 16:19:09]

lgtm (again) w/ a couple nits.

As for the asymmetric messages (didProgrammaticallyScroll from renderer and
showTopControls back), we think having the ability to show and hide the top
controls from the browser will allow us to push some of the transient show
behavior currently done in the browser back to the renderer.  We just want to
avoid having to pollute the various plumbing places with too many messages in
the future and this will give us the functionality we need in the foreseeable
future.

https://chromiumcodereview.appspot.com/11967015/diff/65001/components/web_con...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/11967015/diff/65001/components/web_con...
components/web_contents_delegate_android/web_contents_delegate_android.cc:312: 
one too many blank lines

https://chromiumcodereview.appspot.com/11967015/diff/65001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/11967015/diff/65001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2496:

remove extra blank line

[palmer, 2013-03-26 16:25:41]

On 2013/03/25 22:44:33, Ted C wrote:

> The plumbing exists because the browser manages whether the top controls
> can hide due to various states (ime shown, page is loading, HTTPS
> certificate is broken).  Notifying the browser allows it to decide whether
> it should hide the controls, delay the hiding until hiding is enabled
> again, or discard the request if its not valid.
> 
> If the render talks directly to the renderer then we'll need to track when
> to process the request.  Also, we'd need to clear the request on
> navigation.  It seems cleaner to keep all the state management in the
> browser to avoid these syncing annoyances.

I'm not sure I understand, please forgive me if I'm off-base.

For correctness and security, we need to have the browser process decide when to
hide or show the controls; that decision should never be in the hands of the
renderer. Is that the case with this CL?

[John Knottenbelt, 2013-03-26 16:43:16]

https://chromiumcodereview.appspot.com/11967015/diff/65001/components/web_con...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/11967015/diff/65001/components/web_con...
components/web_contents_delegate_android/web_contents_delegate_android.cc:312: 
On 2013/03/26 16:19:10, Ted C wrote:
> one too many blank lines

Done.

https://chromiumcodereview.appspot.com/11967015/diff/65001/content/public/and...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://chromiumcodereview.appspot.com/11967015/diff/65001/content/public/and...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2496:

On 2013/03/26 16:19:10, Ted C wrote:
> remove extra blank line

Done.

[Ted C, 2013-03-26 16:47:08]

On 2013/03/26 16:25:41, Chris P. wrote:
> On 2013/03/25 22:44:33, Ted C wrote:
> 
> > The plumbing exists because the browser manages whether the top controls
> > can hide due to various states (ime shown, page is loading, HTTPS
> > certificate is broken).  Notifying the browser allows it to decide whether
> > it should hide the controls, delay the hiding until hiding is enabled
> > again, or discard the request if its not valid.
> > 
> > If the render talks directly to the renderer then we'll need to track when
> > to process the request.  Also, we'd need to clear the request on
> > navigation.  It seems cleaner to keep all the state management in the
> > browser to avoid these syncing annoyances.
> 
> I'm not sure I understand, please forgive me if I'm off-base.
> 
> For correctness and security, we need to have the browser process decide when
to
> hide or show the controls; that decision should never be in the hands of the
> renderer. Is that the case with this CL?

Currently, this more about correctness (but does cover security as well).  By
sending this back through the browser process, we can disregard the request if
we are disabling hiding of the top controls for security reasons (i.e. broken
https certificate).

But also allows us to delay and honor the hide request if forcing the top
controls to be visible  for other reasons (page is loading, ime is showing). 
Once either/both of these states are done, we can honor a pages request to hide
the top controls.

[palmer, 2013-03-26 17:40:39]

> Currently, this more about correctness (but does cover security as well).  By
> sending this back through the browser process, we can disregard the request if
> we are disabling hiding of the top controls for security reasons (i.e. broken
> https certificate).
> 
> But also allows us to delay and honor the hide request if forcing the top
> controls to be visible  for other reasons (page is loading, ime is showing). 
> Once either/both of these states are done, we can honor a pages request to
hide
> the top controls.

Right, so we agree that the browser process is setting hide/show policy. Thanks!