Bug 1419880 first round of TODO/FIXME comment updates, r=me draft
authorShane Caraveo <scaraveo@mozilla.com>
Wed, 22 Nov 2017 11:54:07 -0800
changeset 702142 1755e084e8ed3faa7bf4ba3743156e0238e89396
parent 702065 4affa6e0a8c622e4c4152872ffc14b73103830ac
child 741381 79e2d739d76286a5bd4d57f8f78b1a94480ce054
push id90394
push usermixedpuppy@gmail.com
push dateWed, 22 Nov 2017 19:55:12 +0000
reviewersme
bugs1419880
milestone59.0a1
Bug 1419880 first round of TODO/FIXME comment updates, r=me MozReview-Commit-ID: 6GsqWSBKB2d
toolkit/components/extensions/ExtensionContent.jsm
toolkit/components/extensions/ProxyScriptContext.jsm
toolkit/components/extensions/Schemas.jsm
toolkit/components/extensions/ext-cookies.js
toolkit/components/extensions/ext-downloads.js
toolkit/components/extensions/test/mochitest/head_webrequest.js
toolkit/components/extensions/test/mochitest/test_ext_clipboard.html
toolkit/components/extensions/test/mochitest/test_ext_cookies.html
toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html
toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html
toolkit/components/extensions/test/mochitest/test_ext_webrequest_background_events.html
toolkit/components/extensions/test/xpcshell/head_telemetry.js
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js
toolkit/components/extensions/test/xpcshell/test_ext_downloads_misc.js
toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js
toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
--- a/toolkit/components/extensions/ExtensionContent.jsm
+++ b/toolkit/components/extensions/ExtensionContent.jsm
@@ -487,17 +487,17 @@ class ContentScriptContextChild extends 
     Schemas.exportLazyGetter(this.sandbox, "chrome", () => this.chromeObj);
   }
 
   injectAPI() {
     if (!this.isExtensionPage) {
       throw new Error("Cannot inject extension API into non-extension window");
     }
 
-    // This is an iframe with content script API enabled (bug 1214658)
+    // This is an iframe with content script API enabled (See Bug 1214658)
     Schemas.exportLazyGetter(this.contentWindow,
                              "browser", () => this.chromeObj);
     Schemas.exportLazyGetter(this.contentWindow,
                              "chrome", () => this.chromeObj);
   }
 
   get cloneScope() {
     return this.sandbox;
@@ -513,17 +513,17 @@ class ContentScriptContextChild extends 
     super.unload();
 
     if (this.contentWindow) {
       for (let script of this.scripts) {
         script.cleanup(this.contentWindow);
       }
 
       // Overwrite the content script APIs with an empty object if the APIs objects are still
-      // defined in the content window (bug 1214658).
+      // defined in the content window (See Bug 1214658).
       if (this.isExtensionPage) {
         Cu.createObjectIn(this.contentWindow, {defineAs: "browser"});
         Cu.createObjectIn(this.contentWindow, {defineAs: "chrome"});
       }
     }
     Cu.nukeSandbox(this.sandbox);
     this.sandbox = null;
   }
--- a/toolkit/components/extensions/ProxyScriptContext.jsm
+++ b/toolkit/components/extensions/ProxyScriptContext.jsm
@@ -132,17 +132,17 @@ const ProxyInfoData = {
       return defaultProxyInfo;
     }
     let {type, host, port, username, password, proxyDNS, failoverTimeout} =
         ProxyInfoData.validate(proxyDataList[proxyDataListIndex]);
     if (type === PROXY_TYPES.DIRECT) {
       return defaultProxyInfo;
     }
     let failoverProxy = this.createProxyInfoFromData(proxyDataList, defaultProxyInfo, proxyDataListIndex + 1);
-    // When Bug 1360404 is fixed use ProxyService.newProxyInfoWithAuth() for all types.
+    // TODO When Bug 1360404 is fixed use ProxyService.newProxyInfoWithAuth() for all types.
     if (type === PROXY_TYPES.SOCKS || type === PROXY_TYPES.SOCKS4) {
       return ProxyService.newProxyInfoWithAuth(
               type, host, port, username, password, proxyDNS ? TRANSPARENT_PROXY_RESOLVES_HOST : 0,
               failoverTimeout ? failoverTimeout : PROXY_TIMEOUT_SEC, failoverProxy);
     }
     return ProxyService.newProxyInfo(
             type, host, port, proxyDNS ? TRANSPARENT_PROXY_RESOLVES_HOST : 0,
             failoverTimeout ? failoverTimeout : PROXY_TIMEOUT_SEC, failoverProxy);
@@ -286,17 +286,17 @@ class ProxyScriptContext extends BaseCon
    * @param {Object} service A reference to the Protocol Proxy Service.
    * @param {Object} uri The URI for which these proxy settings apply.
    * @param {Object} defaultProxyInfo The proxy (or list of proxies) that
    *     would be used by default for the given URI. This may be null.
    * @returns {Object} The proxy info to apply for the given URI.
    */
   applyFilter(service, uri, defaultProxyInfo) {
     try {
-      // Bug 1337001 - provide path and query components to non-https URLs.
+      // TODO Bug 1337001 - provide path and query components to non-https URLs.
       let ret = this.FindProxyForURL(uri.prePath, uri.host, this.contextInfo);
       return this.proxyInfoFromProxyData(ret, defaultProxyInfo);
     } catch (e) {
       let error = this.normalizeError(e);
       this.extension.emit("proxy-error", {
         message: error.message,
         fileName: error.fileName,
         lineNumber: error.lineNumber,
--- a/toolkit/components/extensions/Schemas.jsm
+++ b/toolkit/components/extensions/Schemas.jsm
@@ -2273,17 +2273,17 @@ FunctionEntry = class FunctionEntry exte
         apiImpl = null;
       },
     };
   }
 };
 
 // Represents an "event" defined in a schema namespace.
 //
-// TODO(rpl): we should be able to remove the eslint-disable-line that follows
+// TODO Bug 1369722: we should be able to remove the eslint-disable-line that follows
 // once Bug 1369722 has been fixed.
 Event = class Event extends CallEntry { // eslint-disable-line no-native-reassign
   static parseSchema(event, path) {
     let extraParameters = Array.from(event.extraParameters || [], param => ({
       type: Schemas.parseSchema(param, path, ["name", "optional", "default"]),
       name: param.name,
       optional: param.optional || false,
       default: param.default == undefined ? null : param.default,
--- a/toolkit/components/extensions/ext-cookies.js
+++ b/toolkit/components/extensions/ext-cookies.js
@@ -342,17 +342,17 @@ this.cookies = class extends ExtensionAP
 
           return self.cookies.get(details);
         },
 
         remove: function(details) {
           for (let {cookie, storeId} of query(details, ["url", "name", "storeId"], context)) {
             Services.cookies.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
 
-            // Todo: could there be multiple per subdomain?
+            // TODO Bug 1387957: could there be multiple per subdomain?
             return Promise.resolve({
               url: details.url,
               name: details.name,
               storeId,
             });
           }
 
           return Promise.resolve(null);
--- a/toolkit/components/extensions/ext-downloads.js
+++ b/toolkit/components/extensions/ext-downloads.js
@@ -148,17 +148,17 @@ class DownloadItem {
       this.prechange[field] = this[field];
     }
   }
 }
 
 
 // DownloadMap maps back and forth betwen the numeric identifiers used in
 // the downloads WebExtension API and a Download object from the Downloads jsm.
-// todo: make id and extension info persistent (bug 1247794)
+// TODO Bug 1247794: make id and extension info persistent
 const DownloadMap = new class extends EventEmitter {
   constructor() {
     super();
 
     this.currentId = 0;
     this.loadPromise = null;
 
     // Maps numeric id -> DownloadItem
@@ -233,18 +233,18 @@ const DownloadMap = new class extends Ev
     const id = ++this.currentId;
     let item = new DownloadItem(id, download, extension);
     this.byId.set(id, item);
     this.byDownload.set(download, item);
     return item;
   }
 
   erase(item) {
-    // This will need to get more complicated for bug 1255507 but for now we
-    // only work with downloads in the DownloadList from getAll()
+    // TODO Bug 1255507: for now we only work with downloads in the DownloadList
+    // from getAll()
     return this.getDownloadList().then(list => {
       list.remove(item.download);
     });
   }
 }();
 
 // Create a callable function that filters a DownloadItem based on a
 // query object of the type passed to search() or erase().
--- a/toolkit/components/extensions/test/mochitest/head_webrequest.js
+++ b/toolkit/components/extensions/test/mochitest/head_webrequest.js
@@ -172,17 +172,17 @@ function background(events) {
       } else {
         // Save any values we want to validate in later events.
         expected.test.requestId = details.requestId;
         expected.test.tabId = details.tabId;
       }
       // Tests we don't need to do every event.
       browser.test.assertTrue(details.type.toUpperCase() in browser.webRequest.ResourceType, `valid resource type ${details.type}`);
       if (details.type == "main_frame") {
-        browser.test.assertEq(0, details.frameId, "frameId is zero when type is main_frame bug 1329299");
+        browser.test.assertEq(0, details.frameId, "frameId is zero when type is main_frame, see bug 1329299");
       }
     },
     onBeforeSendHeaders(expected, details, result) {
       if (expected.headers && expected.headers.request) {
         result.requestHeaders = processHeaders("request", expected, details);
       }
       if (expected.redirect) {
         browser.test.log(`${name} redirect request`);
--- a/toolkit/components/extensions/test/mochitest/test_ext_clipboard.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_clipboard.html
@@ -45,17 +45,17 @@ add_task(async function test_background_
   let extension = ExtensionTestUtils.loadExtension(extensionData);
   await extension.startup();
 
   await extension.awaitMessage("ready");
 
   await extension.unload();
 });
 
-/** Selecting text in a bg page is not possible, bug 1272869.
+/** TODO Bug 1272869 Selecting text in a bg page is not possible.
 add_task(function* test_background_clipboard_copy() {
   function backgroundScript() {
     browser.test.onMessage.addListener(txt => {
       browser.test.assertEq(true, doCopy(txt),
         "copy should be allowed with permission");
     });
     browser.test.sendMessage("ready");
   }
--- a/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_cookies.html
@@ -43,17 +43,18 @@ add_task(async function test_cookies() {
           url: [{
             urlPrefix: PRIVATE_TEST_URL,
           }],
         });
       });
       // This tab is opened for two purposes:
       // 1. To allow tests to run content scripts in the context of a tab,
       //    for fetching the value of document.cookie.
-      // 2. To work around bug 1309637, based on the analysis in comment 8.
+      // 2. TODO Bug 1309637 To work around cookies in incognito windows,
+      //    based on the analysis in comment 8.
       let {id: windowId} = await browser.windows.create({
         incognito: true,
         url: PRIVATE_TEST_URL,
       });
       let tabId = await tabReadyPromise;
       return {windowId, tabId};
     }
 
@@ -120,17 +121,17 @@ add_task(async function test_cookies() {
     browser.test.assertEq(null, cookie, "removed cookie not found");
 
     let stores = await browser.cookies.getAllCookieStores();
     browser.test.assertEq(1, stores.length, "expected number of stores returned");
     browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
     browser.test.assertEq(1, stores[0].tabIds.length, "one tabId returned for store");
     browser.test.assertEq("number", typeof stores[0].tabIds[0], "tabId is a number");
 
-    // Opening private windows/tabs is not supported on Android - bug 1372178
+    // TODO bug 1372178: Opening private windows/tabs is not supported on Android
     if (browser.windows) {
       let {windowId} = await openPrivateWindowAndTab(TEST_URL);
       let stores = await browser.cookies.getAllCookieStores();
 
       browser.test.assertEq(2, stores.length, "expected number of stores returned");
       browser.test.assertEq(STORE_ID, stores[0].id, "expected store id returned");
       browser.test.assertEq(1, stores[0].tabIds.length, "one tab returned for store");
       browser.test.assertEq(PRIVATE_STORE_ID, stores[1].id, "expected private store id returned");
@@ -212,17 +213,17 @@ add_task(async function test_cookies() {
     details = await browser.cookies.remove({url: TEST_URL, name: "name1"});
     assertExpected({url: TEST_URL, name: "name1", storeId: STORE_ID}, details);
 
     cookie = await browser.cookies.set({url: TEST_URL});
     browser.test.assertEq("", cookie.name, "default name set");
     browser.test.assertEq("", cookie.value, "default value set");
     browser.test.assertEq(true, cookie.session, "no expiry date created session cookie");
 
-    // Opening private windows/tabs is not supported on Android - bug 1372178
+    // TODO bug 1372178: Opening private windows/tabs is not supported on Android
     if (browser.windows) {
       let {tabId, windowId} = await openPrivateWindowAndTab(TEST_URL);
 
       browser.test.assertEq("", await getDocumentCookie(tabId), "initially no cookie");
 
       let cookie = await browser.cookies.set({url: TEST_URL, name: "store", value: "private", expirationDate: THE_FUTURE, storeId: PRIVATE_STORE_ID});
       browser.test.assertEq("private", cookie.value, "set the private cookie");
 
--- a/toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html
@@ -63,17 +63,17 @@ function getExtension() {
         });
         return result;
       }
 
       let tab = await browser.tabs.create({url: "http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_sample.html"});
       let result = await testSubFrameResource(tab.id);
       browser.test.assertEq("loaded", result[0], "frame 1 loaded");
       browser.test.assertEq(redirectUrl, result[1], "frame 1 redirected");
-      // Bug 1390346 If jar caching breaks redirects, this next test will fail.
+      // If jar caching breaks redirects, this next test will fail (See Bug 1390346).
       result = await testSubFrameResource(tab.id);
       browser.test.assertEq("loaded", result[0], "frame 2 loaded");
       browser.test.assertEq(redirectUrl, result[1], "frame 2 redirected");
       await browser.tabs.remove(tab.id);
       browser.test.sendMessage("requestsCompleted");
     },
   });
 }
--- a/toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_webnavigation.html
@@ -299,17 +299,17 @@ add_task(async function webnav_transitio
 
   found = received.find((data) => (data.event == "onCommitted" && data.url == REDIRECT));
 
   ok(found, "Got the onCommitted event");
 
   if (found) {
     is(found.details.transitionType, "auto_subframe",
        "Got the expected 'auto_subframe' transitionType in the OnCommitted event");
-    // BUG 1264936: currently the server_redirect is not detected in sub-frames
+    // TODO BUG 1264936: currently the server_redirect is not detected in sub-frames
     // once we fix it we can test it here:
     //
     // ok(Array.isArray(found.details.transitionQualifiers) &&
     //    found.details.transitionQualifiers.find((q) => q == "server_redirect"),
     //    "Got the expected 'server_redirect' transitionQualifiers in the OnCommitted events");
   }
 
   // transitionType: manual_subframe
--- a/toolkit/components/extensions/test/mochitest/test_ext_webrequest_background_events.html
+++ b/toolkit/components/extensions/test/mochitest/test_ext_webrequest_background_events.html
@@ -83,17 +83,17 @@ add_task(async function test_webRequest_
         "onCompleted",
       ]);
 
       function listener(name, details) {
         browser.test.assertTrue(eventNames.has(name), `recieved ${name}`);
         eventNames.delete(name);
 
         if (eventNames.size === 0) {
-          browser.test.assertEq("xmlhttprequest", details.type, "correct type for fetch [bug 1366710]");
+          browser.test.assertEq("xmlhttprequest", details.type, "correct type for fetch [see bug 1366710]");
           browser.test.assertEq(0, eventNames.size, "messages recieved");
           browser.test.sendMessage("done");
         }
       }
 
       for (let name of eventNames) {
         browser.webRequest[name].addListener(
           listener.bind(null, name),
--- a/toolkit/components/extensions/test/xpcshell/head_telemetry.js
+++ b/toolkit/components/extensions/test/xpcshell/head_telemetry.js
@@ -20,20 +20,19 @@ function clearHistograms() {
 }
 
 function getSnapshots(process) {
   return Services.telemetry.snapshotHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
                                                true /* subsession */,
                                                false /* clear */)[process];
 }
 
-// There is no good way to make sure that the parent received the histogram
-// entries from the extension and content processes.
-// Let's stick to the ugly, spinning the event loop until we have a good
-// approach (Bug 1357509).
+// TODO Bug 1357509: There is no good way to make sure that the parent received
+// the histogram entries from the extension and content processes.  Let's stick
+// to the ugly, spinning the event loop until we have a good approach.
 function promiseTelemetryRecorded(id, process, expectedCount) {
   let condition = () => {
     let snapshot = Services.telemetry.snapshotHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
                                                          true /* subsession */,
                                                          false /* clear */)[process][id];
     return snapshot && arraySum(snapshot.counts) >= expectedCount;
   };
   return ContentTaskUtils.waitForCondition(condition);
--- a/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js
@@ -696,18 +696,18 @@ add_task(async function test_contentscri
 
 
 /**
  * Tests that the correct CSP is applied to loads of inline content
  * depending on whether the load was initiated by an extension or the
  * content page.
  */
 add_task(async function test_contentscript_csp() {
-  // We currently don't get the full set of CSP reports when running in network
-  // scheduling chaos mode (bug 1408193). It's not entirely clear why.
+  // TODO bug 1408193: We currently don't get the full set of CSP reports when
+  // running in network scheduling chaos mode. It's not entirely clear why.
   let chaosMode = parseInt(env.get("MOZ_CHAOSMODE"), 16);
   let checkCSPReports = !(chaosMode === 0 || chaosMode & 0x02);
 
   gContentSecurityPolicy = `default-src 'none'; script-src 'nonce-deadbeef' 'unsafe-eval'; report-uri ${CSP_REPORT_PATH};`;
 
   let extension = ExtensionTestUtils.loadExtension(EXTENSION_DATA);
   await extension.startup();
 
--- a/toolkit/components/extensions/test/xpcshell/test_ext_downloads_misc.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_downloads_misc.js
@@ -308,17 +308,17 @@ add_task(async function test_cancel() {
   equal(msg.status, "success", "got created and changed events");
 
   await progressPromise;
   do_print(`download reached ${INT_PARTIAL_LEN} bytes`);
 
   msg = await runInExtension("cancel", id);
   equal(msg.status, "success", "cancel() succeeded");
 
-  // This sequence of events is bogus (bug 1256243)
+  // TODO bug 1256243: This sequence of events is bogus
   msg = await runInExtension("waitForEvents", [
     {
       type: "onChanged",
       data: {
         state: {
           previous: "in_progress",
           current: "interrupted",
         },
--- a/toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_native_messaging.js
@@ -495,17 +495,17 @@ add_task(async function test_stderr() {
 
   let lines = STDERR_LINES.map(line => messages.findIndex(msg => msg.message.includes(line)));
   notEqual(lines[0], -1, "Saw first line of stderr output on the console");
   notEqual(lines[1], -1, "Saw second line of stderr output on the console");
   notEqual(lines[0], lines[1], "Stderr output lines are separated in the console");
 });
 
 // Test that calling connectNative() multiple times works
-// (bug 1313980 was a previous regression in this area)
+// (see bug 1313980 for a previous regression in this area)
 add_task(async function test_multiple_connects() {
   async function background() {
     function once() {
       return new Promise(resolve => {
         let MSG = "hello";
         let port = browser.runtime.connectNative("echo");
 
         port.onMessage.addListener(msg => {
--- a/toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_storage_sync.js
@@ -822,19 +822,19 @@ add_task(async function ensureCanSync_ha
 
       deepEqual(afterWipeBody.keys.collections[extensionId], extensionKey.keyPairB64,
                 `decrypted new post should have preserved the key for ${extensionId}`);
     });
   });
 });
 
 add_task(async function ensureCanSync_handles_flushes() {
-  // One of the ways that bug 1359879 presents is as bug 1350088. This
-  // seems to be the symptom that results when the user had two
-  // devices, one of which was not syncing at the time the keyring was
+  // See Bug 1359879 and Bug 1350088. One of the ways that 1359879 presents is
+  // as 1350088. This seems to be the symptom that results when the user had
+  // two devices, one of which was not syncing at the time the keyring was
   // lost. Ensure we can recover for these users as well.
   const extensionId = uuid();
   const extensionId2 = uuid();
   await withContextAndServer(async function(context, server) {
     server.installCollection("storage-sync-crypto");
     server.installDeleteBucket();
     await withSignedInUser(loggedInUser, async function(extensionStorageSync, fxaService) {
       server.etag = 700;