Skip to content

Enterprise Felt-is-Firefox with addons#10

Merged
lissyx merged 6 commits intoenterprise-mainfrom
_enterprise-FELTisFirefoxWithAddons_
Sep 11, 2025
Merged

Enterprise Felt-is-Firefox with addons#10
lissyx merged 6 commits intoenterprise-mainfrom
_enterprise-FELTisFirefoxWithAddons_

Conversation

@lissyx
Copy link
Contributor

@lissyx lissyx commented Sep 4, 2025

No description provided.

@lissyx lissyx force-pushed the _enterprise-FELTisFirefoxWithAddons_ branch from c123214 to 8ec612e Compare September 4, 2025 15:53
@lissyx lissyx force-pushed the _enterprise-FELTisFirefoxWithAddons_ branch from 8ec612e to af7e6b8 Compare September 4, 2025 16:18
@lissyx lissyx force-pushed the _enterprise-FELTisFirefoxWithAddons_ branch from af7e6b8 to 69261eb Compare September 4, 2025 16:44
@lissyx lissyx force-pushed the _enterprise-FELTisFirefoxWithAddons_ branch from c2cd2fd to f841947 Compare September 5, 2025 04:49
@lissyx lissyx force-pushed the _enterprise-FELTisFirefoxWithAddons_ branch from f841947 to e84b61d Compare September 5, 2025 04:49
[profile.release.build-override]
opt-level = 1

# optimizing glsl more makes a big difference in swgl build times
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels totally random? Merge error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was rebased so i need to check that


case "FeltParent:FirefoxAbnormalExit":
Services.ppmm.removeMessageListener("FeltParent:FirefoxAbnormalExit", this);
// TODO: What should we do, restart Firefox?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The launcher needs to do this I think to set up the IPC channel correctly.

);

if (this.feltXPCOM.isFeltUI()) {
this.feltXPCOM.setConsoleAddr("http://localhost:8000");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we thought about how to supply this in production?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet, that is something I'd like we discuss next monday

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add it to the doc so we can go over it (probably Tuesday with fiji).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea I had is that this is just setting a pref so it could be set at install time somehow ? ac21fc7#diff-6d0fd0afe07ce9bcbb6ddb882c13066bcbb500ab4bfcc96792c8005fd4b6fd99R375

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A repack?

"use strict";

browser.runtime.onUpdateAvailable.addListener(_details => {
// The New Tab built-in add-on is not designed to be updated at runtime.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New Tab? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah :p

@lissyx
Copy link
Contributor Author

lissyx commented Sep 9, 2025

@1rneh I'd like your review on the UI part and maybe on the tests as well

@lissyx
Copy link
Contributor Author

lissyx commented Sep 9, 2025

@mkaply I'd like your review on the Live Policies changes

Copy link
Contributor

@mkaply mkaply left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments, but from my perspective this looks sound as long as tests pass.

I'm going to build it locally this week.

)
);
}
_maybeCallbackPolicy(policyImpl, parsedParameters = undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you comment as to what this function does?Can you add a comment as to what this function does?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is code that was here and I moved out of the function because at some point it proved to be useful.

this._policies = lazy.macOSPoliciesParser.readPolicies(prefReader);
}

onPoliciesChanges(handler) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, but grammatically this doesn't make sense. What exactly is changing here?

Copy link
Contributor

@1rneh 1rneh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some comments and questions. No blockers though, since our goal is to get this landed. But I'd like to re-visit this with a bit more time on our hands.

addJSWindowActors(actors) {
this._addActors(actors, "JSWindowActor");
},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use ChromeUtils.registerWindowActor in onStartUp like we do with ChromeUtils.unregisterWindowActor in onShutDown. It's not clear to me why we need to register and unregister the felt actors via preference?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was complicated because we were depending on the matches pref and the JSWindowActor C++ code expects things to be set once. So we could not really register until we had the proper information.

With the recent evolutions, this might not be required anymore?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, now we have all prefs on startup and we can register them right away.

);

if (this.feltXPCOM.isFeltUI()) {
this.feltXPCOM.setConsoleAddr("http://localhost:8000");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A repack?

Services.cpmm.addMessageListener("FeltMain:RedirectURL", this);
Services.cpmm.sendAsyncMessage("FeltChild:Loaded", {});
}, 0);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use actorCreated here, so we don't need setTimeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

return;
}

this.sendAsyncMessage("FeltChild:StartFirefox", {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid that the user sees a glimpse of the dashboard in the sso frame before we start another Firefox instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's macOS specific as I dont get what you mean :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll raise this again once I merged my UI changes.

Services.ppmm.removeMessageListener("FeltChild:Loaded", this);
const redirect_after_sso = Services.prefs.getStringPref("browser.felt.redirect_after_sso");
Services.ppmm.broadcastAsyncMessage("FeltMain:RedirectURL", redirect_after_sso);
break;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason why FeltChild doesn't just get the preference once it receives the FeltParent:Done message? This way we could save ourselves the FeltChildLoaded and FeltMain:RedirectURL messages.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized that the child never receives the message FeltParent:Done because the parent doesn't send it anywhere, so we never redirect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I broke things at some point and the FeltParent:Done message went missing. Writing tests made me catch this.

For the prefs it's likely a consequences of the old way of fetching prefs from console. I'll see if we can simplify this right now.

Alexandre Lissy added 5 commits September 11, 2025 15:37
addJSWindowActors(actors) {
this._addActors(actors, "JSWindowActor");
},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, now we have all prefs on startup and we can register them right away.

Comment on lines +9 to +105
let JSWINDOWACTORS = {};

JSWINDOWACTORS.Felt = {
parent: {
esModuleURI: "chrome://felt/content/FeltParent.sys.mjs",
},

child: {
esModuleURI: "chrome://felt/content/FeltChild.sys.mjs",
events: {
DOMContentLoaded: {},
load: {},
},
},

allFrames: true,
matches: [""],

onAddActor(register, unregister) {
let isRegistered = false;

const maybeRegister = () => {
const isEnabled = Services.prefs.getBoolPref(
"browser.felt.enabled",
false
);
if (isEnabled) {
JSWINDOWACTORS.Felt.matches = Services.prefs
.getStringPref("browser.felt.matches")
.split(",");
if (!isRegistered) {
register();
isRegistered = true;
}
} else if (isRegistered) {
unregister();
isRegistered = false;
}
};

Services.prefs.addObserver("browser.felt.enabled", maybeRegister);
maybeRegister();
},
};

let ActorManagerParent = {
_addActors(actors, kind) {
let register, unregister;
switch (kind) {
case "JSWindowActor":
register = ChromeUtils.registerWindowActor;
unregister = ChromeUtils.unregisterWindowActor;
break;
default:
throw new Error("Invalid JSActor kind " + kind);
}
for (let [actorName, actor] of Object.entries(actors)) {
// The actor defines its own register/unregister logic.
if (actor.onAddActor) {
actor.onAddActor(
() => register(actorName, actor),
() => unregister(actorName, actor)
);
continue;
}

// If enablePreference is set, only register the actor while the
// preference is set to true.
if (actor.enablePreference) {
Services.prefs.addObserver(actor.enablePreference, () => {
const isEnabled = Services.prefs.getBoolPref(
actor.enablePreference,
false
);
if (isEnabled) {
register(actorName, actor);
} else {
unregister(actorName, actor);
}
if (actor.onPreferenceChanged) {
actor.onPreferenceChanged(isEnabled);
}
});

if (!Services.prefs.getBoolPref(actor.enablePreference, false)) {
continue;
}
}

register(actorName, actor);
}
},

addJSWindowActors(actors) {
this._addActors(actors, "JSWindowActor");
},
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any use of enablePreference on our actors, so the whole actor registration logic can be streamlined to

registerActors() {
    ChromeUtils.registerWindowActor("Felt", {
      parent: {
        esModuleURI: "chrome://felt/content/FeltParent.sys.mjs",
      },

      child: {
        esModuleURI: "chrome://felt/content/FeltChild.sys.mjs",
        events: {
          DOMContentLoaded: {},
          load: {},
        },
      },
      allFrames: true,
      matches: Services.prefs.getStringPref("browser.felt.matches").split(","),
    });
  }

and then calling this.registerActors() instead of ActorManagerParent.addJSWindowActors(JSWINDOWACTORS); in onStartUp.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, now we should be able to do it. I'll look at that in a follow up, or if you want to fold it in your patches it's fine as well

return;
}

this.sendAsyncMessage("FeltChild:StartFirefox", {});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll raise this again once I merged my UI changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants