Make commands consistent across security interstitials

Make commands consistent across security interstitials

Committed: https://crrev.com/1c42a82a61d33ea102701a5621e52b3c8f4455d2
Cr-Commit-Position: refs/heads/master@{#320107}

[felt, 2015-03-06 16:44:22]

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

[felt, 2015-03-06 16:44:30]

meacer, ptal

[meacer, 2015-03-06 18:29:22]

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:72: "<a
id=\"privacy-link\" href=\"\" onclick=\"sendCommand(10); "
How about using |sendCommand(%d)| here and filling the actual enum value in
|PopulateExtendedReportingOption|? One fewer harcoded value.

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:299: std::string
bad_url_spec = unsafe_resources_[element_index].url.spec();
While you are at it, could you have a reference to
unsafe_resources_[element_index] and use that below? It would make the code
below cleaner.

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/ssl/capti...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/ssl/capti...
chrome/browser/ssl/captive_portal_blocking_page.cc:195: bool retval =
base::StringToInt(command, &cmd);
|command| has quotes around it (e.g. "1"), need to remove them before passing it
base::StringToInt. And since all security interstitials have to do this, you
could have a helper method in SecurityInterstitialPage for it.

[meacer, 2015-03-07 01:00:04]

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/ssl/capti...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/ssl/capti...
chrome/browser/ssl/captive_portal_blocking_page.cc:195: bool retval =
base::StringToInt(command, &cmd);
On 2015/03/06 18:29:22, Mustafa Emre Acer wrote:
> |command| has quotes around it (e.g. "1"), need to remove them before passing
it
> base::StringToInt. And since all security interstitials have to do this, you
> could have a helper method in SecurityInterstitialPage for it.

Correction: |command| shouldn't normally have quotes around it as you said, when
sent from the interstitial. But Captive portal and SB tests call
|WaitForRenderFrameReady()| which sends out an extra "pageLoadComplete" message
when the interstitial fully loads:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

So you'll need to ignore that message explicitly.

[felt, 2015-03-07 04:02:39]

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:72: "<a
id=\"privacy-link\" href=\"\" onclick=\"sendCommand(10); "
On 2015/03/06 18:29:22, Mustafa Emre Acer wrote:
> How about using |sendCommand(%d)| here and filling the actual enum value in
> |PopulateExtendedReportingOption|? One fewer harcoded value.

Done.

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:299: std::string
bad_url_spec = unsafe_resources_[element_index].url.spec();
On 2015/03/06 18:29:22, Mustafa Emre Acer wrote:
> While you are at it, could you have a reference to
> unsafe_resources_[element_index] and use that below? It would make the code
> below cleaner.

Done.

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/ssl/capti...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/ssl/capti...
chrome/browser/ssl/captive_portal_blocking_page.cc:195: bool retval =
base::StringToInt(command, &cmd);
On 2015/03/07 01:00:04, Mustafa Emre Acer wrote:
> On 2015/03/06 18:29:22, Mustafa Emre Acer wrote:
> > |command| has quotes around it (e.g. "1"), need to remove them before
passing
> it
> > base::StringToInt. And since all security interstitials have to do this, you
> > could have a helper method in SecurityInterstitialPage for it.
> 
> Correction: |command| shouldn't normally have quotes around it as you said,
when
> sent from the interstitial. But Captive portal and SB tests call
> |WaitForRenderFrameReady()| which sends out an extra "pageLoadComplete"
message
> when the interstitial fully loads:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
> 
> So you'll need to ignore that message explicitly.

Done.

[meacer, 2015-03-07 05:23:10]

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:299: std::string
bad_url_spec = unsafe_resources_[element_index].url.spec();
On 2015/03/07 04:02:39, felt wrote:
> On 2015/03/06 18:29:22, Mustafa Emre Acer wrote:
> > While you are at it, could you have a reference to
> > unsafe_resources_[element_index] and use that below? It would make the code
> > below cleaner.
> 
> Done.

Why not |const UnsafeResource& unsafe_resource = ...|? Looks okay as it is but
could be just one less copy.

https://codereview.chromium.org/981243003/diff/60001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/60001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:210: if (page_cmd ==
"pageLoadComplete") {
Need to check "pageLoadComplete" (with quotes) here.

You could also make "pageLoadComplete" a constant, but it's ok if you ignore it
for this CL :)

https://codereview.chromium.org/981243003/diff/60001/chrome/browser/ssl/capti...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/60001/chrome/browser/ssl/capti...
chrome/browser/ssl/captive_portal_blocking_page.cc:194: if (command ==
"pageLoadComplete") {
Same as before, need to check "pageLoadComplete" with quotes.

[felt, 2015-03-07 05:50:04]

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/40001/chrome/browser/safe_brow...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:299: std::string
bad_url_spec = unsafe_resources_[element_index].url.spec();
On 2015/03/07 05:23:09, Mustafa Emre Acer wrote:
> On 2015/03/07 04:02:39, felt wrote:
> > On 2015/03/06 18:29:22, Mustafa Emre Acer wrote:
> > > While you are at it, could you have a reference to
> > > unsafe_resources_[element_index] and use that below? It would make the
code
> > > below cleaner.
> > 
> > Done.
> 
> Why not |const UnsafeResource& unsafe_resource = ...|? Looks okay as it is but
> could be just one less copy.

Sure. Leaving the int as a separate definition because it will need to be
defined later, shouldn't always be 0.

https://codereview.chromium.org/981243003/diff/60001/chrome/browser/ssl/capti...
File chrome/browser/ssl/captive_portal_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/60001/chrome/browser/ssl/capti...
chrome/browser/ssl/captive_portal_blocking_page.cc:194: if (command ==
"pageLoadComplete") {
On 2015/03/07 05:23:09, Mustafa Emre Acer wrote:
> Same as before, need to check "pageLoadComplete" with quotes.

Whoops, you got to this just barely after the trybots finished. ;) Fixed now.

[meacer, 2015-03-07 06:01:14]

Looks good, but not sure why the tryjobs are failing this time :/

[meacer, 2015-03-07 07:50:59]

Lgtm with one comment.

https://codereview.chromium.org/981243003/diff/120001/chrome/browser/ssl/capt...
File chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc (right):

https://codereview.chromium.org/981243003/diff/120001/chrome/browser/ssl/capt...
chrome/browser/ssl/captive_portal_blocking_page_browsertest.cc:45: EXPECT_EQ(1,
content::ExecuteScriptAndExtractInt(
EXPECT_TRUE(content::ExecuteScriptAndExtractInt(...

instead.

[felt, 2015-03-07 07:54:32]

hold up, the tests still aren't passing (why i didn't mail yet).
linux_chromium_chromeos_rel_ng is failing. i'm sleepy now so i'll figure it out
tomorrow :)

[meacer, 2015-03-07 07:58:07]

Yeah, I saw the test failure just after sending the comment :p

https://codereview.chromium.org/981243003/diff/120001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/981243003/diff/120001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:404: void
SendCommand(const int command) {
nit: const not needed.

[felt, 2015-03-07 17:24:44]

OK, *now* the tests are passing if you want to take a look. :)

[felt, 2015-03-07 17:24:57]

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

[felt, 2015-03-07 17:24:58]

mattm, PTAL

[meacer, 2015-03-07 20:45:34]

Ah, I forgot about the InterstitialDisplayingText one. Still lgtm.

[mattm, 2015-03-11 00:10:38]

lgtm

[felt, 2015-03-11 03:43:55]

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

[felt, 2015-03-11 03:44:32]

bernhard, ptal

[Bernhard Bauer, 2015-03-11 08:42:14]

Nice!

Can you add a bug number, or remove the BUG= field?

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:26: enum
SecurityInterstitialCommands {
Can you make this private?

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:220: if (command ==
CMD_DO_REPORT) {
I *think* you could turn this into a switch statement now. The only tricky part
I can see is the |proceed_blocked| check below, but you could solve that by
falling through to the next case if proceeding is disabled, and breaking out of
the switch statement only if it is allowed. With sufficient comments, that might
make the control flow even a bit clearer than it is right now.

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:304: size_t
element_index = 0;
Move this into the if-statement block?

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:404: void
SendCommand(int command) {
You could use the enum type here, and then maybe the compiler would be able to
check the value that is passed in?

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:504: default: {
Can we remove the default statement, so that the compiler will check we're
handling all cases?

[felt, 2015-03-11 15:25:10]

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:26: enum
SecurityInterstitialCommands {
On 2015/03/11 08:42:14, Bernhard Bauer wrote:
> Can you make this private?

The tests (for the subclasses) use it. I'd have to friend them on this class so
that they can access it. That seems undesirable to me. Given that, do you have a
strong opinion?

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:220: if (command ==
CMD_DO_REPORT) {
On 2015/03/11 08:42:14, Bernhard Bauer wrote:
> I *think* you could turn this into a switch statement now. The only tricky
part
> I can see is the |proceed_blocked| check below, but you could solve that by
> falling through to the next case if proceeding is disabled, and breaking out
of
> the switch statement only if it is allowed. With sufficient comments, that
might
> make the control flow even a bit clearer than it is right now.

Done.

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:304: size_t
element_index = 0;
On 2015/03/11 08:42:14, Bernhard Bauer wrote:
> Move this into the if-statement block?

Done.

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/safe_bro...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:404: void
SendCommand(int command) {
On 2015/03/11 08:42:14, Bernhard Bauer wrote:
> You could use the enum type here, and then maybe the compiler would be able to
> check the value that is passed in?

Done.

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:504: default: {
On 2015/03/11 08:42:14, Bernhard Bauer wrote:
> Can we remove the default statement, so that the compiler will check we're
> handling all cases?

Done.

[Bernhard Bauer, 2015-03-11 16:19:20]

LGTM w/ a question below:

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/981243003/diff/140001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:26: enum
SecurityInterstitialCommands {
On 2015/03/11 15:25:09, felt wrote:
> On 2015/03/11 08:42:14, Bernhard Bauer wrote:
> > Can you make this private?
> 
> The tests (for the subclasses) use it. I'd have to friend them on this class
so
> that they can access it. That seems undesirable to me. Given that, do you have
a
> strong opinion?

Oh, right! Okay then, fair enough.

https://codereview.chromium.org/981243003/diff/160001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/160001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:502: NOTREACHED() << "Unexpected
command: " << command;
This changes the semantics from the previous version; before, we would ignore
these commands, and now we DCHECK. I assume that's on purpose?

[felt, 2015-03-11 16:25:21]

https://codereview.chromium.org/981243003/diff/160001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/981243003/diff/160001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_blocking_page.cc:502: NOTREACHED() << "Unexpected
command: " << command;
On 2015/03/11 16:19:20, Bernhard Bauer wrote:
> This changes the semantics from the previous version; before, we would ignore
> these commands, and now we DCHECK. I assume that's on purpose?

Yes, I changed my mind and decided a DCHECK would be more appropriate (since
receiving these commands would mean there is a bug in the JavaScript logic).

[felt, 2015-03-11 16:28:04]

The CQ bit was checked by felt@chromium.org

[felt, 2015-03-11 16:28:05]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org,
mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/981243003/#ps160001
(title: "if -> switch in SafeBrowsingBlockingPage")

[commit-bot: I haz the power, 2015-03-11 16:28:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981243003/160001

[commit-bot: I haz the power, 2015-03-11 17:46:54]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-11 17:49:34]

Patchset 9 (id:??) landed as
https://crrev.com/1c42a82a61d33ea102701a5621e52b3c8f4455d2
Cr-Commit-Position: refs/heads/master@{#320107}