Bug 1341350 - Change the hosts prefix algorithm to prefer secure urls, but also to be able to swap back and forth depending on typed entries in history. r?mak draft
authorMark Banner <standard8@mozilla.com>
Tue, 02 May 2017 18:29:05 +0100
changeset 572713 000de1bdae310c907b50348870ffa102395ccf15
parent 572679 7208a840a89f17f45afe8faad7a11f34e9eb5457
child 627100 8ac57f79a433e8d993c91351f446777a46556496
push id57156
push userbmo:standard8@mozilla.com
push dateThu, 04 May 2017 15:33:50 +0000
reviewersmak
bugs1341350
milestone55.0a1
Bug 1341350 - Change the hosts prefix algorithm to prefer secure urls, but also to be able to swap back and forth depending on typed entries in history. r?mak MozReview-Commit-ID: BuK16J0JUaf
toolkit/components/places/nsPlacesTriggers.h
toolkit/components/places/tests/unifiedcomplete/test_trimming.js
toolkit/components/places/tests/unit/test_hosts_triggers.js
--- a/toolkit/components/places/nsPlacesTriggers.h
+++ b/toolkit/components/places/nsPlacesTriggers.h
@@ -58,38 +58,40 @@
   "OR rev_host = get_unreversed_host(host || '.') || '.www.'"
 
 /**
  * Select the best prefix for a host, based on existing pages registered for it.
  * Prefixes have a priority, from the top to the bottom, so that secure pages
  * have higher priority, and more generically "www." prefixed hosts come before
  * unprefixed ones.
  * Given a host, examine associated pages and:
- *  - if all of the typed pages start with https://www. return https://www.
- *  - if all of the typed pages start with https:// return https://
+ *  - if at least half the typed pages start with https://www. return https://www.
+ *  - if at least half the typed pages start with https:// return https://
  *  - if all of the typed pages start with ftp: return ftp://
- *  - if all of the typed pages start with www. return www.
+ *     - This is because mostly people will want to visit the http version
+ *       of the site.
+ *  - if at least half the typed pages start with www. return www.
  *  - otherwise don't use any prefix
  */
 #define HOSTS_PREFIX_PRIORITY_FRAGMENT \
   "SELECT CASE " \
-    "WHEN 1 = ( " \
-      "SELECT min(substr(url,1,12) = 'https://www.') FROM moz_places h " \
+    "WHEN ( " \
+      "SELECT round(avg(substr(url,1,12) = 'https://www.')) FROM moz_places h " \
       "WHERE (" HOST_TO_REVHOST_PREDICATE ") AND +h.typed = 1 " \
     ") THEN 'https://www.' " \
-    "WHEN 1 = ( " \
-      "SELECT min(substr(url,1,8) = 'https://') FROM moz_places h " \
+    "WHEN ( " \
+      "SELECT round(avg(substr(url,1,8) = 'https://')) FROM moz_places h " \
       "WHERE (" HOST_TO_REVHOST_PREDICATE ") AND +h.typed = 1 " \
     ") THEN 'https://' " \
     "WHEN 1 = ( " \
       "SELECT min(substr(url,1,4) = 'ftp:') FROM moz_places h " \
       "WHERE (" HOST_TO_REVHOST_PREDICATE ") AND +h.typed = 1 " \
     ") THEN 'ftp://' " \
-    "WHEN 1 = ( " \
-      "SELECT min(substr(url,1,11) = 'http://www.') FROM moz_places h " \
+    "WHEN ( " \
+      "SELECT round(avg(substr(url,1,11) = 'http://www.')) FROM moz_places h " \
       "WHERE (" HOST_TO_REVHOST_PREDICATE ") AND +h.typed = 1 " \
     ") THEN 'www.' " \
   "END "
 
 /**
  * These triggers update the hostnames table whenever moz_places changes.
  */
 #define CREATE_PLACES_AFTERINSERT_TRIGGER NS_LITERAL_CSTRING( \
--- a/toolkit/components/places/tests/unifiedcomplete/test_trimming.js
+++ b/toolkit/components/places/tests/unifiedcomplete/test_trimming.js
@@ -126,57 +126,57 @@ add_task(function* test_priority_1() {
   yield check_autocomplete({
     search: "mo",
     autofilled: "mozilla.org/",
     completed: "mozilla.org/"
   });
   yield cleanup();
 });
 
-add_task(function* test_periority_2() {
+add_task(function* test_priority_2() {
   do_print( "Ensuring correct priority 2");
   yield PlacesTestUtils.addVisits([
     { uri: NetUtil.newURI("https://mozilla.org/test/"), transition: TRANSITION_TYPED },
     { uri: NetUtil.newURI("ftp://mozilla.org/test/"), transition: TRANSITION_TYPED },
     { uri: NetUtil.newURI("http://www.mozilla.org/test/"), transition: TRANSITION_TYPED },
     { uri: NetUtil.newURI("http://mozilla.org/test/"), transition: TRANSITION_TYPED }
   ]);
   yield check_autocomplete({
     search: "mo",
     autofilled: "mozilla.org/",
     completed: "mozilla.org/"
   });
   yield cleanup();
 });
 
-add_task(function* test_periority_3() {
+add_task(function* test_priority_3() {
   do_print("Ensuring correct priority 3");
   yield PlacesTestUtils.addVisits([
     { uri: NetUtil.newURI("ftp://mozilla.org/test/"), transition: TRANSITION_TYPED },
     { uri: NetUtil.newURI("http://www.mozilla.org/test/"), transition: TRANSITION_TYPED },
     { uri: NetUtil.newURI("http://mozilla.org/test/"), transition: TRANSITION_TYPED }
   ]);
   yield check_autocomplete({
     search: "mo",
     autofilled: "mozilla.org/",
     completed: "mozilla.org/"
   });
   yield cleanup();
 });
 
-add_task(function* test_periority_4() {
+add_task(function* test_priority_4() {
   do_print("Ensuring correct priority 4");
   yield PlacesTestUtils.addVisits([
     { uri: NetUtil.newURI("http://www.mozilla.org/test/"), transition: TRANSITION_TYPED },
     { uri: NetUtil.newURI("http://mozilla.org/test/"), transition: TRANSITION_TYPED }
   ]);
   yield check_autocomplete({
     search: "mo",
     autofilled: "mozilla.org/",
-    completed: "mozilla.org/"
+    completed: "www.mozilla.org/"
   });
   yield cleanup();
 });
 
 add_task(function* test_priority_5() {
   do_print("Ensuring correct priority 5");
   yield PlacesTestUtils.addVisits([
     { uri: NetUtil.newURI("ftp://mozilla.org/test/"), transition: TRANSITION_TYPED },
--- a/toolkit/components/places/tests/unit/test_hosts_triggers.js
+++ b/toolkit/components/places/tests/unit/test_hosts_triggers.js
@@ -214,60 +214,146 @@ add_task(function* test_moz_hosts_update
                 },
                 { uri: TEST_URI_2
                 }];
   yield PlacesTestUtils.addVisits(places);
 
   checkHostInMozHosts(TEST_URI_1, true, "https://www.");
 });
 
-function getTestSection(baseURL1, baseURL2, extra) {
+function getTestSection(baseURL1, baseURL2, baseURL2Prefix, extra) {
   let extraStr = "";
   let expectedSimplePrefix = null;
-  let expectedSecurePrefix = "https://";
+  let expectedUpgradePrefix = baseURL2Prefix;
   if (extra) {
     extraStr = ` (${extra})`;
     expectedSimplePrefix = `${extra}.`;
-    expectedSecurePrefix = `https://${extra}.`;
+    expectedUpgradePrefix = `${baseURL2Prefix}${extra}.`;
   }
   return [{
-    title: `Test simple insecure${extraStr}`,
+    title: `Test simple url${extraStr}`,
     visits: [{ uri: baseURL1, transition: TRANSITION_TYPED }],
-    expect: [baseURL1, true, expectedSimplePrefix ]
+    expect: [baseURL1, true, expectedSimplePrefix]
   }, {
-    title: `Test upgrade secure${extraStr}`,
+    title: `Test upgrade url${extraStr}`,
     visits: [{ uri: baseURL2, transition: TRANSITION_TYPED }],
-    expect: [baseURL2, true, null]
+    expect: [baseURL2, true, expectedUpgradePrefix]
   }, {
-    title: `Test remove insecure completely${extraStr}`,
+    title: `Test remove simple completely${extraStr}`,
     remove: baseURL1,
-    expect: [baseURL2, true, expectedSecurePrefix]
+    expect: [baseURL2, true, expectedUpgradePrefix]
   }, {
     title: `Test add more visits${extraStr}`,
     visits: [
       { uri: baseURL2, transition: TRANSITION_TYPED },
       { uri: baseURL1, transition: TRANSITION_TYPED },
     ],
-    expect: [baseURL2, true, null]
+    expect: [baseURL2, true, expectedUpgradePrefix]
   }, {
-    title: `Test switch to insecure${extraStr}`,
-    visits: [{ uri: baseURL1, transition: TRANSITION_TYPED }],
-    expect: [baseURL2, true, null]
+    title: `Test remove upgrade url${extraStr}`,
+    remove: baseURL2,
+    expect: [baseURL2, true, expectedSimplePrefix]
   }];
 }
 
-const updateTestURL1 = "http://example.com/";
-const updateTestURL2 = "https://example.com/";
-
 const hostsUpdateTests = [{
   title: "Upgrade Secure/Downgrade Insecure",
-  tests: getTestSection("http://example.com", "https://example.com")
+  tests: getTestSection("http://example.com", "https://example.com", "https://")
 }, {
   title: "Upgrade Secure/Downgrade Insecure (www)",
-  tests: getTestSection("http://www.example1.com", "https://www.example1.com", "www")
+  tests: getTestSection("http://www.example1.com", "https://www.example1.com", "https://", "www")
+}, {
+  title: "Upgrade Secure/Downgrade non-www to www",
+  tests: getTestSection("http://example3.com", "http://www.example3.com", "www.")
+}, {
+  title: "Switch to/from ftp",
+  tests: [{
+    title: `Test normal url`,
+    visits: [{ uri: "http://example4.com", transition: TRANSITION_TYPED }],
+    expect: ["http://example4.com", true, null]
+  }, {
+    title: `Test switch to ftp`,
+    visits: [{ uri: "ftp://example4.com", transition: TRANSITION_TYPED }],
+    // ftp is only switched to if all pages are ftp://
+    remove: ["http://example4.com"],
+    expect: ["ftp://example4.com", true, "ftp://"]
+  }, {
+    title: `Test visit http`,
+    visits: [{ uri: "http://example4.com", transition: TRANSITION_TYPED }],
+    expect: ["ftp://example4.com", true, null]
+  }]
+}, {
+  title: "Multiple URLs for source",
+  tests: [{
+    title: `Test simple insecure`,
+    visits: [{ uri: "http://example2.com", transition: TRANSITION_TYPED }],
+    expect: ["http://example2.com", true, null]
+  }, {
+    title: `Test upgrade secure`,
+    visits: [{ uri: "https://example2.com", transition: TRANSITION_TYPED }],
+    expect: ["https://example2.com", true, "https://"]
+  }, {
+    title: `Test extra insecure visit`,
+    visits: [{ uri: "http://example2.com/fake", transition: TRANSITION_TYPED }],
+    expect: ["https://example2.com", true, null]
+  }, {
+    title: `Test extra secure visits`,
+    visits: [
+      { uri: "https://example2.com/foo", transition: TRANSITION_TYPED },
+      { uri: "https://example2.com/bar", transition: TRANSITION_TYPED },
+    ],
+    expect: ["https://example2.com", true, "https://"]
+  }, {
+    title: `Test remove secure`,
+    remove: ["https://example2.com", "https://example2.com/foo", "https://example2.com/bar"],
+    expect: ["https://example2.com", true, null]
+  }]
+}, {
+  title: "Test upgrade tree",
+  tests: [{
+    title: `Add ftp`,
+    visits: [{ uri: "ftp://example5.com", transition: TRANSITION_TYPED }],
+    expect: ["http://example5.com", true, "ftp://"]
+  }, {
+    title: `Add basic http`,
+    visits: [{ uri: "http://example5.com", transition: TRANSITION_TYPED }],
+    expect: ["http://example5.com", true, null]
+  }, {
+    title: `Add basic www`,
+    visits: [
+      // Add multiples to exceed the average.
+      { uri: "http://www.example5.com", transition: TRANSITION_TYPED },
+      { uri: "http://www.example5.com/past", transition: TRANSITION_TYPED }
+    ],
+    expect: ["http://example5.com", true, "www."]
+  }, {
+    title: `Add https`,
+    visits: [
+      // Add multiples to exceed the average.
+      { uri: "https://example5.com", transition: TRANSITION_TYPED },
+      { uri: "https://example5.com/past", transition: TRANSITION_TYPED },
+      { uri: "https://example5.com/mak", transition: TRANSITION_TYPED },
+      { uri: "https://example5.com/standard8", transition: TRANSITION_TYPED }
+    ],
+    expect: ["https://example5.com", true, "https://"]
+  }, {
+    title: `Add https www`,
+    visits: [
+      // Add multiples to exceed the average.
+      { uri: "https://www.example5.com", transition: TRANSITION_TYPED },
+      { uri: "https://www.example5.com/quantum", transition: TRANSITION_TYPED },
+      { uri: "https://www.example5.com/photon", transition: TRANSITION_TYPED },
+      { uri: "https://www.example5.com/dash", transition: TRANSITION_TYPED },
+      { uri: "https://www.example5.com/flow", transition: TRANSITION_TYPED },
+      { uri: "https://www.example5.com/persona", transition: TRANSITION_TYPED },
+      { uri: "https://www.example5.com/ff_fx", transition: TRANSITION_TYPED },
+      { uri: "https://www.example5.com/search", transition: TRANSITION_TYPED }
+    ],
+    expect: ["https://example5.com", true, "https://www."]
+  }]
 }];
 
 add_task(function* test_moz_hosts_update() {
   for (const section of hostsUpdateTests) {
     do_print(section.title);
 
     for (const test of section.tests) {
       do_print(test.title);