Browser: Simplify the History class and fix back/forward history push

This code was previously relying on the PageView::on_load_start hook
firing synchronously when calling PageView::load(). This was not
happening with WebContentView, so it broke the back/forward history.

Instead, we now differentiate between history navigations and normal
loads in Tab::load(). History navigations don't push new entries into
history, but instead just move the history pointer.

Applications/Browser/CMakeLists.txt

@@ -3,6 +3,7 @@ set(SOURCES
     BrowserConsoleClient.cpp
     ConsoleWidget.cpp
     DownloadWidget.cpp
+    History.cpp
     InspectorWidget.cpp
     main.cpp
     Tab.cpp

Applications/Browser/History.cpp

@@ -0,0 +1,73 @@
+/*
+ * Copyright (c) 2020, Andreas Kling <kling@serenityos.org>
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright notice, this
+ *    list of conditions and the following disclaimer.
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright notice,
+ *    this list of conditions and the following disclaimer in the documentation
+ *    and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
+ * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "History.h"
+
+namespace Browser {
+
+void History::dump() const
+{
+    dbg() << "Dump " << m_items.size() << " item(s)";
+    int i = 0;
+    for (auto& item : m_items) {
+        dbg() << "[" << i << "] " << item << " " << (m_current == i ? '*' : ' ');
+        ++i;
+    }
+}
+
+void History::push(const URL& url)
+{
+    m_items.shrink(m_current + 1);
+    m_items.append(url);
+    m_current++;
+}
+
+URL History::current() const
+{
+    if (m_current == -1)
+        return {};
+    return m_items[m_current];
+}
+
+void History::go_back()
+{
+    ASSERT(can_go_back());
+    m_current--;
+}
+
+void History::go_forward()
+{
+    ASSERT(can_go_forward());
+    m_current++;
+}
+
+void History::clear()
+{
+    m_items = {};
+    m_current = -1;
+}
+
+}

Applications/Browser/History.h

@@ -26,15 +26,17 @@
 
 #pragma once
 
-#include <AK/Optional.h>
-#include <AK/String.h>
+#include <AK/URL.h>
 #include <AK/Vector.h>
 
-template<typename T>
-class History final {
+namespace Browser {
+
+class History {
 public:
-    void push(const T& item);
-    T current() const;
+    void dump() const;
+
+    void push(const URL&);
+    URL current() const;
 
     void go_back();
     void go_forward();
@@ -45,43 +47,8 @@ public:
     void clear();
 
 private:
-    Vector<T> m_items;
+    Vector<URL> m_items;
     int m_current { -1 };
 };
 
-template<typename T>
-inline void History<T>::push(const T& item)
-{
-    m_items.shrink(m_current + 1);
-    m_items.append(item);
-    m_current++;
-}
-
-template<typename T>
-inline T History<T>::current() const
-{
-    if (m_current == -1)
-        return {};
-    return m_items[m_current];
-}
-
-template<typename T>
-inline void History<T>::go_back()
-{
-    ASSERT(can_go_back());
-    m_current--;
-}
-
-template<typename T>
-inline void History<T>::go_forward()
-{
-    ASSERT(can_go_forward());
-    m_current++;
-}
-
-template<typename T>
-inline void History<T>::clear()
-{
-    m_items = {};
-    m_current = -1;
 }

Applications/Browser/Tab.cpp

@@ -88,36 +88,14 @@ Tab::Tab(Type type)
     else
         m_web_content_view = widget.add<WebContentView>();
 
-    m_go_back_action = GUI::CommonActions::make_go_back_action([this](auto&) {
-        m_history.go_back();
-        update_actions();
-        TemporaryChange<bool> change(m_should_push_loads_to_history, false);
-        load(m_history.current());
-    },
-        this);
-
-    m_go_forward_action = GUI::CommonActions::make_go_forward_action([this](auto&) {
-        m_history.go_forward();
-        update_actions();
-        TemporaryChange<bool> change(m_should_push_loads_to_history, false);
-        load(m_history.current());
-    },
-        this);
+    m_go_back_action = GUI::CommonActions::make_go_back_action( [this](auto&) { go_back(); }, this);
+    m_go_forward_action = GUI::CommonActions::make_go_forward_action([this](auto&) { go_forward(); }, this);
 
     toolbar.add_action(*m_go_back_action);
     toolbar.add_action(*m_go_forward_action);
 
-    toolbar.add_action(GUI::CommonActions::make_go_home_action([this](auto&) {
-        load(g_home_url);
-    },
-        this));
-
-    m_reload_action = GUI::CommonActions::make_reload_action(
-        [this](auto&) {
-            TemporaryChange<bool> change(m_should_push_loads_to_history, false);
-            reload();
-        },
-        this);
+    toolbar.add_action(GUI::CommonActions::make_go_home_action([this](auto&) { load(g_home_url); }, this));
+    m_reload_action = GUI::CommonActions::make_reload_action( [this](auto&) { reload(); }, this);
 
     toolbar.add_action(*m_reload_action);
 
@@ -155,8 +133,6 @@ Tab::Tab(Type type)
     hooks().on_load_start = [this](auto& url) {
         m_location_box->set_icon(nullptr);
         m_location_box->set_text(url.to_string());
-        if (m_should_push_loads_to_history)
-            m_history.push(url);
         update_actions();
         update_bookmark_button(url.to_string());
     };
@@ -369,6 +345,9 @@ Tab::Tab(Type type)
             }
         },
         this));
+    debug_menu.add_action(GUI::Action::create("Dump history", { Mod_Ctrl, Key_H }, [&](auto&) {
+        m_history.dump();
+    }));
     debug_menu.add_separator();
     auto line_box_borders_action = GUI::Action::create_checkable(
         "Line box borders", [this](auto& action) {
@@ -413,8 +392,11 @@ Tab::~Tab()
 {
 }
 
-void Tab::load(const URL& url)
+void Tab::load(const URL& url, LoadType load_type)
 {
+    if (load_type == LoadType::Normal)
+        m_history.push(url);
+
     if (m_type == Type::InProcessWebView)
         m_page_view->load(url);
     else
@@ -433,6 +415,20 @@ void Tab::reload()
     load(url());
 }
 
+void Tab::go_back()
+{
+    m_history.go_back();
+    update_actions();
+    load(m_history.current(), LoadType::HistoryNavigation);
+}
+
+void Tab::go_forward()
+{
+    m_history.go_forward();
+    update_actions();
+    load(m_history.current(), LoadType::HistoryNavigation);
+}
+
 void Tab::update_actions()
 {
     m_go_back_action->set_enabled(m_history.can_go_back());
@@ -497,5 +493,6 @@ Web::WebViewHooks& Tab::hooks()
 {
     if (m_type == Type::InProcessWebView)
         return *m_page_view;
-    return *m_web_content_view;}
+    return *m_web_content_view;
+}
 }

Applications/Browser/Tab.h

@@ -53,8 +53,15 @@ public:
 
     URL url() const;
 
-    void load(const URL&);
+    enum class LoadType {
+        Normal,
+        HistoryNavigation,
+    };
+
+    void load(const URL&, LoadType = LoadType::Normal);
     void reload();
+    void go_back();
+    void go_forward();
 
     void did_become_active();
     void context_menu_requested(const Gfx::IntPoint& screen_position);
@@ -78,7 +85,7 @@ private:
 
     Type m_type;
 
-    History<URL> m_history;
+    History m_history;
 
     RefPtr<Web::PageView> m_page_view;
     RefPtr<WebContentView> m_web_content_view;
@@ -102,8 +109,6 @@ private:
 
     String m_title;
     RefPtr<const Gfx::Bitmap> m_icon;
-
-    bool m_should_push_loads_to_history { true };
 };
 
 }