1439684 - Add caching to NewTabUtils for saved to Pocket items

Status: RESOLVED FIXED

(Firefox :: New Tab Page, enhancement, P1)

Description

[Ursula Sarracini (:ursula)]

As discussed on irc with Mardak, we'll want to add a pocket in-memory cache to NewTabUtils with various pocket wrappers (save, delete, archive) expiring the cache as well as expiring after an hour. HighlightsFeed will just always call without excludePocket.

Comment 1

[Ursula Sarracini (:ursula)]

Attachment: Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items

Review commit: https://reviewboard.mozilla.org/r/221742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/221742/

Comment 2

[Ed Lee :Mardak]

Comment on attachment 8952527 [details]
Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items

https://reviewboard.mozilla.org/r/221742/#review227660

::: toolkit/modules/NewTabUtils.jsm:1411
(Diff revision 1)
> +   *
> +   *@returns {Promise} Returns a promise at completion
> +   */
> +  addPocketEntry(aUrl, aTitle) {
> +    this._savedPocketStories = null;
> +    return new Promise((success, error) => {

Instead of having `PlacesFeed.saveToPocket` check `isPocketUserLoggedIn` to conditionally call `Pocket.savePage`, that should probably just happen here as the cache should be expired in both cases.. Although in practice, if the user wasn't logged in, there shouldn't have been a cache to expire? Going down this path also means we wouldn't need to wrap `isUserLoggedIn`.

Perhaps have the savePage path resolve successfully but `null` to differentiate that we don't actually have data to update the page… Or.. do we fake a payload and assume the user did end up creating an account and hope the user doesn't try to unpocket some non-existent pocketId?

```js
addPocketEntry(…) {
  this._saved… = null;
  if (!isUserLoggedIn()) {
    Pocket.savePage(…);
    return Promise.resolve(null);
  }
  return new Promise((success, error)…);
}
```

::: toolkit/modules/NewTabUtils.jsm:1448
(Diff revision 1)
> -      results.push(...await ActivityStreamProvider.getRecentlyPocketed(aOptions));
> +      // If we do not have saved to Pocket stories already cached, or it has been
> +      // more than 1 hour since we last got Pocket stories, invalidate the cache,
> +      // get new stories, and update the timestamp
> +      if (!this._savedPocketStories || (Date.now() - this._pocketLastUpdated > POCKET_UPDATE_TIME)) {
> +        this._savedPocketStories = await ActivityStreamProvider.getRecentlyPocketed(aOptions);
> +        this._pocketLastUpdated = Date.now();

Any particular reason to do this cache check here in `getHighlights` instead of in `getRecentlyPocketed`? I would have expected it in the latter, but this could be fine if nobody else calls the other.

Comment 3

[Ursula Sarracini (:ursula)]

Comment on attachment 8952527 [details]
Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items

https://reviewboard.mozilla.org/r/221742/#review227660

> Instead of having `PlacesFeed.saveToPocket` check `isPocketUserLoggedIn` to conditionally call `Pocket.savePage`, that should probably just happen here as the cache should be expired in both cases.. Although in practice, if the user wasn't logged in, there shouldn't have been a cache to expire? Going down this path also means we wouldn't need to wrap `isUserLoggedIn`.
> 
> Perhaps have the savePage path resolve successfully but `null` to differentiate that we don't actually have data to update the page… Or.. do we fake a payload and assume the user did end up creating an account and hope the user doesn't try to unpocket some non-existent pocketId?
> 
> ```js
> addPocketEntry(…) {
>   this._saved… = null;
>   if (!isUserLoggedIn()) {
>     Pocket.savePage(…);
>     return Promise.resolve(null);
>   }
>   return new Promise((success, error)…);
> }
> ```

If they log in, save a bunch of stuff to Pocket, then log out and try to save something, should we invalidate the cache in that case? Meaning, maybe we should we do:

```js
addPocketEntry(…) {
  if (!isUserLoggedIn()) {
    Pocket.savePage(…);
    return Promise.resolve(null);
  }
  this._saved… = null;
  return new Promise((success, error)…);
}
```
Maybe this is more clear, since we don't want to do *anything* if they're not logged in - just show the panel and return. It's more clear to invalidate the cache only if we know we're going to be saving something directly

Comment 4

[Ursula Sarracini (:ursula)]

Comment on attachment 8952527 [details]
Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items

https://reviewboard.mozilla.org/r/221742/#review227660

> Any particular reason to do this cache check here in `getHighlights` instead of in `getRecentlyPocketed`? I would have expected it in the latter, but this could be fine if nobody else calls the other.

After trying it in ```getRecentlyPocketed```, I think I actually prefer it in ```getHighlights```. I think it's nice to keep all the things that have to do with getting the links themselves in ActivityStreamLinks and have AcivityStreamProvider just return raw data like it does with the other queries for highlights. That way if we need to call getRecentlyPocketed directly for some reason (testing, or another feature, etc) then it's not mixed up with Activity Stream's cached links.
What do you think Ed?

Comment 5

[Ed Lee :Mardak]

(In reply to Ursula Sarracini (:ursula) from comment #4)
> After trying it in ```getRecentlyPocketed```, I think I actually prefer it
> in ```getHighlights```.
That's fine. When there's actual caching from the pocket side, we'll need to fix things up anyway. I figured the provider code was closer to where caching would have actually happened, but we can keep the logic in `getHighlights`.

Comment 6

[Ed Lee :Mardak]

(In reply to Ursula Sarracini (:ursula) from comment #3)
> ```js
> addPocketEntry(…) {
>   if (!isUserLoggedIn()) …
>   this._saved… = null;
>   return new Promise((success, error)…);
> ```
> Maybe this is more clear, since we don't want to do *anything* if they're
> not logged in - just show the panel and return.
Okay. Although that got me wondering what should be done if the user isn't logged in and archives a page… Or even without doing an action but just having cached pocket items and logging out and `getHighlights` uses the cache. I suppose a different user logging in would have complications too.. for a followup? ;)

Comment 7

[Ursula Sarracini (:ursula)]

Comment on attachment 8952527 [details]
Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221742/diff/1-2/

Comment 8

[Ed Lee :Mardak]

Comment on attachment 8952527 [details]
Bug 1439684 - Add caching to NewTabUtils for saved to Pocket items

https://reviewboard.mozilla.org/r/221742/#review227954

Looks good for now. We'll want a followup bug too

Comment 9

[Ursula Sarracini (:ursula)]

Filed bug 1440047

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/103600892a2e
Add caching to NewTabUtils for saved to Pocket items r=Mardak

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/103600892a2e