Closed
Bug 1398819
Opened 7 years ago
Closed 7 years ago
Turn on prerendered version of activity stream in aboutNewTabService
Categories
(Firefox :: New Tab Page, defect)
Firefox
New Tab Page
Tracking
()
RESOLVED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: k88hudson, Assigned: k88hudson)
References
Details
Attachments
(1 file)
This patch will add conditional code to use the prerendered version of Activity Stream if the prerendered pref is true.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → khudson
Comment 3•7 years ago
|
||
If this can land before the activity stream prerender export, there'll be different patches needed for the all_files_referenced test
Blocks: 1394533
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
This could land before, but I'd have to set the default state of the pref to false (and we could change it back to true in the export)
Assignee | ||
Updated•7 years ago
|
Attachment #8906681 -
Flags: review?(edilee)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
Ok, I've updated this patch to remove the unreferenced file, so it should be good to go
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8906681 [details] Bug 1398819 - Turn on prerendered version of activity stream in aboutNewTabService https://reviewboard.mozilla.org/r/178400/#review183752 I believe the always toggling in pref change is undesired. ::: browser/components/newtab/aboutNewTabService.js:28 (Diff revision 3) > > const IS_MAIN_PROCESS = Services.appinfo.processType === Services.appinfo.PROCESS_TYPE_DEFAULT; > > // Pref that tells if activity stream is enabled > const PREF_ACTIVITY_STREAM_ENABLED = "browser.newtabpage.activity-stream.enabled"; > +const PREF_ACTIVITY_STREAM_PRERENDERED_ENABLED = "browser.newtabpage.activity-stream.prerender"; one says "PRERENDERED_ENABLED" the other says "prerender" and another says `_activityStreamPrerendered` … oh well :p ::: browser/components/newtab/aboutNewTabService.js:94 (Diff revision 3) > > observe(subject, topic, data) { > switch (topic) { > case "nsPref:changed": > + this.toggleActivityStream(); > + Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL); Was this intended? Seems to always toggle then toggle again? ::: browser/components/newtab/aboutNewTabService.js:101 (Diff revision 3) > - if (this.toggleActivityStream()) { > + if (this.toggleActivityStream()) { > - Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL); > + Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL); > - } > + } > + } else if (data === PREF_ACTIVITY_STREAM_PRERENDERED_ENABLED) { > + this._activityStreamPrerendered = Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_PRERENDERED_ENABLED); > + Services.obs.notifyObservers(null, "newtab-default-url-changed"); Any reason not to just reuse "newtab-url-changed"? ::: browser/components/newtab/aboutNewTabService.js:158 (Diff revision 3) > get defaultURL() { > if (this.activityStreamEnabled) { > + if (this.activityStreamPrerendered) { > + return this.activityStreamPrerenderedURL; > + } > return this.activityStreamURL; nit: could just make this a return with conditional ternary operator
Attachment #8906681 -
Flags: review?(edilee) → review-
Assignee | ||
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8906681 [details] Bug 1398819 - Turn on prerendered version of activity stream in aboutNewTabService https://reviewboard.mozilla.org/r/178400/#review183780 ::: browser/components/newtab/aboutNewTabService.js:101 (Diff revision 3) > - if (this.toggleActivityStream()) { > + if (this.toggleActivityStream()) { > - Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL); > + Services.obs.notifyObservers(null, "newtab-url-changed", ABOUT_URL); > - } > + } > + } else if (data === PREF_ACTIVITY_STREAM_PRERENDERED_ENABLED) { > + this._activityStreamPrerendered = Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_PRERENDERED_ENABLED); > + Services.obs.notifyObservers(null, "newtab-default-url-changed"); URL changed seems to be for the actual newtab url, i.e. "about:newtab", which is slightly different from the defaultUrl that backs it
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8906681 [details] Bug 1398819 - Turn on prerendered version of activity stream in aboutNewTabService https://reviewboard.mozilla.org/r/178400/#review183810 Just some test cleanup (to correctly `cleanup` ;)) ::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:96 (Diff revision 4) > + await new Promise(resolve => { > + Services.obs.addObserver(function observer(aSubject, aTopic, aData) { > + Services.obs.removeObserver(observer, aTopic); > + resolve(); > + }, "newtab-url-changed"); > + Services.prefs.setBoolPref("browser.newtabpage.activity-stream.prerender", false); Looks like you can now just use `nextChangeNotificationPromise` probably like: ```js const notificationPromise = nextChangeNotificationPromise("about:newtab"); Services.prefs.setBoolPref("browser.newtabpage.activity-stream.prerender", false); ``` Also, we should be cleaning up these pref changes in `cleanup` at top. And looks like it's cleaning up incorrectly by setting activity stream to false. Probably should just be doing `Services.prefs.clearUserPref`
Attachment #8906681 -
Flags: review?(edilee) → review+
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
Pushed by edilee@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8475a8df9f16 Turn on prerendered version of activity stream in aboutNewTabService r=Mardak
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8475a8df9f16
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Updated•5 years ago
|
Component: Activity Streams: Newtab → New Tab Page
You need to log in
before you can comment on or make changes to this bug.
Description
•