1249157 - Wrap preference hash entry flags into a class, separate them from nsIPrefBranch enums

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

We are mixing enums from prefapi.h and nsIPrefBranch.idl, using the same names, counting on values being the same.  Make the ones from prefapi to an enum class, so that type checking helps.  Hide the internal values for flags, make it harder for the user to check the wrong thing, or set the type to be both string and int and bool.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

This is a first step in attempting larger things with preferences, but it should stand on its own and make things cleaner and safer.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: MozReview Request: Bug 1249157: prefapi enums into class enums, explicit conversion, cleanup. r=bsmedberg

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

Comment 3

[Benjamin Smedberg]

Comment on attachment 8720544 [details]
MozReview Request: Bug 1249157: prefapi enums into class enums, explicit conversion, cleanup.  r=bsmedberg

https://reviewboard.mozilla.org/r/35359/#review32029

::: modules/libpref/prefapi.h:98
(Diff revision 1)
> +  

Whitespace at end of line.

::: modules/libpref/prefapi.h:106
(Diff revision 1)
> +  enum { 

whitespace

::: modules/libpref/prefapi.h:115
(Diff revision 1)
> +  uint16_t mValue;

Did you consider using c++ bitfields instead of an int here? e.g.

uint16_t mType:2;
bool mIsLocked:1;
bool mIsUserSet:1;
bool mIsConfig:1;

I'm not sure it really matters, so I'll leave it up to you, but manual bit-twiddling always worries me.

::: modules/libpref/prefapi.cpp:652
(Diff revision 1)
> -    }
> +        }

I may be being faked out by reviewboard here, but are there extraneous merge markers here?

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

I'll check for the extra merge markers and fix the whitespace.

Bitfield is a good idea.  I left what we had there before, so I'll either create a second patch for this bug or deal with that in another bug.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8720544 [details]
MozReview Request: Bug 1249157: prefapi enums into class enums, explicit conversion, cleanup.  r=bsmedberg

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

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=06b5ddc148b7 for the updates.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd911452e3f7

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=22110980&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/effc082711b0

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Thanks.  I pushed the previous version, instead of the one from comment 7.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Bug 1249157: prefapi enums into class enums, explicit conversion, cleanup. Carry r=bsmedberg

The actual version to land.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe53dc162b59 for one more sanity check.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27f40946fe2d

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/27f40946fe2d