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)

defect
Not set
normal

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.
Assignee: nobody → khudson
If this can land before the activity stream prerender export, there'll be different patches needed for the all_files_referenced test
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)
Attachment #8906681 - Flags: review?(edilee)
Depends on: 1398239
Ok, I've updated this patch to remove the unreferenced file, so it should be good to go
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-
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 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+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8475a8df9f16
Turn on prerendered version of activity stream in aboutNewTabService r=Mardak
Blocks: 1399226
https://hg.mozilla.org/mozilla-central/rev/8475a8df9f16
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.