Handle per-tab AUTOMATIC_DOWNLOADS setting in DownloadRequestLimiter.

chrome/browser/download/download_request_limiter.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_DOWNLOAD_DOWNLOAD_REQUEST_LIMITER_H_
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_REQUEST_LIMITER_H_
 
 #include <stddef.h>
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "base/scoped_observer.h"
+#include "components/content_settings/core/browser/content_settings_observer.h"
 #include "components/content_settings/core/common/content_settings.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents_observer.h"
 
 class HostContentSettingsMap;
 
 namespace content {
 class NavigationController;

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

Nit: I think you can remove NavigationController

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

Done.

 class WebContents;
 }
 
 // DownloadRequestLimiter is responsible for determining whether a download
 // should be allowed or not. It is designed to keep pages from downloading
 // multiple files without user interaction. DownloadRequestLimiter is invoked
 // from ResourceDispatcherHost any time a download begins
 // (CanDownloadOnIOThread). The request is processed on the UI thread, and the
 // request is notified (back on the IO thread) as to whether the download should
 // be allowed or denied.
@@ skipping 28 matching lines @@
 
   // The callback from CanDownloadOnIOThread. This is invoked on the io thread.
   // The boolean parameter indicates whether or not the download is allowed.
   typedef base::Callback<void(bool /*allow*/)> Callback;
 
   // TabDownloadState maintains the download state for a particular tab.
   // TabDownloadState prompts the user with an infobar as necessary.
   // TabDownloadState deletes itself (by invoking
   // DownloadRequestLimiter::Remove) as necessary.
   // TODO(gbillock): just make this class implement PermissionRequest.
-  class TabDownloadState : public content::NotificationObserver,
+  class TabDownloadState : public content_settings::Observer,
                            public content::WebContentsObserver {
    public:
     // Creates a new TabDownloadState. |controller| is the controller the
     // TabDownloadState tracks the state of and is the host for any dialogs that
     // are displayed. |originating_controller| is used to determine the host of
     // the initial download. If |originating_controller| is null, |controller|
     // is used. |originating_controller| is typically null, but differs from
     // |controller| in the case of a constrained popup requesting the download.
     TabDownloadState(DownloadRequestLimiter* host,
                      content::WebContents* web_contents,
                      content::WebContents* originating_web_contents);
     ~TabDownloadState() override;
 
     // Status of the download.

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

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/02/07 16:16:29]

Done.

-    void set_download_status(DownloadRequestLimiter::DownloadStatus status) {
-      status_ = status;
-    }
+    void SetDownloadStatusAndNotify(
+        DownloadRequestLimiter::DownloadStatus status);
     DownloadRequestLimiter::DownloadStatus download_status() const {
       return status_;
     }
 
     // Number of "ALLOWED" downloads.
     void increment_download_count() {
       download_count_++;
     }
     size_t download_count() const {
       return download_count_;
@@ skipping 24 matching lines @@
    protected:
     // Used for testing.
     TabDownloadState();
 
    private:
     // Are we showing a prompt to the user?  Determined by whether
     // we have an outstanding weak pointer--weak pointers are only
     // given to the info bar delegate or permission bubble request.
     bool is_showing_prompt() const;
 
-    // content::NotificationObserver method.
-    void Observe(int type,
-                 const content::NotificationSource& source,
-                 const content::NotificationDetails& details) override;
+    // content_settings::Observer overrides.
+    void OnContentSettingChanged(
+        const ContentSettingsPattern& primary_pattern,
+        const ContentSettingsPattern& secondary_pattern,
+        ContentSettingsType content_type,
+        std::string resource_identifier) override;
 
     // Remember to either block or allow automatic downloads from this origin.
     void SetContentSetting(ContentSetting setting);
 
     // Notifies the callbacks as to whether the download is allowed or not.
-    // Updates status_ appropriately.
-    void NotifyCallbacks(bool allow);
+    // Returns false if it didn't notify all callbacks.
+    bool NotifyCallbacks(bool allow);
 
     content::WebContents* web_contents_;
 
     DownloadRequestLimiter* host_;
 
     // Host of the first page the download started on. This may be empty.
     std::string initial_page_host_;
 
     DownloadRequestLimiter::DownloadStatus status_;
 
     size_t download_count_;
 
     // Callbacks we need to notify. This is only non-empty if we're showing a
     // dialog.
     // See description above CanDownloadOnIOThread for details on lifetime of
     // callbacks.
     std::vector<DownloadRequestLimiter::Callback> callbacks_;
 
-    // Used to remove observers installed on NavigationController.
-    content::NotificationRegistrar registrar_;
+    ScopedObserver<HostContentSettingsMap, content_settings::Observer>
+        observer_;
 
     // Weak pointer factory for generating a weak pointer to pass to the
     // infobar.  User responses to the throttling prompt will be returned
     // through this channel, and it can be revoked if the user prompt result
     // becomes moot.
     base::WeakPtrFactory<DownloadRequestLimiter::TabDownloadState> factory_;
 
     DISALLOW_COPY_AND_ASSIGN(TabDownloadState);
   };
 
@@ skipping 53 matching lines @@
 
   static HostContentSettingsMap* content_settings_;
   static HostContentSettingsMap* GetContentSettings(
       content::WebContents* contents);
 
   // Maps from tab to download state. The download state for a tab only exists
   // if the state is other than ALLOW_ONE_DOWNLOAD. Similarly once the state
   // transitions from anything but ALLOW_ONE_DOWNLOAD back to ALLOW_ONE_DOWNLOAD
   // the TabDownloadState is removed and deleted (by way of Remove).
   typedef std::map<content::WebContents*, TabDownloadState*> StateMap;
   StateMap state_map_;

[Bernhard Bauer, 2017/01/25 19:01:14]

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]

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]

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]

Only added a TODO here, sorry.

 
   // Weak ptr factory used when |CanDownload| asks the delegate asynchronously
   // about the download.
   base::WeakPtrFactory<DownloadRequestLimiter> factory_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadRequestLimiter);
 };
 
 #endif  // CHROME_BROWSER_DOWNLOAD_DOWNLOAD_REQUEST_LIMITER_H_