Use QUrl with QWebHistory

Now that we have our own history implementation, we get the URLs as QUrl
and not as string - so no point in converting them to string and back.

3 files changed, 30 insertions, 27 deletions

diff --git a/qutebrowser/browser/history.py b/qutebrowser/browser/history.py
index 1c3d7d5a6..5b061c1c4 100644
--- a/qutebrowser/browser/history.py
+++ b/qutebrowser/browser/history.py
@@ -26,7 +26,7 @@ from PyQt5.QtCore import pyqtSignal, QUrl, QObject
 from PyQt5.QtWebKit import QWebHistoryInterface
 
 from qutebrowser.commands import cmdutils
-from qutebrowser.utils import utils, objreg, standarddir, log
+from qutebrowser.utils import utils, objreg, standarddir, log, qtutils
 from qutebrowser.config import config
 from qutebrowser.misc import lineparser
 
@@ -38,31 +38,34 @@ class Entry:
     Attributes:
         atime: The time the page was accessed.
         url: The URL which was accessed as QUrl.
-        url_string: The URL which was accessed as string.
         hidden: If True, don't save this entry to disk
     """
 
     def __init__(self, atime, url, title, hidden=False):
         self.atime = float(atime)
-        self.url = QUrl(url)
-        self.url_string = url
+        self.url = url
         self.title = title
         self.hidden = hidden
+        qtutils.ensure_valid(url)
 
     def __repr__(self):
         return utils.get_repr(self, constructor=True, atime=self.atime,
-                              url=self.url.toDisplayString(), title=self.title,
+                              url=self.url_str(), title=self.title,
                               hidden=self.hidden)
 
     def __str__(self):
-        return '{} {} {}'.format(int(self.atime), self.url_string, self.title)
+        return '{} {} {}'.format(int(self.atime), self.url_str(), self.title)
 
     def __eq__(self, other):
         return (self.atime == other.atime and
                 self.title == other.title and
-                self.url_string == other.url_string and
+                self.url == other.url and
                 self.hidden == other.hidden)
 
+    def url_str(self):
+        """Get the URL as a lossless string."""
+        return self.url.toString(QUrl.FullyEncoded | QUrl.RemovePassword)
+
     @classmethod
     def from_str(cls, line):
         data = line.split(maxsplit=2)
@@ -73,6 +76,11 @@ class Entry:
             atime, url, title = data
         else:
             raise ValueError("2 or 3 fields expected")
+
+        url = QUrl(url)
+        if not url.isValid():
+            raise ValueError("Invalid URL: {}".format(url.errorString()))
+
         if atime.startswith('\0'):
             log.init.debug(
                 "Removing NUL bytes from entry {!r} - see "
@@ -213,8 +221,9 @@ class WebHistory(QObject):
         """Add an entry to self.history_dict or another given OrderedDict."""
         if target is None:
             target = self.history_dict
-        target[entry.url_string] = entry
-        target.move_to_end(entry.url_string)
+        url_str = entry.url_str()
+        target[url_str] = entry
+        target.move_to_end(url_str)
 
     def get_recent(self):
         """Get the most recent history entries."""
@@ -243,18 +252,16 @@ class WebHistory(QObject):
         self._saved_count = 0
         self.cleared.emit()
 
-    def add_url(self, url_string, title="", hidden=False):
+    def add_url(self, url, title="", hidden=False):
         """Called by WebKit when an URL should be added to the history.
 
         Args:
-            url_string: An url as string to add to the history.
+            url: An url (as QUrl) to add to the history.
             hidden: Whether to hide the entry from the on-disk history
         """
-        if not url_string:
-            return
         if config.get('general', 'private-browsing'):
             return
-        entry = Entry(time.time(), url_string, title, hidden=hidden)
+        entry = Entry(time.time(), url, title, hidden=hidden)
         if self._initial_read_done:
             self._add_entry(entry)
             if not entry.hidden:
diff --git a/qutebrowser/browser/webview.py b/qutebrowser/browser/webview.py
index 9cf13eccf..9fbebf78f 100644
--- a/qutebrowser/browser/webview.py
+++ b/qutebrowser/browser/webview.py
@@ -154,12 +154,8 @@ class WebView(QWebView):
                                       QUrl.UrlFormattingOption(0)):
             # If the url of the page is different than the url of the link
             # originally clicked, save them both.
-            url = self._orig_url.toString(QUrl.FullyEncoded |
-                                          QUrl.RemovePassword)
-            history.add_url(url, self.title(), hidden=True)
-        url = self.cur_url.toString(QUrl.FullyEncoded | QUrl.RemovePassword)
-
-        history.add_url(url, self.title())
+            history.add_url(self._orig_url, self.title(), hidden=True)
+        history.add_url(self.cur_url, self.title())
 
     def _init_page(self):
         """Initialize the QWebPage used by this view."""
diff --git a/tests/unit/browser/test_history.py b/tests/unit/browser/test_history.py
index 8fdf1e72c..aaadc02b0 100644
--- a/tests/unit/browser/test_history.py
+++ b/tests/unit/browser/test_history.py
@@ -41,7 +41,7 @@ def test_adding_item_during_async_read(qtbot, hist):
 
     with qtbot.assertNotEmitted(hist.add_completion_item), \
             qtbot.assertNotEmitted(hist.item_added):
-        hist.add_url('http://www.example.com/')
+        hist.add_url(QUrl('http://www.example.com/'))
 
     with qtbot.waitSignals([hist.add_completion_item,
                             hist.async_read_done]):
@@ -61,7 +61,7 @@ def test_private_browsing(qtbot, tmpdir, fake_save_manager, config_stub):
     # Before initial read
     with qtbot.assertNotEmitted(private_hist.add_completion_item), \
             qtbot.assertNotEmitted(private_hist.item_added):
-        private_hist.add_url('http://www.example.com/')
+        private_hist.add_url(QUrl('http://www.example.com/'))
     assert not private_hist._temp_history
 
     # read
@@ -73,7 +73,7 @@ def test_private_browsing(qtbot, tmpdir, fake_save_manager, config_stub):
     # after read
     with qtbot.assertNotEmitted(private_hist.add_completion_item), \
             qtbot.assertNotEmitted(private_hist.item_added):
-        private_hist.add_url('http://www.example.com/')
+        private_hist.add_url(QUrl('http://www.example.com/'))
 
     assert not private_hist._temp_history
     assert not private_hist._new_history
@@ -84,18 +84,18 @@ def test_private_browsing(qtbot, tmpdir, fake_save_manager, config_stub):
     (
         # old format without title
         '12345 http://example.com/',
-        history.Entry(atime=12345, url='http://example.com/', title='',)
+        history.Entry(atime=12345, url=QUrl('http://example.com/'), title='',)
     ),
     (
         # new format with title
         '12345 http://example.com/ this is a title',
-        history.Entry(atime=12345, url='http://example.com/',
+        history.Entry(atime=12345, url=QUrl('http://example.com/'),
                       title='this is a title')
     ),
     (
         # weird NUL bytes
         '\x0012345 http://example.com/',
-        history.Entry(atime=12345, url='http://example.com/', title=''),
+        history.Entry(atime=12345, url=QUrl('http://example.com/'), title=''),
     ),
 ])
 def test_entry_parse_valid(line, expected):
@@ -105,7 +105,7 @@ def test_entry_parse_valid(line, expected):
 
 @pytest.mark.parametrize('line', [
     '12345',  # one field
-    '12345 onlyscheme:',  # invalid URL
+    '12345 ::',  # invalid URL
     'xyz http://www.example.com/',  # invalid timestamp
 ])
 def test_entry_parse_invalid(line):