PreviewProvider Messaging API

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: tspurway, Assigned: oyiptong)

Tracking

(Blocks 1 bug)

unspecified
Firefox 48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

()

Attachments

(4 attachments)

Reporter

Description

4 years ago
This API provides access to the FX thumbs service for the remote newtab page.
Assignee

Updated

4 years ago
Assignee: nobody → oyiptong
Attachment #8728013 - Flags: review?(ursulasarracini)
Comment on attachment 8728013 [details]
MozReview Request: Bug 1239119 - Move provider init for newtab to NewTabMessages.jsm r?ursula

https://reviewboard.mozilla.org/r/38733/#review36605

::: browser/components/newtab/tests/browser/browser_newtabwebchannel.js:17
(Diff revision 1)
>  
>  const TEST_URL = "https://example.com/browser/browser/components/newtab/tests/browser/newtabwebchannel_basic.html";
>  const TEST_URL_2 = "http://mochi.test:8888/browser/browser/components/newtab/tests/browser/newtabwebchannel_basic.html";
>  
> +function setup(mode = "test") {
> +  NewTabMessages.uninit();

why uninit here before init?
Comment on attachment 8728014 [details]
MozReview Request: Bug 1239119 - NewTab webchannel messages for thumbnails r?ursula

https://reviewboard.mozilla.org/r/38735/#review36609

::: browser/components/newtab/NewTabMessages.jsm:52
(Diff revision 1)
>  
>    /*
>     * Return to the originator all newtabpage prefs. A point-to-point request.
>     */
>    handlePrefRequest(actionName, {target}) {
> -    if (ACTIONS.prefs.action_types.has(actionName)) {
> +    if (ACTIONS.prefs.inPrefs === actionName) {

did you mean to revert this? i thought (ACTIONS.prefs.action_types.has(actionName)) was correct

::: browser/components/newtab/NewTabMessages.jsm:59
(Diff revision 1)
>        NewTabWebChannel.send(ACTIONS.prefs.outPrefs, results, target);
>      }
>    },
>  
> +  handlePreviewRequest(actionName, {data, target}) {
> +    if (ACTIONS.preview.inThumb === actionName) {

same goes here: should we use (ACTIONS.prefs.action_types.has(actionName) ?

::: browser/components/newtab/tests/browser/browser_newtabmessages.js:15
(Diff revision 1)
>  
>  function setup() {
>    Preferences.set("browser.newtabpage.enhanced", true);
>    Preferences.set("browser.newtabpage.remote.mode", "test");
>    Preferences.set("browser.newtabpage.remote", true);
>    NewTabMessages.init();

should we init NewTabWebChannel in setup here too?
Attachment #8728014 - Flags: review?(ursulasarracini)
Comment on attachment 8728011 [details]
MozReview Request: Bug 1239119 - BackgroundPageThumbs captureIfMissing promise resolves when task is truly done r?ursula

https://reviewboard.mozilla.org/r/38729/#review36703
Attachment #8728011 - Flags: review?(ursulasarracini) → review+
Assignee

Comment 9

3 years ago
https://reviewboard.mozilla.org/r/38735/#review36609

> did you mean to revert this? i thought (ACTIONS.prefs.action_types.has(actionName)) was correct

I did mean to do that. Since there is only one pref expected, I just tightened the expectation.

> same goes here: should we use (ACTIONS.prefs.action_types.has(actionName) ?

same as above:

I did mean to do that. Since there is only one pref expected, I just tightened the expectation.

> should we init NewTabWebChannel in setup here too?

NewTabMessages init should init a NewTabWebChannel as well
https://reviewboard.mozilla.org/r/38735/#review36753

::: browser/components/newtab/NewTabMessages.jsm:52
(Diff revision 1)
>  
>    /*
>     * Return to the originator all newtabpage prefs. A point-to-point request.
>     */
>    handlePrefRequest(actionName, {target}) {
> -    if (ACTIONS.prefs.action_types.has(actionName)) {
> +    if (ACTIONS.prefs.inPrefs === actionName) {

ah gotcha! is there a need for the ```action_types``` set then? it doesn't look like it's being used if we only have the one action coming in. For both the prefs and the thumbs
Comment on attachment 8728012 [details]
MozReview Request: Bug 1239119 - PreviewProvider module produces thumbnail data URIs given a url r?ursula

https://reviewboard.mozilla.org/r/38731/#review36771

::: browser/components/newtab/PreviewProvider.jsm:26
(Diff revision 1)
> +  /**
> +   * Returns a thumbnail as a data URI for a url, creating it if necessary
> +   *
> +   * @param {String} url
> +   *        a url to obtain a thumbnail for
> +   * @return {Promise} A Promise that resolves with a base64 encoded thumbnail

Does this really return a Promise? It looks like it encondedData is just a value

::: browser/components/newtab/PreviewProvider.jsm:45
(Diff revision 1)
> +      let bytes = yield file.read();
> +      let encodedData = btoa(String.fromCharCode.apply(null, bytes));
> +      file.close();
> +      return `data:${contentType};base64,${encodedData}`;
> +    } catch (err) {
> +      Cu.reportError(`PreviewProvider_getThumbnail error: ${err}`);

Do we need to think about a fallback when the page thumb fails? Like, what happens to the rest of newtab when this error throws? Does it die completely or do we still load the rest of the page and just have the thumbnail unavailable?
Attachment #8728012 - Flags: review?(ursulasarracini)
Assignee

Comment 12

3 years ago
https://reviewboard.mozilla.org/r/38731/#review36771

> Does this really return a Promise? It looks like it encondedData is just a value

It does. The object parameter `getThumbnail` uses `Task.async` as a helper to define the body of the function. Task.async returns a function that returns a promise.

> Do we need to think about a fallback when the page thumb fails? Like, what happens to the rest of newtab when this error throws? Does it die completely or do we still load the rest of the page and just have the thumbnail unavailable?

IMO, it should be handled inside of the function that calls `getThumbnail`. The promise will be rejected here, with `err` as the rejection parameter.

In the big picture, should we fail to obtain a screenshot, I think we should just not send the data the image data.

As implemented in patch https://reviewboard-hg.mozilla.org/gecko/rev/0bab6b22769c, if an error happens, content will not get a message stating there is no screenshot.
We *could* send an error message, but I'm unsure what content can do if it receives the error. The way we set up the event handling in redux, I'm not sure we wait for a response per se.

What were you thinking in terms of fallback?
Assignee

Comment 13

3 years ago
https://reviewboard.mozilla.org/r/38735/#review36753

> ah gotcha! is there a need for the ```action_types``` set then? it doesn't look like it's being used if we only have the one action coming in. For both the prefs and the thumbs

Very pertinent point. The Sets are actually removed in a downstream patch: https://reviewboard-hg.mozilla.org/gecko/rev/3818ef650eb7
Comment on attachment 8728014 [details]
MozReview Request: Bug 1239119 - NewTab webchannel messages for thumbnails r?ursula

https://reviewboard.mozilla.org/r/38735/#review37029
https://reviewboard.mozilla.org/r/38731/#review37031

::: browser/components/newtab/PreviewProvider.jsm:45
(Diff revision 1)
> +      let bytes = yield file.read();
> +      let encodedData = btoa(String.fromCharCode.apply(null, bytes));
> +      file.close();
> +      return `data:${contentType};base64,${encodedData}`;
> +    } catch (err) {
> +      Cu.reportError(`PreviewProvider_getThumbnail error: ${err}`);

Not sending a message is fine, I just wanted to make sure that the thumbnail failing here doesn't cause the *rest* of the newtab page to error out down the line.
Comment on attachment 8728012 [details]
MozReview Request: Bug 1239119 - PreviewProvider module produces thumbnail data URIs given a url r?ursula

https://reviewboard.mozilla.org/r/38731/#review37033
Attachment #8728012 - Flags: review+
Assignee

Comment 17

3 years ago
Comment on attachment 8728013 [details]
MozReview Request: Bug 1239119 - Move provider init for newtab to NewTabMessages.jsm r?ursula

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38733/diff/1-2/
Attachment #8728013 - Flags: review?(ursulasarracini)
Assignee

Comment 18

3 years ago
Comment on attachment 8728014 [details]
MozReview Request: Bug 1239119 - NewTab webchannel messages for thumbnails r?ursula

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

Comment 19

3 years ago
https://reviewboard.mozilla.org/r/38733/#review36605

> why uninit here before init?

because i was sloppy on that file. this is now fixed
Attachment #8728013 - Flags: review?(ursulasarracini) → review+
Comment on attachment 8728013 [details]
MozReview Request: Bug 1239119 - Move provider init for newtab to NewTabMessages.jsm r?ursula

https://reviewboard.mozilla.org/r/38733/#review37043
Assignee

Comment 21

3 years ago
Comment on attachment 8728011 [details]
MozReview Request: Bug 1239119 - BackgroundPageThumbs captureIfMissing promise resolves when task is truly done r?ursula

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

Comment 22

3 years ago
Comment on attachment 8728012 [details]
MozReview Request: Bug 1239119 - PreviewProvider module produces thumbnail data URIs given a url r?ursula

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

Comment 23

3 years ago
Comment on attachment 8728013 [details]
MozReview Request: Bug 1239119 - Move provider init for newtab to NewTabMessages.jsm r?ursula

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38733/diff/2-3/
Assignee

Comment 24

3 years ago
Comment on attachment 8728014 [details]
MozReview Request: Bug 1239119 - NewTab webchannel messages for thumbnails r?ursula

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38735/diff/2-3/
Follow-up push: https://hg.mozilla.org/integration/mozilla-inbound/rev/7600e2f05b97 Move semicolon from end of if block to end of function assigned to document.onreadystatechange (discovered by eslint). r=eslint-fix
Depends on: 1409691
You need to log in before you can comment on or make changes to this bug.