Wire up download attribution enable finch experiment

Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467, 667784
Committed: https://crrev.com/4e3c4b4536b96f914132d4c53f70b65fadeaf87d
Cr-Commit-Position: refs/heads/master@{#440162}

[Jialiu Lin, 2016-12-15 23:40:38]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 23:41:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 01:15:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 01:15:14]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2016-12-19 19:38:46]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 19:39:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2016-12-19 19:39:41]

Description was changed from

==========
Add new download attribution to download protection service and enable
finch trial

BUG=639467
==========

to

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

BUG=639467
==========

[Jialiu Lin, 2016-12-19 20:31:04]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 20:32:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2016-12-19 21:20:39]

Description was changed from

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

BUG=639467
==========

to

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467
==========

[Jialiu Lin, 2016-12-19 21:22:43]

jialiul@chromium.org changed reviewers:
+ nparker@chromium.org

[Jialiu Lin, 2016-12-19 21:22:44]

Hi nparker@, 
Could you take a look at this short CL? It wires up download attribution code
and enables the finch experiment. 

Thanks,

https://codereview.chromium.org/2578363002/diff/40001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc (right):

https://codereview.chromium.org/2578363002/diff/40001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:119:
nav_event.is_user_initiated = true;
This is a bug fixing, not affecting browser_tests though. 

For most browser-initiated navigations, user gesture happens before navigation
start (e.g. click on bookmark, hit enter on address bar), except for address-bar
auto-complete navigation.
For this type of navigation, navigation starts when the first auto-complete
suggest is highlighted (which is before user hitting enter to confirm).
Therefore, has_user_gesture_ will be false here.

[commit-bot: I haz the power, 2016-12-19 22:27:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 22:27:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Jialiu Lin, 2016-12-19 22:52:28]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 22:53:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2016-12-19 22:55:53]

jialiul@chromium.org changed reviewers:
+ clamy@chromium.org

[Jialiu Lin, 2016-12-19 22:55:54]

clamy@, I added two more tests to
browser-side-navigation.linux.browser_tests.filter. They are failing because of
the same problem in NavigationHandle::IsRendererInitiated().

[Jialiu Lin, 2016-12-19 22:58:12]

Description was changed from

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467
==========

to

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467,667784
==========

[commit-bot: I haz the power, 2016-12-20 00:19:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 00:19:17]

Dry run: This issue passed the CQ dry run.

[Nathan Parker, 2016-12-20 01:01:33]

I mostly have little questions, nothing critical.  LGTM

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1824:
*out_request->add_referrer_chain() = entry;
Is there a way to move/swap the entry, since you don't need it anymore?  Don't
work too hard on it if it's not obvious.

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:123:
has_user_gesture_ = false;
So this consumes the user gesture only if there really was one... correct?  Just
trying to make sure I understand it.

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:314:
SafeBrowsingNavigationObserverManager::kDownloadAttribution)) {
Should this go in the ServiceDelegateImpl, so it'll only get instantiated on
desktop?  I guess we will want it on Android soon.  And WebView? Maybe it's ok
to leave it here...

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:133: const
scoped_refptr<SafeBrowsingNavigationObserverManager>&
I think the scoped_refptrs are better to return by copy:

scoped_refptr<SafeBrowsingNavigationObserverManager> nav_obs_mgr();

That way you can return null and have it generate a temporary.  We ran into
trouble with returning a ref with the DB manager, and I'm thinking we should
switch them all at some point.

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:234: if
(safe_browsing::SafeBrowsingNavigationObserverManager::IsEnabledAndReady(
Would it be tidier to keep all this logic in the nav_observer_mgr.cc file? Like,
the SafeBrowsingTabObserver above handles the conditional creation internally:

https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

[Jialiu Lin, 2016-12-20 01:55:22]

The CQ bit was checked by jialiul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 01:55:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Jialiu Lin, 2016-12-20 02:07:51]

Thanks!

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/download_protection_service.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/download_protection_service.cc:1824:
*out_request->add_referrer_chain() = entry;
On 2016/12/20 at 01:01:33, Nathan Parker wrote:
> Is there a way to move/swap the entry, since you don't need it anymore?  Don't
work too hard on it if it's not obvious.

Not sure if proto messages have move constructor by default. Let me put
std::move(entry) around entry here, and let compiler to do the optimization if
it can.

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_navigation_observer.cc:123:
has_user_gesture_ = false;
On 2016/12/20 at 01:01:33, Nathan Parker wrote:
> So this consumes the user gesture only if there really was one... correct? 
Just trying to make sure I understand it.

Yes. That's correct.

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.cc:314:
SafeBrowsingNavigationObserverManager::kDownloadAttribution)) {
On 2016/12/20 at 01:01:33, Nathan Parker wrote:
> Should this go in the ServiceDelegateImpl, so it'll only get instantiated on
desktop?  I guess we will want it on Android soon.  And WebView? Maybe it's ok
to leave it here...

Ack.

Eventually this navigation observer manager will be used for getting referrer
chain for interstitials too, then it'll get instantiated on both desktop and
android. That's why I feel it's probably better to leave it in
SafeBrowsingService.

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_service.h (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_service.h:133: const
scoped_refptr<SafeBrowsingNavigationObserverManager>&
On 2016/12/20 at 01:01:33, Nathan Parker wrote:
> I think the scoped_refptrs are better to return by copy:
> 
> scoped_refptr<SafeBrowsingNavigationObserverManager> nav_obs_mgr();
> 
> That way you can return null and have it generate a temporary.  We ran into
trouble with returning a ref with the DB manager, and I'm thinking we should
switch them all at some point.

I see. let's only change SafeBrowsingNavigationObserverManager for now, and
clean up other scope_refptr later.

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/ui/tab_h...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2578363002/diff/60001/chrome/browser/ui/tab_h...
chrome/browser/ui/tab_helpers.cc:234: if
(safe_browsing::SafeBrowsingNavigationObserverManager::IsEnabledAndReady(
On 2016/12/20 at 01:01:33, Nathan Parker wrote:
> Would it be tidier to keep all this logic in the nav_observer_mgr.cc file?
Like, the SafeBrowsingTabObserver above handles the conditional creation
internally:
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

You're right. Move all the checkings to SafeBrowsingNavigationObserver.

[Jialiu Lin, 2016-12-20 02:11:02]

jialiul@chromium.org changed reviewers:
+ avi@chromium.org

[Jialiu Lin, 2016-12-20 02:11:03]

avi@chromium.org: Please review changes in tab_helper.cc

[Avi (use Gerrit), 2016-12-20 02:44:26]

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:35: #include
"chrome/browser/safe_browsing/safe_browsing_service.h"
It doesn't seem like you need this include.

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:82: #include
"chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h"
It doesn't seem like you need this include.

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:235: web_contents);
You only need the includes for these objects.

[Jialiu Lin, 2016-12-20 03:16:54]

I moved some checking logic in previous patch and forgot to clean up includes.
Thanks for catching these unnecessary includes!

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
File chrome/browser/ui/tab_helpers.cc (right):

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:35: #include
"chrome/browser/safe_browsing/safe_browsing_service.h"
On 2016/12/20 02:44:26, Avi wrote:
> It doesn't seem like you need this include.

Oops, sorry. 
I moved some checking logic  and forgot to remove these includes. Thanks for
catching these.

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:82: #include
"chrome/browser/safe_browsing/safe_browsing_navigation_observer_manager.h"
On 2016/12/20 02:44:26, Avi wrote:
> It doesn't seem like you need this include.

Done.

https://codereview.chromium.org/2578363002/diff/100001/chrome/browser/ui/tab_...
chrome/browser/ui/tab_helpers.cc:235: web_contents);
On 2016/12/20 02:44:26, Avi wrote:
> You only need the includes for these objects.

Done.

[clamy, 2016-12-20 14:35:13]

Lgtm

[Jialiu Lin, 2016-12-20 23:44:30]

avi@, let me know if you have other comments. Thanks!

[Avi (use Gerrit), 2016-12-21 04:09:48]

lgtm

[Jialiu Lin, 2016-12-21 17:34:31]

The CQ bit was checked by jialiul@chromium.org

[Jialiu Lin, 2016-12-21 17:34:31]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2578363002/#ps120001
(title: "Remove unnecessary includes")

[commit-bot: I haz the power, 2016-12-21 17:34:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 18:38:56]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482341671106670,
"parent_rev": "f1d6b3793e4601289b35bd2518ae4d7addceb95e", "commit_rev":
"a6a13441f7e9e70bcb8ec329f78881c00f58c718"}

[commit-bot: I haz the power, 2016-12-21 18:39:28]

Description was changed from

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467,667784
==========

to

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467,667784

Review-Url: https://codereview.chromium.org/2578363002
==========

[commit-bot: I haz the power, 2016-12-21 18:39:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-21 18:41:38]

Description was changed from

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467,667784

Review-Url: https://codereview.chromium.org/2578363002
==========

to

==========
Add new download attribution logic to download protection service and enable
finch trial (a.k.a 50% on Canary only)

Also, a minor bug fixing in handling user gesture.

BUG=639467,667784

Committed: https://crrev.com/4e3c4b4536b96f914132d4c53f70b65fadeaf87d
Cr-Commit-Position: refs/heads/master@{#440162}
==========

[commit-bot: I haz the power, 2016-12-21 18:41:39]

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