Bug 1152121 - Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions. r=Gijs, r=mcomella, a=sledru
authorMargaret Leibovic <margaret.leibovic@gmail.com>
Mon, 06 Apr 2015 16:07:03 -0700
changeset 258506 7a10ff7fd9e4
parent 258505 ccb54262291d
child 258507 f9d36adcdf51
push id4682
push userryanvm@gmail.com
push date2015-04-16 18:48 +0000
treeherdermozilla-beta@45a5eaa7813b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs, mcomella, sledru
bugs1152121
milestone38.0
Bug 1152121 - Factor out logic to get original URL from reader URL into shared place, and handle malformed URI excpetions. r=Gijs, r=mcomella, a=sledru
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
@@ -203,16 +203,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
@@ -156,17 +156,17 @@
               }
               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
@@ -165,49 +165,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"],
@@ -4502,26 +4504,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) {
     let uri = Services.io.newURI(doc.location.href, null, null);