Closed
Bug 1239119
Opened 8 years ago
Closed 8 years ago
PreviewProvider Messaging API
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
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 | ||
Updated•8 years ago
|
Blocks: newtab-message-apis
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → oyiptong
Assignee | ||
Comment 1•8 years ago
|
||
code tracked on github at: https://github.com/mozilla/newtab-dev/tree/bug_1239119_previewprovider_messaging_api
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38729/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38729/
Attachment #8728011 -
Flags: review?(ursulasarracini)
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38731/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38731/
Attachment #8728012 -
Flags: review?(ursulasarracini)
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38733/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38733/
Attachment #8728013 -
Flags: review?(ursulasarracini)
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/38735/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/38735/
Attachment #8728014 -
Flags: review?(ursulasarracini)
Updated•8 years ago
|
Attachment #8728013 -
Flags: review?(ursulasarracini)
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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•8 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
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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•8 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•8 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
Updated•8 years ago
|
Attachment #8728014 -
Flags: review+
Comment 14•8 years ago
|
||
Comment on attachment 8728014 [details] MozReview Request: Bug 1239119 - NewTab webchannel messages for thumbnails r?ursula https://reviewboard.mozilla.org/r/38735/#review37029
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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•8 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•8 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•8 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
Updated•8 years ago
|
Attachment #8728013 -
Flags: review?(ursulasarracini) → review+
Comment 20•8 years ago
|
||
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•8 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•8 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•8 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•8 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/
Comment 25•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0704310fc2f https://hg.mozilla.org/integration/mozilla-inbound/rev/c8728788af45 https://hg.mozilla.org/integration/mozilla-inbound/rev/e17ded471c3f https://hg.mozilla.org/integration/mozilla-inbound/rev/14ebae70ac37
Comment 26•8 years ago
|
||
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
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d0704310fc2f https://hg.mozilla.org/mozilla-central/rev/c8728788af45 https://hg.mozilla.org/mozilla-central/rev/e17ded471c3f https://hg.mozilla.org/mozilla-central/rev/14ebae70ac37 https://hg.mozilla.org/mozilla-central/rev/7600e2f05b97
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in
before you can comment on or make changes to this bug.
Description
•