Closed Bug 1220547 Opened 6 years ago Closed 6 years ago

Setting favicon and keyword for a bookmark in distribution.ini

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: hectorz, Assigned: hectorz)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file)

(In reply to Marco Bonardo [::mak] from bug 1207965 comment #4)
> (In reply to Hector Zhao [:hectorz] | Off 30/Sept - 11/Oct from bug 1207965 comment #3)
> 
> > Later we began to rely on the ability to set
> > favicon and keyword of bookmarks.html. Maybe I should submit a patch to add
> > the favicon and keyword support to distribution.ini instead?
> 
> I'd not object on that.
>
Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r?mak
Attachment #8681798 - Flags: review?(mak77)
Comment on attachment 8681798 [details]
MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak

https://reviewboard.mozilla.org/r/23909/#review21535

::: browser/components/distribution.js:21
(Diff revision 1)
> +                                  "resource://gre/modules/Promise.jsm");

Please use DOM Promises instead.

That means either using the DOM Promise constructor like

new Promise((resolve, reject) => {
  ...
})

Or, if you really need a defer, you can use PromiseUtils.defer()

::: browser/components/distribution.js:207
(Diff revision 1)
> +        if (item.icon && item.iconUri) {

I'd probably put the favicon uri into icon and have an iconData field. Should be less confusing and more consistent with the codebase

::: browser/components/distribution.js:218
(Diff revision 1)
> +            yield deferred.promise;

So, I think it's not critical to wait for the icon, we can just let it go on and proceed. Moreover the callback is currently not invoked on error, that means we may block the whole distribution import by waiting :(

I'd rather just invoke the method and move on.
that means the test may have to poll for the icon, since it can't just wait...

::: browser/components/distribution.js:227
(Diff revision 1)
> +        }

I'd suggest to try catch around this as well.

::: browser/components/places/tests/unit/test_browserGlue_distribution.js:96
(Diff revision 1)
> +  Assert.equal(faviconItem.uri.spec, "https://example.org/favicon.png");

here I'd suggest to poll for the icon. So create a separate helper that checks every N ms and returns when it finds the expected data, after M tries it should just throw.
It can return a promise and here you can just yield waitForFavicon(...);
Attachment #8681798 - Flags: review?(mak77)
Comment on attachment 8681798 [details]
MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23909/diff/1-2/
Attachment #8681798 - Flags: review?(mak77)
Comment on attachment 8681798 [details]
MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23909/diff/2-3/
Comment on attachment 8681798 [details]
MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak

https://reviewboard.mozilla.org/r/23909/#review22545

::: browser/components/places/tests/unit/head_bookmarks.js:142
(Diff revision 3)
> +function waitForResolvedPromise(promiseFn, timeoutMsg, tryCount=NUMBER_OF_TRIES) {

you are not setting timeoutMsg anywhere...

::: browser/components/places/tests/unit/head_bookmarks.js:164
(Diff revision 3)
> +}

I think this can be largely simplify, I didn't test this code but off-hand should do the same
var waitForResolvedPromise = Task.Async(function* (promiseFn, timeoutMsg, tryCount=NUMBER_OF_TRIES) {
  let tries = 0;
  do {
    try {
      return promiseFn();
    } catch (ex) {}
    yield new Promise(resolve => do_timeout(100, resolve));
  } while (++tries <= tryCount);
  throw(timeoutMsg);
});
Attachment #8681798 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/23909/#review22545

> I think this can be largely simplify, I didn't test this code but off-hand should do the same
> var waitForResolvedPromise = Task.Async(function* (promiseFn, timeoutMsg, tryCount=NUMBER_OF_TRIES) {
>   let tries = 0;
>   do {
>     try {
>       return promiseFn();
>     } catch (ex) {}
>     yield new Promise(resolve => do_timeout(100, resolve));
>   } while (++tries <= tryCount);
>   throw(timeoutMsg);
> });

I'll try this tomorrow. One quick question, do you mean |return promiseFn()| or |yield promise()|? I really should get myself familiar with Task.Async.
by returning there I exit the whole Task and the return value is passed out as the Task resolution value.
Btw, I'm not sure if the try catch is properly catching the promiseFn reject there... I should probably do:
try {
  let val = yield promiseFn();
  return val;
} catch...
Attachment #8681798 - Flags: review?(mak77)
Comment on attachment 8681798 [details]
MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23909/diff/3-4/
Assignee: nobody → bzhao
Status: NEW → ASSIGNED
Comment on attachment 8681798 [details]
MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak

https://reviewboard.mozilla.org/r/23909/#review23483

r=me with the following fixed and a "green" Try run.

::: browser/components/distribution.js:213
(Diff revision 4)
> +              PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE);

Due to bug 1119386 we need to add a loading principal to the API calls here.

PlacesUtils.favicons.replaceFaviconDataFromDataURL(faviconURI, item.iconData, 0, Services.scriptSecurityManager.getSystemPrincipal());

PlacesUtils.favicons.setAndFetchFaviconForPage(this._makeURI(item.link), faviconURI, false, PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE,  null, Services.scriptSecurityManager.getSystemPrincipal());

It should work even if bug 1119386 didn't land yet, the additional argument should just be ignored.

::: browser/components/places/tests/unit/head_bookmarks.js:26
(Diff revision 4)
> -});
> +  "resource://gre/modules/Timer.jsm");

you don't need to import Timer.jsm anymore
Attachment #8681798 - Flags: review?(mak77) → review+
Comment on attachment 8681798 [details]
MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23909/diff/4-5/
Attachment #8681798 - Attachment description: MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r?mak → MozReview Request: Bug 1220547 - Setting favicon and keyword for a bookmark in distribution.ini. r=mak
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c0ce24b4658

Looks like the r+ is carried over in Bugzilla but not in MozReview.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f6e89ea4a19f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Duplicate of this bug: 468359
Hector: Have you tested this lately? It's not working for me...
(In reply to Mike Kaply [:mkaply] from comment #18)
> Hector: Have you tested this lately? It's not working for me...

I'll give it a try on 4th Feb. after the Chinese New Year holiday.

Also ping Yanfang, in case she could answer this sooner.
Flags: needinfo?(bzhao)
(In reply to Mike Kaply [:mkaply] from comment #18)
> Hector: Have you tested this lately? It's not working for me...

Hi Mike,

I tested it on Firefox 51.0.1, it works fine.
Flags: needinfo?(bzhao)
Sorry, forgot to update. The sites I was using were spitting out icon instead of x-icon.

Everything is working great.

I'm wondering why you chose to require iconData though. Would you mind a patch that used icon even if iconData wasn't there?
(In reply to Mike Kaply [:mkaply] from comment #21)
> 
> I'm wondering why you chose to require iconData though.

I think we kinda want to make sure the favicon is available even when Firefox is first opened w/o internet access.

> Would you mind a patch that used icon even if iconData wasn't there?

We will keep providing the data uri in our distribution.ini, I'm fine with iconData being optional if any other partner prefers to omit it.
Duplicate of this bug: 1231470
You need to log in before you can comment on or make changes to this bug.