Safe Browsing phishing interstitial redesign.

Safe Browsing phishing interstitial redesign.

UI tweaks for both phishing and malware pages.

BUG=148870, 143999
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156948

[mattm, 2012-09-13 03:18:28]

dbeam: chrome/browser/resources/
jcivelli: .grd, chrome/browser/safe_browsing/

screens @ http://www/~mattm/no_crawl/chrome/sb_interstitial6/

[mattm, 2012-09-13 03:32:15]

On 2012/09/13 03:18:28, mattm wrote:
> dbeam: chrome/browser/resources/
> jcivelli: .grd, chrome/browser/safe_browsing/
> 
> screens @ http://www/%7Emattm/no_crawl/chrome/sb_interstitial6/

Minor update. Added nowrap to the "learn more" link.

new screens @ http://www/~mattm/no_crawl/chrome/sb_interstitial7

[Dan Beam, 2012-09-13 04:54:26]

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/resources/s...
File chrome/browser/resources/safe_browsing/malware_block_v2.html (right):

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/resources/s...
chrome/browser/resources/safe_browsing/malware_block_v2.html:43: <a
id="report-error-link" href="#" i18n-content="report_error"></a>
nit: 80 cols

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:676: return;
nit: \n here IMO

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1174:
strings->SetString("details", "");
what are these empty strings about?

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_v2_test.cc
(right):

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page_v2_test.cc:711:
ASSERT_TRUE(WaitForReady());
maybe make a setup method here? this is the same code everywhere it looks
like...

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page_v2_test.cc:714:
AssertNoInterstitial(true);    // Assert the interstitial is gone
nit: why 4 \s?

[mattm, 2012-09-13 20:12:07]

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/resources/s...
File chrome/browser/resources/safe_browsing/malware_block_v2.html (right):

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/resources/s...
chrome/browser/resources/safe_browsing/malware_block_v2.html:43: <a
id="report-error-link" href="#" i18n-content="report_error"></a>
On 2012/09/13 04:54:26, Dan Beam wrote:
> nit: 80 cols

Done.

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:676: return;
On 2012/09/13 04:54:26, Dan Beam wrote:
> nit: \n here IMO

Done.

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1174:
strings->SetString("details", "");
On 2012/09/13 04:54:26, Dan Beam wrote:
> what are these empty strings about?

Since we use the same html for both malware and phishing pages, some parts are
set blank if they are not used.  (Even if the items are hidden with jsdisplay,
we still get console.log spam about undefined data if we don't set them to
something.)

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_v2_test.cc
(right):

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page_v2_test.cc:711:
ASSERT_TRUE(WaitForReady());
On 2012/09/13 04:54:26, Dan Beam wrote:
> maybe make a setup method here? this is the same code everywhere it looks
> like...

Done.

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page_v2_test.cc:714:
AssertNoInterstitial(true);    // Assert the interstitial is gone
On 2012/09/13 04:54:26, Dan Beam wrote:
> nit: why 4 \s?

dunno, that's what all the other tests had. :p

fixed.

[mattm, 2012-09-14 17:28:41]

updated: added a few more tweaks from UI leads feedback.

[mattm, 2012-09-14 17:35:47]

On 2012/09/14 17:28:41, mattm wrote:
> updated: added a few more tweaks from UI leads feedback.

screens @ http://www/~mattm/no_crawl/chrome/sb_interstitial9/linux/

[Jay Civelli, 2012-09-14 20:26:51]

lgtm

[Dan Beam, 2012-09-14 21:27:10]

lgtm

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/10928173/diff/10001/chrome/browser/safe_browsi...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:1174:
strings->SetString("details", "");
On 2012/09/13 20:12:07, mattm wrote:
> On 2012/09/13 04:54:26, Dan Beam wrote:
> > what are these empty strings about?
> 
> Since we use the same html for both malware and phishing pages, some parts are
> set blank if they are not used.  (Even if the items are hidden with jsdisplay,
> we still get console.log spam about undefined data if we don't set them to
> something.)

ahhh, I understand

[commit-bot: I haz the power, 2012-09-14 22:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/10928173/1014

[commit-bot: I haz the power, 2012-09-15 01:30:27]

Change committed as 156948