Updated comments around create tab API.

I have investigated the bug in great detail. My previous fix is entirely
correct and complete. Removed the TODO, updated the comment to give a
better explanation, will close issue upon review.

BUG=66864
TEST=none

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

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

2 files changed, 13 insertions, 7 deletions

diff --git a/ceee/ie/broker/tab_api_module.cc b/ceee/ie/broker/tab_api_module.cc
index f1fbba4..d067952 100644
--- a/ceee/ie/broker/tab_api_module.cc
+++ b/ceee/ie/broker/tab_api_module.cc
@@ -970,10 +970,8 @@ void CreateTab::Execute(const ListValue& args, int request_id) {
     HRESULT hr = executor->Navigate(base::win::ScopedBstr(url_wstring.c_str()),
                                     flags, base::win::ScopedBstr(L"_blank"));
     if (FAILED(hr)) {
-      // Log the error without DCHECKING There are legit reasons for Navigate
+      // Log the error without DCHECKING. There are legit reasons for Navigate
       // to fail, as explained in comments in CeeeExecutor::Navigate.
-      // TODO(motek@chromium.org) See why exactly we fail here in some
-      // integration tests.
       LOG(ERROR) << "Failed to create tab. " << com::LogHr(hr);
       result->PostError("Internal error while trying to create tab.");
       return;
diff --git a/ceee/ie/plugin/bho/executor.cc b/ceee/ie/plugin/bho/executor.cc
index 00571d4..29c58b5 100644
--- a/ceee/ie/plugin/bho/executor.cc
+++ b/ceee/ie/plugin/bho/executor.cc
@@ -791,13 +791,21 @@ STDMETHODIMP CeeeExecutor::Navigate(BSTR url, long flags, BSTR target) {
 
   hr = tab_browser->Navigate(url, &CComVariant(flags), &CComVariant(target),
                              &CComVariant(), &CComVariant());
-  // We don't DCHECK here since there are cases where we get an error
-  // 0x800700aa "The requested resource is in use." if the main UI
+  // We don't DCHECK here since we found out that Navigate has at least
+  // two legitimate failure modes we can do nothing about. They are not
+  // entirely unexpected, even though they are rare:
+  // 1. 0x800700aa "The requested resource is in use." if the main UI
   // thread is currently blocked... and sometimes... it is blocked by
   // us... if we are too slow to respond (e.g. because too busy
   // navigating when the user performs an extension action that causes
-  // navigation again and again and again)... So we might as well
-  // abandon ship and let the UI thread be happy...
+  // navigation again and again and again). So we might as well
+  // abandon ship and let the UI thread be happy.
+  // 2. When the window is not in the state where it can navigate, E_FAIL is
+  // returned. For instance, when a modal message box is displayed (IE7/8
+  // security warning, script error etc.).
+  // TODO(motek@chromium.org): find a method of figuring out the state of
+  // the tab_browser (preferably prior to invoking the function) and DCHECK if
+  // we are indeed hitting one of these predetermined conditions.
   LOG_IF(ERROR, FAILED(hr)) << "Failed to navigate tab: " << hwnd_ <<
        " to " << url << ". " << com::LogHr(hr);
   return hr;