Fix the loading progress bar for Android.

Fix the loading progress bar for Android.

Switch to using a load method that allows us to distinguish
between frames to ensure we only update the loading progress bar
on main frame loads.  Also, clears the progress when starting a load
to nofity the user earlier of a change.

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

[Ted C, 2013-01-08 22:15:11]

+dtrainor for observer changes
+sky for changes to web_contents_impl.cc

[David Trainor- moved to gerrit, 2013-01-08 22:22:17]

lgtm

https://chromiumcodereview.appspot.com/11825007/diff/1/content/browser/androi...
File content/browser/android/web_contents_observer_android.cc (right):

https://chromiumcodereview.appspot.com/11825007/diff/1/content/browser/androi...
content/browser/android/web_contents_observer_android.cc:149:
ScopedJavaLocalRef<jobject> obj = weak_java_observer_.get(env);
maybe use obj(...); (copy constructor?)

https://chromiumcodereview.appspot.com/11825007/diff/1/content/browser/androi...
content/browser/android/web_contents_observer_android.cc:152:
ScopedJavaLocalRef<jstring> jstring_url =
copy constructor?

[Ted C, 2013-01-08 22:43:17]

https://codereview.chromium.org/11825007/diff/1/content/browser/android/web_c...
File content/browser/android/web_contents_observer_android.cc (right):

https://codereview.chromium.org/11825007/diff/1/content/browser/android/web_c...
content/browser/android/web_contents_observer_android.cc:149:
ScopedJavaLocalRef<jobject> obj = weak_java_observer_.get(env);
On 2013/01/08 22:22:18, dtrainor wrote:
> maybe use obj(...); (copy constructor?)

Done.

https://codereview.chromium.org/11825007/diff/1/content/browser/android/web_c...
content/browser/android/web_contents_observer_android.cc:152:
ScopedJavaLocalRef<jstring> jstring_url =
On 2013/01/08 22:22:18, dtrainor wrote:
> copy constructor?

Done.  Updated other usages in this file to keep us copy paste errors down in
the future.

[sky, 2013-01-08 22:54:07]

This seems wrong to me. Why is LoadProgressChanged android specific?

[Ted C, 2013-01-08 23:04:44]

On 2013/01/08 22:54:07, sky wrote:
> This seems wrong to me. Why is LoadProgressChanged android specific?

Why this particular call or why does the method exist at all?

LoadProgressChanged is only exists for OS_ANDROID in the delegate.  It exists
because Android needs a 0-100 value for displaying our determinate progress bar
(other platforms use just the spinny).  No one else needs an explicit value, so
we only expose it to android.

Without resetting it to zero in the start loading call, we think the value
hasn't changed and report the value from the previous page.  This allows us to
clear it back to 0.

The crux of the problem is that DidStartProvisionalLoadForFrame is defined in
the observer, while load progress is defined/maintained in the delegate.  If I
added a DidStartProvisionalLoadForFrame to the delegate, than I could
internalize the resetting of this value in the android explicit code.  I just
thought setting it here was a lesser of the two evils than adding a new method
to the delegate.

[sky, 2013-01-09 00:42:36]

On Tue, Jan 8, 2013 at 3:04 PM,  <tedchoc@chromium.org> wrote:
> On 2013/01/08 22:54:07, sky wrote:
>>
>> This seems wrong to me. Why is LoadProgressChanged android specific?
>
>
> Why this particular call or why does the method exist at all?

Is there a compelling reason to make it android only? Is it expensive
to compute?

  -Scott

> LoadProgressChanged is only exists for OS_ANDROID in the delegate.  It
> exists
> because Android needs a 0-100 value for displaying our determinate progress
> bar
> (other platforms use just the spinny).  No one else needs an explicit value,
> so
> we only expose it to android.
>
> Without resetting it to zero in the start loading call, we think the value
> hasn't changed and report the value from the previous page.  This allows us
> to
> clear it back to 0.
>
> The crux of the problem is that DidStartProvisionalLoadForFrame is defined
> in
> the observer, while load progress is defined/maintained in the delegate.  If
> I
> added a DidStartProvisionalLoadForFrame to the delegate, than I could
> internalize the resetting of this value in the android explicit code.  I
> just
> thought setting it here was a lesser of the two evils than adding a new
> method
> to the delegate.
>
> https://codereview.chromium.org/11825007/

[Ted C, 2013-01-09 00:49:14]

On Tue, Jan 8, 2013 at 4:42 PM, Scott Violet <sky@chromium.org> wrote:

> On Tue, Jan 8, 2013 at 3:04 PM,  <tedchoc@chromium.org> wrote:
> > On 2013/01/08 22:54:07, sky wrote:
> >>
> >> This seems wrong to me. Why is LoadProgressChanged android specific?
> >
> >
> > Why this particular call or why does the method exist at all?
>
> Is there a compelling reason to make it android only? Is it expensive
> to compute?
>

It's already been computed.  The delegate method is the only one that is
actually specific to android.  I'm pretty sure that the piping through
render_view_host_delegate exists for all platforms.  We just ifdef'd it at
the beginning because Android was the only one that needed it.


>
>   -Scott
>
> > LoadProgressChanged is only exists for OS_ANDROID in the delegate.  It
> > exists
> > because Android needs a 0-100 value for displaying our determinate
> progress
> > bar
> > (other platforms use just the spinny).  No one else needs an explicit
> value,
> > so
> > we only expose it to android.
> >
> > Without resetting it to zero in the start loading call, we think the
> value
> > hasn't changed and report the value from the previous page.  This allows
> us
> > to
> > clear it back to 0.
> >
> > The crux of the problem is that DidStartProvisionalLoadForFrame is
> defined
> > in
> > the observer, while load progress is defined/maintained in the delegate.
>  If
> > I
> > added a DidStartProvisionalLoadForFrame to the delegate, than I could
> > internalize the resetting of this value in the android explicit code.  I
> > just
> > thought setting it here was a lesser of the two evils than adding a new
> > method
> > to the delegate.
> >
> > https://codereview.chromium.org/11825007/
>

[sky, 2013-01-09 00:56:11]

On Tue, Jan 8, 2013 at 4:49 PM, Ted Choc <tedchoc@chromium.org> wrote:
> On Tue, Jan 8, 2013 at 4:42 PM, Scott Violet <sky@chromium.org> wrote:
>>
>> On Tue, Jan 8, 2013 at 3:04 PM,  <tedchoc@chromium.org> wrote:
>> > On 2013/01/08 22:54:07, sky wrote:
>> >>
>> >> This seems wrong to me. Why is LoadProgressChanged android specific?
>> >
>> >
>> > Why this particular call or why does the method exist at all?
>>
>> Is there a compelling reason to make it android only? Is it expensive
>> to compute?
>
>
> It's already been computed.  The delegate method is the only one that is
> actually specific to android.  I'm pretty sure that the piping through
> render_view_host_delegate exists for all platforms.  We just ifdef'd it at
> the beginning because Android was the only one that needed it.

Unless there is a compelling reason we generally don't ifdef code of
this scope (meaning smallish and not expensive). Imagine what the code
would look like if we ifdef'd things out only a particular platform
wanted.

  -Scott

>
>>
>>
>>   -Scott
>>
>> > LoadProgressChanged is only exists for OS_ANDROID in the delegate.  It
>> > exists
>> > because Android needs a 0-100 value for displaying our determinate
>> > progress
>> > bar
>> > (other platforms use just the spinny).  No one else needs an explicit
>> > value,
>> > so
>> > we only expose it to android.
>> >
>> > Without resetting it to zero in the start loading call, we think the
>> > value
>> > hasn't changed and report the value from the previous page.  This allows
>> > us
>> > to
>> > clear it back to 0.
>> >
>> > The crux of the problem is that DidStartProvisionalLoadForFrame is
>> > defined
>> > in
>> > the observer, while load progress is defined/maintained in the delegate.
>> > If
>> > I
>> > added a DidStartProvisionalLoadForFrame to the delegate, than I could
>> > internalize the resetting of this value in the android explicit code.  I
>> > just
>> > thought setting it here was a lesser of the two evils than adding a new
>> > method
>> > to the delegate.
>> >
>> > https://codereview.chromium.org/11825007/
>
>

[Ted C, 2013-01-09 01:10:16]

On 2013/01/09 00:56:11, sky wrote:
> On Tue, Jan 8, 2013 at 4:49 PM, Ted Choc <mailto:tedchoc@chromium.org> wrote:
> > On Tue, Jan 8, 2013 at 4:42 PM, Scott Violet <mailto:sky@chromium.org>
wrote:
> >>
> >> On Tue, Jan 8, 2013 at 3:04 PM,  <mailto:tedchoc@chromium.org> wrote:
> >> > On 2013/01/08 22:54:07, sky wrote:
> >> >>
> >> >> This seems wrong to me. Why is LoadProgressChanged android specific?
> >> >
> >> >
> >> > Why this particular call or why does the method exist at all?
> >>
> >> Is there a compelling reason to make it android only? Is it expensive
> >> to compute?
> >
> >
> > It's already been computed.  The delegate method is the only one that is
> > actually specific to android.  I'm pretty sure that the piping through
> > render_view_host_delegate exists for all platforms.  We just ifdef'd it at
> > the beginning because Android was the only one that needed it.
> 
> Unless there is a compelling reason we generally don't ifdef code of
> this scope (meaning smallish and not expensive). Imagine what the code
> would look like if we ifdef'd things out only a particular platform
> wanted.

Cool...removed the ifdef (and this had a very small impact).

> 
>   -Scott
> 
> >
> >>
> >>
> >>   -Scott
> >>
> >> > LoadProgressChanged is only exists for OS_ANDROID in the delegate.  It
> >> > exists
> >> > because Android needs a 0-100 value for displaying our determinate
> >> > progress
> >> > bar
> >> > (other platforms use just the spinny).  No one else needs an explicit
> >> > value,
> >> > so
> >> > we only expose it to android.
> >> >
> >> > Without resetting it to zero in the start loading call, we think the
> >> > value
> >> > hasn't changed and report the value from the previous page.  This allows
> >> > us
> >> > to
> >> > clear it back to 0.
> >> >
> >> > The crux of the problem is that DidStartProvisionalLoadForFrame is
> >> > defined
> >> > in
> >> > the observer, while load progress is defined/maintained in the delegate.
> >> > If
> >> > I
> >> > added a DidStartProvisionalLoadForFrame to the delegate, than I could
> >> > internalize the resetting of this value in the android explicit code.  I
> >> > just
> >> > thought setting it here was a lesser of the two evils than adding a new
> >> > method
> >> > to the delegate.
> >> >
> >> > https://codereview.chromium.org/11825007/
> >
> >

[Ted C, 2013-01-09 01:10:48]

+Nilesh just in case there was a strong reason to make this Android only in the
first place.

[nilesh, 2013-01-09 01:23:26]

https://codereview.chromium.org/11825007/diff/11001/content/public/browser/we...
File content/public/browser/web_contents_delegate.h (right):

https://codereview.chromium.org/11825007/diff/11001/content/public/browser/we...
content/public/browser/web_contents_delegate.h:123: virtual void
LoadProgressChanged(WebContents* source, double progress) {}
If I remember correctly, now the api is lying as this method wont be called for
other platforms.
LoadProgressTracker is not initialized in render_view_impl.cc

[nilesh, 2013-01-09 01:32:46]

Other platforms save (multiple?) IPC messages by not implementing this. I think
this is why this is Android only. Adding Jay as he originally implemented this.

[Ted C, 2013-01-09 01:34:34]

Hmm...that is unfortunate.  I wonder if it would be better to document
LoadProgressChanged will only be called if the LoadProgressTracker is
enabled in the renderer.  I guess that is more error prone than the ifdef.

On Tue, Jan 8, 2013 at 5:23 PM, <nileshagrawal@chromium.org> wrote:

>
> https://codereview.chromium.**org/11825007/diff/11001/**
>
content/public/browser/web_**contents_delegate.h<https://codereview.chromium.org/11825007/diff/11001/content/public/browser/web_contents_delegate.h>
> File content/public/browser/web_**contents_delegate.h (right):
>
> https://codereview.chromium.**org/11825007/diff/11001/**
>
content/public/browser/web_**contents_delegate.h#newcode123<https://codereview.chromium.org/11825007/diff/11001/content/public/browser/web_contents_delegate.h#newcode123>
> content/public/browser/web_**contents_delegate.h:123: virtual void
> LoadProgressChanged(**WebContents* source, double progress) {}
> If I remember correctly, now the api is lying as this method wont be
> called for other platforms.
> LoadProgressTracker is not initialized in render_view_impl.cc
>
>
https://codereview.chromium.**org/11825007/<https://codereview.chromium.org/1...
>

[sky, 2013-01-09 16:57:18]

Is LoadProgressTracker behind an ifdef? The two should be consistent.
So, if we have an ifdef there then continue with the ifdef of the
method like you originally had.

  -Scott

On Tue, Jan 8, 2013 at 5:34 PM, Ted Choc <tedchoc@chromium.org> wrote:
> Hmm...that is unfortunate.  I wonder if it would be better to document
> LoadProgressChanged will only be called if the LoadProgressTracker is
> enabled in the renderer.  I guess that is more error prone than the ifdef.
>
> On Tue, Jan 8, 2013 at 5:23 PM, <nileshagrawal@chromium.org> wrote:
>>
>>
>>
>>
https://codereview.chromium.org/11825007/diff/11001/content/public/browser/we...
>> File content/public/browser/web_contents_delegate.h (right):
>>
>>
>>
https://codereview.chromium.org/11825007/diff/11001/content/public/browser/we...
>> content/public/browser/web_contents_delegate.h:123: virtual void
>> LoadProgressChanged(WebContents* source, double progress) {}
>> If I remember correctly, now the api is lying as this method wont be
>> called for other platforms.
>> LoadProgressTracker is not initialized in render_view_impl.cc
>>
>> https://codereview.chromium.org/11825007/
>
>

[Ted C, 2013-01-09 16:59:53]

The LoadProgressTracker is behind an ifdef, so I'll revert back to my
previous change.


On Wed, Jan 9, 2013 at 8:57 AM, Scott Violet <sky@chromium.org> wrote:

> Is LoadProgressTracker behind an ifdef? The two should be consistent.
> So, if we have an ifdef there then continue with the ifdef of the
> method like you originally had.
>
>   -Scott
>
> On Tue, Jan 8, 2013 at 5:34 PM, Ted Choc <tedchoc@chromium.org> wrote:
> > Hmm...that is unfortunate.  I wonder if it would be better to document
> > LoadProgressChanged will only be called if the LoadProgressTracker is
> > enabled in the renderer.  I guess that is more error prone than the
> ifdef.
> >
> > On Tue, Jan 8, 2013 at 5:23 PM, <nileshagrawal@chromium.org> wrote:
> >>
> >>
> >>
> >>
>
https://codereview.chromium.org/11825007/diff/11001/content/public/browser/we...
> >> File content/public/browser/web_contents_delegate.h (right):
> >>
> >>
> >>
>
https://codereview.chromium.org/11825007/diff/11001/content/public/browser/we...
> >> content/public/browser/web_contents_delegate.h:123: virtual void
> >> LoadProgressChanged(WebContents* source, double progress) {}
> >> If I remember correctly, now the api is lying as this method wont be
> >> called for other platforms.
> >> LoadProgressTracker is not initialized in render_view_impl.cc
> >>
> >> https://codereview.chromium.org/11825007/
> >
> >
>

[Ted C, 2013-01-09 19:01:37]

Ok...rolled back to the previous version.  PTAL

[sky, 2013-01-09 20:39:36]

https://codereview.chromium.org/11825007/diff/15001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11825007/diff/15001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1988: if (delegate_ &&
is_main_frame)
Can this invoke DidChangeLoadProgress so that you don't have an ifdef here?

[Ted C, 2013-01-09 21:26:00]

https://codereview.chromium.org/11825007/diff/15001/content/browser/web_conte...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/11825007/diff/15001/content/browser/web_conte...
content/browser/web_contents/web_contents_impl.cc:1988: if (delegate_ &&
is_main_frame)
On 2013/01/09 20:39:37, sky wrote:
> Can this invoke DidChangeLoadProgress so that you don't have an ifdef here?

Done.

[sky, 2013-01-09 22:26:23]

LGTM

[commit-bot: I haz the power, 2013-01-09 22:41:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tedchoc@chromium.org/11825007/9012

[commit-bot: I haz the power, 2013-01-10 01:43:59]

Change committed as 175974