Make the download request limiter listen to content settings changes.

Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.

Committed: https://crrev.com/8aea480299a0b812aa3705d9a7c84d811ed075ef
Cr-Commit-Position: refs/heads/master@{#375396}

[dominickn, 2016-02-05 05:37:22]

Description was changed from

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
settings per origin. This indicates which sites users has allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after refreshing the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.
==========

to

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading  the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.
==========

[dominickn, 2016-02-05 11:04:47]

dominickn@chromium.org changed reviewers:
+ asanka@chromium.org

[dominickn, 2016-02-05 11:04:47]

PTAL, thanks!

[asanka, 2016-02-08 15:02:14]

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:227: if (setting ==
CONTENT_SETTING_ALLOW) {
Nit: use a switch and cover all the settings so that its explicit what we do for
each setting.

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter_unittest.cc:204:
download_request_limiter_->GetDownloadState(web_contents, NULL, true);
NULL -> nullptr

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter_unittest.cc:505:
DownloadRequestLimiter_ContentSettingChanged) {
The current behavior doesn't reset the download count when the content settings
change. I'm guessing this is intentional since it wouldn't allow an additional
"free" download once the content settings change to ASK.

Assuming this is intentional, would you mind adding a test that verifies the
behavior for the next download following a content settings change?

[dominickn, 2016-02-11 01:25:53]

PTAL, thanks!

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:227: if (setting ==
CONTENT_SETTING_ALLOW) {
On 2016/02/08 15:02:14, asanka wrote:
> Nit: use a switch and cover all the settings so that its explicit what we do
for
> each setting.

Done - made this consistent with DownloadRequestLimiter::CanDownloadImpl.

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter_unittest.cc:204:
download_request_limiter_->GetDownloadState(web_contents, NULL, true);
On 2016/02/08 15:02:14, asanka wrote:
> NULL -> nullptr

Done. Also updated other uses in this file.

https://codereview.chromium.org/1675533002/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter_unittest.cc:505:
DownloadRequestLimiter_ContentSettingChanged) {
On 2016/02/08 15:02:14, asanka wrote:
> The current behavior doesn't reset the download count when the content
settings
> change. I'm guessing this is intentional since it wouldn't allow an additional
> "free" download once the content settings change to ASK.
> 
> Assuming this is intentional, would you mind adding a test that verifies the
> behavior for the next download following a content settings change?

Not sure I understood this 100%, but I added a bunch more explicit counts
checks.

[asanka, 2016-02-12 16:56:51]

lgtm. thanks!

[dominickn, 2016-02-14 23:30:57]

Description was changed from

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading  the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.
==========

to

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.
==========

[dominickn, 2016-02-14 23:31:15]

The CQ bit was checked by dominickn@chromium.org

[commit-bot: I haz the power, 2016-02-14 23:31:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1675533002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1675533002/40001

[commit-bot: I haz the power, 2016-02-15 00:20:23]

Description was changed from

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.
==========

to

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.
==========

[commit-bot: I haz the power, 2016-02-15 00:20:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-16 22:48:29]

Description was changed from

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.
==========

to

==========
Make the download request limiter listen to content settings changes.

The download request limiter persists the automatic downloads content
setting per origin. This indicates which sites users have allowed to
trigger multiple automatic downloads.

However, the limiter does not respect changes made to the content
setting via the OIB or content settings pages. This means that the user
can change the content settings (e.g. accidentally allowed the
permission, visit OIB to revoke permission), but the limiter will not
change its internal state to match the new setting. In this case, the
limiter will continue to allow all downloads, even after reloading the
page.

This CL fixes the bug by making the limiter listen to the
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED notification, updating its
internal state when the automatic downloads setting has been changed.
Unit tests are also added to ensure the correct behaviour.

BUG=584146
TEST=Changing automatic downloads permissions in the OIB or in content
settings will be applied on page reload.

Committed: https://crrev.com/8aea480299a0b812aa3705d9a7c84d811ed075ef
Cr-Commit-Position: refs/heads/master@{#375396}
==========

[commit-bot: I haz the power, 2016-02-16 22:48:30]

Patchset 3 (id:??) landed as
https://crrev.com/8aea480299a0b812aa3705d9a7c84d811ed075ef
Cr-Commit-Position: refs/heads/master@{#375396}