Closed Bug 1412718 Opened 5 years ago Closed 5 years ago

Clean up nsPrefBranch::DeleteBranch()

Categories

(Core :: Preferences: Backend, enhancement)

Product:
Core
Component:
Preferences: Backend
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(2 files)

Bug 1412718 (part 1) - Inline and remove PREF_DeleteBranch(). .
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .
59 bytes, text/x-review-board-request
glandium
: review+
Details 
I have a couple of clean-ups.
Comment on attachment 8923222 [details]
Bug 1412718 (part 2) - Improve nsPrefBranch::DeleteBranch(). .

Nope, this is bogus and fails lots of tests.
Attachment #8923222 - Flags: review?(mh+mozilla)
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.
Attachment #8923221 - Flags: review?(mh+mozilla) → review+
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.
Attachment #8923222 - Flags: review?(mh+mozilla) → review+
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.
Blocks: prefs-cleanup
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.
Attachment #8923222 - Flags: review+ → review?(mh+mozilla)
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.
Attachment #8923222 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/8b473f28baba
https://hg.mozilla.org/mozilla-central/rev/ab0f6594ed42
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.