Closed Bug 1350411 Opened 7 years ago Closed 7 years ago

Add Message Manager to the Activity Stream system add-on

Categories

(Firefox Graveyard :: Activity Streams: General, enhancement, P1)

Product:
Firefox Graveyard
Component:
Activity Streams: General
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: k88hudson, Assigned: k88hudson)

References

Details

Attachments

(1 file)

Bug 1350411 - Add Message Channel for Activity Stream system add-on
59 bytes, text/x-review-board-request
mconley
: review+
Details 
We need to land our Message Manager, which creates a connection with about:newtab content code via the RemotePageManager.

The commits were reviewed on Github in this PR:
https://github.com/mozilla/activity-stream/pull/2278
Assignee: nobody → khudson
This patch adds a MessageManager and a NEW_TAB_INITIAL_STATE action, which initializes each new tab page with a copy of the parent Redux state. To manually test it, you can flip the browser.newtabpage.activity-stream.enabled pref and make sure an unstyled list of default Top Sites (Facebook, Yahoo, etc.) shows up when a new tab page loads.
Attachment #8856543 - Flags: review?(mconley)
Blocks: 1354559
Priority: -- → P1
Comment on attachment 8856543 [details]
Bug 1350411 - Add Message Channel for Activity Stream system add-on

https://reviewboard.mozilla.org/r/128490/#review131548

This looks good, as far as I can tell - really, I feel like MessageManager.jsm is the only one of these changes that I really feel comfortable reviewing. The rest, I feel, you'd need someone more familiar with what's going on inside Activity Stream.

Out of curiosity, how chatty is Activity Stream over this message channel? (I feel like I've already asked this before, but I've forgotten the answer).

::: browser/extensions/activity-stream/lib/MessageManager.jsm:10
(Diff revision 1)
> +
> +"use strict";
> +
> +const {utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/RemotePageManager.jsm");

This should be a lazy module getter, I think.

::: browser/extensions/activity-stream/lib/MessageManager.jsm:13
(Diff revision 1)
> +const {
> +  actionUtils: au,
> +  actionCreators: ac,
> +  actionTypes: at
> +} = Cu.import("resource://activity-stream/common/Actions.jsm", {});

You might want to make each of these a lazy module getter as well.

::: browser/extensions/activity-stream/lib/MessageManager.jsm:25
(Diff revision 1)
> +  "resource:///modules/AboutNewTab.jsm");
> +
> +const ABOUT_NEW_TAB_URL = "about:newtab";
> +
> +const DEFAULT_OPTIONS = {
> +  dispatch(action) { dump(`\nMessage manager: Received action ${action.type}, but no dispatcher was defined.\n`); },

This should probably throw instead.

::: browser/extensions/activity-stream/lib/MessageManager.jsm:31
(Diff revision 1)
> +  pageURL: ABOUT_NEW_TAB_URL,
> +  outgoingMessageName: "ActivityStream:MainToContent",
> +  incomingMessageName: "ActivityStream:ContentToMain"
> +};
> +
> +class MessageManager {

We already have something called a MessageManager in Gecko - and I worry that overloading the name like this could add confusion down the road.

Can we call this something else instead?

::: browser/extensions/activity-stream/lib/MessageManager.jsm:32
(Diff revision 1)
> +  /**
> +   * MessageManager - This module connects a Redux store to a RemotePageManager in Firefox.
> +   *                  Call .createChannel to start the connection, and .destroyChannel to destroy it.
> +   *                  You should use the BroadcastToContent, SendToContent, and SendToMain action creators
> +   *                  in common/Actions.jsm to help you create actions that will be automatically routed
> +   *                  to the correct location.
> +   *

Great documentation - thanks.

::: browser/extensions/activity-stream/lib/MessageManager.jsm:41
(Diff revision 1)
> +   * @param  {string} options.pageURL The URL to which a RemotePageManager should be attached.
> +   *                                  Note that if it is about:newtab, the existing RemotePageManager
> +   *                                  for about:newtab will also be disabled

Out of curiosity, why the configurability here? Do we expect to use this library for another page?

::: browser/extensions/activity-stream/lib/MessageManager.jsm:192
(Diff revision 1)
> +this.MessageManager = MessageManager;
> +this.DEFAULT_OPTIONS = DEFAULT_OPTIONS;
> +this.EXPORTED_SYMBOLS = ["MessageManager", "DEFAULT_OPTIONS"];

The first two are redundant and can be removed.

For EXPORTED_SYMBOLS, those generally go towards the top of the file - usually before the first Cu.imports - though, if these are now convention under the activity-stream folder, well, so be it.

::: browser/extensions/activity-stream/lib/NewTabInit.jsm:7
(Diff revision 1)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +const {utils: Cu} = Components;
> +const {actionTypes: at, actionCreators: ac} = Cu.import("resource://activity-stream/common/Actions.jsm", {});

Should make these lazy, if you can.
Attachment #8856543 - Flags: review?(mconley) → review-
Comment on attachment 8856543 [details]
Bug 1350411 - Add Message Channel for Activity Stream system add-on

https://reviewboard.mozilla.org/r/128490/#review132254

Thanks k88hudson, I dig the new name.

I've started looking at this through a performance lens now, and I've been looking at the rest of the patch (and not just the ActivityStreamMessageChannel), and I'm curious about something:

Is it necessary to have so many JSM's? One of the things that Quantum Flow has identified is that accessing JSM's results in a non-zero overhead cost because of something called "cross-compartment wrappers". The more things can be contained within a single JSM, the better, because we avoid that overhead entirely.

I see you're adding some new JSM's here, and I'm just a little worried that we're adding to that problem for Activity Stream. This doesn't have to happen in this patch, but I want your opinion: would it be possible to combine many of these JSM's into a single JSM? That way, you could at least avoid the overhead of them talking amongst each other.

I'm eager to hear your thoughts on that.

::: browser/extensions/activity-stream/data/content/activity-stream.html:15
(Diff revision 2)
> +      <ul id="top-sites"></ul>
> +      <script>
> +        const topSitesEl = document.getElementById("top-sites");
> +        addMessageListener("ActivityStream:MainToContent", msg => {
> +          if (msg.data.type === "NEW_TAB_INITIAL_STATE") {
> +            msg.data.data.TopSites.rows.forEach(row => {

Can you use `for...of` instead? Unless there's a really great reason, I suspect the function construction overhead is not worth it (see https://bugzilla.mozilla.org/show_bug.cgi?id=1355874).

::: browser/extensions/activity-stream/data/content/activity-stream.html:16
(Diff revision 2)
> +              const li = document.createElement("li");
> +              const a = document.createElement("a");
> +              a.href = row.url;
> +              a.textContent = row.title;
> +              li.appendChild(a);
> +              topSitesEl.appendChild(li);

It strikes me that we should probably do this using documentFragment, to avoid the cost of adding each one of these to the DOM one by one.

::: browser/extensions/activity-stream/lib/ActivityStreamMessageChannel.jsm:28
(Diff revision 2)
> +
> +const ABOUT_NEW_TAB_URL = "about:newtab";
> +
> +const DEFAULT_OPTIONS = {
> +  dispatch(action) {
> +    throw new Error(`\nMessage manager: Received action ${action.type}, but no dispatcher was defined.\n`);

Probably should change this to "MessageChannel" too.
Attachment #8856543 - Flags: review?(mconley) → review-
Comment on attachment 8856543 [details]
Bug 1350411 - Add Message Channel for Activity Stream system add-on

https://reviewboard.mozilla.org/r/128490/#review131548

> You might want to make each of these a lazy module getter as well.

In this case Actions.jsm will definitely be loaded by Activity Stream (which is itself lazily loaded, so I don't think it will be necessary here

> Out of curiosity, why the configurability here? Do we expect to use this library for another page?

It also needs to be used for about:home, so we might need to use it for that case depending on how it's implemented

> The first two are redundant and can be removed.
> 
> For EXPORTED_SYMBOLS, those generally go towards the top of the file - usually before the first Cu.imports - though, if these are now convention under the activity-stream folder, well, so be it.

Unfortunately there is a really weird thing that happens when Cu.import is used with destructuring if variables are not explictly added to this... however, I can move the this.MessageManager to the class definition at least.

> Should make these lazy, if you can.

Like the above, actions are definitely going to be already imported
Comment on attachment 8856543 [details]
Bug 1350411 - Add Message Channel for Activity Stream system add-on

https://reviewboard.mozilla.org/r/128490/#review132254

Personally, I'm a fan of using appropriately-named smaller modules – I find that it makes code easier to test, reuse, and refactor. We did do a little bit of work to refactor into fewer files, but if there's still a signficant performance overhead, we can do some more; we'll just need to discuss it with the group. I filed an issue on our github repo: https://github.com/mozilla/activity-stream/issues/2419
Comment on attachment 8856543 [details]
Bug 1350411 - Add Message Channel for Activity Stream system add-on

https://reviewboard.mozilla.org/r/128490/#review133922

This seems okay to me, let's do it. Thanks k88hudson!
Attachment #8856543 - Flags: review?(mconley) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ce2f726a2b86
Add Message Channel for Activity Stream system add-on r=mconley
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ce2f726a2b86
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Blocks: 1359481
Depends on: 1350409
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: