Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(64)

Issue 1967013002: Migrate the site engagement service to use DidFinishNavigation. (Closed)

Created:
4 years, 7 months ago by dominickn
Modified:
4 years, 7 months ago
Reviewers:
calamity
CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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}

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -19 lines) Patch
M chrome/browser/engagement/site_engagement_helper.h View 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/engagement/site_engagement_helper.cc View 4 chunks +14 lines, -16 lines 2 comments Download

Messages

Total messages: 11 (3 generated)
dominickn
PTAL, thanks!
4 years, 7 months ago (2016-05-12 06:43:27 UTC) #2
calamity
lgtm w/ question. https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/site_engagement_helper.cc File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/site_engagement_helper.cc#newcode209 chrome/browser/engagement/site_engagement_helper.cc:209: handle->IsSamePage() || handle->IsErrorPage() || !record_engagement_) { ...
4 years, 7 months ago (2016-05-12 07:58:14 UTC) #3
dominickn
Thanks! https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/site_engagement_helper.cc File chrome/browser/engagement/site_engagement_helper.cc (right): https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/site_engagement_helper.cc#newcode209 chrome/browser/engagement/site_engagement_helper.cc:209: handle->IsSamePage() || handle->IsErrorPage() || !record_engagement_) { On 2016/05/12 ...
4 years, 7 months ago (2016-05-12 08:03:06 UTC) #4
commit-bot: I haz the power
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
4 years, 7 months ago (2016-05-12 08:40:00 UTC) #6
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 7 months ago (2016-05-12 09:24:32 UTC) #7
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/d1c739e9e0c25c6cd8bc86d0bdaf048027a84e7a Cr-Commit-Position: refs/heads/master@{#393213}
4 years, 7 months ago (2016-05-12 09:25:23 UTC) #9
calamity
On 2016/05/12 08:03:06, dominickn wrote: > Thanks! > > https://codereview.chromium.org/1967013002/diff/1/chrome/browser/engagement/site_engagement_helper.cc > File chrome/browser/engagement/site_engagement_helper.cc (right): > ...
4 years, 7 months ago (2016-05-12 23:45:55 UTC) #10
dominickn
4 years, 7 months ago (2016-05-13 01:23:28 UTC) #11
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld 408576698