Refactor BaseBlockingPage to reduce duplicated code

Refactor BaseBlockingPage to reduce duplicated code

Duplicated code handling ReportDetails is moved out of
CreateControllerClient() into a static method in the base class
(GetReportingInfo). CreateControllerClient now uses the |ui_manager|
directly, reducing the number of arguments it requires and reducing
clutter when constructing the blocking pages.

Chrome-only subresource checks at the end of OnProceed() have been moved
into a new method (HandleSubresourcesAfterProceed). This is empty in the
base class, since the base class ignores this case.

The base class OnProceed() method now calls FinishThreatDetails() like
chrome requires. This is to be consistent with the other
FinishThreatDetails() calls we kept in the base class. If the base class
ever really implements this, then we would also want to call it for
OnProceed. This method is currently a NOOP, so this presents no change
in behavior. Along with this change, |threat_details_proceed_delay_ms_|
has been componentized as well, moved to private, and a test-only setter
has been added. Because of these changes, Chrome now uses the component
layer OnProceed method.

ShowBlockingPage() is restructured to be more readable. No change beyond
that, since I couldn't think of a clean way to reduce code duplication.

Lastly, unnecessary includes have been removed.

BUG=

Review-Url: https://codereview.chromium.org/2700323002
Cr-Commit-Position: refs/heads/master@{#452717}
Committed: https://chromium.googlesource.com/chromium/src/+/fef42f9a072d257f7f4ef0300ea172f45754167a

[Nate Fischer, 2017-02-18 02:05:07]

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

[commit-bot: I haz the power, 2017-02-18 02:06:17]

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-02-18 04:08:36]

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

[commit-bot: I haz the power, 2017-02-18 04:08:37]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Nate Fischer, 2017-02-21 20:08:48]

ntfschr@chromium.org changed reviewers:
+ vakh@chromium.org

[Nate Fischer, 2017-02-21 20:12:33]

PTAL. This is a follow-up to our initial componentization, since I felt we could
do better on readability and reduce code duplication. This should have no change
in behavior.

[Nate Fischer, 2017-02-23 21:04:17]

On 2017/02/21 at 20:12:33, Nate Fischer wrote:
> PTAL. This is a follow-up to our initial componentization, since I felt we
could do better on readability and reduce code duplication. This should have no
change in behavior.

vakh@ ping. Let me know if you'll have time to take a look at this, otherwise I
can pass this off to jialiul@ now that she's back (since it's componentization
anyway).

[Nate Fischer, 2017-02-23 22:45:37]

ntfschr@chromium.org changed reviewers:
+ jialiul@chromium.org
- vakh@chromium.org

[Nate Fischer, 2017-02-23 22:46:14]

On 2017/02/23 at 21:04:17, Nate Fischer wrote:
> On 2017/02/21 at 20:12:33, Nate Fischer wrote:
> > PTAL. This is a follow-up to our initial componentization, since I felt we
could do better on readability and reduce code duplication. This should have no
change in behavior.
> 
> vakh@ ping. Let me know if you'll have time to take a look at this, otherwise
I can pass this off to jialiul@ now that she's back (since it's componentization
anyway).

+jialiul@ could you review, since it looks like vakh@ may be busy?

[Jialiu Lin, 2017-02-23 23:07:26]

Sure, I'll send you feedback by the end of today.

[Nate Fischer, 2017-02-23 23:08:06]

On 2017/02/23 at 23:07:26, jialiul wrote:
> Sure, I'll send you feedback by the end of today.

Thanks, Jialiu!

[Jialiu Lin, 2017-02-24 00:07:00]

lgtm. Much cleaner now. Thanks for cleaning up these files!

[Nate Fischer, 2017-02-24 00:55:05]

The CQ bit was checked by ntfschr@chromium.org

[commit-bot: I haz the power, 2017-02-24 00:55:50]

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-02-24 02:15:53]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1487897705229790, "parent_rev":
"a7370185fac30c1a9f35fd928cf7c790671248a7", "commit_rev":
"fef42f9a072d257f7f4ef0300ea172f45754167a"}

[commit-bot: I haz the power, 2017-02-24 02:16:25]

Description was changed from

==========
Refactor BaseBlockingPage to reduce duplicated code

Duplicated code handling ReportDetails is moved out of
CreateControllerClient() into a static method in the base class
(GetReportingInfo). CreateControllerClient now uses the |ui_manager|
directly, reducing the number of arguments it requires and reducing
clutter when constructing the blocking pages.

Chrome-only subresource checks at the end of OnProceed() have been moved
into a new method (HandleSubresourcesAfterProceed). This is empty in the
base class, since the base class ignores this case.

The base class OnProceed() method now calls FinishThreatDetails() like
chrome requires. This is to be consistent with the other
FinishThreatDetails() calls we kept in the base class. If the base class
ever really implements this, then we would also want to call it for
OnProceed. This method is currently a NOOP, so this presents no change
in behavior. Along with this change, |threat_details_proceed_delay_ms_|
has been componentized as well, moved to private, and a test-only setter
has been added. Because of these changes, Chrome now uses the component
layer OnProceed method.

ShowBlockingPage() is restructured to be more readable. No change beyond
that, since I couldn't think of a clean way to reduce code duplication.

Lastly, unnecessary includes have been removed.

BUG=
==========

to

==========
Refactor BaseBlockingPage to reduce duplicated code

Duplicated code handling ReportDetails is moved out of
CreateControllerClient() into a static method in the base class
(GetReportingInfo). CreateControllerClient now uses the |ui_manager|
directly, reducing the number of arguments it requires and reducing
clutter when constructing the blocking pages.

Chrome-only subresource checks at the end of OnProceed() have been moved
into a new method (HandleSubresourcesAfterProceed). This is empty in the
base class, since the base class ignores this case.

The base class OnProceed() method now calls FinishThreatDetails() like
chrome requires. This is to be consistent with the other
FinishThreatDetails() calls we kept in the base class. If the base class
ever really implements this, then we would also want to call it for
OnProceed. This method is currently a NOOP, so this presents no change
in behavior. Along with this change, |threat_details_proceed_delay_ms_|
has been componentized as well, moved to private, and a test-only setter
has been added. Because of these changes, Chrome now uses the component
layer OnProceed method.

ShowBlockingPage() is restructured to be more readable. No change beyond
that, since I couldn't think of a clean way to reduce code duplication.

Lastly, unnecessary includes have been removed.

BUG=

Review-Url: https://codereview.chromium.org/2700323002
Cr-Commit-Position: refs/heads/master@{#452717}
Committed:
https://chromium.googlesource.com/chromium/src/+/fef42f9a072d257f7f4ef0300ea1...
==========

[commit-bot: I haz the power, 2017-02-24 02:16:27]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/fef42f9a072d257f7f4ef0300ea1...

[vakh (use Gerrit instead), 2017-02-25 00:44:42]

vakh@chromium.org changed reviewers:
+ vakh@chromium.org

[vakh (use Gerrit instead), 2017-02-25 00:44:43]

I'm sorry I totally missed this.