FormatErrorMessage() functions are now publicly available from ExtensionErrorUtils.

ExtensionTabsModule implements a bunch of error_messages.

Extension Calls now always deliver a response to the calling context and route error messages if any to the window.console.error log.

Review URL: http://codereview.chromium.org/113105

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@15853 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 224 insertions, 165 deletions

diff --git a/chrome/browser/extensions/extension.cc b/chrome/browser/extensions/extension.cc
index 0e83f3e..993deff 100644
--- a/chrome/browser/extensions/extension.cc
+++ b/chrome/browser/extensions/extension.cc
@@ -11,6 +11,7 @@
 #include "base/string_util.h"
 #include "net/base/net_util.h"
 #include "chrome/browser/extensions/extension_error_reporter.h"
+#include "chrome/browser/extensions/extension_error_utils.h"
 #include "chrome/common/extensions/user_script.h"
 #include "chrome/common/url_constants.h"
 
@@ -210,23 +211,6 @@ FilePath Extension::GetResourcePath(const FilePath& extension_path,
   return ret_val;
 }
 
-// Creates an error messages from a pattern.
-static std::string FormatErrorMessage(const std::string& format,
-                                      const std::string s1) {
-  std::string ret_val = format;
-  ReplaceFirstSubstringAfterOffset(&ret_val, 0, "*", s1);
-  return ret_val;
-}
-
-static std::string FormatErrorMessage(const std::string& format,
-                                      const std::string s1,
-                                      const std::string s2) {
-  std::string ret_val = format;
-  ReplaceFirstSubstringAfterOffset(&ret_val, 0, "*", s1);
-  ReplaceFirstSubstringAfterOffset(&ret_val, 0, "*", s2);
-  return ret_val;
-}
-
 Extension::Extension(const FilePath& path) {
   DCHECK(path.IsAbsolute());
 
@@ -254,8 +238,8 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
   if (content_script->HasKey(kRunAtKey)) {
     std::string run_location;
     if (!content_script->GetString(kRunAtKey, &run_location)) {
-      *error = FormatErrorMessage(kInvalidRunAtError,
-                                  IntToString(definition_index));
+      *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidRunAtError,
+          IntToString(definition_index));
       return false;
     }
 
@@ -264,8 +248,8 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
     } else if (run_location == kRunAtDocumentEndValue) {
       result->set_run_location(UserScript::DOCUMENT_END);
     } else {
-      *error = FormatErrorMessage(kInvalidRunAtError,
-                                  IntToString(definition_index));
+      *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidRunAtError,
+          IntToString(definition_index));
       return false;
     }
   }
@@ -273,30 +257,28 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
   // matches
   ListValue* matches = NULL;
   if (!content_script->GetList(kMatchesKey, &matches)) {
-    *error = FormatErrorMessage(kInvalidMatchesError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidMatchesError,
+        IntToString(definition_index));
     return false;
   }
 
   if (matches->GetSize() == 0) {
-    *error = FormatErrorMessage(kInvalidMatchCountError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidMatchCountError,
+        IntToString(definition_index));
     return false;
   }
   for (size_t j = 0; j < matches->GetSize(); ++j) {
     std::string match_str;
     if (!matches->GetString(j, &match_str)) {
-      *error = FormatErrorMessage(kInvalidMatchError,
-                                  IntToString(definition_index),
-                                  IntToString(j));
+      *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidMatchError,
+          IntToString(definition_index), IntToString(j));
       return false;
     }
 
     URLPattern pattern;
     if (!pattern.Parse(match_str)) {
-      *error = FormatErrorMessage(kInvalidMatchError,
-                                  IntToString(definition_index),
-                                  IntToString(j));
+      *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidMatchError,
+          IntToString(definition_index), IntToString(j));
       return false;
     }
 
@@ -307,23 +289,23 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
   ListValue* js = NULL;
   if (content_script->HasKey(kJsKey) &&
       !content_script->GetList(kJsKey, &js)) {
-    *error = FormatErrorMessage(kInvalidJsListError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidJsListError,
+        IntToString(definition_index));
     return false;
   }
 
   ListValue* css = NULL;
   if (content_script->HasKey(kCssKey) &&
       !content_script->GetList(kCssKey, &css)) {
-    *error = FormatErrorMessage(kInvalidCssListError,
-      IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidCssListError,
+        IntToString(definition_index));
     return false;
   }
 
   // The manifest needs to have at least one js or css user script definition.
   if (((js ? js->GetSize() : 0) + (css ? css->GetSize() : 0)) == 0) {
-    *error = FormatErrorMessage(kMissingFileError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kMissingFileError,
+        IntToString(definition_index));
     return false;
   }
 
@@ -333,9 +315,8 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
       Value* value;
       std::wstring relative;
       if (!js->Get(script_index, &value) || !value->GetAsString(&relative)) {
-        *error = FormatErrorMessage(kInvalidJsError,
-                                    IntToString(definition_index),
-                                    IntToString(script_index));
+        *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidJsError,
+            IntToString(definition_index), IntToString(script_index));
         return false;
       }
       // TODO(georged): Make GetResourceURL accept wstring too
@@ -351,9 +332,8 @@ bool Extension::LoadUserScriptHelper(const DictionaryValue* content_script,
       Value* value;
       std::wstring relative;
       if (!css->Get(script_index, &value) || !value->GetAsString(&relative)) {
-        *error = FormatErrorMessage(kInvalidCssError,
-                                    IntToString(definition_index),
-                                    IntToString(script_index));
+        *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidCssError,
+            IntToString(definition_index), IntToString(script_index));
         return false;
       }
       // TODO(georged): Make GetResourceURL accept wstring too
@@ -377,14 +357,14 @@ PageAction* Extension::LoadPageActionHelper(
   // Read the page action |icon|.
   std::string icon;
   if (!page_action->GetString(kIconPathKey, &icon)) {
-    *error = FormatErrorMessage(kInvalidPageActionIconPathError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(
+        kInvalidPageActionIconPathError, IntToString(definition_index));
     return NULL;
   }
   FilePath icon_path = path_.AppendASCII(icon);
   if (!file_util::PathExists(icon_path)) {
-    *error = FormatErrorMessage(kMissingPageActionIcon,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kMissingPageActionIcon,
+        IntToString(definition_index));
     return NULL;
   }
   result->set_icon_path(icon_path);
@@ -392,7 +372,8 @@ PageAction* Extension::LoadPageActionHelper(
   // Read the page action |id|.
   std::string id;
   if (!page_action->GetString(kIdKey, &id)) {
-    *error = FormatErrorMessage(kInvalidIdError, IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidIdError,
+        IntToString(definition_index));
     return NULL;
   }
   result->set_id(id);
@@ -400,8 +381,8 @@ PageAction* Extension::LoadPageActionHelper(
   // Read the page action |name|.
   std::string name;
   if (!page_action->GetString(kNameKey, &name)) {
-    *error = FormatErrorMessage(kInvalidNameError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidNameError,
+        IntToString(definition_index));
     return NULL;
   }
   result->set_name(name);
@@ -409,8 +390,8 @@ PageAction* Extension::LoadPageActionHelper(
   // Read the page action |tooltip|.
   std::string tooltip;
   if (!page_action->GetString(kTooltipKey, &tooltip)) {
-    *error = FormatErrorMessage(kInvalidPageActionTooltipError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(
+        kInvalidPageActionTooltipError, IntToString(definition_index));
     return NULL;
   }
   result->set_tooltip(tooltip);
@@ -422,8 +403,8 @@ PageAction* Extension::LoadPageActionHelper(
     result->set_type(PageAction::PERMANENT);
   } else if (!LowerCaseEqualsASCII(type, kPageActionTypeTab) &&
              !LowerCaseEqualsASCII(type, kPageActionTypePermanent)) {
-    *error = FormatErrorMessage(kInvalidPageActionTypeValueError,
-                                IntToString(definition_index));
+    *error = ExtensionErrorUtils::FormatErrorMessage(
+        kInvalidPageActionTypeValueError, IntToString(definition_index));
     return NULL;
   } else {
     if (LowerCaseEqualsASCII(type, kPageActionTypeTab))
@@ -614,7 +595,8 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     for (size_t i = 0; i < list_value->GetSize(); ++i) {
       std::string toolstrip;
       if (!list_value->GetString(i, &toolstrip)) {
-        *error = FormatErrorMessage(kInvalidToolstripError, IntToString(i));
+        *error = ExtensionErrorUtils::FormatErrorMessage(kInvalidToolstripError,
+            IntToString(i));
         return false;
       }
       toolstrips_.push_back(toolstrip);
@@ -632,8 +614,8 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     for (size_t i = 0; i < list_value->GetSize(); ++i) {
       DictionaryValue* content_script;
       if (!list_value->GetDictionary(i, &content_script)) {
-        *error = FormatErrorMessage(kInvalidContentScriptError,
-          IntToString(i));
+        *error = ExtensionErrorUtils::FormatErrorMessage(
+            kInvalidContentScriptError, IntToString(i));
         return false;
       }
 
@@ -656,8 +638,8 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     for (size_t i = 0; i < list_value->GetSize(); ++i) {
       DictionaryValue* page_action_value;
       if (!list_value->GetDictionary(i, &page_action_value)) {
-        *error = FormatErrorMessage(kInvalidPageActionError,
-                                    IntToString(i));
+        *error = ExtensionErrorUtils::FormatErrorMessage(
+            kInvalidPageActionError, IntToString(i));
         return false;
       }
 
@@ -673,7 +655,8 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
   if (source.HasKey(kPermissionsKey)) {
     ListValue* hosts = NULL;
     if (!source.GetList(kPermissionsKey, &hosts)) {
-      *error = FormatErrorMessage(kInvalidPermissionsError, "");
+      *error = ExtensionErrorUtils::FormatErrorMessage(
+          kInvalidPermissionsError, "");
       return false;
     }
 
@@ -685,23 +668,23 @@ bool Extension::InitFromValue(const DictionaryValue& source, bool require_id,
     for (size_t i = 0; i < hosts->GetSize(); ++i) {
       std::string host_str;
       if (!hosts->GetString(i, &host_str)) {
-        *error = FormatErrorMessage(kInvalidPermissionError,
-                                    IntToString(i));
+        *error = ExtensionErrorUtils::FormatErrorMessage(
+            kInvalidPermissionError, IntToString(i));
         return false;
       }
 
       URLPattern pattern;
       if (!pattern.Parse(host_str)) {
-        *error = FormatErrorMessage(kInvalidPermissionError,
-                                    IntToString(i));
+        *error = ExtensionErrorUtils::FormatErrorMessage(
+            kInvalidPermissionError, IntToString(i));
         return false;
       }
 
       // Only accept http/https persmissions at the moment.
       if ((pattern.scheme() != chrome::kHttpScheme) &&
           (pattern.scheme() != chrome::kHttpsScheme)) {
-        *error = FormatErrorMessage(kInvalidPermissionSchemeError,
-                                    IntToString(i));
+        *error = ExtensionErrorUtils::FormatErrorMessage(
+            kInvalidPermissionSchemeError, IntToString(i));
         return false;
       }
 
diff --git a/chrome/browser/extensions/extension_error_utils.cc b/chrome/browser/extensions/extension_error_utils.cc
new file mode 100755
index 0000000..c43587b
--- /dev/null
+++ b/chrome/browser/extensions/extension_error_utils.cc
@@ -0,0 +1,25 @@
+// Copyright (c) 2009 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_error_utils.h"
+
+#include "base/string_util.h"
+
+std::string ExtensionErrorUtils::FormatErrorMessage(
+    const std::string& format,
+    const std::string s1) {
+  std::string ret_val = format;
+  ReplaceFirstSubstringAfterOffset(&ret_val, 0, "*", s1);
+  return ret_val;
+}
+
+std::string ExtensionErrorUtils::FormatErrorMessage(
+    const std::string& format,
+    const std::string s1,
+    const std::string s2) {
+  std::string ret_val = format;
+  ReplaceFirstSubstringAfterOffset(&ret_val, 0, "*", s1);
+  ReplaceFirstSubstringAfterOffset(&ret_val, 0, "*", s2);
+  return ret_val;
+}
diff --git a/chrome/browser/extensions/extension_error_utils.h b/chrome/browser/extensions/extension_error_utils.h
new file mode 100755
index 0000000..1d7283a
--- /dev/null
+++ b/chrome/browser/extensions/extension_error_utils.h
@@ -0,0 +1,24 @@
+// Copyright (c) 2006-2009 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UTILS_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_ERROR_UTILS_H_
+
+#include <string>
+
+class ExtensionErrorUtils {
+public:
+  // Creates an error messages from a pattern. Places first instance if "*"
+  // with |s1|.
+  static std::string FormatErrorMessage(const std::string& format,
+    const std::string s1);
+
+  // Creates an error messages from a pattern. Places first instance if "*"
+  // with |s1| and second instance of "*" with |s2|.
+  static std::string FormatErrorMessage(const std::string& format,
+    const std::string s1,
+    const std::string s2);
+};
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_FORMAT_MESSAGE_UTILS_H_
diff --git a/chrome/browser/extensions/extension_function.cc b/chrome/browser/extensions/extension_function.cc
index 6b22998..bfc2f2c 100644
--- a/chrome/browser/extensions/extension_function.cc
+++ b/chrome/browser/extensions/extension_function.cc
@@ -10,14 +10,8 @@
 void ExtensionFunction::SendResponse(bool success) {
   if (bad_message_) {
     dispatcher_->HandleBadMessage(this);
-  } else if (success) {
-    if (has_callback()) {
-      dispatcher_->SendResponse(this);
-    }
   } else {
-    // TODO(aa): In case of failure, send the error message to an error
-    // callback.
-    LOG(WARNING) << error_;
+    dispatcher_->SendResponse(this, success);
   }
 }
 
@@ -28,4 +22,3 @@ std::string ExtensionFunction::extension_id() {
 Profile* ExtensionFunction::profile() {
   return dispatcher_->profile();
 }
-
diff --git a/chrome/browser/extensions/extension_function.h b/chrome/browser/extensions/extension_function.h
index af1fc6e..b9074b7 100644
--- a/chrome/browser/extensions/extension_function.h
+++ b/chrome/browser/extensions/extension_function.h
@@ -33,16 +33,14 @@ class ExtensionFunction {
   }
   void set_args(Value* args) { args_ = args; }
 
-  void set_callback_id(int callback_id) { callback_id_ = callback_id; }
-  int callback_id() { return callback_id_; }
+  void set_request_id(int request_id) { request_id_ = request_id; }
+  int request_id() { return request_id_; }
 
   Value* result() { return result_.get(); }
   const std::string& error() { return error_; }
 
-  // Whether the extension has registered a callback and is waiting for a
-  // response. APIs can use this to avoid doing unnecessary work in the case
-  // that the extension is not expecting a response.
-  bool has_callback() { return callback_id_ != -1; }
+  void set_has_callback(bool has_callback) { has_callback_ = has_callback; } 
+  bool has_callback() { return has_callback_; }
 
   // Execute the API. Clients should call set_args() and set_callback_id()
   // before calling this method. Derived classes should populate result_ and
@@ -75,8 +73,12 @@ class ExtensionFunction {
   ExtensionFunctionDispatcher* dispatcher_;
 
  private:
-  // Id of js function to callback upon completion. -1 represents no callback.
-  int callback_id_;
+  // Id of this request, used to map the response back to the caller.
+  int request_id_;
+
+  // True if the js caller provides a callback function to receive the response
+  // of this call.
+  bool has_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ExtensionFunction);
 };
diff --git a/chrome/browser/extensions/extension_function_dispatcher.cc b/chrome/browser/extensions/extension_function_dispatcher.cc
index b303778..bdc930a 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.cc
+++ b/chrome/browser/extensions/extension_function_dispatcher.cc
@@ -57,8 +57,8 @@ FactoryRegistry::FactoryRegistry() {
   factories_["GetWindow"] = &NewExtensionFunction<GetWindowFunction>;
   factories_["GetCurrentWindow"] =
       &NewExtensionFunction<GetCurrentWindowFunction>;
-  factories_["GetFocusedWindow"] =
-      &NewExtensionFunction<GetFocusedWindowFunction>;
+  factories_["GetLastFocusedWindow"] =
+      &NewExtensionFunction<GetLastFocusedWindowFunction>;
   factories_["GetAllWindows"] = &NewExtensionFunction<GetAllWindowsFunction>;
   factories_["CreateWindow"] = &NewExtensionFunction<CreateWindowFunction>;
   factories_["RemoveWindow"] = &NewExtensionFunction<RemoveWindowFunction>;
@@ -140,7 +140,8 @@ Browser* ExtensionFunctionDispatcher::GetBrowser() {
 
 void ExtensionFunctionDispatcher::HandleRequest(const std::string& name,
                                                 const std::string& args,
-                                                int callback_id) {
+                                                int request_id,
+                                                bool has_callback) {
   scoped_ptr<Value> value;
   if (!args.empty()) {
     JSONReader reader;
@@ -160,18 +161,21 @@ void ExtensionFunctionDispatcher::HandleRequest(const std::string& name,
       FactoryRegistry::instance()->NewFunction(name));
   function->set_dispatcher(this);
   function->set_args(value.get());
-  function->set_callback_id(callback_id);
+  function->set_request_id(request_id);
+  function->set_has_callback(has_callback);
   function->Run();
 }
 
-void ExtensionFunctionDispatcher::SendResponse(ExtensionFunction* function) {
+void ExtensionFunctionDispatcher::SendResponse(ExtensionFunction* function,
+                                               bool success) {
   std::string json;
 
   // Some functions might not need to return any results.
-  if (function->result())
+  if (success && function->result())
     JSONWriter::Write(function->result(), false, &json);
 
-  render_view_host_->SendExtensionResponse(function->callback_id(), json);
+  render_view_host_->SendExtensionResponse(function->request_id(), success,
+      json, function->error());
 }
 
 void ExtensionFunctionDispatcher::HandleBadMessage(ExtensionFunction* api) {
diff --git a/chrome/browser/extensions/extension_function_dispatcher.h b/chrome/browser/extensions/extension_function_dispatcher.h
index 43d2a36..fed54791 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.h
+++ b/chrome/browser/extensions/extension_function_dispatcher.h
@@ -35,10 +35,10 @@ class ExtensionFunctionDispatcher {
 
   // Handle a request to execute an extension function.
   void HandleRequest(const std::string& name, const std::string& args,
-                     int callback_id);
+                     int request_id, bool has_callback);
 
   // Send a response to a function.
-  void SendResponse(ExtensionFunction* api);
+  void SendResponse(ExtensionFunction* api, bool success);
 
   // Gets the browser extension functions should operate relative to. For
   // example, for positioning windows, or alert boxes, or creating tabs.
diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc
index 39066c3..6e61976 100644
--- a/chrome/browser/extensions/extension_tabs_module.cc
+++ b/chrome/browser/extensions/extension_tabs_module.cc
@@ -7,6 +7,7 @@
 #include "base/string_util.h"
 #include "chrome/browser/browser.h"
 #include "chrome/browser/browser_list.h"
+#include "chrome/browser/extensions/extension_error_utils.h"
 #include "chrome/browser/extensions/extension_function_dispatcher.h"
 #include "chrome/browser/renderer_host/render_view_host_delegate.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
@@ -33,13 +34,29 @@ const wchar_t* kTopKey = L"top";
 const wchar_t* kWidthKey = L"width";
 const wchar_t* kHeightKey = L"height";
 const wchar_t* kTabsKey = L"tabs";
+
+// errors
+const char* kWindowNotFoundError = "No window with id: *.";
+const char* kTabNotFoundError = "No tab with id: *.";
+const char* kInvalidUrlError = "Invalid url: \"*\".";
 }
 
 // Forward declare static helper functions defined below.
 static DictionaryValue* CreateWindowValue(Browser* browser, bool populate_tabs);
 static ListValue* CreateTabList(Browser* browser);
+
+// |error_message| can optionally be passed in a will be set with an appropriate
+// message if the window cannot be found by id.
 static Browser* GetBrowserInProfileWithId(Profile* profile,
-                                          const int window_id);
+                                          const int window_id,
+                                          std::string* error_message);
+
+// |error_message| can optionally be passed in a will be set with an appropriate
+// message if the tab cannot be found by id.
+static bool GetTabById(int tab_id, Profile* profile, Browser** browser,
+                       TabStripModel** tab_strip,
+                       TabContents** contents,
+                       int* tab_index, std::string* error_message);
 
 // ExtensionTabUtil
 int ExtensionTabUtil::GetWindowId(const Browser* browser) {
@@ -123,35 +140,30 @@ bool ExtensionTabUtil::GetTabById(int tab_id, Profile* profile,
   return false;
 }
 
-static bool GetWindowFunctionHelper(Browser* browser, Profile* profile,
-                            scoped_ptr<Value>* result) {
-  // TODO(rafaelw): need "not found" error message.
-  if (browser == NULL || browser->profile() != profile)
-    return false;
-
-  result->reset(CreateWindowValue(browser, false));
-
-  return true;
-}
-
 // Windows ---------------------------------------------------------------------
 
 bool GetWindowFunction::RunImpl() {
   int window_id;
   EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&window_id));
 
-  Browser* target = GetBrowserInProfileWithId(profile(), window_id);
-  return GetWindowFunctionHelper(target, profile(), &result_);
+  Browser* browser = GetBrowserInProfileWithId(profile(), window_id, &error_);
+  if (!browser)
+    return false;
+
+  result_.reset(CreateWindowValue(browser, false));
+  return true;
 }
 
 bool GetCurrentWindowFunction::RunImpl() {
-  return GetWindowFunctionHelper(dispatcher_->GetBrowser(), profile(),
-                                 &result_);
+  Browser* browser = dispatcher_->GetBrowser();
+  result_.reset(CreateWindowValue(browser, false));
+  return true;
 }
 
-bool GetFocusedWindowFunction::RunImpl() {
-  return GetWindowFunctionHelper(BrowserList::GetLastActive(), profile(),
-      &result_);
+bool GetLastFocusedWindowFunction::RunImpl() {
+  Browser* browser = BrowserList::GetLastActiveWithProfile(profile());
+  result_.reset(CreateWindowValue(browser, false));
+  return true;
 }
 
 bool GetAllWindowsFunction::RunImpl() {
@@ -185,7 +197,8 @@ bool CreateWindowFunction::RunImpl() {
       EXTENSION_FUNCTION_VALIDATE(args->GetString(kUrlKey, &url_input));
       url.reset(new GURL(url_input));
       if (!url->is_valid()) {
-        // TODO(rafaelw): need error message/callback here
+        error_ = ExtensionErrorUtils::FormatErrorMessage(kInvalidUrlError,
+            url_input);
         return false;
       }
     }
@@ -244,24 +257,11 @@ bool RemoveWindowFunction::RunImpl() {
   int window_id;
   EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&window_id));
 
-  Browser* target = NULL;
-  for (BrowserList::const_iterator browser = BrowserList::begin();
-      browser != BrowserList::end(); ++browser) {
-    // Only examine browsers in the current profile.
-    if ((*browser)->profile() == profile()) {
-      if (ExtensionTabUtil::GetWindowId(*browser) == window_id) {
-        target = *browser;
-        break;
-      }
-    }
-  }
-
-  if (target == NULL) {
-    // TODO(rafaelw): need error message.
+  Browser* browser = GetBrowserInProfileWithId(profile(), window_id, &error_);
+  if (!browser)
     return false;
-  }
 
-  target->CloseWindow();
+  browser->CloseWindow();
 
   return true;
 }
@@ -269,12 +269,15 @@ bool RemoveWindowFunction::RunImpl() {
 // Tabs ---------------------------------------------------------------------
 
 bool GetSelectedTabFunction::RunImpl() {
-  int window_id;
   Browser* browser;
   // windowId defaults to "current" window.
+  int window_id = -1;
+
   if (!args_->IsType(Value::TYPE_NULL)) {
     EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&window_id));
-    browser = GetBrowserInProfileWithId(profile(), window_id);
+    browser = GetBrowserInProfileWithId(profile(), window_id, &error_);
+    if (!browser)
+      return false;
   } else {
     browser = dispatcher_->GetBrowser();
   }
@@ -288,12 +291,14 @@ bool GetSelectedTabFunction::RunImpl() {
 }
 
 bool GetAllTabsInWindowFunction::RunImpl() {
-  int window_id;
   Browser* browser;
   // windowId defaults to "current" window.
+  int window_id = -1;
   if (!args_->IsType(Value::TYPE_NULL)) {
     EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&window_id));
-    browser = GetBrowserInProfileWithId(profile(), window_id);
+    browser = GetBrowserInProfileWithId(profile(), window_id, &error_);
+    if (!browser)
+      return false;
   } else {
     browser = dispatcher_->GetBrowser();
   }
@@ -307,14 +312,16 @@ bool CreateTabFunction::RunImpl() {
   EXTENSION_FUNCTION_VALIDATE(args_->IsType(Value::TYPE_DICTIONARY));
   const DictionaryValue* args = static_cast<const DictionaryValue*>(args_);
 
-  int window_id;
-  Browser* browser;
+  Browser *browser;
   // windowId defaults to "current" window.
+  int window_id = -1;
   if (args->HasKey(kWindowIdKey)) {
     EXTENSION_FUNCTION_VALIDATE(args->GetInteger(kWindowIdKey, &window_id));
-    browser = GetBrowserInProfileWithId(profile(), window_id);
+    browser = GetBrowserInProfileWithId(profile(), window_id, &error_);
+    if (!browser)
+      return false;
   } else {
-    browser = dispatcher_->GetBrowser();
+      browser = dispatcher_->GetBrowser();
   }
 
   TabStripModel* tab_strip = browser->tabstrip_model();
@@ -328,9 +335,11 @@ bool CreateTabFunction::RunImpl() {
   if (args->HasKey(kUrlKey)) {
     EXTENSION_FUNCTION_VALIDATE(args->GetString(kUrlKey, &url_string));
     url.reset(new GURL(url_string));
-    // TODO(rafaelw): return an "invalid url" error.
-    if (!url->is_valid())
+    if (!url->is_valid()) {
+      error_ = ExtensionErrorUtils::FormatErrorMessage(kInvalidUrlError,
+          url_string);
       return false;
+    }
   }
 
   // Default to foreground for the new tab. The presence of 'selected' property
@@ -371,9 +380,8 @@ bool GetTabFunction::RunImpl() {
   TabStripModel* tab_strip = NULL;
   TabContents* contents = NULL;
   int tab_index = -1;
-  // TODO(rafaelw): return "tab_id not found" error.
-  if (!ExtensionTabUtil::GetTabById(tab_id, profile(), NULL, &tab_strip,
-                                    &contents, &tab_index))
+  if (!GetTabById(tab_id, profile(), NULL, &tab_strip, &contents, &tab_index,
+      &error_))
     return false;
 
   result_.reset(ExtensionTabUtil::CreateTabValue(contents, tab_strip,
@@ -392,9 +400,8 @@ bool UpdateTabFunction::RunImpl() {
   TabStripModel* tab_strip = NULL;
   TabContents* contents = NULL;
   int tab_index = -1;
-  // TODO(rafaelw): return "tab_id not found" error.
-  if (!ExtensionTabUtil::GetTabById(tab_id, profile(), NULL, &tab_strip,
-                                    &contents, &tab_index))
+  if (!GetTabById(tab_id, profile(), NULL, &tab_strip, &contents, &tab_index,
+      &error_))
     return false;
 
   NavigationController& controller = contents->controller();
@@ -409,9 +416,10 @@ bool UpdateTabFunction::RunImpl() {
     EXTENSION_FUNCTION_VALIDATE(update_props->GetString(kUrlKey, &url));
     GURL new_gurl(url);
 
-    // TODO(rafaelw): return "invalid url" here.
-    if (!new_gurl.is_valid())
+    if (!new_gurl.is_valid()) {
+      error_ = ExtensionErrorUtils::FormatErrorMessage(kInvalidUrlError, url);
       return false;
+    }
 
     controller.LoadURL(new_gurl, GURL(), PageTransition::LINK);
   }
@@ -445,19 +453,18 @@ bool MoveTabFunction::RunImpl() {
   Browser* source_browser = NULL;
   TabStripModel* source_tab_strip = NULL;
   int tab_index = -1;
-  // TODO(rafaelw): return "tab_id not found" error.
-  if (!ExtensionTabUtil::GetTabById(tab_id, profile(), &source_browser,
-                                    &source_tab_strip, NULL,
-                                    &tab_index))
+  if (!GetTabById(tab_id, profile(), &source_browser, &source_tab_strip, NULL,
+      &tab_index, &error_))
     return false;
 
   if (update_props->HasKey(kWindowIdKey)) {
+    Browser* target_browser;
     int window_id;
     EXTENSION_FUNCTION_VALIDATE(update_props->GetInteger(kWindowIdKey,
         &window_id));
-    Browser* target_browser = GetBrowserInProfileWithId(profile(), window_id);
-    // TODO(rafaelw): return "window_id not found" error.
-    if (!target_browser) 
+    target_browser = GetBrowserInProfileWithId(profile(), window_id,
+        &error_);
+    if (!target_browser)
       return false;
 
     // If windowId is different from the current window, move between windows.
@@ -465,10 +472,12 @@ bool MoveTabFunction::RunImpl() {
         ExtensionTabUtil::GetWindowId(source_browser)) {
       TabStripModel* target_tab_strip = target_browser->tabstrip_model();
       TabContents *contents = source_tab_strip->DetachTabContentsAt(tab_index);
-      // TODO(rafaelw): return a "tab not found" error.
-      if (!contents)
+      if (!contents) {
+        error_ = ExtensionErrorUtils::FormatErrorMessage(
+            kTabNotFoundError, IntToString(tab_id));
         return false;
-      
+      }
+
       // Clamp move location to the last position.
       // This is ">" because it can append to a new index position.
       if (new_index > target_tab_strip->count())
@@ -478,15 +487,15 @@ bool MoveTabFunction::RunImpl() {
           false, true);
 
       return true;
-    } 
+    }
   }
- 
+
   // Perform a simple within-window move.
   // Clamp move location to the last position.
   // This is ">=" because the move must be to an existing location.
   if (new_index >= source_tab_strip->count())
     new_index = source_tab_strip->count() - 1;
-    
+
   if (new_index != tab_index)
     source_tab_strip->MoveTabContentsAt(tab_index, new_index, false);
 
@@ -495,16 +504,12 @@ bool MoveTabFunction::RunImpl() {
 
 
 bool RemoveTabFunction::RunImpl() {
-  // TODO(rafaelw): This should have a callback, but it can't because it could
-  // close it's own tab.
   int tab_id;
   EXTENSION_FUNCTION_VALIDATE(args_->GetAsInteger(&tab_id));
 
   Browser* browser = NULL;
   TabContents* contents = NULL;
-  // TODO(rafaelw): return "tab_id not found" error.
-  if (!ExtensionTabUtil::GetTabById(tab_id, profile(), &browser, NULL,
-                                    &contents, NULL))
+  if (!GetTabById(tab_id, profile(), &browser, NULL, &contents, NULL, &error_))
     return false;
 
   browser->CloseTabContents(contents);
@@ -512,6 +517,7 @@ bool RemoveTabFunction::RunImpl() {
 }
 
 // static helpers
+
 // if |populate| is true, each window gets a list property |tabs| which contains
 // fully populated tab objects.
 static DictionaryValue* CreateWindowValue(Browser* browser,
@@ -546,12 +552,34 @@ static ListValue* CreateTabList(Browser* browser) {
 }
 
 static Browser* GetBrowserInProfileWithId(Profile* profile,
-                                          const int window_id) {
+                                          const int window_id,
+                                          std::string* error_message) {
   for (BrowserList::const_iterator browser = BrowserList::begin();
       browser != BrowserList::end(); ++browser) {
     if ((*browser)->profile() == profile &&
         ExtensionTabUtil::GetWindowId(*browser) == window_id)
       return *browser;
   }
+
+  if (error_message)
+    *error_message= ExtensionErrorUtils::FormatErrorMessage(
+        kWindowNotFoundError, IntToString(window_id));
+
   return NULL;
 }
+
+static bool GetTabById(int tab_id, Profile* profile, Browser** browser,
+                       TabStripModel** tab_strip,
+                       TabContents** contents,
+                       int* tab_index,
+                       std::string* error_message) {
+  if (ExtensionTabUtil::GetTabById(tab_id, profile, browser, tab_strip,
+                                   contents, tab_index))
+    return true;
+  
+  if (error_message)
+    *error_message = ExtensionErrorUtils::FormatErrorMessage(
+        kTabNotFoundError, IntToString(tab_id));
+    
+  return false;
+}
diff --git a/chrome/browser/extensions/extension_tabs_module.h b/chrome/browser/extensions/extension_tabs_module.h
index 04f884f..b8763fc 100644
--- a/chrome/browser/extensions/extension_tabs_module.h
+++ b/chrome/browser/extensions/extension_tabs_module.h
@@ -36,7 +36,7 @@ class GetWindowFunction : public SyncExtensionFunction {
 class GetCurrentWindowFunction : public SyncExtensionFunction {
   virtual bool RunImpl();
 };
-class GetFocusedWindowFunction : public SyncExtensionFunction {
+class GetLastFocusedWindowFunction : public SyncExtensionFunction {
   virtual bool RunImpl();
 };
 class GetAllWindowsFunction : public SyncExtensionFunction {