993904 - Use cached directory links instead of locally packaged links

Status: VERIFIED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

Bug 986521 implements fetching and caching of remotely hosted links, and this bug will be to use those links and stop using the packaged links.

Comment 1

[Olivier Yiptong [:oyiptong]]

adw commented on a related bug975211 that we should be using OS.File if we are to read from a local file.

This involves the modification of DirectoryLinksProvider.jsm.

If no file is found, or the file cannot be read, DirectoryLinksProvider should return no link upon getLinks() invocation.

Comment 2

[Ed Lee :Mardak]

In the case of no links then later remotely fetched links, we'll probably need to call Links.populateCache or Links.resetCache. This resetting functionality should maybe exist in bug 986521 after a successful fetch?

Comment 3

[maxim zhilyaev]

Attachment: v1. Read directory links from cached file.

Comment 4

[Drew Willcoxon :adw]

Comment on attachment 8414577 [details] [diff] [review]
v1. Read directory links from cached file.

Review of attachment 8414577 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, but I'd like to reserve r+ until I see how the other patches shake out (plus the promise question I have below).  I'll leave the r? for now.

::: toolkit/modules/DirectoryLinksProvider.jsm
@@ +229,5 @@
>    },
>  
>    /**
> +   * Reads directory links file and parses its content
> +   * @param retruns a promise resolved to valid json or []

retruns -> returns

Also, it's more correct to say "a valid list of links" since JSON is a string encoding of an object and not the object itself.  Call me pedantic. :-)

@@ +231,5 @@
>    /**
> +   * Reads directory links file and parses its content
> +   * @param retruns a promise resolved to valid json or []
> +   */
> +  _readDirectoryLinksFile: function DirectoryLinksProvider_readDirectoryLinksFile(aCallback) {

Not a big deal, but these recent patches uses promises instead of callbacks, so why not be consistent here?

Comment 5

[maxim zhilyaev]

Attachment: v2. Read directory links from cached file

misspellings are fixed.  
Tried refactoring _readDirectoryLinksFile() with a promise and decided against it:
the code came out more bulky without any obvious advantages in clarity.
However, if you feel strongly for the promise I will be happy to refactor it again.

Comment 6

[Drew Willcoxon :adw]

Comment on attachment 8419743 [details] [diff] [review]
v2. Read directory links from cached file

Review of attachment 8419743 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to maxim zhilyaev from comment #5)
> However, if you feel strongly for the promise I will be happy to refactor it
> again.

Nope, no problem, thanks for trying.  Although isn't the only thing you'd need to do is to replace the three aCallback() calls with deferred.resolve() (plus creating the deferred at the beginning and returning its promise at the end)?  Anyhow, no big deal.

Comment 7

[maxim zhilyaev]

Attachment: v3. final patch with reviewer concerns addressed

Comment 8

[Marco Mucci [:MarcoM]]

Removed from iteration.  Will be tracked the same way we track "non-regular" contributions from the community.

Comment 9

[maxim zhilyaev]

Attachment: v3.1 rebased onto bug993901

Comment 10

[Ed Lee :Mardak]

Comment on attachment 8426544 [details] [diff] [review]
v3.1 rebased onto bug993901

>diff --git a/toolkit/modules/DirectoryLinksProvider.jsm b/toolkit/modules/DirectoryLinksProvider.jsm
>   /**
>+   * Reads directory links file and parses its content
>+   * @param returns a promise resolved to valid list of links or []
This still says it returns a promise even though it takes a callback. But I think the code could actually be simplified with a promise. See below.

>+  _readDirectoryLinksFile: function DirectoryLinksProvider_readDirectoryLinksFile(aCallback) {
>+    try {
This try/catch block shouldn't be necessary. OS.File.read uses promises to reject failures, so this shouldn't throw.

>+      OS.File.read(this._directoryFilePath).then(binaryData => {
We should be returning the promise returned by the then(), so we don't need to create our own explicit promise.

>+        let output;
>+        try {
>+          let locale = this.locale;
>+          let json = gTextDecoder.decode(binaryData);
>+          output = JSON.parse(json)[locale];
>+        }
>+        catch (e) {
>+          Cu.reportError(e);
>+        }
>+        aCallback(output || []);
return output || [];
That'll have the promise returned by then resolve to output or [].

>+      },
>+      error => {
>+        Cu.reportError(error);
>+        aCallback([]);
return [];

>+      });
>+    }
>+    catch (e) {
>+      Cu.reportError("failed to read " + directoryLinksFile);
>+      aCallback([]);
>+    }
>+  },
As above, this catch is unnecessary.

Comment 11

[maxim zhilyaev]

Attachment: v4. Refactor file read with a promise

Comment 12

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/9e93a0ac18a0

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/9e93a0ac18a0

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/mozilla-central/rev/3689163e72a8 because it depended on changes made in bug 993901, which I just backed out for bustage.

Comment 15

[maxim zhilyaev]

Attachment: v5. repopulate newtab cache on prefrence change

Fix to intermittent failures in browser_tabopen_reflows.js
Repopulating newtab cache on change to directory source pref fixes the failure.
re-triggered on try server 4 times - all green

Comment 16

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/fx-team/rev/0712726ce2ab

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0712726ce2ab