SafeBrowsing: allow WebView to customize the Help Center URL

SafeBrowsing: allow WebView to customize the Help Center URL

This adds a 'plink' field to SBErrorDisplayOptions, allowing WebView
to specify its own plink (the p= URL query parameter) when visiting the
Help Center. This will allow the Help Center to differentiate between
Chrome and WebView and show appropriate information for each.

SBErrorDisplayOptions.plink is set to nullptr for Chrome, to indicate
that we should use a default value (p=cpn_safe_browsing).

Right now, the _wv suffix in the plink is ignored by the help center,
and is treated just like "cpn_safe_browsing" would be. So there should
be no change in high-level behavior, the URL will just vary slightly
between WebView and Chrome.

BUG=723125
Review-Url: https://codereview.chromium.org/2890703002
Cr-Commit-Position: refs/heads/master@{#473271}
Committed: https://chromium.googlesource.com/chromium/src/+/e952a3ef7450aae662c291c8c54986561ab65360

[Nate Fischer, 2017-05-17 00:11:31]

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

[commit-bot: I haz the power, 2017-05-17 00:12:18]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-17 00:13:48]

ntfschr@chromium.org changed reviewers:
+ jialiul@chromium.org, sgurun@chromium.org

[Nate Fischer, 2017-05-17 00:28:06]

jialiul@: for SafeBrowsing-related changes
sgurun@: for android_webview/

[Nate Fischer, 2017-05-17 00:31:28]

Description was changed from

==========
SafeBrowsing: allow WebView to customize the Help Center URL

This adds a 'plink' field to SBErrorDisplayOptions, allowing WebView
to specify its own plink (the p= URL query parameter) when visiting the
Help Center. This will allow the Help Center to differentiate between
Chrome and WebView and show appropriate information for each.

SBErrorDisplayOptions.plink is set to nullptr for Chrome, to indicate
that we should use a default value (p=cpn_safe_browsing).

BUG=723125
==========

to

==========
SafeBrowsing: allow WebView to customize the Help Center URL

This adds a 'plink' field to SBErrorDisplayOptions, allowing WebView
to specify its own plink (the p= URL query parameter) when visiting the
Help Center. This will allow the Help Center to differentiate between
Chrome and WebView and show appropriate information for each.

SBErrorDisplayOptions.plink is set to nullptr for Chrome, to indicate
that we should use a default value (p=cpn_safe_browsing).

Right now, the _wv suffix in the plink is ignored by the help center,
and is treated just like "cpn_safe_browsing" would be. So there should
be no change in high-level behavior, the URL will just vary slightly
between WebView and Chrome.

BUG=723125
==========

[sgurun-gerrit only, 2017-05-17 02:51:48]

On 2017/05/17 00:28:06, Nate Fischer wrote:
> jialiul@: for SafeBrowsing-related changes
> sgurun@: for android_webview/

what will happen if there is no server side code that understands this plink, in
other words, how can we disable showing learn more completely?

[commit-bot: I haz the power, 2017-05-17 05:40:18]

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

[commit-bot: I haz the power, 2017-05-17 05:40:19]

Dry run: This issue passed the CQ dry run.

[Jialiu Lin, 2017-05-17 15:47:46]

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:80: nullptr);
nit:  some in-line comment will be helpful. e.g. 

nullptr /* default help article link */);

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ui_manager_unittest.cc (right):

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager_unittest.cc:355: nullptr)) {
nit: add some inline comment.

https://codereview.chromium.org/2890703002/diff/1/components/security_interst...
File components/security_interstitials/core/base_safe_browsing_error_ui.h
(right):

https://codereview.chromium.org/2890703002/diff/1/components/security_interst...
components/security_interstitials/core/base_safe_browsing_error_ui.h:36: const
char* plink)
nit: prefer const std::string&  over const char*

https://codereview.chromium.org/2890703002/diff/1/components/security_interst...
components/security_interstitials/core/base_safe_browsing_error_ui.h:71: const
char* plink;
nit: plink is a very general name, unless you want to reuse it for other
purpose, how about just called help_center_article_link?  And prefer const
std::string over char*.

[felt, 2017-05-17 15:55:48]

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

[felt, 2017-05-17 15:55:52]

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:80: nullptr);
On 2017/05/17 15:47:46, Jialiu Lin wrote:
> nit:  some in-line comment will be helpful. e.g. 
> 
> nullptr /* default help article link */);

I'd prefer to have Chrome pass in the default link instead of nullptr. It seems
less prone to error. Could we do that instead?

[Nate Fischer, 2017-05-17 21:35:30]

> in other words, how can we disable showing learn more completely?

This is feasible. Selim, do you think it's better to have no learn-more-link at
all? Let me know your thoughts so I can close this CL and open a new one which
removes the link.

[sgurun-gerrit only, 2017-05-17 21:39:45]

On 2017/05/17 21:35:30, Nate Fischer wrote:
> > in other words, how can we disable showing learn more completely?
> 
> This is feasible. Selim, do you think it's better to have no learn-more-link
at
> all? Let me know your thoughts so I can close this CL and open a new one which
> removes the link.

I think the CL should provide both, i.e. a plink for learn more, and a way to
remove "learn more" for platforms that don't have support. for example right now
there is no support, so it should be disabled. Once the support lands, we can
enable it, and use plink as the location.

[Nate Fischer, 2017-05-17 21:41:37]

On 2017/05/17 21:39:45, sgurun wrote:
> On 2017/05/17 21:35:30, Nate Fischer wrote:
> > > in other words, how can we disable showing learn more completely?
> > 
> > This is feasible. Selim, do you think it's better to have no learn-more-link
> at
> > all? Let me know your thoughts so I can close this CL and open a new one
which
> > removes the link.
> 
> I think the CL should provide both, i.e. a plink for learn more, and a way to
> remove "learn more" for platforms that don't have support. for example right
now
> there is no support, so it should be disabled. Once the support lands, we can
> enable it, and use plink as the location.

Ok, I'll add it to this CL after addressing the current comments. Thanks

[chromium-reviews, 2017-05-17 21:42:09]

usually the plinks are populated much later than when the code lands on
trunk. they typically won't put up the content until right before the
feature goes to beta. so i don't think you should wait for the content to
go up before including the plink

On Wed, May 17, 2017 at 2:39 PM <sgurun@chromium.org> wrote:

> On 2017/05/17 21:35:30, Nate Fischer wrote:
> > > in other words, how can we disable showing learn more completely?
> >
> > This is feasible. Selim, do you think it's better to have no
> learn-more-link
> at
> > all? Let me know your thoughts so I can close this CL and open a new one
> which
> > removes the link.
>
> I think the CL should provide both, i.e. a plink for learn more, and a way
> to
> remove "learn more" for platforms that don't have support. for example
> right now
> there is no support, so it should be disabled. Once the support lands, we
> can
> enable it, and use plink as the location.
>
> https://codereview.chromium.org/2890703002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[felt, 2017-05-17 21:43:50]

ah my emailed comment was messed up, trying again:

usually the plinks are populated much later than when the code lands on trunk.
they typically won't put up the content until right before the feature goes to
beta. so i don't think you should wait for the content to go up before including
the plink

[Nate Fischer, 2017-05-17 22:31:55]

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

[commit-bot: I haz the power, 2017-05-17 22:33:55]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-17 22:34:43]

jialiul@ & felt@, PTAL

sgurun@, I'll upload code for disabling learn-more-link in the next patchset.

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:80: nullptr);
On 2017/05/17 15:55:50, felt wrote:
> On 2017/05/17 15:47:46, Jialiu Lin wrote:
> > nit:  some in-line comment will be helpful. e.g. 
> > 
> > nullptr /* default help article link */);
> 
> I'd prefer to have Chrome pass in the default link instead of nullptr. It
seems
> less prone to error. Could we do that instead?

Sure, I went with Felt's suggestion

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ui_manager_unittest.cc (right):

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager_unittest.cc:355: nullptr)) {
On 2017/05/17 15:47:46, Jialiu Lin wrote:
> nit: add some inline comment.

Acknowledged. I'm assigning the value from a function call now, so the meaning
should be clear. Let me know if you'd still like an explanatory comment. I added
comments for all the `false`s, to make them clearer.

https://codereview.chromium.org/2890703002/diff/1/components/security_interst...
File components/security_interstitials/core/base_safe_browsing_error_ui.h
(right):

https://codereview.chromium.org/2890703002/diff/1/components/security_interst...
components/security_interstitials/core/base_safe_browsing_error_ui.h:36: const
char* plink)
On 2017/05/17 15:47:46, Jialiu Lin wrote:
> nit: prefer const std::string&  over const char*

Done

https://codereview.chromium.org/2890703002/diff/1/components/security_interst...
components/security_interstitials/core/base_safe_browsing_error_ui.h:71: const
char* plink;
On 2017/05/17 15:47:46, Jialiu Lin wrote:
> nit: plink is a very general name, unless you want to reuse it for other
> purpose, how about just called help_center_article_link?  And prefer const
> std::string over char*.

Done

[Nate Fischer, 2017-05-17 22:46:05]

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

[commit-bot: I haz the power, 2017-05-17 22:47:44]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-17 22:56:28]

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

[commit-bot: I haz the power, 2017-05-17 22:56:47]

Dry run: CQ is trying da patch.

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

[Nate Fischer, 2017-05-18 00:01:13]

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

[commit-bot: I haz the power, 2017-05-18 00:03:53]

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-05-18 03:17:47]

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

[commit-bot: I haz the power, 2017-05-18 03:17:48]

Dry run: This issue passed the CQ dry run.

[Nate Fischer, 2017-05-19 00:13:32]

jialiul@ and sgurun@, PTAL.

Per the offline email thread, it sounds like we won't need to remove the
learn-more link. Correct me if I'm misunderstanding.

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ui_manager_unittest.cc (right):

https://codereview.chromium.org/2890703002/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager_unittest.cc:355: nullptr)) {
On 2017/05/17 22:34:43, Nate Fischer wrote:
> On 2017/05/17 15:47:46, Jialiu Lin wrote:
> > nit: add some inline comment.
> 
> Acknowledged. I'm assigning the value from a function call now, so the meaning
> should be clear. Let me know if you'd still like an explanatory comment. I
added
> comments for all the `false`s, to make them clearer.

I couldn't get the method to actually work nicely, so I just hard code the
string literal instead. I added a comment in the latest patchset.

[Jialiu Lin, 2017-05-19 00:16:01]

LGTM Thanks!

[sgurun-gerrit only, 2017-05-19 03:31:56]

On 2017/05/19 00:16:01, Jialiu Lin wrote:
> LGTM Thanks!

aw lgtm, thanks!

[Nate Fischer, 2017-05-19 16:48:03]

Thanks, all!

[Nate Fischer, 2017-05-19 16:48:27]

The CQ bit was checked by ntfschr@chromium.org

[commit-bot: I haz the power, 2017-05-19 16:49:22]

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-05-19 17:03:30]

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

[commit-bot: I haz the power, 2017-05-19 17:03:31]

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

[Nate Fischer, 2017-05-19 17:26:45]

The CQ bit was checked by ntfschr@chromium.org

[Nate Fischer, 2017-05-19 17:26:45]

The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org,
jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2890703002/#ps100001
(title: "Rebase and fix compile errors, no logic change")

[commit-bot: I haz the power, 2017-05-19 17:27:34]

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-05-19 19:15:34]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1495214805264850,
"parent_rev": "b8de8c51c48c9bf5cc72c3e7363d14cd8b5b66c6", "commit_rev":
"e952a3ef7450aae662c291c8c54986561ab65360"}

[commit-bot: I haz the power, 2017-05-19 19:15:47]

Description was changed from

==========
SafeBrowsing: allow WebView to customize the Help Center URL

This adds a 'plink' field to SBErrorDisplayOptions, allowing WebView
to specify its own plink (the p= URL query parameter) when visiting the
Help Center. This will allow the Help Center to differentiate between
Chrome and WebView and show appropriate information for each.

SBErrorDisplayOptions.plink is set to nullptr for Chrome, to indicate
that we should use a default value (p=cpn_safe_browsing).

Right now, the _wv suffix in the plink is ignored by the help center,
and is treated just like "cpn_safe_browsing" would be. So there should
be no change in high-level behavior, the URL will just vary slightly
between WebView and Chrome.

BUG=723125
==========

to

==========
SafeBrowsing: allow WebView to customize the Help Center URL

This adds a 'plink' field to SBErrorDisplayOptions, allowing WebView
to specify its own plink (the p= URL query parameter) when visiting the
Help Center. This will allow the Help Center to differentiate between
Chrome and WebView and show appropriate information for each.

SBErrorDisplayOptions.plink is set to nullptr for Chrome, to indicate
that we should use a default value (p=cpn_safe_browsing).

Right now, the _wv suffix in the plink is ignored by the help center,
and is treated just like "cpn_safe_browsing" would be. So there should
be no change in high-level behavior, the URL will just vary slightly
between WebView and Chrome.

BUG=723125

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

[commit-bot: I haz the power, 2017-05-19 19:15:49]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/e952a3ef7450aae662c291c8c549...