Fixed Screenshot extension example

chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js

Index: chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
diff --git a/chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js b/chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
index 4124857248261f40e234d7c8f49e94bfc92cd0b6..1516ac7149156e697408abfdc006eedb9aeebabb 100644
--- a/chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
+++ b/chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
@@ -9,49 +9,43 @@
 // open.
 var id = 100;
 
-function takeScreenshot() {
-  chrome.tabs.captureVisibleTab(null, function(img) {
+// Listen for a click on the camera icon. On that click, take a screenshot.
+chrome.browserAction.onClicked.addListener(function() {
+
+  chrome.tabs.captureVisibleTab(function(img) {
     var screenshotUrl = img;
     var viewTabUrl = chrome.extension.getURL('screenshot.html?id=' + id++)
+    var targetId = null;
 
-    chrome.tabs.create({url: viewTabUrl}, function(tab) {
-      var targetId = tab.id;
-
-      var addSnapshotImageToTab = function(tabId, changedProps) {
-        // We are waiting for the tab we opened to finish loading.
-        // Check that the the tab's id matches the tab we opened,
-        // and that the tab is done loading.
-        if (tabId != targetId || changedProps.status != "complete")
-          return;
+    var addSnapshotImageToTab = function(tabId, changedProps) {
+      // We are waiting for the tab we opened to finish loading.
+      // Check that the tab's id matches the tab we opened,
+      // and that the tab is done loading.
+      if (tabId != targetId || changedProps.status != "complete")
+        return;
 
-        // Passing the above test means this is the event we were waiting for.
-        // There is nothing we need to do for future onUpdated events, so we
-        // use removeListner to stop geting called when onUpdated events fire.
-        chrome.tabs.onUpdated.removeListener(addSnapshotImageToTab);
+      // Passing the above test means this is the event we were waiting for.
+      // There is nothing we need to do for future onUpdated events, so we
+      // use removeListner to stop getting called when onUpdated events fire.
+      chrome.tabs.onUpdated.removeListener(addSnapshotImageToTab);
 
-        // Look through all views to find the window which will display
-        // the screenshot.  The url of the tab which will display the
-        // screenshot includes a query parameter with a unique id, which
-        // ensures that exactly one view will have the matching URL.
-        var views = chrome.extension.getViews();
-        for (var i = 0; i < views.length; i++) {
-          var view = views[i];
-          if (view.location.href == viewTabUrl) {
-            view.setScreenshotUrl(screenshotUrl);
-            break;
-          }
+      // Look through all views to find the window which will display
+      // the screenshot.  The url of the tab which will display the
+      // screenshot includes a query parameter with a unique id, which
+      // ensures that exactly one view will have the matching URL.
+      var views = chrome.extension.getViews();
+      for (var i = 0; i < views.length; i++) {
+        var view = views[i];
+        if (view.location.href == viewTabUrl) {
+          view.setScreenshotUrl(screenshotUrl);
+          break;
         }
-      };
-      chrome.tabs.onUpdated.addListener(addSnapshotImageToTab);
+      }
+    };
+
+    chrome.tabs.onUpdated.addListener(addSnapshotImageToTab);

[not at google - send to devlin, 2014/08/22 14:20:51]

Feel free not to do this, but I've seen a nice pattern of:

chrome.tabs.onUpdated.addListener(function listener() {
  /* check stuff... */

  chrome.tabs.onUpdated.removeListener(listener);

  /* do stuff... */
});

which would avoid needing to have that temporary addSnapshotImageToTab variable.

[François Beaufort, 2014/08/25 07:35:57]

I love this pattern.
Thanks!

+    chrome.tabs.create({url: viewTabUrl}, function(tab) {

[not at google - send to devlin, 2014/08/22 14:20:51]

And also - you can avoid |targetId| if you nest things even more:

chrome.tabs.create({url: viewTabUrl}, function(tab) {
  chrome.tabs.onUpdated.addListener(function listener() {
    /* use tab.id rather than targetId */
  }
});

More nesting, but fewer variables escaping out of their necessary scope. If you
went back to having that separate takeScreenshot() function it might help the
nesting a little.

[François Beaufort, 2014/08/25 07:35:57]

Sadly, adding the listener chrome.tabs.onUpdated after chrome.tabs.create will
always fail the first time we load the extension whereas it works fine after.
I'm not sure why sadly ;(

[not at google - send to devlin, 2014/08/25 16:32:06]

Ah ok. Bummer. I can see why.

+      targetId = tab.id;
     });
   });
-}
-
-// Listen for a click on the camera icon.  On that click, take a screenshot.
-chrome.browserAction.onClicked.addListener(function(tab) {
-  if (tab.url.match("^https?://code.google.com")) {
-    takeScreenshot();
-  } else {
-    alert('This sample can only take screenshots of code.google.com pages');
-  }
 });