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

 // Copyright 2013 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 <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"
 #include "chrome/browser/extensions/api/tabs/tabs_windows_api.h"
 #include "chrome/browser/extensions/api/tabs/windows_event_router.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #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"
 #include "content/public/browser/navigation_entry.h"
 #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;
 using base::FundamentalValue;
 using content::NavigationController;
 using content::WebContents;
 using ui_zoom::ZoomController;
 
 namespace extensions {
 
@@ skipping 15 matching lines @@
                                               properties_value);
   event_args->Set(1, properties_value);
 
   // Overwrite the third arg with our tab value as seen by this extension.
   event_args->Set(2, ExtensionTabUtil::CreateTabValue(contents, extension));
   return true;
 }
 
 }  // 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_() {
 }
 
 base::DictionaryValue* TabsEventRouter::TabEntry::UpdateLoadState(
     const WebContents* contents) {
   // The tab may go in & out of loading (for instance if iframes navigate).
   // We only want to respond to the first change from loading to !loading after
   // the NAV_ENTRY_COMMITTED was fired.
   if (!complete_waiting_on_load_ || contents->IsLoading())
     return NULL;
@@ skipping 15 matching lines @@
                                 tabs_constants::kStatusValueLoading);
 
   if (contents->GetURL() != url_) {
     url_ = contents->GetURL();
     changed_properties->SetString(tabs_constants::kUrlKey, url_.spec());
   }
 
   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());
 
   BrowserList::AddObserver(this);
 
   // Init() can happen after the browser is running, so catch up with any
   // windows that already exist.
   for (chrome::BrowserIterator it; !it.done(); it.Next()) {
     RegisterForBrowserNotifications(*it);
 
@@ skipping 282 matching lines @@
                 args.Pass(),
                 EventRouter::USER_GESTURE_UNKNOWN);
 }
 
 void TabsEventRouter::TabUpdated(WebContents* contents, bool did_navigate) {
   TabEntry* entry = GetTabEntry(contents);
   scoped_ptr<base::DictionaryValue> changed_properties;
 
   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
     changed_properties.reset(entry->UpdateLoadState(contents));
 
   if (changed_properties)
     DispatchTabUpdatedEvent(contents, changed_properties.Pass());
 }
 
 void TabsEventRouter::FaviconUrlUpdated(WebContents* contents) {
@@ skipping 34 matching lines @@
 
   DispatchEvent(profile,
                 event_name,
                 args.Pass(),
                 EventRouter::USER_GESTURE_UNKNOWN);
 }
 
 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);
 
   // The state of the tab (as seen from the extension point of view) has
   // changed.  Send a notification to the extension.
   scoped_ptr<base::ListValue> args_base(new base::ListValue);
 
   // First arg: The id of the tab that changed.
   args_base->AppendInteger(ExtensionTabUtil::GetTabId(contents));
 
   // Second arg: An object containing the changes to the tab state.  Filled in
   // by WillDispatchTabUpdatedEvent as a copy of changed_properties, if the
   // extension has the tabs permission.
 
   // Third arg: An object containing the state of the tab. Filled in by
   // 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());
 }
 
 TabsEventRouter::TabEntry* TabsEventRouter::GetTabEntry(WebContents* contents) {
   int tab_id = ExtensionTabUtil::GetTabId(contents);
   std::map<int, TabEntry>::iterator i = tab_entries_.find(tab_id);
   if (tab_entries_.end() == i)
     return NULL;
   return &i->second;
 }
@@ skipping 90 matching lines @@
   // Dispatch the |onZoomChange| event.
   Profile* profile = Profile::FromBrowserContext(
       data.web_contents->GetBrowserContext());
   DispatchEvent(profile,
                 tabs::OnZoomChange::kEventName,
                 api::tabs::OnZoomChange::Create(zoom_change_info),
                 EventRouter::USER_GESTURE_UNKNOWN);
 }
 
 }  // namespace extensions