More Blimp browser tests for navigation

blimp/engine/browser_tests/engine_browsertest.cc

 // Copyright 2016 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 "base/memory/ptr_util.h"
 #include "blimp/client/core/contents/ime_feature.h"
 #include "blimp/client/core/contents/mock_ime_feature_delegate.h"
 #include "blimp/client/core/contents/mock_navigation_feature_delegate.h"
 #include "blimp/client/core/contents/navigation_feature.h"
 #include "blimp/client/core/contents/tab_control_feature.h"
@@ skipping 97 matching lines @@
 };
 
 IN_PROC_BROWSER_TEST_F(EngineBrowserTest, LoadUrl) {
   EXPECT_CALL(client_rw_feature_delegate_, OnRenderWidgetCreated(1));
   ExpectPageLoad();
   NavigateToLocalUrl("/page1.html");
   RunAndVerify();
   EXPECT_EQ("page1", last_page_title_);
 }
 
-IN_PROC_BROWSER_TEST_F(EngineBrowserTest, GoBack) {
+IN_PROC_BROWSER_TEST_F(EngineBrowserTest, Reload) {
   ExpectPageLoad();
   NavigateToLocalUrl("/page1.html");
   RunAndVerify();
+  EXPECT_EQ("page1", last_page_title_);

[Kevin M, 2016/08/29 23:31:52]

Turn these into named constants that are declared at the top, e.g. kPage1,
kPageTitle1

[Brian Goldman, 2016/08/29 23:38:16]

This might just be a standard Chromium test style thing, but I have to ask: why?
The constant names wouldn't be any shorter or more clear.

[Kevin M, 2016/08/29 23:41:30]

It is a general styleguide rule in Chromium/Google. Typos in named values are
statically caught by the compiler, whereas accidentally mangled literals lead to
headaches at runtime.

[Brian Goldman, 2016/08/30 00:43:07]

Done.

+
+  ExpectPageLoad();
+  client_session_->GetNavigationFeature()->Reload(kDummyTabId);
+  RunAndVerify();
+  EXPECT_EQ("page1", last_page_title_);
+}
+
+IN_PROC_BROWSER_TEST_F(EngineBrowserTest, GoBackAndGoForward) {
+  ExpectPageLoad();
+  NavigateToLocalUrl("/page1.html");
+  RunAndVerify();
   EXPECT_EQ("page1", last_page_title_);
 
   ExpectPageLoad();
   NavigateToLocalUrl("/page2.html");
   RunAndVerify();
   EXPECT_EQ("page2", last_page_title_);
 
   ExpectPageLoad();
   client_session_->GetNavigationFeature()->GoBack(kDummyTabId);
   RunAndVerify();
   EXPECT_EQ("page1", last_page_title_);
+
+  ExpectPageLoad();
+  client_session_->GetNavigationFeature()->GoForward(kDummyTabId);
+  RunAndVerify();
+  EXPECT_EQ("page2", last_page_title_);
 }
 

[Kevin M, 2016/08/29 23:31:52]

Would it make sense to also test going back with an empty stack, or going
forward w/o first going back?

[Brian Goldman, 2016/08/29 23:38:16]

I'm almost certain that right now in Blimp if the engine gets a go-back request
with an empty stack, it hits a DCHECK and dies. Forward might be the same; I
haven't looked. I could do a separate CL to add a non-crash behavior (no-op?)
and a browser test.

[Kevin M, 2016/08/29 23:41:30]

OK, could you investigate it for potentially adding to this CL? My preference
would be to not have super fine-grained CLs.

[Brian Goldman, 2016/08/30 00:43:07]

Done.

 }  // namespace
 }  // namespace blimp