Closed
Bug 1412718
Opened 5 years ago
Closed 5 years ago
Clean up nsPrefBranch::DeleteBranch()
Categories
(Core :: Preferences: Backend, enhancement)
Core
Preferences: Backend
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files)
I have a couple of clean-ups.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Comment 3•5 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 6•5 years ago
|
||
mozreview-review |
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 7•5 years ago
|
||
mozreview-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 8•5 years ago
|
||
mozreview-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.
![]() |
Assignee | |
Updated•5 years ago
|
Blocks: prefs-cleanup
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
![]() |
Assignee | |
Comment 11•5 years ago
|
||
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 12•5 years ago
|
||
mozreview-review |
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+
![]() |
Assignee | |
Comment 13•5 years ago
|
||
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•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8b473f28baba https://hg.mozilla.org/mozilla-central/rev/ab0f6594ed42
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•