Skip to content

feat: Add --chromium-pref to CLI options - #3664

Merged
Rob--W merged 14 commits into
mozilla:masterfrom
PatrykKuniczak:feat/enable-dev-mode-on-chromium
Apr 27, 2026
Merged

feat: Add --chromium-pref to CLI options#3664
Rob--W merged 14 commits into
mozilla:masterfrom
PatrykKuniczak:feat/enable-dev-mode-on-chromium

Conversation

@PatrykKuniczak

@PatrykKuniczak PatrykKuniczak commented Mar 20, 2026

Copy link
Copy Markdown
Contributor

Basing on #2912, I've taken couple commits and a bunch of code.

I've added request changes of @Rob--W review.

I've converted customPrefs to Map() because of https://github.com/mozilla/web-ext/pull/2912/changes#r1704363575

Let's guide me, how to merge this PR successfully, because a bunch of community of wxt-dev/wxt#137 waiting for this, and i want to do it and use in our codebase 😸

I hope this is understable and nothing'll block me to do this 😆

@PatrykKuniczak PatrykKuniczak changed the title feat: Add --chromium-pref` to CLI options feat: Add --chromium-pref to CLI options Mar 20, 2026
@PatrykKuniczak

PatrykKuniczak commented Mar 20, 2026

Copy link
Copy Markdown
Contributor Author

Wait a second coerceCLICustomChromiumPreference is the same as coerceCLICustomPreference, but there's const customPrefs = {};

I'll remove coerceCLICustomChromiumPreference because that's redundant, but @Rob--W tell me, if you want Map() like it's now, and i'll convert coerceCLICustomPreference to use Map() and remove regexp.

I'm waiting to your response and then i'll quickly adjust this PR 😄

@Rob--W Rob--W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This PR makes more changes than strictly necessary to implement the functionality, such as moving several functions around without obvious reason.

Could you undo these unnecessary changes to make it easier to review the code? It has been a long while since I have reviewed the original unmerged PR so I basically have to review from scratch because the PR has escaped my human context window in the meantime.

@PatrykKuniczak

Copy link
Copy Markdown
Contributor Author

@Rob--W Which changes are you want me to undo?

@PatrykKuniczak
PatrykKuniczak requested a review from Rob--W March 21, 2026 20:15
@Rob--W

Rob--W commented Mar 22, 2026

Copy link
Copy Markdown
Member

@Rob--W Which changes are you want me to undo?

Take a look at the diff of your PR: https://github.com/mozilla/web-ext/pull/3664/files
There are a few places where code moved around or modified with no apparent connection to the feature this patch is intended to provide.

@PatrykKuniczak

PatrykKuniczak commented Mar 23, 2026

Copy link
Copy Markdown
Contributor Author

@Rob--W DONE

Pls review it.

I'm able to convert everything from object to Map, if you'll want that, but in other PR.

preferences.js

  if (/[^\w{@}.-]/.test(key)) {
      throw new UsageError(`Invalid custom preference name: ${key}`);
    }

This can be removed as you've suggested here, but maybe not on this PR, because it's not related.

But your words:
including ":" characters which are rejected by your current code
It will be also rejected here, maybe omit this regexp for chrome?

Let's give me move advice i want to do it, not walking in the dark

@PatrykKuniczak

Copy link
Copy Markdown
Contributor Author

@Rob--W Any moves forward?

@Rob--W

Rob--W commented Mar 28, 2026

Copy link
Copy Markdown
Member

Thanks for fixing the import order. I'll take a look next week.

@PatrykKuniczak

Copy link
Copy Markdown
Contributor Author

@Rob--W Fun fact, i've fixed it then, but i've forgot to push it, and today i found it by mistake and pushed it XD

@PatrykKuniczak

Copy link
Copy Markdown
Contributor Author

@Rob--W This week? :)

@PatrykKuniczak

Copy link
Copy Markdown
Contributor Author

@willdurand Maybe you're able to check it?

@Rob--W

Rob--W commented Apr 13, 2026

Copy link
Copy Markdown
Member

I was away, am back now. I'll take a look later this week, ideally before Thursday.

@PatrykKuniczak

Copy link
Copy Markdown
Contributor Author

@Rob--W Thanks for response, i'll be able to make any adjustments on the end of this week, if it'll be necessary 😸

@Rob--W Rob--W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. Thanks for your contribution!

The PR currently has a CI failure, due to audit failures that are likely unrelated. Could you rebase the patch (or merge the main branch) to see if the tests pass?

@PatrykKuniczak
PatrykKuniczak force-pushed the feat/enable-dev-mode-on-chromium branch from cfe0b24 to d261160 Compare April 16, 2026 09:09
@PatrykKuniczak

PatrykKuniczak commented Apr 16, 2026

Copy link
Copy Markdown
Contributor Author

@Rob--W WOOOW, looks good :)

I'm thrilled to see it on next release.
Thanks for support

@Rob--W Rob--W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I tried out your patch, expecting Developer Mode to be turned on by default. However, that did not work. It turns out that you omitted a prefs: this.getPrefs(), call. I have suggested an implementation change that fixes the issue.

However, the unit tests pass with and without the change, signaling the lack of necessary test coverage. Could you add the necessary test coverage?

  • verify that the developer_mode pref defaults to true. See the installs and runs the extension test for inspiration on how you can verify the launch options. The calledWithMatch accepts objects when the specified properties overlap, so you don't have to include all other properties.
  • merges default and custom preferences from an object should also check that the passed prefs are forwarded to the launcher.

Comment thread src/extension-runners/chromium.js
@PatrykKuniczak

PatrykKuniczak commented Apr 23, 2026

Copy link
Copy Markdown
Contributor Author

I tried out your patch, expecting Developer Mode to be turned on by default. However, that did not work. It turns out that you omitted a prefs: this.getPrefs(), call. I have suggested an implementation change that fixes the issue.

However, the unit tests pass with and without the change, signaling the lack of necessary test coverage. Could you add the necessary test coverage?

  • verify that the developer_mode pref defaults to true. See the installs and runs the extension test for inspiration on how you can verify the launch options. The calledWithMatch accepts objects when the specified properties overlap, so you don't have to include all other properties.
  • merges default and custom preferences from an object should also check that the passed prefs are forwarded to the launcher.

@Rob--W Yeah no problem, done :)

Comment thread tests/unit/test-extension-runners/test.chromium.js Outdated
Comment thread tests/unit/test-extension-runners/test.chromium.js Outdated

@Rob--W Rob--W left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should be good now, thanks!

There is still a CI error, but that is due to an unrelated audit failure.

@Rob--W
Rob--W merged commit 3b2a518 into mozilla:master Apr 27, 2026
3 of 4 checks passed
@PatrykKuniczak
PatrykKuniczak deleted the feat/enable-dev-mode-on-chromium branch April 27, 2026 17:02
@PatrykKuniczak

Copy link
Copy Markdown
Contributor Author

@Rob--W WOW :)

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.

3 participants