feat: Add --chromium-pref to CLI options#3664
feat: Add --chromium-pref to CLI options#3664PatrykKuniczak wants to merge 2 commits intomozilla:masterfrom
--chromium-pref to CLI options#3664Conversation
--chromium-pref` to CLI options--chromium-pref to CLI options
|
Wait a second I'll remove I'm waiting to your response and then i'll quickly adjust this PR 😄 |
Rob--W
left a comment
There was a problem hiding this comment.
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.
|
@Rob--W Which changes are you want me to undo? |
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#r1704363575I hope you'll like that, because you've suggested to convert only
coerceCLICustomChromiumPreferencefunc, but i thought it's good idea to useMap()like it's now, but maybe that's not good idea.I see
web-ext/src/firefox/preferences.js
Line 120 in 308b344
have object
And there's a question, are you want my solution and probably want to convert
coerceCLICustomPreferenceand entireprefto beMap()(In other PR) or want me to revert that change and use object for all places excludingcoerceCLICustomChromiumPreferenceor maybe you want 1 approach here and other incoerceCLICustomPreference(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 😆