1407494 - Some Preferences clean-ups

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

There's some stuff that can be cleaned up.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 1) - Remove C-isms. .

Review commit: https://reviewboard.mozilla.org/r/188576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188576/

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 2) - Remove TEST_PREFREAD. .

It's unclear how to compile it; it isn't a proper test, because it just prints
stuff out without explicitly checking anything; and I bet nobody has run it in
a long time.

Review commit: https://reviewboard.mozilla.org/r/188578/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188578/

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 3) - Remove PR_ALIGN_OF_WORD and WORD_ALIGN_MASK. .

They're unused.

Review commit: https://reviewboard.mozilla.org/r/188580/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188580/

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 4) - Remove have_PrefChangedFunc_typedef. .

This is detritus from old changes that can be cleaned up now.

The patch removes the declaration of PrefChangedFunc from Preferences.cpp
because it's also in Preferences.h, which is included by Preferences.cpp.

The patch also removes the part of the comment about passing a non-zero result
because it's clearly false -- the callback has no return value.

Review commit: https://reviewboard.mozilla.org/r/188582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188582/

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 5) - Replace uses of NS_strdup(), PL_strdup(), PL_strfree() with more standard functions. .

Review commit: https://reviewboard.mozilla.org/r/188584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188584/

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 6) - Remove GetContentChild(). .

We can just use XRE_IsContentProcess() instead.

Review commit: https://reviewboard.mozilla.org/r/188586/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188586/

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 7) - Replace malloc uses with operator new. .

Review commit: https://reviewboard.mozilla.org/r/188588/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188588/

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 8) - Remove forward declaration of PrefSetting. .

It's declared in Preferences.h, which Preferences.cpp includes.

Review commit: https://reviewboard.mozilla.org/r/188590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188590/

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Bug 1407494 (part 9) - Merge PREF_RegisterCallback() and PREF_RegisterPriorityCallback. .

This avoids some code duplication.

Review commit: https://reviewboard.mozilla.org/r/188592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188592/

Comment 10

[Mike Hommey [:glandium]]

Comment on attachment 8917633 [details]
Bug 1407494 (part 1) - Remove C-isms. .

https://reviewboard.mozilla.org/r/188576/#review193882

Comment 11

[Mike Hommey [:glandium]]

Comment on attachment 8917634 [details]
Bug 1407494 (part 2) - Remove TEST_PREFREAD. .

https://reviewboard.mozilla.org/r/188578/#review193884

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8917635 [details]
Bug 1407494 (part 3) - Remove PR_ALIGN_OF_WORD and WORD_ALIGN_MASK. .

https://reviewboard.mozilla.org/r/188580/#review193886

::: commit-message-c9184:3
(Diff revision 1)
> +Bug 1407494 (part 3) - Remove PR_ALIGN_OF_WORD and WORD_ALIGN_MASK. r=glandium.
> +
> +They're unused.

For funz, you may want to add that the only use of WORD_ALIGN_MASK was removed in bug 200524, 14.5 years ago. And PR_ALIGN_OF_WORD was only there for its definition, and only in case NSPR didn't have the define.

Comment 13

[Mike Hommey [:glandium]]

Comment on attachment 8917636 [details]
Bug 1407494 (part 4) - Remove have_PrefChangedFunc_typedef. .

https://reviewboard.mozilla.org/r/188582/#review193888

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 8917637 [details]
Bug 1407494 (part 5) - Replace uses of NS_strdup(), PL_strdup(), PL_strfree() with more standard functions. .

https://reviewboard.mozilla.org/r/188584/#review193890

::: modules/libpref/Preferences.cpp:864
(Diff revision 1)
>      } else {
>        stringVal = pref->mUserPref.mStringVal;
>      }
>  
>      if (stringVal) {
> -      *aValueOut = NS_strdup(stringVal);
> +      *aValueOut = strdup(stringVal);

NS_strdup is infallible. stdup is fallible. You can't change this like that. Either you keep NS_strdup, or you return an OOM error when strdup failed.

::: modules/libpref/Preferences.cpp:2593
(Diff revision 1)
>  {
>    nsresult rv = GetCharPref(aPrefName, aRetVal);
>  
>    if (NS_FAILED(rv) && aArgc == 1) {
>      NS_ENSURE_ARG(aDefaultValue);
> -    *aRetVal = NS_strdup(aDefaultValue);
> +    *aRetVal = strdup(aDefaultValue);

likewise

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8917638 [details]
Bug 1407494 (part 6) - Remove GetContentChild(). .

https://reviewboard.mozilla.org/r/188586/#review193892

::: commit-message-1efd8:3
(Diff revision 1)
> +Bug 1407494 (part 6) - Remove GetContentChild(). r=glandium.
> +
> +We can just use XRE_IsContentProcess() instead.

Might be worth indicating that its result was never used more than as a bool, so XRE_IsContentProcess fulfils the same need.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8917639 [details]
Bug 1407494 (part 7) - Replace malloc uses with operator new. .

https://reviewboard.mozilla.org/r/188588/#review193894

::: modules/libpref/Preferences.cpp:3121
(Diff revision 1)
> -    outArray = (char**)moz_xmalloc(numPrefs * sizeof(char*));
> +    outArray = new char*[numPrefs];
> -    if (!outArray) {

funny one, calling infallible malloc and testing the result.

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8917640 [details]
Bug 1407494 (part 8) - Remove forward declaration of PrefSetting. .

https://reviewboard.mozilla.org/r/188590/#review193896

Comment 18

[Mike Hommey [:glandium]]

Comment on attachment 8917641 [details]
Bug 1407494 (part 9) - Merge PREF_RegisterCallback() and PREF_RegisterPriorityCallback. .

https://reviewboard.mozilla.org/r/188592/#review193898

::: modules/libpref/Preferences.cpp:1326
(Diff revision 1)
> -}
> -
> +  } else {
> +    // Add to the start of the non-priority part of the list.
> -// Adds a node to the end of the callback list.
> -void
> -PREF_RegisterCallback(const char* aPrefNode,
> -                      PrefChangedFunc aCallback,
> -                      void* aData)
> -{
> -  NS_PRECONDITION(aPrefNode, "aPrefNode must not be nullptr");
> -  NS_PRECONDITION(aCallback, "aCallback must not be nullptr");
> -
> -  auto node = new CallbackNode();
> -  node->mDomain = strdup(aPrefNode);
> -  node->mFunc = aCallback;
> -  node->mData = aData;
> -  if (gLastPriorityNode) {
> +    if (gLastPriorityNode) {
> -    node->mNext = gLastPriorityNode->mNext;
> +      node->mNext = gLastPriorityNode->mNext;
> -    gLastPriorityNode->mNext = node;
> +      gLastPriorityNode->mNext = node;
> -  } else {
> +    } else {
> -    node->mNext = gFirstCallback;
> +      node->mNext = gFirstCallback;
> -    gFirstCallback = node;
> +      gFirstCallback = node;
> -  }
> +    }
> -}
> +  }

} else if () {
} else {
}

instead of

} else {
  if () {
  } else {
  }
}

Comment 19

[Nicholas Nethercote [inactive]]

> } else if () {
> } else {
> }
> 
> instead of
> 
> } else {
>   if () {
>   } else {
>   }
> }

I'd prefer to keep it as is, because there's a comment that applies the the outer `else`.

Comment 20

[Nicholas Nethercote [inactive]]

I changed part 5 to use moz_xstrdup().

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8917635 [details]
Bug 1407494 (part 3) - Remove PR_ALIGN_OF_WORD and WORD_ALIGN_MASK. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188580/diff/1-2/

Comment 22

[Nicholas Nethercote [inactive]]

Comment on attachment 8917636 [details]
Bug 1407494 (part 4) - Remove have_PrefChangedFunc_typedef. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188582/diff/1-2/

Comment 23

[Nicholas Nethercote [inactive]]

Comment on attachment 8917637 [details]
Bug 1407494 (part 5) - Replace uses of NS_strdup(), PL_strdup(), PL_strfree() with more standard functions. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188584/diff/1-2/

Comment 24

[Nicholas Nethercote [inactive]]

Comment on attachment 8917638 [details]
Bug 1407494 (part 6) - Remove GetContentChild(). .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188586/diff/1-2/

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 8917639 [details]
Bug 1407494 (part 7) - Replace malloc uses with operator new. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188588/diff/1-2/

Comment 26

[Nicholas Nethercote [inactive]]

Comment on attachment 8917640 [details]
Bug 1407494 (part 8) - Remove forward declaration of PrefSetting. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188590/diff/1-2/

Comment 27

[Nicholas Nethercote [inactive]]

Comment on attachment 8917641 [details]
Bug 1407494 (part 9) - Merge PREF_RegisterCallback() and PREF_RegisterPriorityCallback. .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188592/diff/1-2/

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8917637 [details]
Bug 1407494 (part 5) - Replace uses of NS_strdup(), PL_strdup(), PL_strfree() with more standard functions. .

https://reviewboard.mozilla.org/r/188584/#review194268

Comment 29

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9b156e30219d2c4c83db2457b2cef81b7c53253
Bug 1407494 (part 1) - Remove C-isms. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec1612180aeb371e6cd35f68bfdc0df3e59c1f90
Bug 1407494 (part 2) - Remove TEST_PREFREAD. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/60c539b7c035c9abaa4dcdd9eb88ec1a1e057237
Bug 1407494 (part 3) - Remove PR_ALIGN_OF_WORD and WORD_ALIGN_MASK. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/40efac89e219dbd0daa18b7c68fb4c837bc46767
Bug 1407494 (part 4) - Remove have_PrefChangedFunc_typedef. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/10a071e84601a3c8cf7c1c3fda216939d25b530f
Bug 1407494 (part 5) - Replace uses of NS_strdup(), PL_strdup(), PL_strfree() with more standard functions. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/06ad4faaa205ba4eab7e48735f9ccd287a0e4007
Bug 1407494 (part 6) - Remove GetContentChild(). r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/39b4ec7b889a6a99b79693c87dbac08cad2c5448
Bug 1407494 (part 7) - Replace malloc uses with operator new. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/312ea2b713c074ca7f0fb184a7ea788ad48bb2a9
Bug 1407494 (part 8) - Remove forward declaration of PrefSetting. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3d210f0c178d1b81816cf54b520a04a003f55b5
Bug 1407494 (part 9) - Merge PREF_RegisterCallback() and PREF_RegisterPriorityCallback. r=glandium.

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d9b156e30219
https://hg.mozilla.org/mozilla-central/rev/ec1612180aeb
https://hg.mozilla.org/mozilla-central/rev/60c539b7c035
https://hg.mozilla.org/mozilla-central/rev/40efac89e219
https://hg.mozilla.org/mozilla-central/rev/10a071e84601
https://hg.mozilla.org/mozilla-central/rev/06ad4faaa205
https://hg.mozilla.org/mozilla-central/rev/39b4ec7b889a
https://hg.mozilla.org/mozilla-central/rev/312ea2b713c0
https://hg.mozilla.org/mozilla-central/rev/d3d210f0c178

Comment 31

[Jorg K (CEST = GMT+2)]

Hi Nicholas, could you tell me whether I should do some similar clean-up in comm-central. I could replace NS_strdup(), PL_strdup() and PL_strfree().

Comment 32

[Nicholas Nethercote [inactive]]

(In reply to Jorg K (GMT+2) from comment #31)
> Hi Nicholas, could you tell me whether I should do some similar clean-up in
> comm-central. I could replace NS_strdup(), PL_strdup() and PL_strfree().

Those functions still have quite a few uses in mozilla-central. I replaced them in this file while changing a bunch of other things, but I'm not sure it's worth the effort of getting rid of all of them, since you won't be able to remove their definitions.

Comment 33

[Jorg K (CEST = GMT+2)]

I was asking because NS_Free() and NS_Alloc() disappeared the other day and left us busted (bug 1408196). At least NS_strdup() looks like it could suffer a similar fate, but then, the "strdup" is lower-case.

Comment 34

[Nicholas Nethercote [inactive]]

The PL_* ones won't disappear because they're in NSPR. As for NS_strdup(), there are two variants, an 8-bit one and a 16-bit one. The 16-bit one is unlikely to disappear because there's no equivalent. And because it's unlikely to disappear, the 8-bit one is probably also unlikely to disappear.

Comment 35

[Nicholas Nethercote [inactive]]

Attachment: (follow-up) - Revert operator new use to malloc(), to pair with free()

Coverity complained about mismatched new/free for one of these.

Comment 36

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/14cfc090780e0da71712fbbc0267dd94166d75b8
Bug 1407494 (follow-up) - Revert operator new use to malloc(), to pair with free(). r=glandium.

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/14cfc090780e