Clean up error handling logic for extension tab muting

chrome/browser/ui/tabs/tab_utils.cc

Index: chrome/browser/ui/tabs/tab_utils.cc
diff --git a/chrome/browser/ui/tabs/tab_utils.cc b/chrome/browser/ui/tabs/tab_utils.cc
index cfe6282b03aeb2d6ad9a80104fae4874f394a759..fff8019c8318f33fc34b244c91503e616aafc9cf 100644
--- a/chrome/browser/ui/tabs/tab_utils.cc
+++ b/chrome/browser/ui/tabs/tab_utils.cc
@@ -17,16 +17,17 @@
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/animation/multi_animation.h"
 
-struct LastMuteMetadata
-    : public content::WebContentsUserData<LastMuteMetadata> {
+struct MuteMetadata : public content::WebContentsUserData<MuteMetadata> {
   std::string cause;  // Extension ID or constant from header file
                       // or empty string
+  chrome::MuteTokenBucketInfoMap cause_info;
+
  private:
-  explicit LastMuteMetadata(content::WebContents* contents) {}
-  friend class content::WebContentsUserData<LastMuteMetadata>;
+  explicit MuteMetadata(content::WebContents* contents) {}
+  friend class content::WebContentsUserData<MuteMetadata>;
 };
 
-DEFINE_WEB_CONTENTS_USER_DATA_KEY(LastMuteMetadata);
+DEFINE_WEB_CONTENTS_USER_DATA_KEY(MuteMetadata);
 
 namespace chrome {
 
@@ -266,26 +267,83 @@ bool CanToggleAudioMute(content::WebContents* contents) {
   return false;
 }
 
+// In addition to indicating whether a request is rate limited, this also
+// updates the rate limiting state.

[miu, 2015/07/15 01:45:55]

Rather than having a side effect, could you just move the code that subtracts
from the token bucket into the SetTabAudioMuted() function?

[Jared Sohn, 2015/07/15 02:33:34]

Yeah, that's cleaner.

+bool IsTabAudioMutedRateLimited(content::WebContents* contents,
+                                const std::string& cause) {
+  if (!contents)
+    return false;
+
+  if ((cause == kMutedToggleCauseUser) || (cause == kMutedToggleCauseCapture))
+    return false;
+
+  MuteMetadata::CreateForWebContents(contents);  // Create if not exists.
+
+  chrome::MuteTokenBucketInfoMap mute_token_bucket_info_map =
+      MuteMetadata::FromWebContents(contents)->cause_info;
+
+  if (mute_token_bucket_info_map.find(cause) ==

[miu, 2015/07/15 01:45:55]

I like the idea of using a token bucket here.  However, I think there should
just be one token bucket shared by all extensions that attempt to mute the tab,
rather than a separate token bucket per extension.  This would be much simpler,
and provides all the protections we intended in the "evil things" section in the
API design doc.  What do you think?

[Jared Sohn, 2015/07/15 02:33:34]

I'm okay with that.

On 2015/07/15 at 01:45:55, miu wrote:

+      mute_token_bucket_info_map.end()) {
+    MuteMetadata::FromWebContents(contents)->cause_info[cause] =
+        linked_ptr<MuteTokenBucketInfo>(new MuteTokenBucketInfo());
+    mute_token_bucket_info_map =
+        MuteMetadata::FromWebContents(contents)->cause_info;
+  }
+
+  linked_ptr<MuteTokenBucketInfo> cause_info =
+      MuteMetadata::FromWebContents(contents)->cause_info[cause];
+
+  base::TimeTicks now = base::TimeTicks::Now();
+  base::TimeDelta elapsed_since_last_attempt = now - cause_info->last_attempt;
+  cause_info->last_attempt = now;
+
+  // Add tokens to the bucket, proportional to how much time has elapsed, capped
+  // at the maximum capacity.
+  cause_info->token_bucket =
+      std::min(kMuteTokenBucketCapacity,
+               cause_info->token_bucket + elapsed_since_last_attempt);
+
+  // If we can't take kMuteTokenBucketCost seconds worth of tokens
+  // out of the bucket to "pay for this," reject the request to toggle mute.
+  if (cause_info->token_bucket < kMuteTokenBucketCost)
+    return true;  // RATE LIMITED
+
+  // Take kMuteTokenBucketCost seconds worth of tokens from the bucket
+  // and execute the toggle.
+  cause_info->token_bucket -= kMuteTokenBucketCost;
+  return false;
+}
+
 const std::string& GetTabAudioMutedCause(content::WebContents* contents) {
-  LastMuteMetadata::CreateForWebContents(contents);  // Create if not exists.
+  MuteMetadata::CreateForWebContents(contents);  // Create if not exists.
   if (GetTabMediaStateForContents(contents) == TAB_MEDIA_STATE_CAPTURING) {
     // For tab capture, libcontent forces muting off.
-    LastMuteMetadata::FromWebContents(contents)->cause =
-        kMutedToggleCauseCapture;
+    MuteMetadata::FromWebContents(contents)->cause = kMutedToggleCauseCapture;
   }
-  return LastMuteMetadata::FromWebContents(contents)->cause;
+  return MuteMetadata::FromWebContents(contents)->cause;
 }
 
-void SetTabAudioMuted(content::WebContents* contents,
-                      bool mute,
-                      const std::string& cause) {
-  if (!contents || !chrome::CanToggleAudioMute(contents))
-    return;
+TabMutedResult SetTabAudioMuted(content::WebContents* contents,
+                                bool mute,
+                                const std::string& cause) {
+  if (!IsTabAudioMutingFeatureEnabled())
+    return TAB_MUTED_RESULT_FAIL_NOT_ENABLED;
 
-  LastMuteMetadata::CreateForWebContents(contents);  // Create if not exists.
-  LastMuteMetadata::FromWebContents(contents)->cause = cause;
+  if (!contents)

[miu, 2015/07/15 01:45:55]

Instead of this one, I'm thinking we should DCHECK(contents) since |contents|
should be a required argument (not nullptr).  I'm trying to remember why I wrote
the code the way I did initially, but I can't.  ;-)

[Jared Sohn, 2015/07/15 02:33:34]

I considered changing it but was concerned I might cause new crashes.  Will also
change it within IsTabAudioMutedRateLimited.

On 2015/07/15 at 01:45:55, miu wrote:

[miu, 2015/07/16 00:28:25]

I took a quick look at all the callers of SetTabAudioMuted(), and it should
never be nullptr.  So, we should be good here.

+    return TAB_MUTED_RESULT_FAIL_NO_CONTENTS;
+
+  if (!chrome::CanToggleAudioMute(contents))
+    return TAB_MUTED_RESULT_FAIL_TABCAPTURE;
+
+  if (IsTabAudioMutedRateLimited(contents, cause))
+    return TAB_MUTED_RESULT_FAIL_RATE_LIMITED;
+
+  MuteMetadata::CreateForWebContents(contents);  // Create if not exists.
+  MuteMetadata::FromWebContents(contents)->cause = cause;
 
   contents->SetAudioMuted(mute);
+
+  return TAB_MUTED_RESULT_SUCCESS;
 }
 
 bool IsTabAudioMuted(content::WebContents* contents) {