Add a checkbox to the malware interstitial page, for user to opt-in to send m...

Add a checkbox to the malware interstitial page, for user to opt-in to send malware report. The user's choice will be stored as preference in user's profile.

Depends on: http://codereview.chromium.org/4827001/
Design doc: https://docs.google.com/document/edit?id=1s-7qMjm23onV68SyqO6yi-xsfFzz4OBSOyHoV3cY8mQ&authkey=CMCF-MID

BUG=60831
TEST=unit_tests

[panayiotis, 2010-11-18 19:50:32]

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/resources/saf...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/resources/saf...
chrome/browser/resources/safe_browsing_malware_block.html:239: <div
class="main">
We should not show this div if we are in incognito mode, or if the tab's url is
https, since we are not going to send reports in this case anyway.

[lzheng, 2010-11-19 18:50:47]

Ke, Panayiotis, Ian:
We have a safe_browsing_multiple_threat_block.html page. What should we do with
that page for this feature?

http://codereview.chromium.org/5102001/diff/5001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5102001/diff/5001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7452: <message
name="IDS_SAFE_BROWSING_MALWARE_REPORTING_AGREE" desc="afeBrowsing Malware,
agree checkbox text">
typo: S is missing in 'afeBrowsing Malware, agree ...'

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/resources/saf...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/resources/saf...
chrome/browser/resources/safe_browsing_malware_block.html:242: <input
name="checky" id="checky" type="checkbox" jsvalues=".checked:boxchecked"
onclick="savepreference()">&nbsp;<label for="checky"
i18n-content="confirm_text"></label>
'checky' sounds a little strange. Do you mean 'check_yes', or 'checked'?

If user checked this box, what will we show the next time user get into this
page? Will the checkbox be checked? I am not clear how the
jsvalues=".checked:boxchecked" related to the boxchecked in
safe_browsing_blocking_page.cc.

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:420: 
I've pinged Panayiotis that we should have a Browser Test to test the blocking
page. Although the unittest covers some of the cases, the browser test could
help cover logics in SafeBrowsingBlockingPage::CommandReceived. Can you sync
with him to make sure we will test the checking/unchecking logic in the coming
browser_test?

[kewang, 2010-11-20 00:45:51]

http://codereview.chromium.org/5102001/diff/5001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5102001/diff/5001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:7452: <message
name="IDS_SAFE_BROWSING_MALWARE_REPORTING_AGREE" desc="afeBrowsing Malware,
agree checkbox text">
On 2010/11/19 18:50:47, lzheng wrote:
> typo: S is missing in 'afeBrowsing Malware, agree ...'

Done.

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/resources/saf...
File chrome/browser/resources/safe_browsing_malware_block.html (right):

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/resources/saf...
chrome/browser/resources/safe_browsing_malware_block.html:239: <div
class="main">
On 2010/11/18 19:50:32, panayiotis wrote:
> We should not show this div if we are in incognito mode, or if the tab's url
is
> https, since we are not going to send reports in this case anyway. 

added jsdisplay for a control here.

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/resources/saf...
chrome/browser/resources/safe_browsing_malware_block.html:242: <input
name="checky" id="checky" type="checkbox" jsvalues=".checked:boxchecked"
onclick="savepreference()">&nbsp;<label for="checky"
i18n-content="confirm_text"></label>
On 2010/11/19 18:50:47, lzheng wrote:
> 'checky' sounds a little strange. Do you mean 'check_yes', or 'checked'?
changed the name to 'checked' for name, and 'checkreport' for id name.
> 
> If user checked this box, what will we show the next time user get into this
> page? Will the checkbox be checked? I am not clear how the
> jsvalues=".checked:boxchecked" related to the boxchecked in
> safe_browsing_blocking_page.cc. 

Yes, works as you described. If the user checked it, all the following opened
tabs or new browser after exit will have it checked automatically.

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/safe_browsing...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc
(right):

http://codereview.chromium.org/5102001/diff/5001/chrome/browser/safe_browsing...
chrome/browser/safe_browsing/safe_browsing_blocking_page_unittest.cc:420: 
On 2010/11/19 18:50:47, lzheng wrote:
> I've pinged Panayiotis that we should have a Browser Test to test the blocking
> page. Although the unittest covers some of the cases, the browser test could
> help cover logics in SafeBrowsingBlockingPage::CommandReceived. Can you sync
> with him to make sure we will test the checking/unchecking logic in the coming
> browser_test?

Yes, Panos and I worked on it this afternoon and he already has an initial
browser test ready.

[lzheng, 2010-11-23 01:14:49]

A couple of nits below. 

Also, while I am reading this, I realized that because the function that
actually sends the information has not committed yet, should we put this behind
a flag for now and remove the flags once we decide to let use use this feature?

http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):

http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:304:
strings->SetBoolean("displaycheckbox", false);
nit: How about using a const char for "displaycheckbox"? Since it is used
multiple times here. Similar to "boxchecked" below.

http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):

http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/safe_browsing_blocking_page.h:106: // See if we
should even show the malware report option. For example, we
See ->Checks ?

[panayiotis, 2011-01-07 18:02:53]

Landing this with http://codereview.chromium.org/6066011/

On 2010/11/23 01:14:49, lzheng wrote:
> A couple of nits below. 
> 
> Also, while I am reading this, I realized that because the function that
> actually sends the information has not committed yet, should we put this
behind
> a flag for now and remove the flags once we decide to let use use this
feature?
> 
>
http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.cc (right):
> 
>
http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.cc:304:
> strings->SetBoolean("displaycheckbox", false);
> nit: How about using a const char for "displaycheckbox"? Since it is used
> multiple times here. Similar to "boxchecked" below.
> 
>
http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
> File chrome/browser/safe_browsing/safe_browsing_blocking_page.h (right):
> 
>
http://codereview.chromium.org/5102001/diff/17001/chrome/browser/safe_browsin...
> chrome/browser/safe_browsing/safe_browsing_blocking_page.h:106: // See if we
> should even show the malware report option. For example, we
> See ->Checks ?