Add audible, muted to Tab, c.t.query, c.t.update, and c.t.onUpdated where relevant

chrome/test/data/extensions/api_test/tab_capture/api_tests.js

Index: chrome/test/data/extensions/api_test/tab_capture/api_tests.js
diff --git a/chrome/test/data/extensions/api_test/tab_capture/api_tests.js b/chrome/test/data/extensions/api_test/tab_capture/api_tests.js
index 8f3e1d168ffaf9d2717c2d1ad3448292348cdd90..b4a624b4e1db05885562def1bc36d738a671b1d4 100644
--- a/chrome/test/data/extensions/api_test/tab_capture/api_tests.js
+++ b/chrome/test/data/extensions/api_test/tab_capture/api_tests.js
@@ -5,6 +5,7 @@
 var tabCapture = chrome.tabCapture;
 
 chrome.test.runTests([
+
   function captureTabAndVerifyStateTransitions() {
     // Tab capture events in the order they happen.
     var tabCaptureEvents = [];
@@ -29,7 +30,7 @@ chrome.test.runTests([
   },
 
   function getCapturedTabs() {
-    chrome.tabs.create({active:true}, function(secondTab) {
+    chrome.tabs.create({active: true}, function(secondTab) {
       // chrome.tabCapture.capture() will only capture the active tab.
       chrome.test.assertTrue(secondTab.active);
 
@@ -97,6 +98,36 @@ chrome.test.runTests([
     });
   },
 
+  function tabisUnmutedWhenTabCaptured() {

[miu, 2015/05/26 21:28:46]

nit: Capitalize the 'i' in 'is'.

[Jared Sohn, 2015/05/27 14:52:02]

done

+    var stream1 = null;
+
+    chrome.tabs.getCurrent(function(tab) {
+      chrome.test.listenForever(chrome.tabs.onUpdated,
+          function(tabId, changeInfo, updatedTab) {
+        var expectedMutedCause = "capture";
+        var expectedMuted = false;
+
+        if ((typeof changeInfo.mutedCause !== "undefined") &&

[not at google - send to devlin, 2015/05/26 23:09:46]

can also use changeInfo.hasOwnProperty('mutedCause')

or just forget about it altogether, since you're comparing to
|expectedMutedCause| which is a string, so won't be undefined.

[Jared Sohn, 2015/05/27 14:52:02]

done

+          (changeInfo["mutedCause"] === expectedMutedCause) &&

[miu, 2015/05/26 21:28:46]

nit: This line and the next should be two more spaces to the right.

[Jared Sohn, 2015/05/27 14:52:02]

done

+          (changeInfo["muted"] == expectedMuted))

[not at google - send to devlin, 2015/05/26 23:09:46]

Also to the above: not much point using === when comparing to the string
"capture", but arguably you should be using it for the |expectedMuted|
comparison.

[Jared Sohn, 2015/05/27 14:52:02]

done

+        {
+          chrome.test.assertEq(expectedMutedCause, updatedTab.mutedCause);
+          chrome.test.assertEq(expectedMuted, updatedTab.muted);
+          stream1.stop();
+          chrome.test.succeed();
+        }
+      });
+
+      chrome.tabs.update(tab.id, {muted: true}, function(newTab) {
+        setTimeout(function() {
+          tabCapture.capture({audio: true}, function(stream) {
+            stream1 = stream;
+          });
+        }, 200);

[not at google - send to devlin, 2015/05/26 23:09:46]

Ideally you wouldn't need any timeout and the callback wouldn't be run until the
tab has been muted?

[Jared Sohn, 2015/05/27 14:52:02]

Right; the test fails without the timeout. In prior discussions, miu suggested
that there may be a delay between the time tab capture starts and the
notification to the UI to change the tab strip model (and icon) is processed.

[not at google - send to devlin, 2015/05/28 23:04:42]

Mm this is problematic then. Timeout is just going to fail somewhere else, like
on a bot that's having a bad day, and then the test will be disabled anyway.

But yes I'm not surprised that there is a mismatch here - but then, the UI
shouldn't be relevant here. It should all be based on the content API events,
which should be consistent. Is it definitely not a race in there sometimes, like
a bug on our side?

An alternative would be to say you have 2 independent events,
1. tab has started capturing,
2. you got the event which you think corresponds to this change,
so move the assertions and the chrome.test.succeed() call until after *both*
events have arrived.

It could look something like:

var stream1 = null;
var updatedTab = null;

function nextStep() {
  if (!stream1 || !updatedTab) return;
  chrome.test.assertEq(..);
  chrome.test.assertEq(..);
  stream1.stop();
  chrome.test.succeed();
}

// Listen for tab events, when the relevant one arrives,
// set |updatedTab| then call nextStep().

// Start capturing tab, then when it's capturing,
// set |stream1| then call nextStep().

Failure will be a timeout, but that's ok, so long as it's a dependable timeout.

[miu, 2015/05/28 23:36:59]

Explanation: The content API events propagate through a "de-duping" heuristic in
Browser::NavigationStateChanged()
(https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...)
before any changes reach the BrowserTabStripModel and therefore propagate to the
tab event dispatcher code.  So, the "race" is being artificially introduced by
code that attempts to reduce the rate of change in drawing in the browser UI.

[not at google - send to devlin, 2015/06/01 20:33:56]

Ok, thanks for the explanation. Would my suggestion fix the potential flakiness?
Jared, could you give it a go?

[Jared Sohn, 2015/06/02 08:54:38]

miu: Are you saying that there is a delay before the tab dispatcher code knows a
tab is muted, especially prior to tab capture?  If so that makes sense based on
the behavior I am witnessing.  This is a little different than the potential
problem described in my earlier comment (and that kalman's suggestion is based
on.)

kalman: I don't think your solution will work because we care about the order
between making the tab muted and having tab capture unmute it.  If we allow them
both to run independently, then sometimes it will mute and then unmute the tab
like we want and sometimes it will try to mute a tab after it is captured (which
will result in an error message.)

After quite a bit of playing with the code, here is the behavior that I am
observing:

* The basic problem is that we are not getting any onUpdated events if we remove
the setTimeout.

* If I change update code to set {pinned: true}, it does run the onUpdated
event.

* If I add 

            chrome.tabs.getCurrent(function(tab) {
              console.log(tab.muted);
              console.log(tab.mutedCause);
            });

after the tab capture, I find that the mutedCause is always "capture".  (I had
thought the problem might be a timing issue with when the state was set to
captured, but this seems not to be the case.)

* In the above test, muted is always false if the timeout is sufficiently long
(170 ms on my machine) and otherwise varies between true and false.

*  I did some ad hoc testing of combining chrome.tabs.update({"muted"}) with
getCurrent and onUpdated (basically no tab capture), and that worked okay.


So I think the bug can be described as:

If we mute a tab and then tab capture it without waiting long enough before
doing so, then the onUpdated events do not trigger and a race condition can
cause muted to be assigned the wrong value.

If there is a maximum delay before updating the tab event dispatcher code,
perhaps we can just add a setTimeout a little longer than that and be assured
that it will work consistently? 

[not at google - send to devlin, 2015/06/02 18:26:43]

Ok: should we just forget about this portion of the test then? If there's no way
to guarantee that if you mute a tab, then turn around right away and query its
mute state that it's updated... then testing that non-guaranteeable behavior
seems pointless.

Or another option would be in the test setup to remove the the timeout that miu@
refers to... but I'm not sure I trust that, either.

Anyway, I think that a test of
1. query mute state, check is false
2. mute tab
3. wait for update
4. assert that update has mute is true
5. query mute state, check is true

is perfectly strong.

+      });
+    });
+  },
+
   function onlyVideo() {
     tabCapture.capture({video: true}, function(stream) {
       chrome.test.assertTrue(!!stream);