Skip to content

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

Open
PatrykKuniczak wants to merge 8 commits intomozilla:masterfrom
PatrykKuniczak:feat/enable-dev-mode-on-chromium
Open

feat: Add --chromium-pref to CLI options#3664
PatrykKuniczak wants to merge 8 commits intomozilla:masterfrom
PatrykKuniczak:feat/enable-dev-mode-on-chromium

Conversation

@PatrykKuniczak
Copy link
Copy Markdown

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 chromiumPrefs to Map() because of https://github.com/mozilla/web-ext/pull/2912/changes#r1704363575
I hope you'll like that, because you've suggested to convert only coerceCLICustomChromiumPreference func, but i thought it's good idea to use Map() like it's now, but maybe that's not good idea.

I see

const customPrefs = {};

have object

And there's a question, are you want my solution and probably want to convert coerceCLICustomPreference and entire pref to be Map()(In other PR) or want me to revert that change and use object for all places excluding coerceCLICustomChromiumPreference or maybe you want 1 approach here and other in coerceCLICustomPreference(IMO this should be consistent)

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 code 😸

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
Copy link
Copy Markdown
Author

PatrykKuniczak commented Mar 20, 2026

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 😄

Copy link
Copy Markdown
Member

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

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
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
Copy link
Copy Markdown
Member

Rob--W commented Mar 22, 2026

@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
Copy link
Copy Markdown
Author

PatrykKuniczak commented Mar 23, 2026

@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
Author

@Rob--W Any moves forward?

@Rob--W
Copy link
Copy Markdown
Member

Rob--W commented Mar 28, 2026

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

@PatrykKuniczak
Copy link
Copy Markdown
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

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