Fix for back/forward issue with httpequiv patch.

After reissuing a cf navigation we now remove the previous entry from the travellog to preserve the correct navigation history.
Also updating the BackForward unit tests to expect two pairs of BeforeNavigate/NavigateComplete notification when we transition from IE to CF.

TEST=Run *BackForward* unit tests
BUG=32550

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

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

3 files changed, 124 insertions, 20 deletions

diff --git a/chrome_frame/bho.cc b/chrome_frame/bho.cc
index 6756e12..f0bd64e 100644
--- a/chrome_frame/bho.cc
+++ b/chrome_frame/bho.cc
@@ -16,6 +16,7 @@
 #include "base/scoped_variant_win.h"
 #include "base/string_util.h"
 #include "chrome_tab.h" // NOLINT
+#include "chrome_frame/extra_system_apis.h"
 #include "chrome_frame/http_negotiate.h"
 #include "chrome_frame/protocol_sink_wrap.h"
 #include "chrome_frame/utils.h"
@@ -201,6 +202,26 @@ bool DocumentHasEmbeddedItems(IUnknown* browser) {
   return has_embedded_items;
 }
 
+HRESULT DeletePreviousNavigationEntry(IBrowserService* browser) {
+  DCHECK(browser);
+
+  ScopedComPtr<ITravelLog> travel_log;
+  HRESULT hr = browser->GetTravelLog(travel_log.Receive());
+  DCHECK(travel_log);
+  if (travel_log) {
+    ScopedComPtr<ITravelLogEx> travel_log_ex;
+    if (SUCCEEDED(hr = travel_log_ex.QueryFrom(travel_log)) ||
+        SUCCEEDED(hr = travel_log.QueryInterface(__uuidof(IIEITravelLogEx),
+            reinterpret_cast<void**>(travel_log_ex.Receive())))) {
+      hr = travel_log_ex->DeleteIndexEntry(browser, -1);
+    } else {
+      NOTREACHED() << "ITravelLogEx";
+    }
+  }
+
+  return hr;
+}
+
 }  // end namespace
 
 HRESULT Bho::OnHttpEquiv(IBrowserService_OnHttpEquiv_Fn original_httpequiv,
@@ -237,6 +258,7 @@ HRESULT Bho::OnHttpEquiv(IBrowserService_OnHttpEquiv_Fn original_httpequiv,
         DCHECK(FAILED(hr) || moniker != NULL);
         if (moniker) {
           DLOG(INFO) << "Navigating in CF";
+
           ScopedComPtr<IBindCtx> bind_context;
           // This bind context will be discarded by IE and a new one
           // constructed, so it's OK to create a sync bind context.
@@ -254,6 +276,12 @@ HRESULT Bho::OnHttpEquiv(IBrowserService_OnHttpEquiv_Fn original_httpequiv,
 
           hr = NavigateBrowserToMoniker(browser, moniker, headers.c_str(),
                                         bind_context);
+
+          if (SUCCEEDED(hr)) {
+            // Now that we've reissued the request, we need to remove the
+            // original one from the travel log.
+            DeletePreviousNavigationEntry(browser);
+          }
         } else {
           DLOG(ERROR) << "Couldn't get the current moniker";
         }
diff --git a/chrome_frame/extra_system_apis.h b/chrome_frame/extra_system_apis.h
index c951c10..765c5b2 100644
--- a/chrome_frame/extra_system_apis.h
+++ b/chrome_frame/extra_system_apis.h
@@ -9,6 +9,7 @@
 #define CHROME_FRAME_EXTRA_SYSTEM_APIS_H_
 
 #include <mshtml.h>
+#include <shdeprecated.h>
 
 // This is an interface provided by the WebBrowser object. It allows us to
 // notify the browser of navigation events. MSDN documents this interface
@@ -83,4 +84,37 @@ IDocObjectService : public IUnknown {
   STDMETHOD(IsErrorUrl)(LPCTSTR url, BOOL* is_error) = 0;
 };
 
+// Travel log interface that supports deleting entries from the travel log.
+// See for more details:
+// http://msdn.microsoft.com/en-us/library/aa511272.aspx
+// http://www.geoffchappell.com/viewer.htm?doc=studies/windows/ie/shdocvw/interfaces/tlog/itravellogex/createenumentry.htm
+// It seems that in IE8 the interface name was changed to IIEITravelLogEx.
+interface __declspec(uuid("3050F679-98B5-11CF-BB82-00AA00BDCE0B"))
+ITravelLogEx : public IUnknown {
+  STDMETHOD(FindTravelEntryWithUrl)(IUnknown* unk, UINT code_page,
+                                    const wchar_t* url,
+                                    ITravelEntry** entry) = 0;
+  STDMETHOD(TravelToUrl)(IUnknown* unk, UINT code_page, const wchar_t* url) = 0;
+  STDMETHOD(DeleteIndexEntry)(IUnknown* unk, int offset) = 0;
+  STDMETHOD(DeleteUrlEntry)(IUnknown* unk, UINT code_page,
+                            const wchar_t* url) = 0;
+  STDMETHOD(CountEntryNodes)(IUnknown* unk, DWORD flags, DWORD* count) = 0;
+  STDMETHOD(CreateEnumEntry)(IUnknown* unk, IEnumTravelLogEntry** entry_enum,
+                             DWORD flags) = 0;
+  STDMETHOD(DeleteEntry)(IUnknown* unk, ITravelLogEntry* entry) = 0;
+  STDMETHOD(InsertEntry)(IUnknown* unk_relative_to,
+                         ITravelLogEntry* entry_relative_to, BOOL prepend,
+                         IUnknown* unk, ITravelLogEntry** entry) = 0;
+  STDMETHOD(TravelToEntry)(IUnknown* unk, ITravelLogEntry* entry) = 0;
+};
+
+interface __declspec(uuid("DD9E2B32-4D78-44F1-B59B-8CA4C9392140"))
+IIEITravelLogEx : public ITravelLogEx {
+};
+
+// Flags for ITravelLogEx::CountEntryNodes, CreateEnumEntry.
+#define TLEF_RELATIVE_INCLUDE_CURRENT (0x01)  // count the current entry
+#define TLEF_RELATIVE_BACK (0x10)             // count backward entries
+#define TLEF_RELATIVE_FORE (0x20)             // count forward entries
+
 #endif  // CHROME_FRAME_EXTRA_SYSTEM_APIS_H_
diff --git a/chrome_frame/test/chrome_frame_unittests.cc b/chrome_frame/test/chrome_frame_unittests.cc
index 23cfd06..ab12d9e 100644
--- a/chrome_frame/test/chrome_frame_unittests.cc
+++ b/chrome_frame/test/chrome_frame_unittests.cc
@@ -1349,21 +1349,18 @@ const wchar_t kChromeFrameAboutVersion[] =
 // that the operation succeeded.
 // Marking this test FLAKY as it fails at times on the buildbot.
 // http://code.google.com/p/chromium/issues/detail?id=26549
-// TODO(tommi): DISABLED due to
-// http://code.google.com/p/chromium/issues/detail?id=32550
-// NOTE - before being disabled, the test was marked FLAKY.
-TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_AboutChromeFrame) {
+TEST_F(ChromeFrameTestWithWebServer, FLAKY_FullTabModeIE_AboutChromeFrame) {
   chrome_frame_test::TimedMsgLoop loop;
   CComObjectStackEx<MockWebBrowserEventSink> mock;
 
   EXPECT_CALL(mock,
-              OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
-                                testing::StrCaseEq(kSubFrameUrl1)),
+      OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
+                                          testing::StrCaseEq(kSubFrameUrl1)),
                                 _, _, _, _, _))
-      .Times(1)
-      .WillOnce(testing::Return(S_OK));
+      .Times(2).WillRepeatedly(testing::Return(S_OK));
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
-      .WillOnce(testing::Return());
+      .Times(2).WillRepeatedly(testing::Return());
+
   EXPECT_CALL(mock, OnLoad(testing::StrEq(kSubFrameUrl1)))
       .WillOnce(testing::InvokeWithoutArgs(
           &chrome_frame_test::ShowChromeFrameContextMenu));
@@ -1371,7 +1368,6 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_AboutChromeFrame) {
   EXPECT_CALL(mock,
               OnNewWindow3(_, _, _, _,
                            testing::StrCaseEq(kChromeFrameAboutVersion)))
-      .Times(1)
       .WillOnce(QUIT_LOOP(loop));
 
   HRESULT hr = mock.LaunchIEAndNavigate(kSubFrameUrl1);
@@ -1427,14 +1423,25 @@ template <typename T> T** ReceivePointer(scoped_refptr<T>& p) {  // NOLINT
 
 // Full tab mode back/forward test
 // Launch and navigate chrome frame to a set of URLs and test back forward
-// TODO(tommi): DISABLED due to
-// http://code.google.com/p/chromium/issues/detail?id=32550
-TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForward) {
+TEST_F(ChromeFrameTestWithWebServer, FullTabModeIE_BackForward) {
   chrome_frame_test::TimedMsgLoop loop;
   CComObjectStackEx<MockWebBrowserEventSink> mock;
   ::testing::InSequence sequence;   // Everything in sequence
 
-  // Navigate to url 2 after the previous navigation is complete
+  // We will get two BeforeNavigate2/OnNavigateComplete2 notifications due to
+  // switching from IE to CF.
+  // Note that when going backwards, we don't expect that since the extra
+  // navigational entries in the travel log should have been removed.
+
+  EXPECT_CALL(mock,
+      OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
+                                          testing::StrCaseEq(kSubFrameUrl1)),
+                                _, _, _, _, _))
+      .WillOnce(testing::Return(S_OK));
+
+  EXPECT_CALL(mock, OnNavigateComplete2(_, _))
+      .WillOnce(testing::Return());
+
   EXPECT_CALL(mock,
       OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
                                           testing::StrCaseEq(kSubFrameUrl1)),
@@ -1443,13 +1450,15 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForward) {
 
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
+
+  // Navigate to url 2 after the previous navigation is complete.
   EXPECT_CALL(mock, OnLoad(testing::StrEq(kSubFrameUrl1)))
-    .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(
+      .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(
           CreateFunctor(
               &mock, &chrome_frame_test::WebBrowserEventSink::Navigate,
               std::wstring(kSubFrameUrl2)))));
 
-  // Navigate to url 3 after the previous navigation is complete
+  // Expect BeforeNavigate/NavigateComplete twice here as well.
   EXPECT_CALL(mock,
       OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
                                           testing::StrCaseEq(kSubFrameUrl2)),
@@ -1458,6 +1467,17 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForward) {
 
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
+
+  EXPECT_CALL(mock,
+      OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
+                                          testing::StrCaseEq(kSubFrameUrl2)),
+                                _, _, _, _, _))
+      .WillOnce(testing::Return(S_OK));
+
+  EXPECT_CALL(mock, OnNavigateComplete2(_, _))
+      .WillOnce(testing::Return());
+
+  // Navigate to url 3 after the previous navigation is complete
   EXPECT_CALL(mock, OnLoad(testing::StrEq(kSubFrameUrl2)))
       .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(
           CreateFunctor(
@@ -1474,6 +1494,17 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForward) {
 
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
+
+  EXPECT_CALL(mock,
+      OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
+                                          testing::StrCaseEq(kSubFrameUrl3)),
+                                _, _, _, _, _))
+      .WillOnce(testing::Return(S_OK));
+
+  EXPECT_CALL(mock, OnNavigateComplete2(_, _))
+      .WillOnce(testing::Return());
+
+  // Go back.
   EXPECT_CALL(mock, OnLoad(testing::StrEq(kSubFrameUrl3)))
       .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(
           CreateFunctor(ReceivePointer(mock.web_browser2_),
@@ -1489,6 +1520,7 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForward) {
 
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
+
   EXPECT_CALL(mock, OnLoad(testing::StrEq(kSubFrameUrl2)))
       .WillOnce(testing::IgnoreResult(testing::InvokeWithoutArgs(
           CreateFunctor(ReceivePointer(mock.web_browser2_),
@@ -1503,6 +1535,7 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForward) {
 
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
+
   EXPECT_CALL(mock, OnLoad(testing::StrEq(kSubFrameUrl1)))
       .WillOnce(testing::DoAll(
           testing::InvokeWithoutArgs(CreateFunctor(&mock,
@@ -1558,16 +1591,19 @@ const wchar_t kAnchor3Url[] = L"http://localhost:1337/files/anchor.html#a3";
 // Launch and navigate chrome frame to a set of URLs and test back forward
 // Marking this test FLAKY as it fails at times on the buildbot.
 // http://code.google.com/p/chromium/issues/detail?id=26549
-// TODO(tommi): DISABLED due to
-// http://code.google.com/p/chromium/issues/detail?id=32550
-// NOTE - before being disabled, the test was marked FLAKY.
-TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForwardAnchor) {
+TEST_F(ChromeFrameTestWithWebServer, FLAKY_FullTabModeIE_BackForwardAnchor) {
   const char tab_enter_keystrokes[] = { VK_TAB, VK_RETURN, 0 };
   static const std::string tab_enter(tab_enter_keystrokes);
   chrome_frame_test::TimedMsgLoop loop;
   CComObjectStackEx<MockWebBrowserEventSink> mock;
   ::testing::InSequence sequence;   // Everything in sequence
 
+  // We will get two BeforeNavigate2/OnNavigateComplete2 notifications due to
+  // switching from IE to CF.
+  // Note that when going backwards, we don't expect that since the extra
+  // navigational entries in the travel log should have been removed.
+  // Same for navigating to anchors within a page that's already loaded.
+
   // Back/Forward state at this point:
   // Back: 0
   // Forward: 0
@@ -1577,6 +1613,12 @@ TEST_F(ChromeFrameTestWithWebServer, DISABLED_FullTabModeIE_BackForwardAnchor) {
       .WillOnce(testing::Return(S_OK));
   EXPECT_CALL(mock, OnNavigateComplete2(_, _))
       .WillOnce(testing::Return());
+  EXPECT_CALL(mock, OnBeforeNavigate2(_, testing::Field(&VARIANT::bstrVal,
+                                      testing::StrCaseEq(kAnchorUrl)),
+                                      _, _, _, _, _))
+      .WillOnce(testing::Return(S_OK));
+  EXPECT_CALL(mock, OnNavigateComplete2(_, _))
+      .WillOnce(testing::Return());
 
   // Navigate to anchor 1:
   // - First set focus to chrome renderer window