1418156 - Make PrefHashEntry smarter

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

PrefHashEntry is currently a very dumb class. I want to make it much smarter, by moving lots of functions into it. This will make various things nicer.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Remove forward declaration of PrefHashEntry.

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

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Don't zero newly added PrefHashEntrys.

They are guaranteed to be zeroed already by PLDHashTable.

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

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Merge PrefTypeFlags into PrefHashEntry.

PrefTypeFlags is a class with a lot of smarts (i.e. methods). PrefHashEntry is
a class with little smarts. This is silly, because PrefTypeFlags is essentially
an internal implementation detail of PrefHashEntry.

This patch merges PrefTypeFlags into PrefHashEntry, so that PrefHashEntry has
all the smarts. This means lots of `pref->mPrefFlags.Foo()` calls become
`pref->Foo()`.

The patch also changes the representation of the type and flags within
PrefHashEntry to use bitfields, which avoids the need for a Flags type and is
much simpler than the old approach.

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

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move the PLDHashTable ops into PrefHashEntry.

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

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Overhaul GetPrefValueFromEntry.

This patch:

- partially inlines GetPrefValueFromEntry() at its two call sites, to avoid
  repeating the kind check;

- renames it as AssignPrefValueToDomPrefValue().

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

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move pref_GetPrefFromEntry into PrefHashEntry.

And rename it as ToSetting(). This requires also moving
AssignPrefValueToDomPrefValue into PrefHashEntry.

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

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move pref_EntryHasAdvisablySizedValues() into PrefHashEntry.

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

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Rename PrefHashEntry::mKey as mName, and make it private.

This requires adding a constructor, which is used in a placement new.

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

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move pref_SetValue() into PrefHashEntry.

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

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Remove unnecessary forward declaration of pref_SetPref().

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

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8929263 [details]
Bug 1418156 - Remove forward declaration of PrefHashEntry.

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

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8929272 [details]
Bug 1418156 - Remove unnecessary forward declaration of pref_SetPref().

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

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Don't zero newly added PrefHashEntrys.

They are guaranteed to be zeroed already by PLDHashTable.

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

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Merge PrefTypeFlags into PrefHashEntry.

PrefTypeFlags is a class with a lot of smarts (i.e. methods). PrefHashEntry is
a class with little smarts. This is silly, because PrefTypeFlags is essentially
an internal implementation detail of PrefHashEntry.

This patch merges PrefTypeFlags into PrefHashEntry, so that PrefHashEntry has
all the smarts. This means lots of `pref->mPrefFlags.Foo()` calls become
`pref->Foo()`.

The patch also changes the representation of the type and flags within
PrefHashEntry to use bitfields, which avoids the need for a Flags type and is
much simpler than the old approach.

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

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move the PLDHashTable ops into PrefHashEntry.

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

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Overhaul GetPrefValueFromEntry.

This patch:

- partially inlines GetPrefValueFromEntry() at its two call sites, to avoid
  repeating the kind check;

- renames it as AssignPrefValueToDomPrefValue().

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

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move pref_GetPrefFromEntry into PrefHashEntry.

And rename it as ToSetting(). This requires also moving
AssignPrefValueToDomPrefValue into PrefHashEntry.

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

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move pref_EntryHasAdvisablySizedValues() into PrefHashEntry.

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

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Rename PrefHashEntry::mKey as mName, and make it private.

This requires adding a constructor, which is used in a placement new.

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

Comment 20

[Nicholas Nethercote [inactive]]

Attachment: Bug 1418156 - Move pref_SetValue() into PrefHashEntry.

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

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8929272 [details]
Bug 1418156 - Remove unnecessary forward declaration of pref_SetPref().

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

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 8929263 [details]
Bug 1418156 - Remove forward declaration of PrefHashEntry.

https://reviewboard.mozilla.org/r/200554/#review205720

Comment 23

[Mike Hommey [:glandium]]

Comment on attachment 8929277 [details]
Bug 1418156 - Don't zero newly added PrefHashEntrys.

https://reviewboard.mozilla.org/r/200582/#review205724

Comment 24

[Mike Hommey [:glandium]]

Comment on attachment 8929278 [details]
Bug 1418156 - Merge PrefTypeFlags into PrefHashEntry.

https://reviewboard.mozilla.org/r/200584/#review205726

::: modules/libpref/Preferences.cpp:178
(Diff revision 1)
> -  void SetHasUserValue(bool aSetOrUnset)
> -  {
> +  bool IsSticky() const { return mIsSticky; }
> +  void SetIsSticky(bool aValue) { mIsSticky = aValue; }
> -    SetFlag(PREF_FLAG_HAS_USER_VALUE, aSetOrUnset);
> -  }
>  
> -private:
> -  static uint16_t AsInt(PrefType aType) { return (uint16_t)aType; }
> +  bool IsLocked() const { return mIsLocked; }
> +  void SetLocked(bool aValue) { mIsLocked = aValue; }

The discrepancy between IsLocked/SetLocked and IsSticky/SetIsSticky is weird.

Comment 25

[Mike Hommey [:glandium]]

Comment on attachment 8929279 [details]
Bug 1418156 - Move the PLDHashTable ops into PrefHashEntry.

https://reviewboard.mozilla.org/r/200586/#review205732

Comment 26

[Mike Hommey [:glandium]]

Comment on attachment 8929280 [details]
Bug 1418156 - Overhaul GetPrefValueFromEntry.

https://reviewboard.mozilla.org/r/200588/#review205736

Comment 27

[Mike Hommey [:glandium]]

Comment on attachment 8929281 [details]
Bug 1418156 - Move pref_GetPrefFromEntry into PrefHashEntry.

https://reviewboard.mozilla.org/r/200590/#review205750

Comment 28

[Mike Hommey [:glandium]]

Comment on attachment 8929282 [details]
Bug 1418156 - Move pref_EntryHasAdvisablySizedValues() into PrefHashEntry.

https://reviewboard.mozilla.org/r/200592/#review205754

Comment 29

[Mike Hommey [:glandium]]

Comment on attachment 8929283 [details]
Bug 1418156 - Rename PrefHashEntry::mKey as mName, and make it private.

https://reviewboard.mozilla.org/r/200594/#review205758

Comment 30

[Mike Hommey [:glandium]]

Comment on attachment 8929284 [details]
Bug 1418156 - Move pref_SetValue() into PrefHashEntry.

https://reviewboard.mozilla.org/r/200596/#review205760

Comment 31

[Mike Hommey [:glandium]]

Comment on attachment 8929272 [details]
Bug 1418156 - Remove unnecessary forward declaration of pref_SetPref().

https://reviewboard.mozilla.org/r/200572/#review205762

Comment 32

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02b07373f9f23770c8ccf6db456f8d6b1a75f3b2
Bug 1418156 - Remove forward declaration of PrefHashEntry. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4fd4cef48e8210489050bd482edfe491223c42
Bug 1418156 - Don't zero newly added PrefHashEntrys. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/b109b4b29eb7e569c3a862db2f0ef35366596b3d
Bug 1418156 - Merge PrefTypeFlags into PrefHashEntry. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/b16dfe143b271f9b8c7e20677615846842bf58a4
Bug 1418156 - Move the PLDHashTable ops into PrefHashEntry. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/4bec38deb68bc09cedd3d99c5e1f8f7cb72a43e7
Bug 1418156 - Overhaul GetPrefValueFromEntry. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcc25f198c1761c8eab5a68bb4905e86c6a572ad
Bug 1418156 - Move pref_GetPrefFromEntry into PrefHashEntry. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/acfdad86187d760ade47a36528ba340c6c5d4ac1
Bug 1418156 - Move pref_EntryHasAdvisablySizedValues() into PrefHashEntry. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab1f93c8ec0556df05823699b0eb2cb95a93e07
Bug 1418156 - Rename PrefHashEntry::mKey as mName, and make it private. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2c5f2b65d56a05c9a618fe504e6d5cff59b1beb
Bug 1418156 - Move pref_SetValue() into PrefHashEntry. r=glandium

https://hg.mozilla.org/integration/mozilla-inbound/rev/98646e193600531078091d7da9a64df32e008661
Bug 1418156 - Remove unnecessary forward declaration of pref_SetPref(). r=glandium

Comment 33

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/02b07373f9f2
https://hg.mozilla.org/mozilla-central/rev/6f4fd4cef48e
https://hg.mozilla.org/mozilla-central/rev/b109b4b29eb7
https://hg.mozilla.org/mozilla-central/rev/b16dfe143b27
https://hg.mozilla.org/mozilla-central/rev/4bec38deb68b
https://hg.mozilla.org/mozilla-central/rev/bcc25f198c17
https://hg.mozilla.org/mozilla-central/rev/acfdad86187d
https://hg.mozilla.org/mozilla-central/rev/5ab1f93c8ec0
https://hg.mozilla.org/mozilla-central/rev/a2c5f2b65d56
https://hg.mozilla.org/mozilla-central/rev/98646e193600