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

chrome/browser/extensions/api/tabs/tabs_event_router.cc

Index: chrome/browser/extensions/api/tabs/tabs_event_router.cc
diff --git a/chrome/browser/extensions/api/tabs/tabs_event_router.cc b/chrome/browser/extensions/api/tabs/tabs_event_router.cc
index baab0e12ffeef2f666fae69c5454aeff8b6e8550..6c066044a640002b6c7ac282c6e53ca8b271a5d3 100644
--- a/chrome/browser/extensions/api/tabs/tabs_event_router.cc
+++ b/chrome/browser/extensions/api/tabs/tabs_event_router.cc
@@ -2,9 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <string>

[not at google - send to devlin, 2015/03/10 22:30:08]

not needed, header file already includes <string>

[Jared Sohn, 2015/03/16 02:34:28]

I added that to remove a lint error: (build/include_what_you_use)  Do you still
want it removed?  (The lint message suggests that it should go in the header or
the source file, so at the very least it would make more sense to go in the
header file since the method signatures make use of it.)

 #include "chrome/browser/extensions/api/tabs/tabs_event_router.h"
 
 #include "base/json/json_writer.h"
+#include "base/supports_user_data.h"
 #include "base/values.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/api/tabs/tabs_constants.h"
@@ -16,6 +18,7 @@
 #include "chrome/browser/ui/browser_iterator.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/tabs/tab_utils.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "content/public/browser/favicon_status.h"
 #include "content/public/browser/navigation_controller.h"
@@ -23,6 +26,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 
 using base::DictionaryValue;
 using base::ListValue;
@@ -58,7 +62,33 @@ bool WillDispatchTabUpdatedEvent(
 
 }  // namespace
 
+void TabsEventRouter::TabAudibleStateChanged(content::WebContents* contents,
+                                             bool audible) {
+  scoped_ptr<base::DictionaryValue> changed_properties(
+      new base::DictionaryValue());

[not at google - send to devlin, 2015/03/10 22:30:08]

I just submitted https://codereview.chromium.org/1000443002/ which should make
this easier:

scoped_ptr<base::DictionaryValue> changed_properties = DictionaryBuilder()
    .SetBoolean(tabs_constants::kAudibleKey, audible)
    .Build();

etc; even nicer below.

+
+  changed_properties->SetBoolean(tabs_constants::kAudibleKey,
+                                 audible);
+  DispatchTabUpdatedEvent(contents, changed_properties.Pass());

[miu, 2015/03/10 18:51:39]

Instead of dispatching separate events here and in TabMutedStateChanged, can you
create one changed_properties dictionary in TabUpdated()?  For efficiency, all
the changes can be dispatched in a single event with multiple changed
properties.

[Jared Sohn, 2015/03/16 02:34:29]

Done.

+}
+
+void TabsEventRouter::TabMutedStateChanged(content::WebContents* contents,
+                                           bool muted,
+                                           const std::string& cause)  {
+  scoped_ptr<base::DictionaryValue> changed_properties(
+      new base::DictionaryValue());
+
+  changed_properties->SetBoolean(tabs_constants::kMutedKey,
+                                 muted);
+  changed_properties->SetString(tabs_constants::kCauseKey,
+                                 cause);

[miu, 2015/03/10 18:51:39]

nit: indent one less space, and run `git cl format` to fix a few other things

[Jared Sohn, 2015/03/16 02:34:29]

Done.

+
+  DispatchTabUpdatedEvent(contents, changed_properties.Pass());
+}
+
 TabsEventRouter::TabEntry::TabEntry() : complete_waiting_on_load_(false),
+                                        wasAudible_(false),

[miu, 2015/03/10 18:51:39]

A TabEntry might be constructed after a tab is alive and already playing audio
(e.g., if a tab is dragged between browser windows, or TabsEventRouter was
constructed late).  I think this constructor should be:

TabsEventRouter::TabEntry::TabEntry(content::WebContents* contents)
  : ...,
    was_audible_(contents->WasRecentlyAudible()),
    was_muted_(contents->IsAudioMuted()),
    ... {
}

+                                        wasMuted_(false),
                                         url_() {
 }
 
@@ -94,6 +124,24 @@ base::DictionaryValue* TabsEventRouter::TabEntry::DidNavigate(
   return changed_properties;
 }
 
+bool TabsEventRouter::TabEntry::AudibleChanged(bool val) const {

[miu, 2015/03/10 18:51:39]

nit: Your call, but consider combining each pair of these methods into a single
method that sets the new value and returns true if the value actually changed. 
Example:

bool TabsEventRouter::TabEntry::SetAudible(bool new_val) {
  if (was_audible_ == new_val)
    return false;
  was_audible_ = new_val;
  return true;
}

[not at google - send to devlin, 2015/03/10 22:30:08]

+1 and also no parens around these expressions.

[Jared Sohn, 2015/03/16 02:34:29]

Done.

[Jared Sohn, 2015/03/16 02:34:29]

Done.

+  return (wasAudible_ != val);
+}
+
+bool TabsEventRouter::TabEntry::MutedChanged(bool val) const {
+  return (wasMuted_ != val);
+}
+
+void TabsEventRouter::TabEntry::ToggleAudible() {
+  wasAudible_ = !wasAudible_;
+}
+
+void TabsEventRouter::TabEntry::ToggleMuted() {
+  wasMuted_ = !wasMuted_;
+}
+
+
+

[not at google - send to devlin, 2015/03/10 22:30:08]

only 1 blank line

[Jared Sohn, 2015/03/16 02:34:29]

Done.

 TabsEventRouter::TabsEventRouter(Profile* profile) : profile_(profile) {
   DCHECK(!profile->IsOffTheRecord());
 
@@ -396,6 +444,21 @@ void TabsEventRouter::TabUpdated(WebContents* contents, bool did_navigate) {
 
   CHECK(entry);
 
+  bool audible = contents->WasRecentlyAudible();
+  if (entry->AudibleChanged(audible)) {
+    entry->ToggleAudible();
+
+    TabAudibleStateChanged(contents, audible);
+  }
+
+  bool muted = contents->IsAudioMuted();
+  if (entry->MutedChanged(muted)) {
+    entry->ToggleMuted();
+
+    TabMutedStateChanged(contents, muted,
+      chrome::GetTabAudioMutedCause(contents));
+  }

[not at google - send to devlin, 2015/03/10 22:30:08]

This code should be consistent with the existing code - unfortunately the
existing code is odd and doesn't scale very well to more functionality (with
that single |did_navigate| parameter).

How about you change TabUpdated to take a scoped_ptr<base::DictionaryValue>
instead of the contents/boolean:

TabUpdated(contents, true) -->
TabUpdated(GetTabEntry(contents)->DidNavigate());

and likewise

TabUpdate(contents, false) -->
TabUpdated(GetTabEntry(contents)->UpdateLoadState());

That would require TabEntry to hold onto its WebContents, but that seems like
the right thing to do anyway.

[Jared Sohn, 2015/03/16 02:34:29]

At the moment, I am choosing to pass an extensions::DictionaryBuilder object
instead of a scoped_ptr<base::DictionaryValue>.  This simplifies the code; is
there a reason not to pass it instead? (Or maybe you wrote this comment before
you exposed DictionaryBuilders to extensions?)

I have updated the code to include the WebContents as a part of the TabEntry. 
Unfortunately, I don't think we can remove the need for passing in a WebContents
object here since we get the TabEntry from that.

[not at google - send to devlin, 2015/03/16 17:47:08]

Passing a DictionaryBuilder is fine. I hadn't thought of that.

+
   if (did_navigate)
     changed_properties.reset(entry->DidNavigate(contents));
   else
@@ -450,6 +513,7 @@ void TabsEventRouter::DispatchSimpleBrowserEvent(
 void TabsEventRouter::DispatchTabUpdatedEvent(
     WebContents* contents,
     scoped_ptr<base::DictionaryValue> changed_properties) {
+

[miu, 2015/03/10 18:51:39]

nit: remove added newlines throughout this method

[Jared Sohn, 2015/03/16 02:34:28]

Done.

   DCHECK(changed_properties);
   DCHECK(contents);
 
@@ -468,14 +532,17 @@ void TabsEventRouter::DispatchTabUpdatedEvent(
   // WillDispatchTabUpdatedEvent.
   Profile* profile = Profile::FromBrowserContext(contents->GetBrowserContext());
 
+
   scoped_ptr<Event> event(
       new Event(tabs::OnUpdated::kEventName, args_base.Pass()));
   event->restrict_to_browser_context = profile;
+
   event->user_gesture = EventRouter::USER_GESTURE_NOT_ENABLED;
   event->will_dispatch_callback =
       base::Bind(&WillDispatchTabUpdatedEvent,
                  contents,
                  changed_properties.get());
+
   EventRouter::Get(profile)->BroadcastEvent(event.Pass());
 }