tabId support to chrome.extensions.getViews()

chrome/test/data/extensions/api_test/get_views/test.js

 // 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.
 
 // API test for chrome.extension.getViews.
 // browser_tests.exe --gtest_filter=ExtensionApiTest.GetViews
 
 const assertEq = chrome.test.assertEq;
 const assertTrue = chrome.test.assertTrue;
 
 // We need to remember the popupWindowId to be able to find it later.
 var popupWindowId = 0;
 
+// Updated and used later to check getViews() tabId tag for options tab

[Devlin, 2016/07/11 17:23:45]

We're sticklers for grammar in comments, so there shouldn't be any missing words
(so this should be "for *the* options tab"), and each should have proper
punctuation (including the ending period).

[catmullings, 2016/07/12 18:13:47]

Done.

+var optionsTabId = 0;
+
 // This function is called by the popup during the test run.
 function popupCallback() {
   // We have now added an popup so the total count goes up one.
   assertEq(2, chrome.extension.getViews().length);
   assertEq(1, chrome.extension.getViews({"windowId": popupWindowId}).length);
-
-  chrome.tabs.create({"url": chrome.extension.getURL("options.html")});
+  chrome.tabs.create({"url": chrome.extension.getURL("options.html")},
+      function(tab) {
+      // Check if tab returned is defined

[Devlin, 2016/07/11 17:23:45]

indentation for callback functions is always kind of wonky, but in this case I
would recommend:
chrome.tabs.create(...
    function(tab) {
  ...
});

[Devlin, 2016/07/11 17:23:45]

this comment just describes what the code is doing right here, so I don't think
it really adds value.  I'd remove it.

[catmullings, 2016/07/12 18:13:47]

Done.

[catmullings, 2016/07/12 18:13:48]

Done.

+      optionsTabId = tab.id;
+      });
 }
 
 function optionsPageCallback() {
   assertEq(3, chrome.extension.getViews().length);
   assertEq(1, chrome.extension.getViews({"windowId": popupWindowId}).length);
   assertEq(2, chrome.extension.getViews(
-      {"type": "tab", "windowId": window.id}).length);
+        {"type": "tab", "windowId": window.id}).length);
+
+  chrome.tabs.query({"windowId": window.id}, function(tabs) {

[Devlin, 2016/07/11 17:23:45]

I know you're just following the style from the rest of this test, but in JS,
prefer to not have quotes around the key in objects.  So just
{windowId: window.id}
rather than
{"windowId": window.id}

If you wanted to update the rest of this test too, that'd be good, but don't
feel like you have to.

[catmullings, 2016/07/12 18:13:47]

Done.

[catmullings, 2016/07/12 18:13:48]

I'll go ahead and update the rest of the test with this. It would feel
inconsistent otherwise. 

+      // Assert 3 to account for new tab page, web popup, & new tab options.html

[Devlin, 2016/07/11 17:23:45]

2-space indent here.

[catmullings, 2016/07/12 18:13:48]

Done.

+      assertEq(3, tabs.length);
+
+      // Check that only 2 tabs belong to the extension

[Devlin, 2016/07/11 17:23:45]

Here, use a comment to describe why, not what.  That is, rather than saying you
are checking that only two tabs belong to the extension (which is clear from the
code), mention why there are two tabs (i.e., which two tabs?).

[catmullings, 2016/07/12 18:13:47]

Done.

[Devlin, 2016/07/12 18:35:57]

Was this actually done?

+      assertEq(2, chrome.extension.getViews({"type":"tab"}).length);
+
+      for (var i = 0; i < tabs.length; i++) {
+        if (tabs[i].windowId == popupWindowId) {
+          assertEq(1, chrome.extension.getViews({"tabId": tabs[i].id}).length);
+           // Test tabId tag with other parameters
+          assertEq(1, chrome.extension.getViews({"windowId": popupWindowId,
+                                                 "tabId": tabs[i].id}).length);
+          assertEq(1, chrome.extension.getViews({"type": "tab",
+                                                 "tabId": tabs[i].id}).length);
+        } else if (tabs[i].id == optionsTabId) {
+          assertEq(1, chrome.extension.getViews({"tabId": tabs[i].id}).length);
+        }
+      }
+
+  });
+
   chrome.test.notifyPass();
 }
 
 var tests = [
-  function getViews() {

[Devlin, 2016/07/11 17:23:45]

this indentation was probably more correct before.

-    assertTrue(typeof(chrome.extension.getBackgroundPage()) != "undefined");
-    assertEq(1, chrome.extension.getViews().length);
-    assertEq(0, chrome.extension.getViews({"type": "tab"}).length);
-    assertEq(0, chrome.extension.getViews({"type": "popup"}).length);
+function getViews() {
+  //printf("Other fn");

[Devlin, 2016/07/11 17:23:45]

Don't forget to remove debugging code.

[catmullings, 2016/07/12 18:13:47]

Done.

[Devlin, 2016/07/12 18:35:57]

Ditto

+  assertTrue(typeof(chrome.extension.getBackgroundPage()) != "undefined");
+  assertEq(1, chrome.extension.getViews().length);
+  assertEq(0, chrome.extension.getViews({"type": "tab"}).length);
+  assertEq(0, chrome.extension.getViews({"type": "popup"}).length);
 
-    chrome.windows.getAll({populate: true}, function(windows) {
+  chrome.windows.getAll({populate: true}, function(windows) {
       assertEq(1, windows.length);
 
       // Create a popup window.
+
+      // TODO (catmullings): Fix potential race condition when/if
+      // popupCallback() is called before popupWindowId is set below
       chrome.windows.create({"url": chrome.extension.getURL("popup.html"),
-                             "type": "popup"}, function(window) {
+        "type": "popup"}, function(window) {

[Devlin, 2016/07/11 17:23:45]

here, too, indentation was probably right before.

[catmullings, 2016/07/12 18:13:47]

Done.

[Devlin, 2016/07/12 18:35:57]

Ditto

         assertTrue(window.id > 0);
         popupWindowId = window.id;
         // The popup will call back to us through popupCallback (above).
+        });
       });
-    });
-  }
+}
 ];
 
 chrome.test.runTests(tests);