More Blimp browser tests for navigation

blimp/engine/browser_tests/engine_browsertest.cc

Index: blimp/engine/browser_tests/engine_browsertest.cc
diff --git a/blimp/engine/browser_tests/engine_browsertest.cc b/blimp/engine/browser_tests/engine_browsertest.cc
index ac9cec97707e9c9328a22084310b0930f0efe2ac..b2a39501b4a9473cbc7e0c15e421b989e2ab7ec1 100644
--- a/blimp/engine/browser_tests/engine_browsertest.cc
+++ b/blimp/engine/browser_tests/engine_browsertest.cc
@@ -115,7 +115,19 @@ IN_PROC_BROWSER_TEST_F(EngineBrowserTest, LoadUrl) {
   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();
@@ -130,6 +142,11 @@ IN_PROC_BROWSER_TEST_F(EngineBrowserTest, GoBack) {
   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