Bug 1152121 - Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions. r=Gijs, mcomella
authorMargaret Leibovic <margaret.leibovic@gmail.com>
Mon, 06 Apr 2015 16:07:03 -0700
changeset 270499 a490eda23996f14aabb0571d87cbf92dd56846d2
parent 270498 bba36d7d59d20db5fac10bf0fa1d049fbdd9486f
child 270500 225457512400f8056f87b474890582c68770257d
push id863
push userraliiev@mozilla.com
push dateMon, 03 Aug 2015 13:22:43 +0000
treeherdermozilla-release@f6321b14228d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, mcomella
bugs1152121
milestone40.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1152121 - Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions. r=Gijs, mcomella
browser/base/content/browser-readinglist.js
browser/base/content/browser.js
browser/base/content/test/general/browser_readerMode.js
browser/base/content/urlbarBindings.xml
browser/modules/ReaderParent.jsm
mobile/android/chrome/content/Reader.js
mobile/android/chrome/content/browser.js
toolkit/components/reader/AboutReader.jsm
toolkit/components/reader/ReaderMode.jsm
--- a/browser/base/content/browser-readinglist.js
+++ b/browser/base/content/browser-readinglist.js
@@ -247,17 +247,17 @@ let ReadingListUI = {
       // nothing to do if we have no button.
       return;
     }
 
     let uri;
     if (this.enabled && state == "valid") {
       uri = gBrowser.currentURI;
       if (uri.schemeIs("about"))
-        uri = ReaderParent.parseReaderUrl(uri.spec);
+        uri = ReaderMode.getOriginalUrl(uri.spec);
       else if (!uri.schemeIs("http") && !uri.schemeIs("https"))
         uri = null;
     }
 
     let msg = {topic: "UpdateActiveItem", url: null};
     if (!uri) {
       this.toolbarButton.setAttribute("hidden", true);
       if (this.isSidebarOpen)
@@ -304,17 +304,17 @@ let ReadingListUI = {
    * removing otherwise.
    *
    * @param {<xul:browser>} browser - Browser with page to toggle.
    * @returns {Promise} Promise resolved when operation has completed.
    */
   togglePageByBrowser: Task.async(function* (browser) {
     let uri = browser.currentURI;
     if (uri.spec.startsWith("about:reader?"))
-      uri = ReaderParent.parseReaderUrl(uri.spec);
+      uri = ReaderMode.getOriginalUrl(uri.spec);
     if (!uri)
       return;
 
     let item = yield ReadingList.itemForURL(uri);
     if (item) {
       yield item.delete();
     } else {
       yield ReadingList.addItemFromBrowser(browser, uri);
@@ -325,17 +325,17 @@ let ReadingListUI = {
    * Checks if a given item matches the current tab in this window.
    *
    * @param {ReadingListItem} item - Item to check
    * @returns True if match, false otherwise.
    */
   isItemForCurrentBrowser(item) {
     let currentURL = gBrowser.currentURI.spec;
     if (currentURL.startsWith("about:reader?"))
-      currentURL = ReaderParent.parseReaderUrl(currentURL);
+      currentURL = ReaderMode.getOriginalUrl(currentURL);
 
     if (item.url == currentURL || item.resolvedURL == currentURL) {
       return true;
     }
     return false;
   },
 
   /**
--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -208,16 +208,19 @@ XPCOMUtils.defineLazyModuleGetter(this, 
   "resource:///modules/UITour.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "CastingApps",
   "resource:///modules/CastingApps.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "SimpleServiceDiscovery",
   "resource://gre/modules/SimpleServiceDiscovery.jsm");
 
+XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode",
+  "resource://gre/modules/ReaderMode.jsm");
+
 XPCOMUtils.defineLazyModuleGetter(this, "ReaderParent",
   "resource:///modules/ReaderParent.jsm");
 
 let gInitialPages = [
   "about:blank",
   "about:newtab",
   "about:home",
   "about:privatebrowsing",
--- a/browser/base/content/test/general/browser_readerMode.js
+++ b/browser/base/content/test/general/browser_readerMode.js
@@ -12,17 +12,17 @@ const TEST_PREFS = [
   ["browser.readinglist.enabled", true],
   ["browser.readinglist.introShown", false],
 ];
 
 const TEST_PATH = "http://example.com/browser/browser/base/content/test/general/";
 
 let readerButton = document.getElementById("reader-mode-button");
 
-add_task(function* () {
+add_task(function* test_reader_button() {
   registerCleanupFunction(function() {
     // Reset test prefs.
     TEST_PREFS.forEach(([name, value]) => {
       Services.prefs.clearUserPref(name);
     });
     while (gBrowser.tabs.length > 1) {
       gBrowser.removeCurrentTab();
     }
@@ -85,8 +85,21 @@ add_task(function* () {
   yield promiseWaitForCondition(() => readerButton.hidden);
   is_element_hidden(readerButton, "Reader mode button is not present on a non-reader-able page");
 
   // Switch back to the original tab to make sure reader mode button is still visible.
   gBrowser.removeCurrentTab();
   yield promiseWaitForCondition(() => !readerButton.hidden);
   is_element_visible(readerButton, "Reader mode button is present on a reader-able page");
 });
+
+add_task(function* test_getOriginalUrl() {
+  let { ReaderMode } = Cu.import("resource://gre/modules/ReaderMode.jsm", {});
+  let url = "http://foo.com/article.html";
+
+  is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(url)), url, "Found original URL from encoded URL");
+  is(ReaderMode.getOriginalUrl("about:reader?foobar"), null, "Did not find original URL from malformed reader URL");
+  is(ReaderMode.getOriginalUrl(url), null, "Did not find original URL from non-reader URL");
+
+  let badUrl = "http://foo.com/?;$%^^";
+  is(ReaderMode.getOriginalUrl("about:reader?url=" + encodeURIComponent(badUrl)), badUrl, "Found original URL from encoded malformed URL");
+  is(ReaderMode.getOriginalUrl("about:reader?url=" + badUrl), badUrl, "Found original URL from non-encoded malformed URL");
+});
--- a/browser/base/content/urlbarBindings.xml
+++ b/browser/base/content/urlbarBindings.xml
@@ -163,17 +163,17 @@ file, You can obtain one at http://mozil
               }
               case "keyword": // Fall through.
               case "searchengine": {
                 returnValue = action.params.input;
                 break;
               }
             }
           } else {
-            let originalUrl = ReaderParent.parseReaderUrl(aValue);
+            let originalUrl = ReaderMode.getOriginalUrl(aValue);
             if (originalUrl) {
               returnValue = originalUrl;
             }
           }
 
           // Set the actiontype only if the user is not overriding actions.
           if (action && this._noActionsKeys.size == 0) {
             this.setAttribute("actiontype", action.type);
--- a/browser/modules/ReaderParent.jsm
+++ b/browser/modules/ReaderParent.jsm
@@ -167,49 +167,27 @@ let ReaderParent = {
   },
 
   toggleReaderMode: function(event) {
     let win = event.target.ownerDocument.defaultView;
     let browser = win.gBrowser.selectedBrowser;
     let url = browser.currentURI.spec;
 
     if (url.startsWith("about:reader")) {
-      let originalURL = this._getOriginalUrl(url);
+      let originalURL = ReaderMode.getOriginalUrl(url);
       if (!originalURL) {
         Cu.reportError("Error finding original URL for about:reader URL: " + url);
       } else {
         win.openUILinkIn(originalURL, "current", {"allowPinnedTabHostChange": true});
       }
     } else {
       browser.messageManager.sendAsyncMessage("Reader:ParseDocument", { url: url });
     }
   },
 
-  parseReaderUrl: function(url) {
-    if (!url.startsWith("about:reader?")) {
-      return null;
-    }
-    return this._getOriginalUrl(url);
-  },
-
-  /**
-   * Returns original URL from an about:reader URL.
-   *
-   * @param url An about:reader URL.
-   * @return The original URL for the article, or null if we did not find
-   *         a properly formatted about:reader URL.
-   */
-  _getOriginalUrl: function(url) {
-    let searchParams = new URLSearchParams(url.substring("about:reader?".length));
-    if (!searchParams.has("url")) {
-      return null;
-    }
-    return decodeURIComponent(searchParams.get("url"));
-  },
-
   /**
    * Gets an article for a given URL. This method will download and parse a document.
    *
    * @param url The article URL.
    * @param browser The browser where the article is currently loaded.
    * @return {Promise}
    * @resolves JS object representing the article, or null if no article is found.
    */
--- a/mobile/android/chrome/content/Reader.js
+++ b/mobile/android/chrome/content/Reader.js
@@ -1,17 +1,15 @@
 // -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
-XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm");
-
 let Reader = {
   // These values should match those defined in BrowserContract.java.
   STATUS_UNFETCHED: 0,
   STATUS_FETCH_FAILED_TEMPORARY: 1,
   STATUS_FETCH_FAILED_PERMANENT: 2,
   STATUS_FETCH_FAILED_UNSUPPORTED_FORMAT: 3,
   STATUS_FETCHED_ARTICLE: 4,
 
--- a/mobile/android/chrome/content/browser.js
+++ b/mobile/android/chrome/content/browser.js
@@ -108,16 +108,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/SharedPreferences.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Notifications",
                                   "resource://gre/modules/Notifications.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "GMPInstallManager",
                                   "resource://gre/modules/GMPInstallManager.jsm");
 
+XPCOMUtils.defineLazyModuleGetter(this, "ReaderMode", "resource://gre/modules/ReaderMode.jsm");
+
 let lazilyLoadedBrowserScripts = [
   ["SelectHelper", "chrome://browser/content/SelectHelper.js"],
   ["InputWidgetHelper", "chrome://browser/content/InputWidgetHelper.js"],
   ["MasterPassword", "chrome://browser/content/MasterPassword.js"],
   ["PluginHelper", "chrome://browser/content/PluginHelper.js"],
   ["OfflineApps", "chrome://browser/content/OfflineApps.js"],
   ["Linkifier", "chrome://browser/content/Linkify.js"],
   ["ZoomHelper", "chrome://browser/content/ZoomHelper.js"],
@@ -4557,26 +4559,17 @@ Tab.prototype = {
       this.contentDocumentIsDisplayed = false;
       this.hasTouchListener = false;
     } else {
       this.sendViewportUpdate();
     }
   },
 
   _stripAboutReaderURL: function (url) {
-    if (!url.startsWith("about:reader")) {
-      return url;
-    }
-
-    // From ReaderParent._getOriginalUrl (browser/modules/ReaderParent.jsm).
-    let searchParams = new URLSearchParams(url.substring("about:reader?".length));
-    if (!searchParams.has("url")) {
-        return url;
-    }
-    return decodeURIComponent(searchParams.get("url"));
+    return ReaderMode.getOriginalUrl(url) || url;
   },
 
   // Properties used to cache security state used to update the UI
   _state: null,
   _hostChanged: false, // onLocationChange will flip this bit
 
   onSecurityChange: function(aWebProgress, aRequest, aState) {
     // Don't need to do anything if the data we use to update the UI hasn't changed
--- a/toolkit/components/reader/AboutReader.jsm
+++ b/toolkit/components/reader/AboutReader.jsm
@@ -3,16 +3,17 @@
  * You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 let Ci = Components.interfaces, Cc = Components.classes, Cu = Components.utils;
 
 this.EXPORTED_SYMBOLS = [ "AboutReader" ];
 
+Cu.import("resource://gre/modules/ReaderMode.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "Rect", "resource://gre/modules/Geometry.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task", "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "UITelemetry", "resource://gre/modules/UITelemetry.jsm");
 
 const READINGLIST_COMMAND_ID = "readingListSidebar";
@@ -782,22 +783,17 @@ AboutReader.prototype = {
     }.bind(this), 300);
   },
 
   /**
    * Returns the original article URL for this about:reader view.
    */
   _getOriginalUrl: function(win) {
     let url = win ? win.location.href : this._win.location.href;
-    let searchParams = new URLSearchParams(url.split("?")[1]);
-    if (!searchParams.has("url")) {
-      Cu.reportError("Error finding original URL for about:reader URL: " + url);
-      return url;
-    }
-    return decodeURIComponent(searchParams.get("url"));
+    return ReaderMode.getOriginalUrl(url) || url;
   },
 
   _setupSegmentedButton: function Reader_setupSegmentedButton(id, options, initialValue, callback) {
     let doc = this._doc;
     let segmentedButton = doc.getElementById(id);
 
     for (let i = 0; i < options.length; i++) {
       let option = options[i];
--- a/toolkit/components/reader/ReaderMode.jsm
+++ b/toolkit/components/reader/ReaderMode.jsm
@@ -63,16 +63,41 @@ this.ReaderMode = {
         if (aData.startsWith("reader.parse-on-load.")) {
           this.isEnabledForParseOnLoad = this._getStateForParseOnLoad();
         }
         break;
     }
   },
 
   /**
+   * Returns original URL from an about:reader URL.
+   *
+   * @param url An about:reader URL.
+   * @return The original URL for the article, or null if we did not find
+   *         a properly formatted about:reader URL.
+   */
+  getOriginalUrl: function(url) {
+    if (!url.startsWith("about:reader?")) {
+      return null;
+    }
+
+    let searchParams = new URLSearchParams(url.substring("about:reader?".length));
+    if (!searchParams.has("url")) {
+      return null;
+    }
+    let encodedURL = searchParams.get("url");
+    try {
+      return decodeURIComponent(encodedURL);
+    } catch (e) {
+      Cu.reportError("Error decoding original URL: " + e);
+      return encodedURL;
+    }
+  },
+
+  /**
    * Decides whether or not a document is reader-able without parsing the whole thing.
    *
    * @param doc A document to parse.
    * @return boolean Whether or not we should show the reader mode button.
    */
   isProbablyReaderable: function(doc) {
     // Only care about 'real' HTML documents:
     if (doc.mozSyntheticDocument || !(doc instanceof doc.defaultView.HTMLDocument)) {