Clean up error handling logic for extension tab muting

chrome/browser/ui/tabs/tab_utils.cc

 // 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.
 
 #include "chrome/browser/ui/tabs/tab_utils.h"
 
 #include "base/command_line.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/media/media_capture_devices_dispatcher.h"
 #include "chrome/browser/media/media_stream_capture_indicator.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/grit/generated_resources.h"
 #include "content/public/browser/web_contents.h"
 #include "grit/theme_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #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 {
 
 const char kMutedToggleCauseUser[] = "user";
 const char kMutedToggleCauseCapture[] = "capture";
 
 namespace {
 
 // Interval between frame updates of the tab indicator animations.  This is not
 // the usual 60 FPS because a trade-off must be made between tab UI animation
@@ skipping 219 matching lines @@
     case TAB_MEDIA_STATE_AUDIO_MUTING:
       return IsTabAudioMutingFeatureEnabled();
     case TAB_MEDIA_STATE_RECORDING:
     case TAB_MEDIA_STATE_CAPTURING:
       return false;
   }
   NOTREACHED();
   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) {
   return contents && contents->IsAudioMuted();
 }
 
 bool AreAllTabsMuted(const TabStripModel& tab_strip,
                      const std::vector<int>& indices) {
   for (std::vector<int>::const_iterator i = indices.begin(); i != indices.end();
        ++i) {
     if (!IsTabAudioMuted(tab_strip.GetWebContentsAt(*i)))
       return false;
   }
   return true;
 }
 
 }  // namespace chrome