526920 - SetAndLoadFaviconForPage should support setting favicons for non-addable URIs that are bookmarked

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

For example, fennec's default "about:firefox" bookmark. "about:firefox" is not an addable URI, according to nsNavHistory::CanAddURI, but we want to be able to set a chrome:// branding URI as its favicon anyways.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

There are some tests for the negative cases, but they're commented out because they fail... there isn't really any way to tell whether setAndLoadFaviconForPage returned early without planning to do any work. I guess I should just remove them...

I also split out the setAndLoadFaviconForPage tests from test_favicons - the current test there doesn't actually test setAndLoadFaviconForPage - it only passes because the previous tests has synchronously added data for that pageURI (via setFaviconData).

Comment 2

[Marco Bonardo [:mak]]

(In reply to comment #1)
> I also split out the setAndLoadFaviconForPage tests from test_favicons - the
> current test there doesn't actually test setAndLoadFaviconForPage - it only
> passes because the previous tests has synchronously added data for that pageURI
> (via setFaviconData).

This method was synchronous before, so i can imagine the test has not been updated when moving to async and is still luckily passing :\

Btw, soon we plan to add a callback to SetAndLoadFaviconForPage to be called  when the icon has been set (or the work is done, tbd).

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 410677 [details] [diff] [review]
patch

we have various patches in bug 423126 that are touching this code, i'll have to update them to take in count these changes

>diff --git a/toolkit/components/places/src/nsFaviconService.cpp b/toolkit/components/places/src/nsFaviconService.cpp
>--- a/toolkit/components/places/src/nsFaviconService.cpp
>+++ b/toolkit/components/places/src/nsFaviconService.cpp
>@@ -539,59 +539,57 @@ nsFaviconService::DoSetAndLoadFaviconFor
>                                              nsIURI* aFaviconURI,
>                                              PRBool aForceReload)
> {

>+  // If history is disabled or the page isn't addable to history, only load
>+  // favicons if the page is bookmarked.
>+  if (!canAdd || history->IsHistoryDisabled()) {
>+    // check to see if this favicon could be for a bookmark

please ucfirst and end with period (maybe "whether" in place of "if"? You know the language better then me, i'll let you choice).

>diff --git a/toolkit/components/places/tests/unit/head_bookmarks.js b/toolkit/components/places/tests/unit/head_bookmarks.js

>+function addBookmark(aURI) {
>+  try {
>+    var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+                getService(Ci.nsINavBookmarksService);
>+  } catch (ex) {
>+    do_throw("Could not get bookmark service\n");
>+  }
>+
>+  var bmId = bmsvc.insertBookmark(bmsvc.unfiledBookmarksFolder,
>+                                  aURI,
>+                                  bmsvc.DEFAULT_INDEX,
>+                                  aURI.spec);
>+  do_check_true(bmId > 0);
>+}

i'm unsure about the usefulness of this method, the service can be instantiated at the start of the test and most of the tests are doing that, at that point is replacing one function with one function (with less arguments, thus less usefulness).
I'd prefer if this would stay in the tests, and be renamed to AddUnsortedBookmark (but you can ignore me on the name)

then
checking the returned id > 0 is useless (yes i know we do that in various tests, but is a nonsense) since insertBookmark would throw instead and never set a negative bookmark id.
Also catching the getService throw and rethrowing is useless (ditto about stupid things in our tests)
And it should return the just added bookmark id.

>+
>+/*
>+ * readFileData()

i can see the method name by myself, get rid of this useless comment please.
rather javadoc @param

>+ *
>+ * Reads the data from the specified nsIFile, and returns an array of bytes.
>+ */
>+function readFileData(aFile) {

>+
>+/*
>+ * compareArrays

ditto

>+ *
>+ * Compares two arrays, and throws if there's a difference.

it should return true or false, and the test using this should do whatever it needs to do
there could be cases where one needs to follow different paths based on the result, returning a bool would allow that
also a failure here will confuse people reporting oranges (i'vee seen already couple cases where people filed oranges against head_ files...)
At finding different entries it should just print("Arrays differ at index (index): value 1 != value 2") (With vars obviously)
please fix it

>+ */
>+function compareArrays(aArray1, aArray2) {
>+  do_check_eq(aArray1.length, aArray2.length);
>+
>+  for (var i = 0; i < aArray1.length; i++)
>+      if (aArray1[i] != aArray2[i])
>+          throw "arrays differ at index " + i;
>+}

>diff --git a/toolkit/components/places/tests/unit/test_doSetAndLoadFaviconForPage.js b/toolkit/components/places/tests/unit/test_doSetAndLoadFaviconForPage.js

>+// Get favicon service
>+try {
>+  var iconsvc = Cc["@mozilla.org/browser/favicon-service;1"].
>+                getService(Ci.nsIFaviconService);
>+} catch(ex) {
>+  do_throw("Could not get favicon service\n");
>+}
>+
>+// Get history services
>+try {
>+  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                getService(Ci.nsINavHistoryService);
>+  var bhist = histsvc.QueryInterface(Ci.nsIBrowserHistory);
>+} catch(ex) {
>+  do_throw("Could not get history services\n");
>+}

don't rethrow, they will already throw, and checking the line is just as easy.


>+var favicons = [
>+  {
>+    get uri() {
>+      var file = do_get_file("favicon-normal32.png");
>+      delete this.uri;
>+      return this.uri = iosvc.newFileURI(file);
>+    },
>+    data: readFileData(do_get_file("favicon-normal32.png")),

why uri is a getter but data is not?

>+    mimetype: "image/png"
>+  }
>+];
>+
>+var tests = [
>+  {
>+    desc: "test setAndLoadFaviconForPage",

..." for valid history uri"

>+    pageURI: uri("http://test1.bar/"),
>+    go: function () {
>+      this.favicon = favicons[0];
>+
>+      iconsvc.setAndLoadFaviconForPage(this.pageURI, this.favicon.uri, true);
>+    },
>+    check: function () {
>+      checkAddSucceeded(this.pageURI, this.favicon.mimetype, this.favicon.data);
>+    }
>+  },
>+  //{

space tests by 1 newline please

>+  //  desc: "test setAndLoadFaviconForPage for about: URIs",
>+  //  pageURI: uri("about:test1"),
>+  //  go: function () {
>+  //    iconsvc.setAndLoadFaviconForPage(this.pageURI, favicons[0].uri, true);
>+  //  },
>+  //  check: function () {
>+  //    checkAddFailed(this.pageURI);
>+  //  }
>+  //},

i'm fine with leaving this tests disabled, but please file a bug about the fact there is no easy way to know if setting a favicon failed.
and comment on them with a // TODO bug XXXXXX: There is no easy way to know if setting a favicon failed.

>+  {
>+    desc: "test setAndLoadFaviconForPage for bookmarked about: URIs",
>+    pageURI: uri("about:test2"),
>+    favicon: favicons[0],
>+    go: function () {
>+      // Add as bookmark
>+      addBookmark(this.pageURI);
>+
>+      iconsvc.setAndLoadFaviconForPage(this.pageURI, this.favicon.uri, true);
>+    },
>+    check: function () {
>+      checkAddSucceeded(this.pageURI, this.favicon.mimetype, this.favicon.data);
>+    }
>+  },
>+  //{
>+  //  desc: "test setAndLoadFaviconForPage with history disabled",
>+  //  pageURI: uri("http://test2.bar/"),
>+  //  go: function () {
>+  //    // disable history
>+  //    var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>+  //    prefs.setIntPref("browser.history_expire_days", 0);
>+  //
>+  //    iconsvc.setAndLoadFaviconForPage(this.pageURI, favicons[0].uri, true);
>+  //
>+  //    try {
>+  //      prefs.clearUserPref("browser.history_expire_days");
>+  //    } catch (ex) {}
>+  //  },
>+  //  check: function () {
>+  //    checkAddFailed(this.pageURI);
>+  //  }
>+  //},

ditto for the TODO

>+    go: function () {
>+      // disable history
>+      var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
>+      prefs.setIntPref("browser.history_expire_days", 0);
>+
>+      // Add as bookmark
>+      addBookmark(this.pageURI);
>+
>+      iconsvc.setAndLoadFaviconForPage(this.pageURI, this.favicon.uri, true);
>+
>+      try {
>+        prefs.clearUserPref("browser.history_expire_days");
>+      } catch (ex) {}
>+    },
>+    check: function () {
>+      checkAddSucceeded(this.pageURI, this.favicon.mimetype, this.favicon.data);
>+    }
>+  },

please add a test in PrivateBrowsing mode, should practically be equal to this.

>+var currentTestIndex = 0;
>+function run_test() {
>+  do_test_pending();
>+
>+  // check that the favicon loaded correctly
>+  do_check_eq(favicons[0].data.length, 344);
>+
>+  var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsINavHistoryService);
>+  hs.addObserver(historyObserver, false);

you have a history global var, what is this for?

>+  // Start the tests
>+  tests[currentTestIndex].go();
>+};

r=me with these addressed

Comment 4

[Marco Bonardo [:mak]]

PS: bhist looks unused.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Attachment: interdiff for addressing comments

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: comments addressed

Comment 7

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/07e74f1f0561

Comment 8

[Marco Bonardo [:mak]]

you forgot to make addBookmark return the id, and print() different values in compareArrays if they are found. Apart those 2 glitches, good work!

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 410834 [details] [diff] [review]
comments addressed

a192=beltzner with the test changes suggested by Marco, above.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/82a810285c42

https://hg.mozilla.org/releases/mozilla-1.9.2/rev/cf3d1704311d