1653231 - Use literals instead of run-time length calculation, for StaticPrefs and others

Status: RESOLVED FIXED

(Core :: Preferences: Backend, task)

Comment 1

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Use literals instead of run-time conversion to nsLiteralCString. r=njn

Depends on D83771

Comment 2

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Simplify PrefName. r=njn

It doesn't need to use a Variant anymore, it always stores a nsCString.

Comment 3

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Pass const char* argument by value. r=njn

Depends on D83916

Comment 4

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Pass const nsCString& rather than const char* where possible. r=njn

Depends on D83772

Comment 5

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Store a nsDependentCString in Pref. r=njn

Depends on D83918

Comment 6

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Change SharedPrefMapBuilder::Add to accept a const nsCString&. r=njn

Depends on D84141

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Change PrefsHasher::Lookup to const nsCString. r=njn

Depends on D84142

Comment 8

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Removed Pref::Name and its remaining uses. r=njn

Depends on D84143

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed60bfa0c948
Simplify PrefName. r=njn
https://hg.mozilla.org/integration/autoland/rev/c2adc13661cd
Pass const char* argument by value. r=njn
https://hg.mozilla.org/integration/autoland/rev/e08347e255d4
Use literals instead of run-time conversion to nsLiteralCString. r=njn
https://hg.mozilla.org/integration/autoland/rev/b08b791a12ef
Pass const nsCString& rather than const char* where possible. r=njn
https://hg.mozilla.org/integration/autoland/rev/7ad5e5ca155b
Store a nsDependentCString in Pref. r=njn
https://hg.mozilla.org/integration/autoland/rev/6e14cc79a42d
Change SharedPrefMapBuilder::Add to accept a const nsCString&. r=njn

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/ed60bfa0c948
https://hg.mozilla.org/mozilla-central/rev/c2adc13661cd
https://hg.mozilla.org/mozilla-central/rev/e08347e255d4
https://hg.mozilla.org/mozilla-central/rev/b08b791a12ef
https://hg.mozilla.org/mozilla-central/rev/7ad5e5ca155b
https://hg.mozilla.org/mozilla-central/rev/6e14cc79a42d

Comment 11

[Simon Giesecke [:sg] [he/him]]

I now have this huge patch (https://hg.mozilla.org/try/rev/62a2518d444d1d737449b16b45ea55d2f8b24941) that changes the Preferences API to accept const nsCString& rather than const char*, which builds fine on Linux debug (still need to check for platform-specific issues elsewhere). I am not completely sure what to do with that. As is, it cannot be split up, but that's probably very hard to review. I could try to change it into a sequence of:

• One patch that adds new overloads that accept const nsCString&
• Several patches per subdirectory that change the uses of the API to use the new overloads
• One patch that removes the old overloads that accept const char*

Nicholas, what do you think?

Comment 12

[Nicholas Nethercote [inactive]]

I think a single patch is fine. Although it will be very large, it's basically doing one thing, so from a reviewability point of view a single patch isn't any worse.

Having said that, a single patch is worse in terms of investigating problems if something goes wrong. But I'll let you decide how best to handle that risk :)

Comment 13

[Nicholas Nethercote [inactive]]

But please do that change in a separate bug! :)

Comment 14

[Simon Giesecke [:sg] [he/him]]

(In reply to Nicholas Nethercote [:njn] from comment #12)

I think a single patch is fine. Although it will be very large, it's basically doing one thing, so from a reviewability point of view a single patch isn't any worse.

Having said that, a single patch is worse in terms of investigating problems if something goes wrong. But I'll let you decide how best to handle that risk :)

I think I will submit a single patch then. I think to really improve investigating problems the granularity would need to be even smaller than I was thinking of and that would result in a really large number of patches.

(In reply to Nicholas Nethercote [:njn] from comment #13)

But please do that change in a separate bug! :)

Sure, I opened Bug 1657293 for that.

Comment 15

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1653231 - Remove now-unused Add/Has overloads. r=njn

Depends on D84145

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sg, maybe it's time to close this bug?

Comment 17

[Simon Giesecke [:sg] [he/him]]

The remaining patches haven't landed yet, because I was not sure if they make sense without Bug 1657293, and that's pretty stalled. I will check that again and either move them over to that Bug, move them to a new bug, or land them. In either case, this Bug can then eventually be closed.

Comment 18

[Phabricator Automation]

Comment on attachment 9164680 [details]
Bug 1653231 - Change PrefsHasher::Lookup to const nsCString. r=njn

Revision D84143 was moved to bug 1657293. Setting attachment 9164680 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9164682 [details]
Bug 1653231 - Removed Pref::Name and its remaining uses. r=njn

Revision D84145 was moved to bug 1657293. Setting attachment 9164682 [details] to obsolete.

Comment 20

[Phabricator Automation]

Comment on attachment 9168464 [details]
Bug 1653231 - Remove now-unused Add/Has overloads. r=njn

Revision D86187 was moved to bug 1657293. Setting attachment 9168464 [details] to obsolete.

Comment 21

[Simon Giesecke [:sg] [he/him]]

Moved remaining patches over to Bug 1657293. The other patches went into FF80, updating the milestone accordingly.