Add Perceived PageLoad Time (PPLT) metrics for Prerender Experiments,...

Add Perceived PageLoad Time (PPLT) metrics for Prerender Experiments.
BUG=None
Test=None


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

[tburkard, 2011-01-26 19:29:17]

Hi Chris and Brett,

please find enclosed the change to record the Perceived Pageload Time (PPLT) on
the browser side for the prerendering field trials.

Thanks.

[cbentzel, 2011-01-26 19:38:36]

Adding gavinp because he's also been doing some changes with prerender/prefetch
field trial setup.

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:128:
prerender_manager_->RemoveEntry(this);
Move this to a different CL.

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:272: void
PrerenderContents::DidStopLoading() {
DCHECK(has_stopped_loading_) - can this ever be called multiple times in client
issued redirect case?

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:28: kDisabled,
Chrome style uses MACRO_MODE for enum values, despite what C++ style says.

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:61: PrerenderManagerMode mode() {
return mode_; }
const

I still think this might not be needed if the histogramming is done with the
PrerenderManager.

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:2822: PrerenderManager* pm =
profile()->GetPrerenderManager();
Can you move this into PrerenderManager or PrerenderContents?

[tburkard, 2011-01-26 20:05:50]

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_contents.cc (right):

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:128:
prerender_manager_->RemoveEntry(this);
On 2011/01/26 19:38:36, cbentzel wrote:
> Move this to a different CL.

Done.

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_contents.cc:272: void
PrerenderContents::DidStopLoading() {
Could happen multiple times.
In some pages, the loading restarts sometimes for some reason.

On 2011/01/26 19:38:36, cbentzel wrote:
> DCHECK(has_stopped_loading_) - can this ever be called multiple times in
client
> issued redirect case?

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:28: kDisabled,
So what does that mean?  WHat do i have to change here?
On 2011/01/26 19:38:36, cbentzel wrote:
> Chrome style uses MACRO_MODE for enum values, despite what C++ style says.

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:61: PrerenderManagerMode mode() {
return mode_; }
On 2011/01/26 19:38:36, cbentzel wrote:
> const
> 
> I still think this might not be needed if the histogramming is done with the
> PrerenderManager. 

Done.

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:2822: PrerenderManager* pm =
profile()->GetPrerenderManager();
On 2011/01/26 19:38:36, cbentzel wrote:
> Can you move this into PrerenderManager or PrerenderContents?

Done.

[brettw, 2011-01-26 20:16:46]

http://codereview.chromium.org/6263014/diff/35001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6263014/diff/35001/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:1658: // Record the beginning of a
new PPLT navigation.
I realized how we can do this just in your component without adding this stuff
in TabContents. There are notifications for both the start & stop which you can
listen for. TabContents has a property_bag() which you can attach random stuff
to. autocomplete_edit_view_win.cc does this.

[tburkard, 2011-01-26 20:20:22]

In one method, this gets called:
  // Notify observers about navigation.
  FOR_EACH_OBSERVER(WebNavigationObserver, web_navigation_observers_,
                    DidStopLoading());

In the other method, the following:
  NotificationService::current()->Notify(
      NotificationType::FRAME_PROVISIONAL_LOAD_START,
      Source<NavigationController>(&controller_),
      Details<ProvisionalLoadDetails>(&details));

So I could register my class in both of these?

Timo


On Wed, Jan 26, 2011 at 12:16 PM, <brettw@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6263014/diff/35001/chrome/browser/tab_contents...
>
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
>
>
http://codereview.chromium.org/6263014/diff/35001/chrome/browser/tab_contents...
> chrome/browser/tab_contents/tab_contents.cc:1658: // Record the
>
> beginning of a new PPLT navigation.
> I realized how we can do this just in your component without adding this
> stuff in TabContents. There are notifications for both the start & stop
> which you can listen for. TabContents has a property_bag() which you can
> attach random stuff to. autocomplete_edit_view_win.cc does this.
>
>
> http://codereview.chromium.org/6263014/
>

[brettw, 2011-01-26 20:24:29]

On Wed, Jan 26, 2011 at 12:20 PM, Timo Burkard <tburkard@chromium.org> wrote:
> In one method, this gets called:
>   // Notify observers about navigation.
>   FOR_EACH_OBSERVER(WebNavigationObserver, web_navigation_observers_,
>                     DidStopLoading());
> In the other method, the following:
>   NotificationService::current()->Notify(
>       NotificationType::FRAME_PROVISIONAL_LOAD_START,
>       Source<NavigationController>(&controller_),
>       Details<ProvisionalLoadDetails>(&details));
> So I could register my class in both of these?

Weird, that WebNavigationObserver is independent of the
NotificationService. I don't see any reason not to register as that in
addition to the NotificationService one.

Brett

[cbentzel, 2011-01-26 21:25:36]

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_manager.h (right):

http://codereview.chromium.org/6263014/diff/15001/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_manager.h:28: kDisabled,
On 2011/01/26 20:05:50, tburkard wrote:
> So what does that mean?  WHat do i have to change here?
> On 2011/01/26 19:38:36, cbentzel wrote:
> > Chrome style uses MACRO_MODE for enum values, despite what C++ style says.
> 

Just name something like PRERENDER_MODE_DISABLED

[tburkard, 2011-01-26 21:29:57]

Thanks, Chris.  I updated the change based on your suggestion.

Brett, I moved all the PLT measurement into a separate class and found a way to
register it in TabContents.  Is this what you were envisioning?

Thanks.

[brettw, 2011-01-26 22:13:28]

LGTM

http://codereview.chromium.org/6263014/diff/25002/chrome/browser/prerender/pr...
File chrome/browser/prerender/prerender_plt_recorder.cc (right):

http://codereview.chromium.org/6263014/diff/25002/chrome/browser/prerender/pr...
chrome/browser/prerender/prerender_plt_recorder.cc:31: bool is_main_frame,
Since these won't fit, all the arguments should just be indented 4 spaces (frame
ID should be moved down to the next line).

http://codereview.chromium.org/6263014/diff/25002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/6263014/diff/25002/chrome/browser/tab_contents...
chrome/browser/tab_contents/tab_contents.cc:478:
AddNavigationObserver(prerender_plt_recorder_.get());
Hmm, I was hoping everything could be in your component, and now I see what you
mean by it being more work by needing an extra class. But it looks like how I
was imagining this would work isn't entirely supported by the current design. So
this is fine.

[tburkard, 2011-01-26 22:20:41]

On Wed, Jan 26, 2011 at 2:13 PM, <brettw@chromium.org> wrote:

> LGTM
>
>
>
>
http://codereview.chromium.org/6263014/diff/25002/chrome/browser/prerender/pr...
> File chrome/browser/prerender/prerender_plt_recorder.cc (right):
>
>
>
http://codereview.chromium.org/6263014/diff/25002/chrome/browser/prerender/pr...
> chrome/browser/prerender/prerender_plt_recorder.cc:31: bool
> is_main_frame,
> Since these won't fit, all the arguments should just be indented 4
> spaces (frame ID should be moved down to the next line).
>
>
Actually, it does fit even if i indent it propertly, done.


>
>
http://codereview.chromium.org/6263014/diff/25002/chrome/browser/tab_contents...
>
> File chrome/browser/tab_contents/tab_contents.cc (right):
>
>
>
http://codereview.chromium.org/6263014/diff/25002/chrome/browser/tab_contents...
> chrome/browser/tab_contents/tab_contents.cc:478:
> AddNavigationObserver(prerender_plt_recorder_.get());
> Hmm, I was hoping everything could be in your component, and now I see
> what you mean by it being more work by needing an extra class. But it
> looks like how I was imagining this would work isn't entirely supported
> by the current design. So this is fine.
>
>
Thanks.

[cbentzel, 2011-01-27 01:08:58]

LGTM