Mark create/update/clear callbacks of notification API as optional

extensions/renderer/resources/last_error.js

 // Copyright 2014 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.
 
 var GetAvailability = requireNative('v8_context').GetAvailability;
 var GetGlobal = requireNative('sendRequest').GetGlobal;
 
 // Utility for setting chrome.*.lastError.
 //
 // A utility here is useful for two reasons:
@@ skipping 95 matching lines @@
  *
  * The target chrome object is the global object's of the callback, so this
  * method won't work if the real callback has been wrapped (etc).
  */
 function run(name, message, stack, callback, args) {
   var targetChrome = GetGlobal(callback).chrome;
   set(name, message, stack, targetChrome);
   try {
     $Function.apply(callback, undefined, args);
   } finally {
+    if (!hasAccessed(targetChrome))

[not at google - send to devlin, 2015/01/20 17:41:35]

This doesn't look right, it'll always print an error no matter whether the
function succeeded or failed.

[robwu, 2015/01/20 22:34:01]

This is correct. Function run sets chrome.runtime.lastError, which should be
printed to the console if the user did not check its value.

[not at google - send to devlin, 2015/01/21 22:21:19]

What am I missing here? If I do:

chrome.someNamespace.someFunction.get(function() {
  // code
});

and that implementation uses lastError.run (most don't), then if it *succeeds*
with an unchecked error then i'll still print "Unchecked runtime.lastError...".

[robwu, 2015/01/21 23:21:09]

callback is a user-supplied callback. That "// code" ought to read the
chrome.runtime.lastError property. If the code does not, then we have to assume
that the developer does not know about the error and print the error to the
console.

[not at google - send to devlin, 2015/01/21 23:24:32]

but if there isn't an error, it'll still print a non-existent error.

[robwu, 2015/01/21 23:28:18]

A few lines earlier, chrome.runtime.lastError is set (set(name, message, ...)).
That's the whole point of calling this function, not? So, if lastError is not
checked, there is certainly a real error that needs to be reported.

+      reportUncheckedLastError(name, message, stack);
     clear(targetChrome);
   }
 }
 
+/**
+ * Call this method when runtime.lastError was set but not checked.
+ * This notifies the developer of the error via the (error) console.
+ *
+ * @param {string=} name - name of API.
+ * @param {string} message - error description.
+ * @param {string=} stack - Stack trace of the call up to the error.
+ */
+function reportUncheckedLastError(name, message, stack) {

[not at google - send to devlin, 2015/01/20 17:41:35]

Given that this method is under the "lastError" namespace, "reportUnchecked"
would be more accurate. Also, I think it should assert that there actually is a
last error at this point.

[robwu, 2015/01/20 22:34:01]

If we couple this method with the chrome object, then we'd better use the
following instead, to make sure that the reported error is consistent to what's
assigned to lastError:

function reportIfUnchecked(name, targetChrome, stack) {
  var error = hasAccessed(targetChrome) && chrome.runtime.lastError;
  if (!error)
    return;
  console.error("Unchecked runtime.lastError while running " +
      (name || "unknown") + ": " + error.message + (stack ? "\n" + stack : ""));
}

[robwu, 2015/01/21 23:21:09]

Done.

+  console.error("Unchecked runtime.lastError while running " +
+      (name || "unknown") + ": " + message + (stack ? "\n" + stack : ""));
+}
+
 exports.clear = clear;
 exports.hasAccessed = hasAccessed;
 exports.hasError = hasError;
 exports.set = set;
 exports.run = run;
+exports.reportUncheckedLastError = reportUncheckedLastError;