Closed Bug 1474410 Opened 6 years ago Closed 5 years ago

Move ActivityStream instantiation and uninit into AboutNewTab

Categories

(Firefox :: New Tab Page, enhancement, P2)

Product:
Firefox
Component:
New Tab Page
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Iteration:
63.2 - July 23
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: Mardak, Assigned: ursula)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1474410 - Move ActivityStream instantiation and uninit into AboutNewTab
59 bytes, text/x-review-board-request
Mardak
: review+
Details
Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4242
52 bytes, text/x-github-pull-request
Details | Review 
Basically doing what was described in bug 1447130 comment 2. This should probably happen after bug 1474406 as that version data comes from install.rdf -> startup options parameter.

To summarize, we'll use AboutNewTab.init to simulate an early bootstrap.startup call to wait for a delayed event to load ActivityStream module and instantiate (we can leave bootstrap.startup to also load and track module unload for now). Additionally have AboutNewTab.uninit trigger the instance's uninit.
We could probably also as part of this bug move AboutNewTab from browser/modules to browser/components/newtab by updating EXTRA_JS_MODULES into moz.build
Blocks: 1329955
Blocks: 1474411
Assignee: nobody → usarracini
Severity: normal → enhancement
Depends on: 1472038
I believe what I can also do now that we're moving the init to AboutNewtab.jsm, is remove all the stuff about the two RemotePages instance and have RemotePage register the about: pages in ActivityStreamMessageChannel.jsm . I can remove the existing pageListener stuff in AboutNewTab.jsm that was there for the old tiles and have one source of truth with RemotePages channel.
Do we actually create RemotePages from ActivityStreamMessageChannel? I would think we only use the one from AboutNewTab (and it's the only one that adds about:welcome right now too…). Notably, RemotePages from AboutNewTab registers earlier than ActivityStream loading.
Yeah we only use the RemotePages if we don't get one from AboutNewTab first (which, I don't know if will ever happens anymore since tiles isn't around), but I think we can consolidate that and just make one in ActivityStreamMessageChannel and just add about:welcome to that one. It happens earlier, but since we're initializing now in AboutNewTab, we're only moving it to slightly later. It would allow us to get rid of all this override/reset stuff, which would make things simpler. I tried a patch out and it seems to be working just fine.
ahillier had to fix some issues with early about:newtab pages in bug 1385090 that added the override passing. I'm not sure if it's still necessary given that we simulate messages for existing pages now. I don't see tests specifically in that bug, so we might need some manual testing to verify the change is good ??

Is ./mach run about:newtab sufficient to test?
Flags: needinfo?(ahillier)
Alternatively, we can keep `earlyRP = new RemotePages()` creation in AboutNewTab on the later startup notificaiton `new ActivityStream(earlyRP)`

Some reason I remember thinking we do want to leverage getting RemotePages started ASAP…
(In reply to Ed Lee :Mardak (PTO July 7-22) from comment #5)
> ahillier had to fix some issues with early about:newtab pages in bug 1385090
> that added the override passing. I'm not sure if it's still necessary given
> that we simulate messages for existing pages now. I don't see tests
> specifically in that bug, so we might need some manual testing to verify the
> change is good ??
> 
> Is ./mach run about:newtab sufficient to test?

I don't remember there being explicit automated testing of this issue. If the RP is being initialised only slightly later then I guess it probably won't be a problem.

I was definitely manually testing it by opening Firefox with lots of tabs (10+) all of which were about:newtab, then quiting and restarting and seeing if any pages got orphaned when they were brought back by session restore. I seem to recall being able to pretty reliably trigger the issue by doing that at the time.
Flags: needinfo?(ahillier)
Interestingly, doing `./mach run about:newtab` with only one instance of RP being created in ASMessageChannel doesn't work but doing Adam's manual testing multiple times seems to work just fine. Doing what you suggested earlier Ed where we create RP in AboutNewTab.jsm and pass it into new ActivityStream() seems to work both with `./mach run about:newtab` and with the manual testing. It's a bit hairier because we've got to go in and add options in the constructor for Store and ActivityStreamMessageChannel. Which probably involves fixing some tests on our side as well. So it does seem like we (for some reason??) need RP to be initialized just slightly earlier than other parts of AS.

So we can either:
1. Leave all the RP stuff exactly as is (maybe do a follow up to try and consolidate stuff later if we want) or
2. Do what you suggested, Ed, with passing RP along into Activity Stream

What do you think?
We can just keep the existing RemotePages behavior for now. There will probably be more than this change that can be optimized when we're a component, so not sure what's a good way to track it, but the cleanup would probably help bug 1439892 some too. I wonder if startup related improvements should block bug 1432587 meta?
Comment on attachment 8991940 [details]
Bug 1474410 - Move ActivityStream instantiation and uninit into AboutNewTab

https://reviewboard.mozilla.org/r/256848/#review263830

Pretty sure not doing the add-on-specific startup/install behavior in AboutNewTab (i.e., unconditionally observing instead of checking startingUp) will fix the startup test failures.

::: browser/modules/AboutNewTab.jsm:41
(Diff revision 2)
>      this.pageListener = pageListener || new RemotePages(["about:home", "about:newtab", "about:welcome"]);
> -    this.pageListener.addMessageListener("NewTab:Customize", this.customize);
> -    this.pageListener.addMessageListener("NewTab:MaybeShowMigrateMessage",
> -      this.maybeShowMigrateMessage);
> +
> +    if (Services.startup.startingUp) {
> +      Services.obs.addObserver(this, BROWSER_READY_NOTIFICATION);
> +    } else {
> +      this.onBrowserReady();

These code paths were to support add-on installation after startup. Because AboutNewTab.init happens later than startingUp, onBrowserReady gets called immediately instead of waiting for the notification. This is what is causing activity-stream/places to load too early.

We don't need the check for startingUp and probably don't need the waitingForBrowserReady stuff either (as I believe that was more for handling very quick pref changes that turned activity stream off and on again).

::: browser/modules/AboutNewTab.jsm:107
(Diff revision 2)
>      return null;
>    },
>  
>    reset(pageListener) {
>      this.isOverridden = false;
>      this.init(pageListener);

Until we clean up the overridden stuff, looks like `init` can be called later, so we probably want to only add the sessionstore observer when init is called with no arguments.
Attachment #8991940 - Flags: review?(edilee) → review-
Comment on attachment 8991940 [details]
Bug 1474410 - Move ActivityStream instantiation and uninit into AboutNewTab

https://reviewboard.mozilla.org/r/256848/#review264046

Looks to be passing tests! At least fix the removeObserver exception.

::: browser/modules/AboutNewTab.jsm:29
(Diff revision 3)
>  
> +  activityStream: null,
> +
> +  /**
> +   * init - Initializes an instance of ActivityStream. This could be called directly
> +   *        in browser.js or once the browser is ready via BROWSER_READY_NOTIFICATION

This comment doesn't seem quite right. `init` isn't actually called from browser.js or via BROWSER_READY_NOTIFICATION.

Probably should describe pageListener parameter behavior too.

::: browser/modules/AboutNewTab.jsm:47
(Diff revision 3)
> -    this.pageListener.addMessageListener("NewTab:MaybeShowMigrateMessage",
> -      this.maybeShowMigrateMessage);
>    },
>  
> -  maybeShowMigrateMessage({ target }) {
> -    AutoMigrate.shouldShowMigratePrompt(target.browser).then((prompt) => {
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference]),
> +  observe(subject, topic, data) {

nit: move the QI to probably the top and perhaps label the sections of the different "interfaces"

```
var AboutNewTab = {
  QueryInterface…
  
  // AboutNewTab
  
  pageListener…
  
  …
  
  // nsIObserver
  
  observe…
};
```

::: browser/modules/AboutNewTab.jsm:85
(Diff revision 3)
>      if (this.pageListener) {
>        this.pageListener.destroy();
>        this.pageListener = null;
>      }
> +
> +    Services.obs.removeObserver(this, BROWSER_READY_NOTIFICATION);

This is unnecessary and causes

14:50:52     INFO -  GECKO(8188) | JavaScript error: resource:///modules/AboutNewTab.jsm, line 85: NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIObserverService.removeObserver]
Attachment #8991940 - Flags: review?(edilee) → review+
Pushed by usarracini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f98112e49582
Move ActivityStream instantiation and uninit into AboutNewTab r=Mardak
https://hg.mozilla.org/mozilla-central/rev/f98112e49582
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/6848522edab49a1af51e7b748d96734da6f6f5ab
chore(mc): Port  Bug 1474410 - Move ActivityStream instantiation and uninit into AboutNewTab r=Mardak  (#4242)
Iteration: 63.3 - Aug 6 → 63.2 - July 23
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.