Report load progress for Android

Make WebContentsDelegate::LoadProgressChanged android only.
    
With this CL LoadProgressChanged is only called on android, earlier it was never called.

BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163704

[Yaron, 2012-10-18 00:19:50]

lgtm

[nilesh, 2012-10-18 22:07:08]

James, Can you please take a look. I need an OWNER approval.

[jamesr, 2012-10-18 22:13:11]

http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
content/renderer/render_view_impl.cc:675: SetReportLoadProgressEnabled(true);
I can't find any other callers to this function in the codebase - is it accurate
that this is enabling a new feature that's never been on before, or am I missing
existing callers?

[nilesh, 2012-10-18 22:18:33]

On 2012/10/18 22:13:11, jamesr wrote:
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> File content/renderer/render_view_impl.cc (right):
> 
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> content/renderer/render_view_impl.cc:675: SetReportLoadProgressEnabled(true);
> I can't find any other callers to this function in the codebase - is it
accurate
> that this is enabling a new feature that's never been on before, or am I
missing
> existing callers?

Yes, this missing call is part of upstreaming the Android port.
I don't think any other platform uses it. This enables the load progress bar on
Android.

[jamesr, 2012-10-18 22:21:40]

On 2012/10/18 22:18:33, nileshagrawal1 wrote:
> On 2012/10/18 22:13:11, jamesr wrote:
> >
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> > File content/renderer/render_view_impl.cc (right):
> > 
> >
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> > content/renderer/render_view_impl.cc:675:
SetReportLoadProgressEnabled(true);
> > I can't find any other callers to this function in the codebase - is it
> accurate
> > that this is enabling a new feature that's never been on before, or am I
> missing
> > existing callers?
> 
> Yes, this missing call is part of upstreaming the Android port.
> I don't think any other platform uses it. This enables the load progress bar
on
> Android.

OK. It seems to either be anticipating more use cases in the future or be a bit
overly complicated.  In the current use, if you plan to just have it on in
OS(ANDROID) and off elsewhere then I think you should just initialize the
LoadProgressTracker in the initializer list and not bother with a setter that's
never called.

[nilesh, 2012-10-18 23:10:52]

On 2012/10/18 22:21:40, jamesr wrote:
> On 2012/10/18 22:18:33, nileshagrawal1 wrote:
> > On 2012/10/18 22:13:11, jamesr wrote:
> > >
> >
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> > > File content/renderer/render_view_impl.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> > > content/renderer/render_view_impl.cc:675:
> SetReportLoadProgressEnabled(true);
> > > I can't find any other callers to this function in the codebase - is it
> > accurate
> > > that this is enabling a new feature that's never been on before, or am I
> > missing
> > > existing callers?
> > 
> > Yes, this missing call is part of upstreaming the Android port.
> > I don't think any other platform uses it. This enables the load progress bar
> on
> > Android.
> 
> OK. It seems to either be anticipating more use cases in the future or be a
bit
> overly complicated.  In the current use, if you plan to just have it on in
> OS(ANDROID) and off elsewhere then I think you should just initialize the
> LoadProgressTracker in the initializer list and not bother with a setter
that's
> never called.

The content/public/browser/web_contents_delegate.h interface says that, to
receive LoadProgressChanged notifications this method should be called on the
RenderView. If we remove SetReportLoadProgressEnabled then we also need to
modify the interface to say that LoadProgressChanged will only be called on
Android (or maybe make it an Android only method). WDYT?

[jamesr, 2012-10-18 23:14:29]

On 2012/10/18 23:10:52, nileshagrawal1 wrote:
> On 2012/10/18 22:21:40, jamesr wrote:
> > On 2012/10/18 22:18:33, nileshagrawal1 wrote:
> > > On 2012/10/18 22:13:11, jamesr wrote:
> > > >
> > >
> >
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> > > > File content/renderer/render_view_impl.cc (right):
> > > > 
> > > >
> > >
> >
>
http://codereview.chromium.org/11194043/diff/8001/content/renderer/render_vie...
> > > > content/renderer/render_view_impl.cc:675:
> > SetReportLoadProgressEnabled(true);
> > > > I can't find any other callers to this function in the codebase - is it
> > > accurate
> > > > that this is enabling a new feature that's never been on before, or am I
> > > missing
> > > > existing callers?
> > > 
> > > Yes, this missing call is part of upstreaming the Android port.
> > > I don't think any other platform uses it. This enables the load progress
bar
> > on
> > > Android.
> > 
> > OK. It seems to either be anticipating more use cases in the future or be a
> bit
> > overly complicated.  In the current use, if you plan to just have it on in
> > OS(ANDROID) and off elsewhere then I think you should just initialize the
> > LoadProgressTracker in the initializer list and not bother with a setter
> that's
> > never called.
> 
> The content/public/browser/web_contents_delegate.h interface says that, to
> receive LoadProgressChanged notifications this method should be called on the
> RenderView. If we remove SetReportLoadProgressEnabled then we also need to
> modify the interface to say that LoadProgressChanged will only be called on
> Android (or maybe make it an Android only method). WDYT?

Yes, but as far as I can tell content/public/browser/web_contents_delegate.h is
referring to a non-exposed function that is currently not called.  Maybe
historically this code was different?  Do you know who did the initial hookup of
this, or if this code was at one point used outside of android?

[nilesh, 2012-10-19 16:49:59]

> Yes, but as far as I can tell content/public/browser/web_contents_delegate.h
is
> referring to a non-exposed function that is currently not called.  Maybe
> historically this code was different?  Do you know who did the initial hookup
of
> this, or if this code was at one point used outside of android?

I talked to jcivelli, who added this method in WebContentsDelegate. It was never
used for non-Android code (other platforms didn't want the extra IPC), but it
was originally intended to allow any embedder to get load progress
notifications. Cleary it is not possible now. I will change it to initialize
LoadProgressTracker for Android. 

To fix the WCD api I can move LoadProgressChanged into a ifdef(ANDROID) block or
just document the current behavior (that the method will only be called on
certain platforms).

[nilesh, 2012-10-19 20:35:24]

PTAL.

Updated the CL to initialize LoadProgressTracker for Android and made
WebContentsDelegate::LoadProgressChanged android only.

[nilesh, 2012-10-19 21:28:46]

Adding John for content/public and content/browser

[nilesh, 2012-10-22 20:46:43]

ping.

[jam, 2012-10-23 16:55:56]

lgtm

[nilesh, 2012-10-23 18:09:57]

James, Does this look alright to you?

[jamesr, 2012-10-23 19:08:36]

lgtm

[commit-bot: I haz the power, 2012-10-23 19:15:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/11194043/16001

[commit-bot: I haz the power, 2012-10-23 19:15:30]

Failed to apply patch for content/renderer/render_view_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/render_view_impl.cc
  Hunk #1 succeeded at 623 (offset -10 lines).
  Hunk #2 FAILED at 877.
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/render_view_impl.cc.rej

Patch:       content/renderer/render_view_impl.cc
Index: content/renderer/render_view_impl.cc
diff --git a/content/renderer/render_view_impl.cc
b/content/renderer/render_view_impl.cc
index
7d8db32089dc2643db70f074fbbef3646ae138ef..9ccbe7bd2b5b971328adfd5071ba83df51bf995e
100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -633,6 +633,8 @@ RenderViewImpl::RenderViewImpl(
       expected_content_intent_id_(0),
       media_player_proxy_(NULL),
       synchronous_find_active_match_ordinal_(-1),
+      ALLOW_THIS_IN_INITIALIZER_LIST(
+          load_progress_tracker_(new LoadProgressTracker(this))),
 #endif
       session_storage_namespace_id_(session_storage_namespace_id),
       handling_select_range_(false),
@@ -875,15 +877,6 @@ WebKit::WebView* RenderViewImpl::webview() const {
   return static_cast<WebKit::WebView*>(webwidget());
 }
 
-void RenderViewImpl::SetReportLoadProgressEnabled(bool enabled) {
-  if (!enabled) {
-    load_progress_tracker_.reset(NULL);
-    return;
-  }
-  if (load_progress_tracker_ == NULL)
-    load_progress_tracker_.reset(new LoadProgressTracker(this));
-}
-
 content::old::GuestToEmbedderChannel*
     RenderViewImpl::GetGuestToEmbedderChannel() const {
   return guest_to_embedder_channel_;

[commit-bot: I haz the power, 2012-10-23 20:32:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nileshagrawal@chromium.org/11194043/15007

[commit-bot: I haz the power, 2012-10-23 22:29:34]

Change committed as 163704