Refrain from sending hit report when user is in incognito mode

Even if user has SafeBrowsing Extended Reporting enabled, we want to refrain from sending a hit report when the user is in incognito mode.

BUG=689149
Review-Url: https://codereview.chromium.org/2886043002
Cr-Commit-Position: refs/heads/master@{#473360}
Committed: https://chromium.googlesource.com/chromium/src/+/cdcd0b7cbe1630435abab4a03ca0543752138b56

[mortonm, 2017-05-17 19:30:48]

The CQ bit was checked by mortonm@google.com to run a CQ dry run

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

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-17 20:42:15]

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

[commit-bot: I haz the power, 2017-05-17 20:42:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mortonm, 2017-05-18 20:05:17]

The CQ bit was checked by mortonm@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-05-18 20:06:44]

Dry run: CQ is trying da patch.

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

[mortonm, 2017-05-18 22:06:43]

mortonm@google.com changed reviewers:
+ jialiul@chromium.org

[Jialiu Lin, 2017-05-18 22:32:13]

Almost there, excellent work!
A couple of minor things:
Please follow the following guidelines to give this CL a better description. 
https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...


Bug number should be 689149.

And a couple of inline comments. Most of them are nits.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:218: bool
hit_report_sent_ = false;
nit: since all the other members are initialized in constructor, you might want
to do the same thing for hit_report_sent_ too.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:740: //
Browser* incognito_browser;
You probably need this one. See my comments below. 
And you need to initialize this member to nullptr in the constructor.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1276:
browser()->profile()->GetPrefs()->SetBoolean(
You're configuring the extended reporting prefs on a irrelevant browser
instance. Shouldn't it be the incognito browser?

You probably need a incognito_browser private member.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:128:
web_contents->GetBrowserContext()->IsOffTheRecord() == false) {
nit: replace "web_contents->GetBrowserContext()->IsOffTheRecord() == false" 
with "!web_contents->GetBrowserContext()->IsOffTheRecord()"

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:73: // Will report only for UMA ||
is_extended_reporting.
You can remove the last line of this comments, since it is already covered by
the comment above ShouldSendHitReport(..)

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:79: static bool
ShouldSendHitReport(const HitReport& hit_report,
How about moving this to protected session?

https://codereview.chromium.org/2886043002/diff/100001/components/safe_browsi...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/100001/components/safe_browsi...
components/safe_browsing/base_ui_manager.cc:249: // UMA || extended_reporting
users.
This comment is no longer accurate. Could you update it? 
Should be ".., only for extended reporting users who are not in incognito mode."

[mortonm, 2017-05-18 22:53:01]

Description was changed from

==========
checkpoint for fixing hit reports in incognito mode

BUG=
==========

to

==========
Even if user has SafeBrowsing Extended Reporting enabled, we want to refrain
from sending a hit report when the user is in incognito mode.

BUG=689149
==========

[Jialiu Lin, 2017-05-19 20:14:02]

LGTM with 1 more comment

Generally, we'd prefer replying every comment in the review. You can reply as
simple as "Done" or "Acknowledged" (which means you're not going to fix), or
whatever you like.

Similar to the initial review, you need to push "Publish+Mail" to inform the
reviewer that it is time to take another look. 

:-)

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

https://codereview.chromium.org/2886043002/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1276: GURL url
= SetupWarningAndNavigate(incognito_browser);    // not incognito
nit: comment "// not incognito"  -> "// incognito"

[mortonm, 2017-05-19 20:21:04]

All comments have been addressed.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:218: bool
hit_report_sent_ = false;
On 2017/05/18 22:32:12, Jialiu Lin wrote:
> nit: since all the other members are initialized in constructor, you might
want
> to do the same thing for hit_report_sent_ too.

Done.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:740: //
Browser* incognito_browser;
On 2017/05/18 22:32:12, Jialiu Lin wrote:
> You probably need this one. See my comments below. 
> And you need to initialize this member to nullptr in the constructor. 

Done.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1276:
browser()->profile()->GetPrefs()->SetBoolean(
On 2017/05/18 22:32:12, Jialiu Lin wrote:
> You're configuring the extended reporting prefs on a irrelevant browser
> instance. Shouldn't it be the incognito browser?
> 
> You probably need a incognito_browser private member.  

Done.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:128:
web_contents->GetBrowserContext()->IsOffTheRecord() == false) {
On 2017/05/18 22:32:12, Jialiu Lin wrote:
> nit: replace "web_contents->GetBrowserContext()->IsOffTheRecord() == false" 
> with "!web_contents->GetBrowserContext()->IsOffTheRecord()"

Done.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:73: // Will report only for UMA ||
is_extended_reporting.
On 2017/05/18 22:32:12, Jialiu Lin wrote:
> You can remove the last line of this comments, since it is already covered by
> the comment above ShouldSendHitReport(..)

Done.

https://codereview.chromium.org/2886043002/diff/100001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:79: static bool
ShouldSendHitReport(const HitReport& hit_report,
On 2017/05/18 22:32:12, Jialiu Lin wrote:
> How about moving this to protected session?

Done.

https://codereview.chromium.org/2886043002/diff/100001/components/safe_browsi...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/100001/components/safe_browsi...
components/safe_browsing/base_ui_manager.cc:249: // UMA || extended_reporting
users.
On 2017/05/18 22:32:12, Jialiu Lin wrote:
> This comment is no longer accurate. Could you update it? 
> Should be ".., only for extended reporting users who are not in incognito
mode."

Done.

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

https://codereview.chromium.org/2886043002/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:1276: GURL url
= SetupWarningAndNavigate(incognito_browser);    // not incognito
On 2017/05/19 20:14:02, Jialiu Lin wrote:
> nit: comment "// not incognito"  -> "// incognito"

Done.

[mortonm, 2017-05-19 20:25:46]

The CQ bit was checked by mortonm@google.com

[mortonm, 2017-05-19 20:25:47]

The patchset sent to the CQ was uploaded after l-g-t-m from jialiul@chromium.org
Link to the patchset: https://codereview.chromium.org/2886043002/#ps140001
(title: "fixed comment typo")

[commit-bot: I haz the power, 2017-05-19 20:26:25]

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 22:20:08]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1495225546662890,
"parent_rev": "065b3d6c931cdf5a1719cac70a1896fadec77047", "commit_rev":
"cdcd0b7cbe1630435abab4a03ca0543752138b56"}

[commit-bot: I haz the power, 2017-05-19 22:20:18]

Description was changed from

==========
Even if user has SafeBrowsing Extended Reporting enabled, we want to refrain
from sending a hit report when the user is in incognito mode.

BUG=689149
==========

to

==========
Even if user has SafeBrowsing Extended Reporting enabled, we want to refrain
from sending a hit report when the user is in incognito mode.

BUG=689149

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

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

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/cdcd0b7cbe1630435abab4a03ca0...

[mortonm, 2017-05-19 22:25:36]

Description was changed from

==========
Even if user has SafeBrowsing Extended Reporting enabled, we want to refrain
from sending a hit report when the user is in incognito mode.

BUG=689149

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

to

==========
Even if user has SafeBrowsing Extended Reporting enabled, we want to refrain
from sending a hit report when the user is in incognito mode.

BUG=689149

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

[vakh (use Gerrit instead), 2017-05-19 22:41:12]

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

[vakh (use Gerrit instead), 2017-05-19 22:41:13]

lgtm

Congrats on your first commit!! Woohoo!!

Sorry for the post-commmit drive-by review. I missed the CL earlier.

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:106: // Static
nit: static

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:108: WebContents* web_contents) {
could web_contents also be const?

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:111: return true;
simpler re-write:
return hit_report.extended_reporting_level != SBER_LEVEL_OFF &&
!web_contents->GetBrowserContext()->IsOffTheRecord()

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:72: // non-empty, the request will be
sent as a POST instead of a GET.
I don't see the |post_data| argument to the function. I think this comment is in
dire need of an update. It also needs to mention that the report is not sent
from Incognito mode.

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
components/safe_browsing/base_ui_manager.cc:247: // A safebrowsing hit is sent
after a blocking page for malware/phishing
nit: SafeBrowsing

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
File components/safe_browsing/base_ui_manager.h (right):

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
components/safe_browsing/base_ui_manager.h:64: // to the server. Can only be
called on UI thread.
Please update this comment also to include the Incognito mode exception.

[mortonm, 2017-05-19 22:52:42]

Going to include changes as per Varun's comments in a different CL.

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:111: return true;
On 2017/05/19 22:41:13, vakh (Varun Khaneja) wrote:
> simpler re-write:
> return hit_report.extended_reporting_level != SBER_LEVEL_OFF &&
> !web_contents->GetBrowserContext()->IsOffTheRecord()

Sounds good. I'm gonna put all these changes in a new CL since the code already
landed.

[mortonm, 2017-05-22 17:03:59]

The changes to the code are in a different CL for which I will request LGTMs
from vakh@ and jialiul@.

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:106: // Static
On 2017/05/19 22:41:13, vakh (Varun Khaneja) wrote:
> nit: static

Done.

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:108: WebContents* web_contents) {
On 2017/05/19 22:41:13, vakh (Varun Khaneja) wrote:
> could web_contents also be const?

Done.

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:111: return true;
On 2017/05/19 22:41:13, vakh (Varun Khaneja) wrote:
> simpler re-write:
> return hit_report.extended_reporting_level != SBER_LEVEL_OFF &&
> !web_contents->GetBrowserContext()->IsOffTheRecord()

Done.

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/2886043002/diff/140001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:72: // non-empty, the request will be
sent as a POST instead of a GET.
On 2017/05/19 22:41:13, vakh (Varun Khaneja) wrote:
> I don't see the |post_data| argument to the function. I think this comment is
in
> dire need of an update. It also needs to mention that the report is not sent
> from Incognito mode.

Done.

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
File components/safe_browsing/base_ui_manager.cc (right):

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
components/safe_browsing/base_ui_manager.cc:247: // A safebrowsing hit is sent
after a blocking page for malware/phishing
On 2017/05/19 22:41:13, vakh (Varun Khaneja) wrote:
> nit: SafeBrowsing

Done.

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
File components/safe_browsing/base_ui_manager.h (right):

https://codereview.chromium.org/2886043002/diff/140001/components/safe_browsi...
components/safe_browsing/base_ui_manager.h:64: // to the server. Can only be
called on UI thread.
On 2017/05/19 22:41:13, vakh (Varun Khaneja) wrote:
> Please update this comment also to include the Incognito mode exception.

Done.