Re-design the malware interstitial page. The change is mostly about: make the...

Re-design the malware interstitial page. The change is mostly about: make the 'go back' button big and green in a central place, to encourage people go back to safety; and more clear description about why we block the page.

Sample screenshots at: https://docs.google.com/document/edit?id=1Kk2wxsj8JjIjlC1WUPamDlEPWZH-iWUoECMRdyQ9t_4&hl=en&authkey=CJKfrM0D

Patch from Ke Wang <kewang@google.com>;

BUG=60813
TEST=unit_tests. To see the UI change, start the browser and try load some sites in malware blacklist.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=64984

[arv (Not doing code reviews), 2010-10-27 22:21:37]

Looks nice

http://codereview.chromium.org/4175003/diff/13001/14001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4175003/diff/13001/14001#newcode7325
chrome/app/generated_resources.grd:7325: Google has found malicious software may
be installed onto your computer if you proceed. If you've visited this site in
the past or you trust this site, it's possible that it has just recently been
compromised by a hacker. You should not proceed, and perhaps try again tomorrow
or go somewhere else.
Do not use tabs

http://codereview.chromium.org/4175003/diff/13001/14002
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/4175003/diff/13001/14002#newcode78
chrome/browser/resources/safe_browsing_malware_block.html:78: outline-width: 0;
Make sure you have some :focus state as well.

http://codereview.chromium.org/4175003/diff/13001/14002#newcode192
chrome/browser/resources/safe_browsing_malware_block.html:192: <div class="main"
i18n-values=".innerHTML:description1"></div><br>
remove <br> and use margin instead

http://codereview.chromium.org/4175003/diff/13001/14002#newcode205
chrome/browser/resources/safe_browsing_malware_block.html:205: <div
class="main">
Can you put the i18n-values here instead of the extra span?

http://codereview.chromium.org/4175003/diff/13001/14002#newcode206
chrome/browser/resources/safe_browsing_malware_block.html:206: <span
i18n-values=".innerHTML:description3"></span><br /><br /><br />
No /> in HTML

http://codereview.chromium.org/4175003/diff/13001/14002#newcode206
chrome/browser/resources/safe_browsing_malware_block.html:206: <span
i18n-values=".innerHTML:description3"></span><br /><br /><br />
Remove <br> and use display: block and margins

http://codereview.chromium.org/4175003/diff/13001/14003
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/4175003/diff/13001/14003#newcode61
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:61: L"<a href=\"\"
onClick=\"sendCommand('proceed'); return false;\" "
onclick

[kewang, 2010-10-28 18:07:51]

http://codereview.chromium.org/4175003/diff/13001/14001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4175003/diff/13001/14001#newcode7325
chrome/app/generated_resources.grd:7325: Google has found malicious software may
be installed onto your computer if you proceed. If you've visited this site in
the past or you trust this site, it's possible that it has just recently been
compromised by a hacker. You should not proceed, and perhaps try again tomorrow
or go somewhere else.
On 2010/10/27 22:21:37, arv wrote:
> Do not use tabs

Done.

http://codereview.chromium.org/4175003/diff/13001/14002
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/4175003/diff/13001/14002#newcode78
chrome/browser/resources/safe_browsing_malware_block.html:78: outline-width: 0;
On 2010/10/27 22:21:37, arv wrote:
> Make sure you have some :focus state as well.
what type of :focus state should I set here? Is it just for the buttons?

http://codereview.chromium.org/4175003/diff/13001/14002#newcode192
chrome/browser/resources/safe_browsing_malware_block.html:192: <div class="main"
i18n-values=".innerHTML:description1"></div><br>
On 2010/10/27 22:21:37, arv wrote:
> remove <br> and use margin instead

Done.

http://codereview.chromium.org/4175003/diff/13001/14002#newcode205
chrome/browser/resources/safe_browsing_malware_block.html:205: <div
class="main">
On 2010/10/27 22:21:37, arv wrote:
> Can you put the i18n-values here instead of the extra span?

Done.

http://codereview.chromium.org/4175003/diff/13001/14002#newcode206
chrome/browser/resources/safe_browsing_malware_block.html:206: <span
i18n-values=".innerHTML:description3"></span><br /><br /><br />
On 2010/10/27 22:21:37, arv wrote:
> No /> in HTML

Done.

http://codereview.chromium.org/4175003/diff/13001/14002#newcode206
chrome/browser/resources/safe_browsing_malware_block.html:206: <span
i18n-values=".innerHTML:description3"></span><br /><br /><br />
On 2010/10/27 22:21:37, arv wrote:
> Remove <br> and use display: block and margins

removed. I found the margin defined by .main is enough here.

http://codereview.chromium.org/4175003/diff/13001/14003
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/4175003/diff/13001/14003#newcode61
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:61: L"<a href=\"\"
onClick=\"sendCommand('proceed'); return false;\" "
On 2010/10/27 22:21:37, arv wrote:
> onclick

Done.

[lzheng, 2010-10-29 00:25:25]

I might not be qualified to be a main reviewer for this since I've never done
anything related to the UI.

I guess arv@ might be able to cover you with that part, as you've include him.

http://codereview.chromium.org/4175003/diff/20001/21001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4175003/diff/20001/21001#newcode7319
chrome/app/generated_resources.grd:7319: Warning: Something's Not Right Here!
I think we should let Ian to take a look at the the wordings here. I am not sure
if we should let legal take a quick glance, just in base. Ian will decide.

http://codereview.chromium.org/4175003/diff/20001/21001#newcode7337
chrome/app/generated_resources.grd:7337: We have already notified <ph
name="HOST_NAME">&lt;strong&gt;$1<ex>evil.com</ex>&lt;/strong&gt;</ph> that we
found malware on the site. For more about the problems found on <ph
name="HOST_NAME2">$2<ex>www.malware.com</ex></ph>, visit the Google <ph
name="DIAGNOSTIC_PAGE">$3<ex>diagnostic page</ex></ph>.
Should this be "we have alrady notified www.goodwebsite.com that ...", intead of
evil.com?

[kewang, 2010-10-29 17:59:58]

http://codereview.chromium.org/4175003/diff/20001/21001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/4175003/diff/20001/21001#newcode7319
chrome/app/generated_resources.grd:7319: Warning: Something's Not Right Here!
On 2010/10/29 00:25:26, lzheng wrote:
> I think we should let Ian to take a look at the the wordings here. I am not
sure
> if we should let legal take a quick glance, just in base. Ian will decide.

The words here are all from Ian. :)

http://codereview.chromium.org/4175003/diff/20001/21001#newcode7337
chrome/app/generated_resources.grd:7337: We have already notified <ph
name="HOST_NAME">&lt;strong&gt;$1<ex>evil.com</ex>&lt;/strong&gt;</ph> that we
found malware on the site. For more about the problems found on <ph
name="HOST_NAME2">$2<ex>www.malware.com</ex></ph>, visit the Google <ph
name="DIAGNOSTIC_PAGE">$3<ex>diagnostic page</ex></ph>.
On 2010/10/29 00:25:26, lzheng wrote:
> Should this be "we have alrady notified http://www.goodwebsite.com that ...",
intead of
> evil.com?

No, we only notify the sites we think are evil and blacklisted. Chrome does
cross-sites warning, and don't think it's feasible to notify all the good sites
that just referring to some bad one.

[arv (Not doing code reviews), 2010-10-29 19:11:31]

I'm not sure how you want to distinguish focused buttons but please ensure that
they are distinguishable.

http://codereview.chromium.org/4175003/diff/20001/21002
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/4175003/diff/20001/21002#newcode169
chrome/browser/resources/safe_browsing_malware_block.html:169: var morebtn =
document.getElementById('more')
moreButton

http://codereview.chromium.org/4175003/diff/20001/21003
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/4175003/diff/20001/21003#newcode58
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:58:
L"onMouseDown=\"return false;\">%ls</a>";
onmousedown

Attributes should be lowercase

[kewang, 2010-10-29 20:49:50]

On 2010/10/29 19:11:31, arv wrote:
> I'm not sure how you want to distinguish focused buttons but please ensure
that
> they are distinguishable.

I added the :focus part for the buttons, please take a look.
> 
> http://codereview.chromium.org/4175003/diff/20001/21002
> File chrome/browser/resources/safe_browsing_malware_block.html (right):
> 
> http://codereview.chromium.org/4175003/diff/20001/21002#newcode169
> chrome/browser/resources/safe_browsing_malware_block.html:169: var morebtn =
> document.getElementById('more')
> moreButton

Done.

> http://codereview.chromium.org/4175003/diff/20001/21003
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):
> 
> http://codereview.chromium.org/4175003/diff/20001/21003#newcode58
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:58:
> L"onMouseDown=\"return false;\">%ls</a>";
> onmousedown
> 
> Attributes should be lowercase

Done.

[arv (Not doing code reviews), 2010-11-01 22:51:27]

I don't see a new snapshot.

erik



On Fri, Oct 29, 2010 at 13:51,  <kewang@google.com> wrote:
> http://codereview.chromium.org/4175003/show
>

[kewang, 2010-11-01 23:00:26]

oops, orgot to run upload. Now it's updated. Really sorry about it.

On Mon, Nov 1, 2010 at 3:50 PM, Erik Arvidsson <arv@chromium.org> wrote:
> I don't see a new snapshot.
>
> erik
>
>
>
> On Fri, Oct 29, 2010 at 13:51,  <kewang@google.com> wrote:
>> http://codereview.chromium.org/4175003/show
>>
>



-- 
Ke

[arv (Not doing code reviews), 2010-11-01 23:05:21]

LGTM

[ian fette, 2010-11-01 23:17:05]

lgtm

On 2010/11/01 23:05:21, arv wrote:
> LGTM

[kewang, 2010-11-02 21:57:51]

Hi, Lei and arv,

As I am not a committer yet, could you please help me submit this CL?
I have updated the CL's description to include the ' Contributed by
...' part.

Thanks a lot,


On Mon, Nov 1, 2010 at 4:17 PM,  <ian@chromium.org> wrote:
> lgtm
>
> On 2010/11/01 23:05:21, arv wrote:
>>
>> LGTM
>
>
>
> http://codereview.chromium.org/4175003/show
>



-- 
Ke

[arv (Not doing code reviews), 2010-11-03 22:48:17]

Since no one else replied I'm committing this for you.

erik


On Tue, Nov 2, 2010 at 14:57, Ke Wang <kewang@google.com> wrote:

> Hi, Lei and arv,
>
> As I am not a committer yet, could you please help me submit this CL?
> I have updated the CL's description to include the ' Contributed by
> ...' part.
>
> Thanks a lot,
>
>
> On Mon, Nov 1, 2010 at 4:17 PM,  <ian@chromium.org> wrote:
> > lgtm
> >
> > On 2010/11/01 23:05:21, arv wrote:
> >>
> >> LGTM
> >
> >
> >
> > http://codereview.chromium.org/4175003/show
> >
>
>
>
> --
> Ke
>

[lzheng1, 2010-11-03 23:22:22]

Thanks Erik. I can help next time.

Lei

On Wed, Nov 3, 2010 at 3:47 PM, Erik Arvidsson <arv@chromium.org> wrote:

> Since no one else replied I'm committing this for you.
>
> erik
>
>
> On Tue, Nov 2, 2010 at 14:57, Ke Wang <kewang@google.com> wrote:
>
>> Hi, Lei and arv,
>>
>> As I am not a committer yet, could you please help me submit this CL?
>> I have updated the CL's description to include the ' Contributed by
>> ...' part.
>>
>> Thanks a lot,
>>
>>
>> On Mon, Nov 1, 2010 at 4:17 PM,  <ian@chromium.org> wrote:
>> > lgtm
>> >
>> > On 2010/11/01 23:05:21, arv wrote:
>> >>
>> >> LGTM
>> >
>> >
>> >
>> > http://codereview.chromium.org/4175003/show
>> >
>>
>>
>>
>> --
>> Ke
>>
>
>