Handle per-tab AUTOMATIC_DOWNLOADS setting in DownloadRequestLimiter.

Handle per-tab AUTOMATIC_DOWNLOADS setting in DownloadRequestLimiter.

BUG=672395
Review-Url: https://codereview.chromium.org/2561673003
Cr-Commit-Position: refs/heads/master@{#454228}
Committed: https://chromium.googlesource.com/chromium/src/+/3db897f00b693287702dc2b88c5ebe757e2babaf

[alshabalin, 2016-12-08 10:50:19]

Description was changed from

==========
Set tab content setting after permission prompt for auto downloads.

BUG=672395
==========

to

==========
Set tab content setting after permission prompt for auto downloads.

BUG=672395
==========

[alshabalin, 2016-12-08 10:50:19]

alshabalin@yandex-team.ru changed reviewers:
+ dominickn@chromium.org

[alshabalin, 2016-12-08 11:39:06]

https://codereview.chromium.org/2561673003/diff/1/chrome/browser/download/dow...
File chrome/browser/download/download_request_limiter.cc (right):

https://codereview.chromium.org/2561673003/diff/1/chrome/browser/download/dow...
chrome/browser/download/download_request_limiter.cc:196: if (tab_settings) {
This is called right after permission prompt decision. Calls to
TabSpecificContentSettings::SetDownloadsBlocked will trigger notification to
DownloadRequestLimiter::TabDownloadState::Observe which will change status_.
However, it would be changed again anyway by call to NotifyCallbacks (from
Cancel() and Accept()).

[alshabalin, 2016-12-08 12:06:54]

Also I've encountered a problem with download_request_limiter_unittest.cc. I
removed TestingProfile, created HostContentSettingsMap from
HostContentSettingsMapFactory for web_contents() and created
TabSpecificContentSettings for web_contents() so tests follow real-world setup
more closely (and I also removed patch from this CL). Two tests failed as a
result: DownloadRequestLimiter_RendererInitiated and
DownloadRequestLimiter_SetHostContentSetting. The second one fails on the second
assertion
ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
download_request_limiter_->GetDownloadStatus(web_contents()));
The actual value is DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS. It gets changed
in the Observe method which is called by
TabSpecificContentSettings::SetDownloadsBlocked which is called (now that
TabSpecificContentSettings exists for web_contents()) from CanDownloadImpl. Is
this a bug or is this a problem in tests?

[alshabalin, 2016-12-08 12:06:55]

Also I've encountered a problem with download_request_limiter_unittest.cc. I
removed TestingProfile, created HostContentSettingsMap from
HostContentSettingsMapFactory for web_contents() and created
TabSpecificContentSettings for web_contents() so tests follow real-world setup
more closely (and I also removed patch from this CL). Two tests failed as a
result: DownloadRequestLimiter_RendererInitiated and
DownloadRequestLimiter_SetHostContentSetting. The second one fails on the second
assertion
ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
download_request_limiter_->GetDownloadStatus(web_contents()));
The actual value is DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS. It gets changed
in the Observe method which is called by
TabSpecificContentSettings::SetDownloadsBlocked which is called (now that
TabSpecificContentSettings exists for web_contents()) from CanDownloadImpl. Is
this a bug or is this a problem in tests?

[dominickn, 2016-12-09 00:53:28]

On 2016/12/08 12:06:55, alshabalin wrote:
> Also I've encountered a problem with download_request_limiter_unittest.cc. I
> removed TestingProfile, created HostContentSettingsMap from
> HostContentSettingsMapFactory for web_contents() and created
> TabSpecificContentSettings for web_contents() so tests follow real-world setup
> more closely (and I also removed patch from this CL). Two tests failed as a
> result: DownloadRequestLimiter_RendererInitiated and
> DownloadRequestLimiter_SetHostContentSetting. The second one fails on the
second
> assertion
> ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
> download_request_limiter_->GetDownloadStatus(web_contents()));
> The actual value is DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS. It gets
changed
> in the Observe method which is called by
> TabSpecificContentSettings::SetDownloadsBlocked which is called (now that
> TabSpecificContentSettings exists for web_contents()) from CanDownloadImpl. Is
> this a bug or is this a problem in tests?

Thanks for looking at this. These issues seem like they might be problems with
the fact that TabSpecificContentSettings::SetDownloadsBlocked sends a
notification that the content setting has changed. But that's extraneous,
because the only thing that calls SetDownloadsBlocked is the request limiter,
which knows what state it's in.

What happens if you remove the Notify() call from
TabSpecificContentSettings::SetDownloadsBlocked?

[alshabalin, 2016-12-09 10:15:22]

On 2016/12/09 00:53:28, dominickn wrote:
> On 2016/12/08 12:06:55, alshabalin wrote:
> > Also I've encountered a problem with download_request_limiter_unittest.cc. I
> > removed TestingProfile, created HostContentSettingsMap from
> > HostContentSettingsMapFactory for web_contents() and created
> > TabSpecificContentSettings for web_contents() so tests follow real-world
setup
> > more closely (and I also removed patch from this CL). Two tests failed as a
> > result: DownloadRequestLimiter_RendererInitiated and
> > DownloadRequestLimiter_SetHostContentSetting. The second one fails on the
> second
> > assertion
> > ASSERT_EQ(DownloadRequestLimiter::PROMPT_BEFORE_DOWNLOAD,
> > download_request_limiter_->GetDownloadStatus(web_contents()));
> > The actual value is DownloadRequestLimiter::ALLOW_ALL_DOWNLOADS. It gets
> changed
> > in the Observe method which is called by
> > TabSpecificContentSettings::SetDownloadsBlocked which is called (now that
> > TabSpecificContentSettings exists for web_contents()) from CanDownloadImpl.
Is
> > this a bug or is this a problem in tests?
> 
> Thanks for looking at this. These issues seem like they might be problems with
> the fact that TabSpecificContentSettings::SetDownloadsBlocked sends a
> notification that the content setting has changed. But that's extraneous,
> because the only thing that calls SetDownloadsBlocked is the request limiter,
> which knows what state it's in.
> 
> What happens if you remove the Notify() call from
> TabSpecificContentSettings::SetDownloadsBlocked?

This notification is required to update the toolbar (via Browser object).
Example scenario:
1.  Disallow auto downloads on a site (e.g. https://permission.site)
2. Navigate to the site from a different tab and initiate multiple downloads
(the above site has a button for it)
If Notify() is removed blocked downloads icon is not shown in the location bar.

[dominickn, 2016-12-12 03:44:47]

Thanks for the clarification. In that case, it makes sense to update the unit
test to the values you're seeing, since this behaviour seems more correct.

[alshabalin, 2016-12-19 15:51:49]

On 2016/12/12 03:44:47, dominickn wrote:
> Thanks for the clarification. In that case, it makes sense to update the unit
> test to the values you're seeing, since this behaviour seems more correct.

Sorry for long delay! I've looked at it some more and I seem to have found the
root issue: there is a conflict between TabSpecificContentSettings and
DownloadRequestLimiter::TabDownloadState. Both manage tab-specific state
of auto downloads permission with respect to navigation. The former just clears
the state on navigation (and reports via
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED)
the latter has a specific logic for renderer-initiated navigations
(https://crbug.com/559515).

So, when we blocked auto downloads and then renderer-initiated navigation
happens,
DownloadRequestLimiter::TabDownloadState does nothing. But
TabSpecificContentSettings
clears its state, sends notification, we get it in
DownloadRequestLimiter::TabDownloadState::Observe
in which we go into HostContentSettingsMap to check the setting for the new url
and update our
state accordingly (which is not what we want, we want to preserve the blocked
state from previous url).

By the way, why DownloadRequestLimiter::TabDownloadState listens to
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED?
It's only ever produced by TabSpecificContentSettings when tab-specific setting
has changed.
Was it originally meant to listen for global content setting changes (i.e. from
HostContentSettingsMap)?
Anyway, as it currently stands, it doesn't get notified about those changes (for
example, changing setting
from ContentSettingBubble doesn't trigger any code in DownloadRequestLimiter).

So, if I understand the desired behavior correctly, I think it would make sense
to do the following:
1. Stop listening to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
2. Start listening to HostContentSettingsMap and do what
DownloadRequestLimiter::TabDownloadState::Observe does
3. Whenever our tab-specific state changes, generate
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED.
4. Remove TabSpecificContentSettings::SetDownloadsBlocked (alternatively, rename
it to NotifyDownloadsStatusChanged
  to keep deprecated NotificationService stuff in one place).
5. TabSpecificContentSettings methods IsContentAllowed and IsContentBlocked will
forward calls to DownloadRequestLimiter for auto downloads.

What do you think?

[dominickn, 2016-12-19 23:54:18]

Thanks for the thorough investigation here!

On 2016/12/19 15:51:49, alshabalin wrote:
> On 2016/12/12 03:44:47, dominickn wrote:
> > Thanks for the clarification. In that case, it makes sense to update the
unit
> > test to the values you're seeing, since this behaviour seems more correct.
> 
> Sorry for long delay! I've looked at it some more and I seem to have found the
> root issue: there is a conflict between TabSpecificContentSettings and
> DownloadRequestLimiter::TabDownloadState. Both manage tab-specific state
> of auto downloads permission with respect to navigation. The former just
clears
> the state on navigation (and reports via
> NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED)
> the latter has a specific logic for renderer-initiated navigations
> (https://crbug.com/559515).
> 
> So, when we blocked auto downloads and then renderer-initiated navigation
> happens,
> DownloadRequestLimiter::TabDownloadState does nothing. But
> TabSpecificContentSettings
> clears its state, sends notification, we get it in
> DownloadRequestLimiter::TabDownloadState::Observe
> in which we go into HostContentSettingsMap to check the setting for the new
url
> and update our
> state accordingly (which is not what we want, we want to preserve the blocked
> state from previous url).
> 
> By the way, why DownloadRequestLimiter::TabDownloadState listens to
> NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED?
> It's only ever produced by TabSpecificContentSettings when tab-specific
setting
> has changed.
> Was it originally meant to listen for global content setting changes (i.e.
from
> HostContentSettingsMap)?
> Anyway, as it currently stands, it doesn't get notified about those changes
(for
> example, changing setting
> from ContentSettingBubble doesn't trigger any code in DownloadRequestLimiter).

Interesting - listening to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED was to pick
up changes that the user made (e.g. by clicking on the lock icon and manually
switching automatic downloads from ALLOW to BLOCK, or vice versa). Prior to
https://crrev.com/1675533002, the limiter would keep its internal state even
when the user manually changed the setting in the page info dialog and reloaded
the page. It looks like what's happening is that the page reload sends
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED from the TabSpecificContentSettings,
which tells the limiter to recheck the content setting. Which is the behaviour
we want, but a roundabout way of getting there.

> So, if I understand the desired behavior correctly, I think it would make
sense
> to do the following:
> 1. Stop listening to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> 2. Start listening to HostContentSettingsMap and do what
> DownloadRequestLimiter::TabDownloadState::Observe does
> 3. Whenever our tab-specific state changes, generate
> NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED.
> 4. Remove TabSpecificContentSettings::SetDownloadsBlocked (alternatively,
rename
> it to NotifyDownloadsStatusChanged
>   to keep deprecated NotificationService stuff in one place).
> 5. TabSpecificContentSettings methods IsContentAllowed and IsContentBlocked
will
> forward calls to DownloadRequestLimiter for auto downloads.

This all makes sense to me, and it will centralise the handling of the content
setting in the download request limiter which is quite nice. Thanks again for
the investigation here!

[alshabalin, 2017-01-23 14:19:01]

Finally found some time to work on this.

Basically did what I sketched in my previous message:

> 1. Stop listening to NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED
> 2. Start listening to HostContentSettingsMap and do what
DownloadRequestLimiter::TabDownloadState::Observe does
> 3. Whenever our tab-specific state changes, generate
NOTIFICATION_WEB_CONTENT_SETTINGS_CHANGED.
> 4. Remove TabSpecificContentSettings::SetDownloadsBlocked.
> 5. TabSpecificContentSettings methods IsContentAllowed and IsContentBlocked
will forward calls to DownloadRequestLimiter for auto downloads.

Now an icon in omnibox is shown whenever tab state enters either
ALLOW_ALL_DOWNLOADS or DOWNLOADS_NOT_ALLOWED. However,  tab state will jump
between ALLOW_ALL_DOWNLOADS and PROMPT_BEFORE_DOWNLOAD for every 50th download:
on 50th download state will change from ALLOW_ALL_DOWNLOADS to
PROMPT_BEFORE_DOWNLOAD; for the next download we will check
HostContentSettingsMap, get CONTENT_SETTING_ALLOW (unless I'm missing something,
ALLOW_ALL_DOWNLOADS can ever be set iff current content setting is
CONTENT_SETTING_ALLOW) and jump back to ALLOW_ALL_DOWNLOADS.

Also, while doing this noticed:
1. We're in CONTENT_SETTING_ASK (i.e. PROMPT_BEFORE_DOWNLOAD) state.
2. Site requested 60 downloads, we've shown a prompt, user allowed it.
3. Only first 50 downloads have been allowed. The other 10 are left waiting
(NotifyCallbacks logic).
Unless site asks for another download, there's no way to resume these 10 since
there's no indication that there're these pending downloads (assuming we're on a
desktop with permission bubbles, not sure what happens on Android with
infobars).

[dominickn, 2017-01-24 06:46:17]

Thanks! This is looking great - I got to it pretty late in my day so I want to
think about it a little more before giving it the lg (hopefully tomorrow).

On 2017/01/23 14:19:01, alshabalin wrote:
> Now an icon in omnibox is shown whenever tab state enters either
> ALLOW_ALL_DOWNLOADS or DOWNLOADS_NOT_ALLOWED. However,  tab state will jump
> between ALLOW_ALL_DOWNLOADS and PROMPT_BEFORE_DOWNLOAD for every 50th
download:
> on 50th download state will change from ALLOW_ALL_DOWNLOADS to
> PROMPT_BEFORE_DOWNLOAD; for the next download we will check
> HostContentSettingsMap, get CONTENT_SETTING_ALLOW (unless I'm missing
something,
> ALLOW_ALL_DOWNLOADS can ever be set iff current content setting is
> CONTENT_SETTING_ALLOW) and jump back to ALLOW_ALL_DOWNLOADS.
> 
> Also, while doing this noticed:
> 1. We're in CONTENT_SETTING_ASK (i.e. PROMPT_BEFORE_DOWNLOAD) state.
> 2. Site requested 60 downloads, we've shown a prompt, user allowed it.
> 3. Only first 50 downloads have been allowed. The other 10 are left waiting
> (NotifyCallbacks logic).
> Unless site asks for another download, there's no way to resume these 10 since
> there's no indication that there're these pending downloads (assuming we're on
a
> desktop with permission bubbles, not sure what happens on Android with
> infobars).

I had a look at the NotifyCallbacks() logic. That stuff is ancient (~2010)! It's
addressing an important issue (stopping the browser crashing on too many
downloads), but you're right in that it's weird that the extra 10 downloads are
just dropped until another download request is made. I'm not really sure what
the best way to keep the protection whilst still being able to retrigger those
callbacks. Perhaps file a bug on it?

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

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:28: class
NavigationController;
Nit: I think you can remove NavigationController

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:91: // Status of the
download.
Nit: "Sets the current limiter state and the underlying automatic downloads
content setting. Sends a notification that the content setting has been changed
(if it has changed)"

[alshabalin, 2017-01-24 10:15:22]

> Thanks! This is looking great - I got to it pretty late in my day so I want to
> think about it a little more before giving it the lg (hopefully tomorrow).

Thanks!
 
> I had a look at the NotifyCallbacks() logic. That stuff is ancient (~2010)!
It's
> addressing an important issue (stopping the browser crashing on too many
> downloads), but you're right in that it's weird that the extra 10 downloads
are
> just dropped until another download request is made. I'm not really sure what
> the best way to keep the protection whilst still being able to retrigger those
> callbacks. Perhaps file a bug on it?
 
Done: https://crbug.com/684425

[dominickn, 2017-01-25 04:43:31]

lgtm. Thanks again for the investigation and fix. :)

[alshabalin, 2017-01-25 09:09:06]

Description was changed from

==========
Set tab content setting after permission prompt for auto downloads.

BUG=672395
==========

to

==========
Handle per-tab AUTOMATIC_DOWNLOADS setting in DownloadRequestLimiter.

BUG=672395
==========

[alshabalin, 2017-01-25 09:09:06]

alshabalin@yandex-team.ru changed reviewers:
+ bauerb@chromium.org

[alshabalin, 2017-01-25 09:11:36]

+bauerb for TabSpecificContentSettings.

PTAL.

[Bernhard Bauer, 2017-01-25 19:01:15]

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:245: if
(content_type == CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS) {
If you are now bypassing TabSpecificContentSettings anyway, you might want to
just use your own subclasses of ContentSettingImageModel and
ContentSettingsBubbleModel that directly use DownloadRequestLimiter.

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

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:65: case
CONTENT_SETTING_DEFAULT:
Are these content settings actually used? DEFAULT should only be used to signal
the absence of a specific setting, and SESSION_ONLY is only used for cookies.

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:90:
observer_.Add(DownloadRequestLimiter::GetContentSettings(contents));
The DownloadRequestLimiter:: prefix doesn't seem necessary if you are already
inside the class.

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:321:
SetDownloadStatusAndNotify(GetStatusFromSetting(setting));
Here you convert the setting to a status, and the first thing
SetDownloadStatusAndNotify() does is convert it back to a content setting. Would
it make more sense to directly pass a content setting? Or alternatively,
directly compare statuses in SetDownloadStatusAndNotify()?

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

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:244: StateMap state_map_;
This is existing code, but is there a reason this rolls its own map of
WebContents instead of attaching the TabDownloadState as WebContentsUserData?

[dominickn, 2017-01-26 07:37:05]

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

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:244: StateMap state_map_;
On 2017/01/25 19:01:14, Bernhard Bauer wrote:
> This is existing code, but is there a reason this rolls its own map of
> WebContents instead of attaching the TabDownloadState as WebContentsUserData?

The last time this line was changed was 2008. The last time line 243 was changed
was May 2012, when the map key changed from a content::NavigationController* to
content::WebContents*. WebContentsUserData was introduced in August 2012. I
guess the answer is that this is very old and long unloved code.

[Bernhard Bauer, 2017-01-26 11:10:00]

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

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:244: StateMap state_map_;
On 2017/01/26 07:37:05, dominickn wrote:
> On 2017/01/25 19:01:14, Bernhard Bauer wrote:
> > This is existing code, but is there a reason this rolls its own map of
> > WebContents instead of attaching the TabDownloadState as
WebContentsUserData?
> 
> The last time this line was changed was 2008. The last time line 243 was
changed
> was May 2012, when the map key changed from a content::NavigationController*
to
> content::WebContents*. WebContentsUserData was introduced in August 2012. I
> guess the answer is that this is very old and long unloved code.

Oh wow. Thanks for the investigation :)

alshabalin, would you mind adding a `TODO(bauerb): Change this to use
WebContentsUserData` (or maybe even do that yourself, if you want to)? Thanks!

[alshabalin, 2017-02-07 16:16:29]

Removed TabSpecificContentSettings from the loop.

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/content_...
File chrome/browser/content_settings/tab_specific_content_settings.cc (right):

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/content_...
chrome/browser/content_settings/tab_specific_content_settings.cc:245: if
(content_type == CONTENT_SETTINGS_TYPE_AUTOMATIC_DOWNLOADS) {
On 2017/01/25 19:01:14, Bernhard Bauer wrote:
> If you are now bypassing TabSpecificContentSettings anyway, you might want to
> just use your own subclasses of ContentSettingImageModel and
> ContentSettingsBubbleModel that directly use DownloadRequestLimiter.

Done.

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

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:65: case
CONTENT_SETTING_DEFAULT:
On 2017/01/25 19:01:14, Bernhard Bauer wrote:
> Are these content settings actually used? DEFAULT should only be used to
signal
> the absence of a specific setting, and SESSION_ONLY is only used for cookies.

Added NOTREACHED() for SESSION_ONLY. But left the DEFAULT untouched for I'm not
sure if it can arrive here or not.

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:90:
observer_.Add(DownloadRequestLimiter::GetContentSettings(contents));
On 2017/01/25 19:01:14, Bernhard Bauer wrote:
> The DownloadRequestLimiter:: prefix doesn't seem necessary if you are already
> inside the class.

Done.

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:321:
SetDownloadStatusAndNotify(GetStatusFromSetting(setting));
On 2017/01/25 19:01:14, Bernhard Bauer wrote:
> Here you convert the setting to a status, and the first thing
> SetDownloadStatusAndNotify() does is convert it back to a content setting.
Would
> it make more sense to directly pass a content setting? Or alternatively,
> directly compare statuses in SetDownloadStatusAndNotify()?

Added UpdateDownloadStatusAndNotify taking ContentSetting instead of
DownloadStatus. I've chosen to compare settings instead of statuses, because
transition ALLOW_ONE -> PROMPT does not change the actual setting and so doesn't
need to fire a global notification.

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

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:28: class
NavigationController;
On 2017/01/24 06:46:17, dominickn wrote:
> Nit: I think you can remove NavigationController

Done.

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:91: // Status of the
download.
On 2017/01/24 06:46:17, dominickn wrote:
> Nit: "Sets the current limiter state and the underlying automatic downloads
> content setting. Sends a notification that the content setting has been
changed
> (if it has changed)"

Done.

https://codereview.chromium.org/2561673003/diff/20001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:244: StateMap state_map_;
On 2017/01/26 11:10:00, Bernhard Bauer wrote:
> On 2017/01/26 07:37:05, dominickn wrote:
> > On 2017/01/25 19:01:14, Bernhard Bauer wrote:
> > > This is existing code, but is there a reason this rolls its own map of
> > > WebContents instead of attaching the TabDownloadState as
> WebContentsUserData?
> > 
> > The last time this line was changed was 2008. The last time line 243 was
> changed
> > was May 2012, when the map key changed from a content::NavigationController*
> to
> > content::WebContents*. WebContentsUserData was introduced in August 2012. I
> > guess the answer is that this is very old and long unloved code.
> 
> Oh wow. Thanks for the investigation :)
> 
> alshabalin, would you mind adding a `TODO(bauerb): Change this to use
> WebContentsUserData` (or maybe even do that yourself, if you want to)? Thanks!

Only added a TODO here, sorry.

[Bernhard Bauer, 2017-02-07 17:02:57]

Thanks!

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

https://codereview.chromium.org/2561673003/diff/40001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:120: void
DownloadRequestLimiter::TabDownloadState::SetDownloadStatusAndNotify(
Okay, if status and setting are not bijective, can we pass in both, and have
call sites get one from the other as required? That way we have the fewest
number of conversions between them, without duplicating code.

https://codereview.chromium.org/2561673003/diff/40001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/2561673003/diff/40001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:95: // Updates the current
limiter state with given content setting and
Nit: Please add empty lines before comments.

[alshabalin, 2017-02-08 11:16:14]

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

https://codereview.chromium.org/2561673003/diff/40001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:120: void
DownloadRequestLimiter::TabDownloadState::SetDownloadStatusAndNotify(
On 2017/02/07 17:02:57, Bernhard Bauer wrote:
> Okay, if status and setting are not bijective, can we pass in both, and have
> call sites get one from the other as required? That way we have the fewest
> number of conversions between them, without duplicating code.

Added SetDownloadStatusAndNotifyImpl taking both arguments.

https://codereview.chromium.org/2561673003/diff/40001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/2561673003/diff/40001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:95: // Updates the current
limiter state with given content setting and
On 2017/02/07 17:02:57, Bernhard Bauer wrote:
> Nit: Please add empty lines before comments.

Done.

[alshabalin, 2017-02-16 08:25:53]

alshabalin@yandex-team.ru changed reviewers:
+ asanka@chromium.org

[alshabalin, 2017-02-16 08:25:55]

+asanka for download_browsertest.cc. Please take a look.

Bernhard, could you, please, take another look?

[Bernhard Bauer, 2017-02-16 13:08:14]

Sorry, I forgot I had this in my drafts.

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

https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:51: default:
Actually, the default isn't necessary if you have handled all cases, and if you
haven't, the compiler will complain. You'll just need to pull the NOTREACHED()
and return out of the switch statement.

https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:153: // guarantee that
|status| and |setting| correspond to each other.
Could you also DCHECK that? Sorry if I'm making things difficult, but this is
surprisingly tricky; it took me a couple of minutes and pen and paper just to
figure out the correspondence between the two...

[asanka, 2017-02-16 17:42:55]

asanka@chromium.org changed reviewers:
+ dtrainor@chromium.org
- asanka@chromium.org

[asanka, 2017-02-16 17:42:55]

downloads -> dtrainor

[David Trainor- moved to gerrit, 2017-02-16 17:51:46]

download_browsertest.cc lgtm.  We need to update owners though.

[alshabalin, 2017-02-17 09:21:11]

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

https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:51: default:
On 2017/02/16 13:08:14, Bernhard Bauer wrote:
> Actually, the default isn't necessary if you have handled all cases, and if
you
> haven't, the compiler will complain. You'll just need to pull the NOTREACHED()
> and return out of the switch statement.

Good point, thank you. Done. However, I'm surprised clang doesn't complain about
unreachable code after the switch statement now.

https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
File chrome/browser/download/download_request_limiter.h (right):

https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
chrome/browser/download/download_request_limiter.h:153: // guarantee that
|status| and |setting| correspond to each other.
On 2017/02/16 13:08:14, Bernhard Bauer wrote:
> Could you also DCHECK that? Sorry if I'm making things difficult, but this is
> surprisingly tricky; it took me a couple of minutes and pen and paper just to
> figure out the correspondence between the two...

No problem at all. Done. Now checking that one of the arguments is converted
from the other one (precisely corresponding to the usage at call sites). The
only potentially uncovered pair is ALLOW_ONE_DOWNLOAD + CONTENT_SETTING_DEFAULT,
but it'll never be set as things stand now. More than that, ALLOW_ONE_DOWNLOAD
is an initial-only state of TabDownloadState: after changing it once, it'll
never have it assigned again.

[Bernhard Bauer, 2017-02-17 10:51:22]

Thanks, LGTM!

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

https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
chrome/browser/download/download_request_limiter.cc:51: default:
On 2017/02/17 09:21:10, alshabalin wrote:
> On 2017/02/16 13:08:14, Bernhard Bauer wrote:
> > Actually, the default isn't necessary if you have handled all cases, and if
> you
> > haven't, the compiler will complain. You'll just need to pull the
NOTREACHED()
> > and return out of the switch statement.
> 
> Good point, thank you. Done. However, I'm surprised clang doesn't complain
about
> unreachable code after the switch statement now.

Haha :-D Turns out, the dirty secret is that the code is not reeally unreachable
-- an enum in C++ could in principle have any value that the underlying type can
have (because the memory at that address could of course contain any value!),
and the generated machine code inside the switch statement only deals with valid
values, but if the memory gets somehow corrupted or cast from a different type,
it might reach the part after the switch.

[alshabalin, 2017-02-17 11:00:46]

The CQ bit was checked by alshabalin@yandex-team.ru

[alshabalin, 2017-02-17 11:00:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
dominickn@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2561673003/#ps100001
(title: "Add more diagnostics.")

[commit-bot: I haz the power, 2017-02-17 11:00:58]

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-02-17 11:03:02]

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

[commit-bot: I haz the power, 2017-02-17 11:03:04]

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_...)

[alshabalin, 2017-02-20 08:41:47]

alshabalin@yandex-team.ru changed reviewers:
+ pkasting@chromium.org

[alshabalin, 2017-02-20 08:41:48]

+pkasting for content_setting_bubble_dialog_browsertest.cc

Please, take a look.

[alshabalin, 2017-02-20 08:59:05]

On 2017/02/17 10:51:22, Bernhard Bauer wrote:
> Thanks, LGTM!
> 
>
https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
> File chrome/browser/download/download_request_limiter.cc (right):
> 
>
https://codereview.chromium.org/2561673003/diff/80001/chrome/browser/download...
> chrome/browser/download/download_request_limiter.cc:51: default:
> On 2017/02/17 09:21:10, alshabalin wrote:
> > On 2017/02/16 13:08:14, Bernhard Bauer wrote:
> > > Actually, the default isn't necessary if you have handled all cases, and
if
> > you
> > > haven't, the compiler will complain. You'll just need to pull the
> NOTREACHED()
> > > and return out of the switch statement.
> > 
> > Good point, thank you. Done. However, I'm surprised clang doesn't complain
> about
> > unreachable code after the switch statement now.
> 
> Haha :-D Turns out, the dirty secret is that the code is not reeally
unreachable
> -- an enum in C++ could in principle have any value that the underlying type
can
> have (because the memory at that address could of course contain any value!),
> and the generated machine code inside the switch statement only deals with
valid
> values, but if the memory gets somehow corrupted or cast from a different
type,
> it might reach the part after the switch.

Ah, yes, I see.

[Peter Kasting, 2017-02-23 02:05:02]

LGTM

[alshabalin, 2017-02-27 09:52:42]

The CQ bit was checked by alshabalin@yandex-team.ru

[alshabalin, 2017-02-27 09:52:43]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dominickn@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2561673003/#ps140001
(title: "Fix ContentSettingBubbleDialogTest")

[commit-bot: I haz the power, 2017-02-27 09:52:53]

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-02-27 10:18:20]

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

[commit-bot: I haz the power, 2017-02-27 10:18:21]

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

[alshabalin, 2017-02-28 10:31:31]

Fixed ContentSettingBubbleControllerTest and a variety of failing unit_tests.

https://codereview.chromium.org/2561673003/diff/160001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm
(left):

https://codereview.chromium.org/2561673003/diff/160001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_browsertest.mm:24:
namespace {
Needed to remove that, so ContentSettingBubbleControllerTest can be referenced
in a friend declaration in download_request_limiter.h

https://codereview.chromium.org/2561673003/diff/160001/chrome/browser/ui/cont...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

https://codereview.chromium.org/2561673003/diff/160001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:1398:
DCHECK(download_request_limiter);
Here I opted for DCHECK on existence of DownloadRequestLimiter, because
logically this bubble should only be created when corresponding icon exists (and
it wouldn't if DRL was nullptr).

https://codereview.chromium.org/2561673003/diff/160001/chrome/browser/ui/cont...
File chrome/browser/ui/content_settings/content_setting_image_model.cc (right):

https://codereview.chromium.org/2561673003/diff/160001/chrome/browser/ui/cont...
chrome/browser/ui/content_settings/content_setting_image_model.cc:553: return;
Also added this check for the sake of unit_tests where there's no
DownloadRequestLimiter in TestingBrowserProcess.

[alshabalin, 2017-02-28 10:32:09]

The CQ bit was checked by alshabalin@yandex-team.ru

[alshabalin, 2017-02-28 10:32:10]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dominickn@chromium.org, dtrainor@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2561673003/#ps160001
(title: "Handle absent DownloadRequestLimiter in tests")

[commit-bot: I haz the power, 2017-02-28 10:32:18]

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-02-28 10:34:20]

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

[commit-bot: I haz the power, 2017-02-28 10:34:21]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[alshabalin, 2017-02-28 15:06:03]

The CQ bit was checked by alshabalin@yandex-team.ru

[alshabalin, 2017-02-28 15:06:03]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dominickn@chromium.org, dtrainor@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2561673003/#ps200001
(title: "Fix after rebase.")

[commit-bot: I haz the power, 2017-02-28 15:06:19]

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

[alshabalin, 2017-02-28 15:33:52]

The CQ bit was unchecked by alshabalin@yandex-team.ru

[alshabalin, 2017-03-01 12:18:23]

dominickn, Bernhard Bauer could you please take another look?

https://codereview.chromium.org/2561673003/diff/220001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/2561673003/diff/220001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter_unittest.cc:152: void
TearDown() override {
DownloadRequestLimiter::TabDownloadState now depends on HostContentSettingsMap
by being its observer. But in these tests HostContentSettingsMap was deleted
before TabDownloadState (which will be deleted when associated WebContents is
destroyed).

I decided not to create custom HostContentSettingsMap anymore and just use one
associated with a BrowserContext. This brings object lifetimes in the tests
closer to the real world.

[dominickn, 2017-03-02 03:32:52]

Thanks for all the work here!

https://codereview.chromium.org/2561673003/diff/220001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/2561673003/diff/220001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter_unittest.cc:213:
DownloadRequestLimiter::GetContentSettings(contents)
Instead of exposing GetContentSettings, can you now just call
HostContentSettingsMapFactory::GetForProfile etc. here? Then
DownloadRequestLimiter::GetContentSettings can just be a private method, which
tidies this up a bit more.

[alshabalin, 2017-03-02 09:54:02]

https://codereview.chromium.org/2561673003/diff/220001/chrome/browser/downloa...
File chrome/browser/download/download_request_limiter_unittest.cc (right):

https://codereview.chromium.org/2561673003/diff/220001/chrome/browser/downloa...
chrome/browser/download/download_request_limiter_unittest.cc:213:
DownloadRequestLimiter::GetContentSettings(contents)
On 2017/03/02 03:32:52, dominickn wrote:
> Instead of exposing GetContentSettings, can you now just call
> HostContentSettingsMapFactory::GetForProfile etc. here? Then
> DownloadRequestLimiter::GetContentSettings can just be a private method, which
> tidies this up a bit more.

Done. DownloadRequestLimiter::GetContentSettings is already private, however:
DownloadRequestLimiterTest is just a friend of DownloadRequestLimiter.

[alshabalin, 2017-03-02 09:54:17]

The CQ bit was checked by alshabalin@yandex-team.ru

[alshabalin, 2017-03-02 09:54:17]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org,
dominickn@chromium.org, dtrainor@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2561673003/#ps240001
(title: "Get HostContentSettingsMap directly in DRL tests")

[commit-bot: I haz the power, 2017-03-02 09:54:30]

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-03-02 10:33:48]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1488448457078460,
"parent_rev": "b060f62b067b36eaef6106c0e258e1ed45d4f38a", "commit_rev":
"3db897f00b693287702dc2b88c5ebe757e2babaf"}

[commit-bot: I haz the power, 2017-03-02 10:34:22]

Description was changed from

==========
Handle per-tab AUTOMATIC_DOWNLOADS setting in DownloadRequestLimiter.

BUG=672395
==========

to

==========
Handle per-tab AUTOMATIC_DOWNLOADS setting in DownloadRequestLimiter.

BUG=672395

Review-Url: https://codereview.chromium.org/2561673003
Cr-Commit-Position: refs/heads/master@{#454228}
Committed:
https://chromium.googlesource.com/chromium/src/+/3db897f00b693287702dc2b88c5e...
==========

[commit-bot: I haz the power, 2017-03-02 10:34:23]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/3db897f00b693287702dc2b88c5e...