Migrate the site engagement service to use DidFinishNavigation.

Migrate the site engagement service to use DidFinishNavigation.

This method will replace DidNavigateMainFrame in ths future.

BUG=610943
Committed: https://crrev.com/d1c739e9e0c25c6cd8bc86d0bdaf048027a84e7a
Cr-Commit-Position: refs/heads/master@{#393213}

[dominickn, 2016-05-12 06:43:26]

dominickn@chromium.org changed reviewers:
+ calamity@chromium.org

[dominickn, 2016-05-12 06:43:27]

PTAL, thanks!

[calamity, 2016-05-12 07:58:14]

lgtm w/ question.

https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_helper.cc (right):

https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_helper.cc:209: handle->IsSamePage() ||
handle->IsErrorPage() || !record_engagement_) {
What are considered to be same pages and error pages?

[dominickn, 2016-05-12 08:03:06]

Thanks!

https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
File chrome/browser/engagement/site_engagement_helper.cc (right):

https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
chrome/browser/engagement/site_engagement_helper.cc:209: handle->IsSamePage() ||
handle->IsErrorPage() || !record_engagement_) {
On 2016/05/12 07:58:14, calamity wrote:
> What are considered to be same pages and error pages?

 - Same page navigations are things like fragment navigations or anchor link
navigations that stay on the same page. Makes sense not to count them as
navigations for engagement (they'll get user interaction points already)
 - Error page navigations are those which end with an error page (interstitial)
being shown to the user

[dominickn, 2016-05-12 08:39:48]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-05-12 08:40:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1967013002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1967013002/1

[commit-bot: I haz the power, 2016-05-12 09:24:32]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-12 09:25:22]

Description was changed from

==========
Migrate the site engagement service to use DidFinishNavigation.

This method will replace DidNavigateMainFrame in ths future.

BUG=610943
==========

to

==========
Migrate the site engagement service to use DidFinishNavigation.

This method will replace DidNavigateMainFrame in ths future.

BUG=610943

Committed: https://crrev.com/d1c739e9e0c25c6cd8bc86d0bdaf048027a84e7a
Cr-Commit-Position: refs/heads/master@{#393213}
==========

[commit-bot: I haz the power, 2016-05-12 09:25:23]

Patchset 1 (id:??) landed as
https://crrev.com/d1c739e9e0c25c6cd8bc86d0bdaf048027a84e7a
Cr-Commit-Position: refs/heads/master@{#393213}

[calamity, 2016-05-12 23:45:55]

On 2016/05/12 08:03:06, dominickn wrote:
> Thanks!
> 
>
https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
> File chrome/browser/engagement/site_engagement_helper.cc (right):
> 
>
https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
> chrome/browser/engagement/site_engagement_helper.cc:209: handle->IsSamePage()
||
> handle->IsErrorPage() || !record_engagement_) {
> On 2016/05/12 07:58:14, calamity wrote:
> > What are considered to be same pages and error pages?
> 
>  - Same page navigations are things like fragment navigations or anchor link
> navigations that stay on the same page. Makes sense not to count them as
> navigations for engagement (they'll get user interaction points already)
>  - Error page navigations are those which end with an error page
(interstitial)
> being shown to the user

So are 404s and the like errors?

[dominickn, 2016-05-13 01:23:28]

On 2016/05/12 23:45:55, calamity wrote:
> On 2016/05/12 08:03:06, dominickn wrote:
> > Thanks!
> > 
> >
>
https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
> > File chrome/browser/engagement/site_engagement_helper.cc (right):
> > 
> >
>
https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/s...
> > chrome/browser/engagement/site_engagement_helper.cc:209:
handle->IsSamePage()
> ||
> > handle->IsErrorPage() || !record_engagement_) {
> > On 2016/05/12 07:58:14, calamity wrote:
> > > What are considered to be same pages and error pages?
> > 
> >  - Same page navigations are things like fragment navigations or anchor link
> > navigations that stay on the same page. Makes sense not to count them as
> > navigations for engagement (they'll get user interaction points already)
> >  - Error page navigations are those which end with an error page
> (interstitial)
> > being shown to the user
> 
> So are 404s and the like errors?

Typically, 404s have a handler that the server or website sends, so they're
almost never errors. When you can't get to the website because DNS failed or you
have no connection - those are errors.