extensions: tabs: set tab id to -1 for app/devtools windows

chrome/browser/extensions/extension_tab_util.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/extensions/extension_tab_util.h"
 
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/extensions/api/tabs/tabs_constants.h"
 #include "chrome/browser/extensions/chrome_extension_function.h"
@@ skipping 332 matching lines @@
   ScrubTabValueForExtension(contents, extension, result);
   return result;
 }
 
 base::ListValue* ExtensionTabUtil::CreateTabList(
     const Browser* browser,
     const Extension* extension) {
   base::ListValue* tab_list = new base::ListValue();
   TabStripModel* tab_strip = browser->tab_strip_model();
   for (int i = 0; i < tab_strip->count(); ++i) {
-    tab_list->Append(CreateTabValue(tab_strip->GetWebContentsAt(i),
-                                    tab_strip,
-                                    i,
-                                    extension));
+    base::DictionaryValue* value =
+        CreateTabValue(tab_strip->GetWebContentsAt(i), tab_strip, i, extension);
+    // Remove tab id for devtools as we don't want them to be
+    // manipulated through the chrome.tabs API.
+    if (browser->is_devtools())

[dgozman, 2015/07/14 15:16:16]

What about TabUpdatedEvent or TabsGetCurrent? Perhaps, we should filter out id
(or even a whole tab entry?) from inside CreateTabValue?

[not at google - send to devlin, 2015/07/14 17:15:13]

CreateTabValue looks right to me.

[not at google - send to devlin, 2015/07/14 17:40:00]

Actually, GetTabID looks even right-er to me. More subtle and wide-reaching, but
skimming over the call sites, it looks safe. Perhaps double-check?

It already returns -1 if there is no tab; you could additionally special-case
devtools to return -1, then not set tab ID here if it's == -1.

[not at google - send to devlin, 2015/07/14 17:41:09]

More comments:

In fact I wonder if this would break backwards compatibility at all. We must
already be setting -1 for the tab ID in some cases already, and not setting it
if it == -1 would break that. In which case, I'd set it to -1 for devtools.

+      value->Remove(keys::kIdKey, nullptr);
+    tab_list->Append(value);
   }
 
   return tab_list;
 }
 
 base::DictionaryValue* ExtensionTabUtil::CreateTabValue(
     WebContents* contents,
     TabStripModel* tab_strip,
     int tab_index) {
   // If we have a matching AppWindow with a controller, get the tab value
@@ skipping 265 matching lines @@
 
   chrome::NavigateParams params(
       chrome::GetSingletonTabNavigateParams(browser, url_to_navigate));
   params.path_behavior = chrome::NavigateParams::IGNORE_AND_NAVIGATE;
   params.url = url_to_navigate;
   chrome::ShowSingletonTabOverwritingNTP(browser, params);
   return true;
 }
 
 }  // namespace extensions