1412718 - Clean up nsPrefBranch::DeleteBranch()

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

I have a couple of clean-ups.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1412718 (part 1) - Inline and remove PREF_DeleteBranch(). .

It has a single call site.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

This patch does the following.

- Removes the no-longer-needed addition of a trailing '.' to the branchName.

- Improves the comment that explains the string matching.

- Changes PL_strncmp() calls to strncmp().

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

Comment 3

[Nicholas Nethercote [inactive]]

Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

Nope, this is bogus and fails lots of tests.

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8923221 [details]
Bug 1412718 (part 1) - Inline and remove PREF_DeleteBranch(). .

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

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

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

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8923221 [details]
Bug 1412718 (part 1) - Inline and remove PREF_DeleteBranch(). .

https://reviewboard.mozilla.org/r/194366/#review199892

::: modules/libpref/Preferences.cpp:2829
(Diff revision 2)
> +  // The following check insures that if the branch name already has a "." at
> +  // the end, we don't end up with a "..". This fixes an incompatibility
> +  // between nsIPref, which needs the period added, and nsIPrefBranch which
> +  // does not. When nsIPref goes away this function should be fixed to never
> +  // add the period at all.
> +  nsAutoCString branch_dot(branchName);

You could initialize branch_dot directly from pref.get(), and get the length from the nsAutoCString instead of strlen.

::: modules/libpref/Preferences.cpp:2837
(Diff revision 2)
> +  }
> +
> +  // Delete a branch. Used for deleting mime types.
> +  const char* to_delete = branch_dot.get();
> +  MOZ_ASSERT(to_delete);
> +  len = strlen(to_delete);

you can get the length from branch_dot directly.

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

https://reviewboard.mozilla.org/r/194368/#review199894

::: modules/libpref/Preferences.cpp:2828
(Diff revision 2)
> -  // does not. When nsIPref goes away this function should be fixed to never
> -  // add the period at all.
>    nsAutoCString branch_dot(branchName);
>    if (len > 1 && branchName[len - 1] != '.') {
>      branch_dot += '.';
> +    len++;

note that nsCString keeps the length as a member variable.

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

https://reviewboard.mozilla.org/r/194368/#review199896

::: modules/libpref/Preferences.cpp:2838
(Diff revision 2)
>  
> -    // Note: if we're deleting "ldap" then we want to delete "ldap.xxx" and
> -    // "ldap" (if such a leaf node exists) but not "ldap_1.xxx".
> -    if (PL_strncmp(entry->mKey, to_delete, len) == 0 ||
> +    // The first disjunct matches branches: e.g. a branchName "foo.bar."
> +    // matches an mKey "foo.bar.baz" (but it won't match "foo.barrel.baz").
> +    // The second disjunct matches leaf nodes: e.g. a branchName "foo.bar."
> +    // matches an mKey "foo.bar" (ignoring the trailing '.').
> +    if (strncmp(entry->mKey, branch_dot.get(), len) == 0 ||

You could use branch_dot.Equals(entry->mKey).

::: modules/libpref/Preferences.cpp:2839
(Diff revision 2)
>          (len - 1 == strlen(entry->mKey) &&
> -         PL_strncmp(entry->mKey, to_delete, len - 1) == 0)) {
> +         strncmp(entry->mKey, branch_dot.get(), len - 1) == 0)) {

There's probably something better that can be done here with some nsAutoCString methods, or taking a substring and using Equals.

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8923221 [details]
Bug 1412718 (part 1) - Inline and remove PREF_DeleteBranch(). .

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

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

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

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

glandium: can you please re-review part 2? I rewrote it to use Gecko strings.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

https://reviewboard.mozilla.org/r/194368/#review200232

::: modules/libpref/Preferences.cpp:2825
(Diff revisions 2 - 3)
> -  const char* branchName = pref.get();
> +  nsAutoCString branchName(pref.get());
> -  size_t len = strlen(branchName);
>  
>    // Add a trailing '.' if it doesn't already have one.
> -  nsAutoCString branch_dot(branchName);
> -  if (len > 1 && branchName[len - 1] != '.') {
> +  if (branchName.Length() > 1 &&
> +      !StringEndsWith(branchName, NS_LITERAL_CSTRING("."))) {

weird that we don't have an EndsWith method on ns*String.

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b473f28babaf5259971961d796d86117ac3960a
Bug 1412718 (part 1) - Inline and remove PREF_DeleteBranch(). r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab0f6594ed425dda1059c7211c2fd8e1d534e5f0
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). r=glandium.

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/8b473f28baba
https://hg.mozilla.org/mozilla-central/rev/ab0f6594ed42