Clean up error handling logic for extension tab muting

Clean up error handling logic for extension tab muting.

1. Have SetTabAudioMuted return an enumerated value indicating why it failed and use a switch to show an error message.  Do not redundantly do checks both in SetTabAudioMuted and in TabsUpdateFunction::RunAsync (in tabs_api.cc)
2. Instead of sending warning messages to the console, set lastError.
3. Put message text in tabs_constants instead of tabs_api

BUG=438903
Committed: https://crrev.com/c337ac9749c9cc84872351a909222e151860e66e
Cr-Commit-Position: refs/heads/master@{#340528}

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

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2015-07-15 01:45:56]

Nice work.  Comments:

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:271: // updates the rate limiting state.
Rather than having a side effect, could you just move the code that subtracts
from the token bucket into the SetTabAudioMuted() function?

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:285: if
(mute_token_bucket_info_map.find(cause) ==
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?

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:332: if (!contents)
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.  ;-)

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.h:37: TAB_MUTED_RESULT_NONE,
Don't need TAB_MUTED_RESULT_NONE.  Also, per my later comment, you probably
don't need TAB_MUTED_RESULT_FAIL_NO_CONTENTS.

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.h:53: extern const base::TimeDelta
kMuteTokenBucketCost =
Chromium requirement: Global constants have to be POD types.  So, you'd want to
declare them here as:

  extern const int kMuteTokenBucketCostSeconds;
  extern const int kMuteTokenBucketCapacitySeconds;

Then, in tab_utils.cc:

  const int kMuteTokenBucketCostSeconds = 15;
  const int kMuteTokenBucketCapacitySeconds = 3 * kMuteTokenBucketCostSeconds;

...and when you use the constants and need a TimeDelta:

  cause_info->token_bucket =
      std::min(base::TimeDelta::FromSeconds(kMuteTokenBucketCapacitySeconds),
               cause_info->token_bucket + elapsed_since_last_attempt);

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

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:271: // updates the rate limiting state.
On 2015/07/15 at 01:45:55, miu wrote:
> Rather than having a side effect, could you just move the code that subtracts
from the token bucket into the SetTabAudioMuted() function?

Yeah, that's cleaner.

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:285: if
(mute_token_bucket_info_map.find(cause) ==
I'm okay with that.

On 2015/07/15 at 01:45:55, miu wrote:
> 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?

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:332: if (!contents)
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:
> 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.  ;-)

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.h:37: TAB_MUTED_RESULT_NONE,
>Don't need TAB_MUTED_RESULT_NONE.

The intention was to set a default enum value, but I suppose it isn't needed
since we'll always set it to something.

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.h:53: extern const base::TimeDelta
kMuteTokenBucketCost =
Will make this change.

[miu, 2015-07-16 00:28:26]

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1233263002/diff/60001/chrome/browser/ui/tabs/...
chrome/browser/ui/tabs/tab_utils.cc:332: if (!contents)
On 2015/07/15 02:33:34, Jared Sohn wrote:
> I considered changing it but was concerned I might cause new crashes.

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

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:277: void
updateTokenBuckets(content::WebContents* contents) {
This function should be private, so please move it to inside the anonymous
namespace (above, before line 101).

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:280:
LastMuteMetadata::CreateForWebContents(contents);  // Create if not exists.
If this function is private, you could omit this call to CreateForWebContents(),
and just make sure to do it in SetTabAudioMuted() beforehand.

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:296: bool
IsTabAudioMutedRateLimited(content::WebContents* contents) {
This function should probably be private as well.  Your call.

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:299:
LastMuteMetadata::CreateForWebContents(contents);  // Create if not exists.
ditto: Also not needed if this function is private (see comment above).

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.h:51: extern const int
kMuteTokenBucketCostSeconds = 15;
style nit: Assignment of the constant needs to be in the .cc file (see my
example from last round of review comments).

Alternatively, you could define these two constants as:

enum {
  kMuteTokenBucketCostSeconds = 15,
  kMuteTokenBucketCapacitySeconds = 45
};

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.h:135: void
updateTokenBuckets(content::WebContents* contents);
style: The "u" in updateTokenBuckets needs to be captialized.

https://codereview.chromium.org/1233263002/diff/100001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.h:135: void
updateTokenBuckets(content::WebContents* contents);
Also, don't declare this function in the header file.  It should be private
within tab_utils.cc.

https://codereview.chromium.org/1233263002/diff/100001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/tabs/basics/muted.js (right):

https://codereview.chromium.org/1233263002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/basics/muted.js:47:
chrome.tabs.update(testTabId_, {muted: true}, pass());
Please add a comment at the top of this function that mentions prior tests in
this file have already toggled the mute state twice, and that this update() will
be the third (and final) time before rate limiting should kick in.

https://codereview.chromium.org/1233263002/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/basics/muted.js:49: assertEq(true,
tab.muted); // state not updated since rate limited
For completeness, consider adding:

  assertTrue(!!chrome.runtime.lastError);
  if (chrome.runtime.lastError) {
    assertEq("<ERROR MESSAGE FOR RATE LIMITING HERE>",
chrome.runtime.lastError.message);
  }

[Jared Sohn, 2015-07-17 08:36:11]

I just posted a new update that I think handles all issues mentioned so far. A
few notes:

* I am now making use of tab_constants for error strings and am relying on
unchecked lastError to show error messages.

* Should the DCHECKs remain in the methods that were just made private?  We know
that contents will not be NULL here since we are checking before we call the
function; also, do we need to indicate in a comment the expectation for contents
in these functions or is this unnecessary because they are private functions?

* I did not add 'assertTrue(!!chrome.runtime.lastError);' within the tests since
I think it is redundant with the other line.  Is there a reason to include that
statement that I am not aware of?

[Jared Sohn, 2015-07-17 19:49:56]

Another question:

Right now, if an extension sets a mute state and the tab already has that mute
state, it counts toward the rate limiting but the user won't notice anything
(other than perhaps seeing the muted cause change if that look at the audio
indicator tooltip.)  Should it work this way or should
WebContentsImpl::SetAudioMuted return whether or not it actually made a change
and have the calling code only apply the rate limiting and change the muted
cause when it did?

[miu, 2015-07-18 00:42:42]

lgtm % nits:

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_constants.h (right):

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_constants.h:97: extern const char
kCannotUpdateMuteDisabled[];
Good idea!  Nice!  :)

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:105: DCHECK(contents);
To answer your question: The DCHECK here and at the top of
IsTabAudioMutedRateLimited() aren't strictly needed, but they are documentative.
 It's up to you whether to keep them or not.

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.h:51: extern const int
kMuteTokenBucketCostSeconds;
Just noticed this: Since these two constants are used only inside tab_utils.cc,
you don't need to declare them in the header file.  So, please delete these two
from this header file.

https://codereview.chromium.org/1233263002/diff/120001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/tabs/basics/muted.js (right):

https://codereview.chromium.org/1233263002/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/basics/muted.js:53:
chrome.test.assertLastError(errMsg);
To answer your question: Yes, this is better than what I proposed; and you don't
need to include the assertTrue(!!chrome.runtime.lastError) as I had mentioned.

[miu, 2015-07-18 00:48:30]

On 2015/07/17 19:49:56, Jared Sohn wrote:
> Another question:
> 
> Right now, if an extension sets a mute state and the tab already has that mute
> state, it counts toward the rate limiting but the user won't notice anything
> (other than perhaps seeing the muted cause change if that look at the audio
> indicator tooltip.)  Should it work this way or should
> WebContentsImpl::SetAudioMuted return whether or not it actually made a change
> and have the calling code only apply the rate limiting and change the muted
> cause when it did?

Hmm...This is up to you.  I can see one of three ways:

1. Just count all all the times an extension thinks it's changing the mute state
(whether or not it really is).

2. Don't count these no-op mute state changes.  Note: You don't need to change
WebContentsImpl::SetAudioMuted() for that.  Instead, you could just call
IsTabAudioMuted() at the top of SetTabAudioMuted() to see if there would be a
change.

3. #2, except *do* count the no-op if the "cause" is a different string.

#3 is probably my personal preference.

[Jared Sohn, 2015-07-18 23:57:23]

jared.sohn@gmail.com changed reviewers:
+ kalman@chromium.org

[Jared Sohn, 2015-07-18 23:57:25]

The latest patch should address issues from all feedback thus far.  I am also
adding kalman as a reviewer since he is the owner for a few files.

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.cc (right):

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.cc:105: DCHECK(contents);
I am removing them and replacing with a general comment since the current code
just says it cannot be a null pointer but doesn't say that it has initialized
LastMutedMetadata. 

On 2015/07/18 at 00:42:42, miu wrote:
> To answer your question: The DCHECK here and at the top of
IsTabAudioMutedRateLimited() aren't strictly needed, but they are documentative.
 It's up to you whether to keep them or not.

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
File chrome/browser/ui/tabs/tab_utils.h (right):

https://codereview.chromium.org/1233263002/diff/120001/chrome/browser/ui/tabs...
chrome/browser/ui/tabs/tab_utils.h:51: extern const int
kMuteTokenBucketCostSeconds;
When this is done, I get an error message that says that the
kMuteTokenBucketCapacitySeconds variable used in the constructor for
LastMuteMetadata is undefined.  It will compile correctly if I define the
constants in the file prior to the struct, but now these variables are in the
global namespace but not externed.  Is this okay?

On 2015/07/18 at 00:42:42, miu wrote:
> Just noticed this: Since these two constants are used only inside
tab_utils.cc, you don't need to declare them in the header file.  So, please
delete these two from this header file.

https://codereview.chromium.org/1233263002/diff/120001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/tabs/basics/muted.js (right):

https://codereview.chromium.org/1233263002/diff/120001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/tabs/basics/muted.js:53:
chrome.test.assertLastError(errMsg);
I have added a new test case to show that setting the same muted/mutedcause does
not hit rate limits.

[not at google - send to devlin, 2015-07-20 22:09:10]

What is the motivation for rate limiting? What is wrong with updating the mute
property 1000 times?

[Jared Sohn, 2015-07-20 23:01:22]

On 2015/07/20 at 22:09:10, kalman wrote:
> What is the motivation for rate limiting? What is wrong with updating the mute
property 1000 times?

From the API proposal document: 

>Rate limiting can prevent two extensions from endlessly “battling” with each
other.

Rate limiting doesn't resolve the battling (a user will likely need to uninstall
or reconfigure one of the extensions), but it does prevent an unpleasant
experiencing of hearing a tab constantly toggle between muted and unmuted as the
user races to uninstall/reconfigure.

[not at google - send to devlin, 2015-07-21 00:04:03]

I'm not sure that API throttling is the right solution to that, like, there are
other nuisance things that an extension can do and we don't rate limit those
(like creating tabs).

A nicer solution would be to do some kind of smoothing on the Chrome side if
this is a problem. For example: if more than N changes happen in T time period,
don't actually adjust the muting until after T has elapsed (though the final
mute value should be used).

[Jared Sohn, 2015-07-22 09:12:35]

On 2015/07/21 at 00:04:03, kalman wrote:
> I'm not sure that API throttling is the right solution to that, like, there
are other nuisance things that an extension can do and we don't rate limit those
(like creating tabs).
> 
> A nicer solution would be to do some kind of smoothing on the Chrome side if
this is a problem. For example: if more than N changes happen in T time period,
don't actually adjust the muting until after T has elapsed (though the final
mute value should be used).

I will put together a couple of simple extensions that battle with each other
and observe what the experience is like.

[Jared Sohn, 2015-07-24 20:35:38]

I wrote a couple of extensions in which one wants all tabs to be muted and the
other wants all tabs to be unmuted.  The behavior without any attempt at rate
limiting is to have a tab switch between being muted and unmuted every 200 ms.

So it takes advantage of Chrome's existing smoothing (the same smoothing I
discovered when testing muted with tabcapture.)  Is the current behavior fine as
is?  If so, do we still want to include some of the cleanup of error message
code / workflow from this patch?

[not at google - send to devlin, 2015-07-24 21:18:56]

On 2015/07/24 20:35:38, Jared Sohn wrote:
> I wrote a couple of extensions in which one wants all tabs to be muted and the
> other wants all tabs to be unmuted.  The behavior without any attempt at rate
> limiting is to have a tab switch between being muted and unmuted every 200 ms.
> 
> So it takes advantage of Chrome's existing smoothing (the same smoothing I
> discovered when testing muted with tabcapture.)  Is the current behavior fine
as
> is?  If so, do we still want to include some of the cleanup of error message
> code / workflow from this patch?

The thing with using rate limiting using the extension API is that it stops the
extension from making any other API calls. It's supposed to be an anti abuse
mechanism. In the case of 2 extensions battling over the same resources, it's
hard to say that extensions should be punished for that. Better to use the
normal error reporting mechanism. What is the cleanup you're talking about?

[Jared Sohn, 2015-07-24 21:46:42]

>What is the cleanup you're talking about?

1. Have SetTabAudioMuted return an enumerated value indicating why it failed and
use a switch to show an error message.  Do not redundantly do checks both in
SetTabAudiMuted and in TabsUpdateFunction::RunAsync (in tabs_api.cc)
2. Instead of sending warning messages to the console, set lastError.
3. Put message text in tabs_constants instead of tabs_api

[not at google - send to devlin, 2015-07-24 21:48:48]

Ah right. Yes let's submit that part.

https://codereview.chromium.org/1233263002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1233263002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_api.cc:1244: TabMutedResult
tabMutedResult = chrome::SetTabAudioMuted(
tab_muted_result

[Jared Sohn, 2015-07-24 22:44:21]

> it's hard to say that extensions should be punished for that. 

The original patch would maintain separate buckets for each extension.  Upon
miu's recommendation, it was changed over to use a single bucket for all
extensions since the end result behavior should be similar while making the code
simpler.  But since no buckets will also have similar behavior and makes the
code even simpler, it seems like a good alternative.

> chrome/browser/extensions/api/tabs/tabs_api.cc:1244: TabMutedResult
tabMutedResult = chrome::SetTabAudioMuted(
tab_muted_result

Done.  (And also removed rate limiting code.)

I also changed the bug's name/description; should it also mention that this
started as a bug to add extension tab muting rate limiting?

[not at google - send to devlin, 2015-07-24 22:47:49]

> 
> I also changed the bug's name/description; should it also mention that this
> started as a bug to add extension tab muting rate limiting?

No I wouldn't worry about mentioning that.

[not at google - send to devlin, 2015-07-24 22:51:25]

lgtm

https://codereview.chromium.org/1233263002/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1233263002/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/api/tabs/tabs_api.cc:1257: case
TAB_MUTED_RESULT_SUCCESS:
I would keep the cases in the same order they appear in the enum declaration, or
at least, put SUCCESS first.

[miu, 2015-07-27 17:27:59]

lgtm

[Jared Sohn, 2015-07-27 17:57:18]

The CQ bit was checked by jared.sohn@gmail.com

[Jared Sohn, 2015-07-27 17:57:18]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1233263002/#ps180001
(title: "include case for TAB_MUTED_RESULT_SUCCESS first within switch to match
enum order")

[commit-bot: I haz the power, 2015-07-27 17:58:47]

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

[commit-bot: I haz the power, 2015-07-27 19:25:22]

Committed patchset #11 (id:180001)

[commit-bot: I haz the power, 2015-07-27 19:25:53]

Patchset 11 (id:??) landed as
https://crrev.com/c337ac9749c9cc84872351a909222e151860e66e
Cr-Commit-Position: refs/heads/master@{#340528}