Componentize safe_browsing: factor out chrome/ deps in ThreatDetailsRedirectsCollector.

Componentize safe_browsing: factor out chrome/ deps in ThreatDetailsRedirectsCollector.

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351, 688629
Review-Url: https://codereview.chromium.org/2796123003
Cr-Commit-Position: refs/heads/master@{#462478}
Committed: https://chromium.googlesource.com/chromium/src/+/5d734ba20ca41dd77486aee0bdcb8ddf34165053

[timvolodine, 2017-04-04 17:25:55]

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

[commit-bot: I haz the power, 2017-04-04 17:26:43]

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

[timvolodine, 2017-04-04 17:33:17]

Description was changed from

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

WIP

BUG=700351
==========

to

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

WIP

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351
==========

[commit-bot: I haz the power, 2017-04-04 17:40:36]

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

[commit-bot: I haz the power, 2017-04-04 17:40:36]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[timvolodine, 2017-04-05 11:35:35]

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

[commit-bot: I haz the power, 2017-04-05 11:35:50]

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, 2017-04-05 11:59:40]

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

[commit-bot: I haz the power, 2017-04-05 11:59:41]

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_...)

[timvolodine, 2017-04-05 15:23:28]

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

[commit-bot: I haz the power, 2017-04-05 15:24:25]

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, 2017-04-05 15:38:24]

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

[commit-bot: I haz the power, 2017-04-05 15:38:24]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[timvolodine, 2017-04-05 16:10:48]

Description was changed from

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

WIP

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351
==========

to

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

WIP

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351
==========

[timvolodine, 2017-04-05 17:42:41]

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

[timvolodine, 2017-04-05 17:44:27]

+ jialiu@: please take a look.
+ cc:nparker@

FYI issues on mac_chromium_rel_ng bot seem to be unrelated to this patch.

[Jialiu Lin, 2017-04-05 18:19:47]

LGTM. :-) 

Thank you timvolodine@!

https://codereview.chromium.org/2796123003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/2796123003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details.cc:188: redirects_collector_ = new
ThreatDetailsRedirectsCollector(
If history_service is not available, maybe we don't need to create
ThreatDetailsRedirectsCollector at all.
How about 
 redirects_collector_ = history_service ?
     new ThreatDetailsRedirectsCollector(                    
         history_service->AsWeakPtr()) : nullptr;


And every time redirects_collector_ is called, do a check first.

[timvolodine, 2017-04-06 14:15:23]

Thanks for the review!

https://codereview.chromium.org/2796123003/diff/20001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/threat_details.cc (right):

https://codereview.chromium.org/2796123003/diff/20001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/threat_details.cc:188: redirects_collector_ = new
ThreatDetailsRedirectsCollector(
On 2017/04/05 18:19:47, Jialiu Lin wrote:
> If history_service is not available, maybe we don't need to create
> ThreatDetailsRedirectsCollector at all.
> How about 
>  redirects_collector_ = history_service ?
>      new ThreatDetailsRedirectsCollector(                    
>          history_service->AsWeakPtr()) : nullptr;
> 
> 
> And every time redirects_collector_ is called, do a check first.

Yes that would seem reasonable, because the redirects_collector_ doesn't do much
when there is no history_service. However simply checking for
redirects_collector != null does not seem sufficient as its callback is used for
further starting cache collection (in OnRedirectionCollectionReady, so this
would need more refactoring of the code. Maybe something for a separate patch..

[timvolodine, 2017-04-06 14:15:51]

Description was changed from

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

WIP

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351
==========

to

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351
==========

[timvolodine, 2017-04-06 14:54:24]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2017-04-06 14:54:37]

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

[timvolodine, 2017-04-06 15:04:07]

The CQ bit was unchecked by timvolodine@chromium.org

[timvolodine, 2017-04-06 15:05:04]

Description was changed from

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351
==========

to

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351, 688629
==========

[timvolodine, 2017-04-06 15:05:45]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2017-04-06 15:06:04]

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, 2017-04-06 15:18:39]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491491145803270,
"parent_rev": "a1f53b7c3cca3156dcf7cd7a57264d888ea318ef", "commit_rev":
"5d734ba20ca41dd77486aee0bdcb8ddf34165053"}

[commit-bot: I haz the power, 2017-04-06 15:19:24]

Description was changed from

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351, 688629
==========

to

==========
Componentize safe_browsing: factor out chrome/ deps in
ThreatDetailsRedirectsCollector.

In preparation for moving browser side reporting code to
the safe_browsing component perform some refactoring
to decrease dependency on the chrome/ layer.

In particular for threat_details_history.{h,cc}:
- factor out Profile,
- pass HistoryService in constructor,
- use HistoryServiceObserver instead of NotificationObserver
and chrome::NOTIFICATION_PROFILE_DESTROYED.

design doc:
https://goo.gl/8zVufq

BUG=700351, 688629

Review-Url: https://codereview.chromium.org/2796123003
Cr-Commit-Position: refs/heads/master@{#462478}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d734ba20ca41dd77486aee0bdcb...
==========

[commit-bot: I haz the power, 2017-04-06 15:19:25]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5d734ba20ca41dd77486aee0bdcb...