Construct the site engagement helper with a site engagement service.

Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise its site
engagement service point at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service' private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322
Committed: https://crrev.com/cd25e0bc03cbe4f606646d4ca7b95000de0fc055
Cr-Commit-Position: refs/heads/master@{#399101}

[dominickn, 2016-06-08 07:08:44]

Description was changed from

==========
Tie the site engagement helper to a single site engagement service.

This CL refactors the site engagement helper to take a site engagement
service object in its constructor. This obviates the need to query for
the site engagement service each time engagement is increased. It also
means that the service is informed of the web contents object on which
any engagement increase occurred, allowing it to signal other observers
in a future CL.

BUG=616322
==========

to

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to take a site engagement
service object in its constructor. This obviates the need to query for
the site engagement service each time engagement is increased. It also
means that the service is informed of the web contents object on which
any engagement increase occurred, allowing it to signal other observers
in a future CL.

BUG=616322
==========

[dominickn, 2016-06-08 07:20:24]

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

[dominickn, 2016-06-08 07:20:24]

PTAL, thanks! This cleans up the helper a bit, and will let me pass the web
contents on to site engagement observers.

[dominickn, 2016-06-08 14:19:36]

Description was changed from

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to take a site engagement
service object in its constructor. This obviates the need to query for
the site engagement service each time engagement is increased. It also
means that the service is informed of the web contents object on which
any engagement increase occurred, allowing it to signal other observers
in a future CL.

BUG=616322
==========

to

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to take a site engagement
service object in its constructor. This obviates the need to query for
the site engagement service each time engagement is increased. The
service' private interface is also refactored to take a WebContents pointer
rather than a GURL on engagement increases, allowing it to signal other
observers of the WebContents in a future CL.

BUG=616322
==========

[calamity, 2016-06-09 01:41:19]

https://codereview.chromium.org/2042243004/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.cc (right):

https://codereview.chromium.org/2042243004/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.cc:170: service_(service),
Can we use the web_contents->browser_context() to get the service here instead?

[dominickn, 2016-06-09 01:59:45]

Description was changed from

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to take a site engagement
service object in its constructor. This obviates the need to query for
the site engagement service each time engagement is increased. The
service' private interface is also refactored to take a WebContents pointer
rather than a GURL on engagement increases, allowing it to signal other
observers of the WebContents in a future CL.

BUG=616322
==========

to

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise its site
engagement service point at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service' private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322
==========

[dominickn, 2016-06-09 03:25:48]

Thanks!

https://codereview.chromium.org/2042243004/diff/40001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_helper.cc (right):

https://codereview.chromium.org/2042243004/diff/40001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_helper.cc:170: service_(service),
On 2016/06/09 01:41:19, calamity wrote:
> Can we use the web_contents->browser_context() to get the service here
instead?

Done.

[dominickn, 2016-06-09 03:32:20]

Description was changed from

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise its site
engagement service point at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service' private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322
==========

to

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise a site
engagement service pointer at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service's private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322
==========

[calamity, 2016-06-09 03:32:20]

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:339:
AddPoints(web_contents->GetLastCommittedURL(),
Why not visible here? Is this going to be the same as the NavigationHandle's URL
above?

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:586:
NavigateAndCommit(url2);
I'm surprised these don't trigger navigation engagement. Any explanation for why
this works?

[dominickn, 2016-06-09 03:45:57]

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:339:
AddPoints(web_contents->GetLastCommittedURL(),
On 2016/06/09 03:32:20, calamity wrote:
> Why not visible here? Is this going to be the same as the NavigationHandle's
URL
> above?

The existing behaviour has the helper sending the URL after a navigation has
committed, so using the LastCommittedURL here should be consistent with the
previous behaviour. I actually explicitly changed this from using VisibleURL for
this reason.

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:586:
NavigateAndCommit(url2);
On 2016/06/09 03:32:20, calamity wrote:
> I'm surprised these don't trigger navigation engagement. Any explanation for
why
> this works?

This test harness doesn't call TabHelpers::AttachTabHelpers, so the site
engagement helper is never created and engagement on navigation isn't triggered.

This is why tests like ScoreIncrementsOnPageRequest in this file manually create
the helper.

[calamity, 2016-06-09 03:48:06]

lgtm.

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service.cc (right):

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service.cc:339:
AddPoints(web_contents->GetLastCommittedURL(),
On 2016/06/09 03:45:57, dominickn wrote:
> On 2016/06/09 03:32:20, calamity wrote:
> > Why not visible here? Is this going to be the same as the NavigationHandle's
> URL
> > above?
> 
> The existing behaviour has the helper sending the URL after a navigation has
> committed, so using the LastCommittedURL here should be consistent with the
> previous behaviour. I actually explicitly changed this from using VisibleURL
for
> this reason.

Acknowledged.

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
File chrome/browser/engagement/site_engagement_service_unittest.cc (right):

https://codereview.chromium.org/2042243004/diff/60001/chrome/browser/engageme...
chrome/browser/engagement/site_engagement_service_unittest.cc:586:
NavigateAndCommit(url2);
On 2016/06/09 03:45:57, dominickn wrote:
> On 2016/06/09 03:32:20, calamity wrote:
> > I'm surprised these don't trigger navigation engagement. Any explanation for
> why
> > this works?
> 
> This test harness doesn't call TabHelpers::AttachTabHelpers, so the site
> engagement helper is never created and engagement on navigation isn't
triggered.
> 
> This is why tests like ScoreIncrementsOnPageRequest in this file manually
create
> the helper.

Acknowledged.

[dominickn, 2016-06-10 02:48:46]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-06-10 02:49:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2042243004/60001

[commit-bot: I haz the power, 2016-06-10 04:45:35]

Description was changed from

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise its site
engagement service point at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service' private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322
==========

to

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise its site
engagement service point at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service' private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322
==========

[commit-bot: I haz the power, 2016-06-10 04:45:37]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-10 04:45:44]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-10 04:48:05]

Description was changed from

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise its site
engagement service point at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service' private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322
==========

to

==========
Construct the site engagement helper with a site engagement service.

This CL refactors the site engagement helper to initialise its site
engagement service point at construction. This obviates the
need to query for the site engagement service each time
engagement is increased. The service' private interface is also
refactored to take a WebContents pointer rather than a GURL
on engagement increases, allowing it to signal other observers
of the WebContents in a future CL.

BUG=616322

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

[commit-bot: I haz the power, 2016-06-10 04:48:06]

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