Closed Bug 1239119 Opened 5 years ago Closed 4 years ago

PreviewProvider Messaging API

Categories

(Firefox :: New Tab Page, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: tspurway, Assigned: oyiptong)

References

(Blocks 1 open bug, )

Details

Attachments

(4 files)

This API provides access to the FX thumbs service for the remote newtab page.
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+
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)
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?
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+
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)
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/
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
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/
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/
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/
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.