Adding offline navigations to the previews blacklist

Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the existing mechanism for opt-outs, and will be transitioned to the shared black list later.

This changes the offline page tab helper fragment check to an IsSamePage check, so reloads that do not use the offline page do not have incorrect info in offline_info_.

BUG=661839
Committed: https://crrev.com/603474151d2ea47cbf53a54b87bf13786aa90cef
Cr-Commit-Position: refs/heads/master@{#431066}

[RyanSturm, 2016-11-03 00:34:14]

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

[commit-bot: I haz the power, 2016-11-03 00:34:48]

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-11-03 00:50:10]

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

[commit-bot: I haz the power, 2016-11-03 00:50:11]

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

[RyanSturm, 2016-11-03 17:30:51]

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

[commit-bot: I haz the power, 2016-11-03 17:31:17]

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

[RyanSturm, 2016-11-03 17:35:08]

Description was changed from

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false).

BUG=661839
==========

to

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the
existing mechanism for opt-outs, and will be transitioned to the shared black
list later.

BUG=661839
==========

[commit-bot: I haz the power, 2016-11-03 18:46:40]

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

[commit-bot: I haz the power, 2016-11-03 18:46:41]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-11-03 20:16:54]

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

[commit-bot: I haz the power, 2016-11-03 20:17:44]

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-11-03 22:42:50]

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

[commit-bot: I haz the power, 2016-11-03 22:42:51]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[RyanSturm, 2016-11-04 18:48:06]

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

[commit-bot: I haz the power, 2016-11-04 18:48:47]

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

[RyanSturm, 2016-11-04 18:50:53]

Description was changed from

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the
existing mechanism for opt-outs, and will be transitioned to the shared black
list later.

BUG=661839
==========

to

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the
existing mechanism for opt-outs, and will be transitioned to the shared black
list later.

This changes the offline page tab helper fragment check to an IsSamePage check,
so reloads that do not use the offline page do not have incorrect info in
offline_info_.

BUG=661839
==========

[RyanSturm, 2016-11-04 20:00:03]

ryansturm@chromium.org changed reviewers:
+ jianli@chromium.org, megjablon@chromium.org

[RyanSturm, 2016-11-04 20:00:04]

megjablon: PTAL

jianli: PTAL, let me know if there is a reason you are doing an explicit
fragment check instead of IsSamePage check.

[RyanSturm, 2016-11-04 20:20:05]

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

[commit-bot: I haz the power, 2016-11-04 20:20:49]

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-11-04 21:21:44]

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

[commit-bot: I haz the power, 2016-11-04 21:21:45]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-11-08 21:35:35]

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

[commit-bot: I haz the power, 2016-11-08 21:36:22]

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

[RyanSturm, 2016-11-08 21:36:49]

jianli, megjablon: ping

I'm OOO for the next couple weeks after Friday, so I'd like to land this by
Friday.

[commit-bot: I haz the power, 2016-11-08 22:40:30]

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

[commit-bot: I haz the power, 2016-11-08 22:40:31]

Dry run: This issue passed the CQ dry run.

[jianli, 2016-11-08 23:13:01]

offline_pages lgtm

[megjablon, 2016-11-09 00:19:11]

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.h:29: typedef
base::Callback<void(bool opt_out)> PreviewsUserLeavePreviewCallback;
Can we call this OnDismissPreviewsInfobarCallback?

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right):

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate_unittest.cc:101: void
infobar_reload_by_user(bool infobar_reload_by_user) {
Use camel case and if you rename OnDismissPreviewsInfobar

[RyanSturm, 2016-11-09 17:43:06]

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

[commit-bot: I haz the power, 2016-11-09 17:43:38]

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-11-09 18:21:59]

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

[commit-bot: I haz the power, 2016-11-09 18:22:00]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-11-09 18:47:13]

megjablon: PTAL

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.h (right):

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.h:29: typedef
base::Callback<void(bool opt_out)> PreviewsUserLeavePreviewCallback;
On 2016/11/09 00:19:11, megjablon wrote:
> Can we call this OnDismissPreviewsInfobarCallback?

Done.

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right):

https://codereview.chromium.org/2472863002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate_unittest.cc:101: void
infobar_reload_by_user(bool infobar_reload_by_user) {
On 2016/11/09 00:19:11, megjablon wrote:
> Use camel case and if you rename OnDismissPreviewsInfobar

Done.

[megjablon, 2016-11-09 19:35:05]

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right):

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate_unittest.cc:101: void
DismissPreviewsInfobar(bool user_opt_out) {
nit: How about OnDismissPreviewsInfobar instead? Otherwise it sounds like you're
telling it to dismiss.

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_tab_helper.cc (right):

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_tab_helper.cc:83:
navigation_handle->GetURL(), previews::PreviewsType::OFFLINE));
Can you add a test to previews_infobar_tab_helper_unittests.cc for this?

[RyanSturm, 2016-11-09 20:42:20]

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

[commit-bot: I haz the power, 2016-11-09 20:42:50]

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

[RyanSturm, 2016-11-09 20:45:47]

megjablon: let me know if there seems like there is a gap in test coverage with
the browser test.

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate_unittest.cc (right):

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate_unittest.cc:101: void
DismissPreviewsInfobar(bool user_opt_out) {
On 2016/11/09 19:35:05, megjablon wrote:
> nit: How about OnDismissPreviewsInfobar instead? Otherwise it sounds like
you're
> telling it to dismiss.

Done.

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_tab_helper.cc (right):

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_tab_helper.cc:83:
navigation_handle->GetURL(), previews::PreviewsType::OFFLINE));
On 2016/11/09 19:35:05, megjablon wrote:
> Can you add a test to previews_infobar_tab_helper_unittests.cc for this?

I wanted to avoid it due to the nature of KeyedService initialization. This
would need a browser test to test anything meaningful. Based on the infobar
delegate test we know the callback mechanism functions correctly, so we need to
test getting the PreviewsService and accessing previews_ui_service() on it.

I'll add a browser test to verify that PreviewsService is created and
initialized properly, but I think a browser test that runs the tab helper code
is excessive. The previews_io_data_unittest and other tests cover all the cases.
With this browser test the only thing that is untested about the callback is
running the code itself, but all of its functionality is tested.

[megjablon, 2016-11-09 22:17:55]

LGTM

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_tab_helper.cc (right):

https://codereview.chromium.org/2472863002/diff/120001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_tab_helper.cc:83:
navigation_handle->GetURL(), previews::PreviewsType::OFFLINE));
On 2016/11/09 20:45:47, Ryan Sturm wrote:
> On 2016/11/09 19:35:05, megjablon wrote:
> > Can you add a test to previews_infobar_tab_helper_unittests.cc for this?
> 
> I wanted to avoid it due to the nature of KeyedService initialization. This
> would need a browser test to test anything meaningful. Based on the infobar
> delegate test we know the callback mechanism functions correctly, so we need
to
> test getting the PreviewsService and accessing previews_ui_service() on it.
> 
> I'll add a browser test to verify that PreviewsService is created and
> initialized properly, but I think a browser test that runs the tab helper code
> is excessive. The previews_io_data_unittest and other tests cover all the
cases.
> With this browser test the only thing that is untested about the callback is
> running the code itself, but all of its functionality is tested.
> 

Ok sounds reasonable.

[commit-bot: I haz the power, 2016-11-09 22:42:55]

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

[commit-bot: I haz the power, 2016-11-09 22:42:56]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2016-11-09 22:47:21]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2016-11-09 22:47:22]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/2472863002/#ps140001
(title: "megjablon comments")

[commit-bot: I haz the power, 2016-11-09 22:47: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, 2016-11-09 22:53:43]

Description was changed from

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the
existing mechanism for opt-outs, and will be transitioned to the shared black
list later.

This changes the offline page tab helper fragment check to an IsSamePage check,
so reloads that do not use the offline page do not have incorrect info in
offline_info_.

BUG=661839
==========

to

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the
existing mechanism for opt-outs, and will be transitioned to the shared black
list later.

This changes the offline page tab helper fragment check to an IsSamePage check,
so reloads that do not use the offline page do not have incorrect info in
offline_info_.

BUG=661839
==========

[commit-bot: I haz the power, 2016-11-09 22:53:44]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-11-09 22:58:32]

Description was changed from

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the
existing mechanism for opt-outs, and will be transitioned to the shared black
list later.

This changes the offline page tab helper fragment check to an IsSamePage check,
so reloads that do not use the offline page do not have incorrect info in
offline_info_.

BUG=661839
==========

to

==========
Adding offline navigations to the previews blacklist

This change adds previews navigation to the blacklist when the user
reloads original (passing true) or the infobar is dismissed by other
means (passing false) for offline previews. Lo-fi and weblite still use the
existing mechanism for opt-outs, and will be transitioned to the shared black
list later.

This changes the offline page tab helper fragment check to an IsSamePage check,
so reloads that do not use the offline page do not have incorrect info in
offline_info_.

BUG=661839

Committed: https://crrev.com/603474151d2ea47cbf53a54b87bf13786aa90cef
Cr-Commit-Position: refs/heads/master@{#431066}
==========

[commit-bot: I haz the power, 2016-11-09 22:58:33]

Patchset 8 (id:??) landed as
https://crrev.com/603474151d2ea47cbf53a54b87bf13786aa90cef
Cr-Commit-Position: refs/heads/master@{#431066}