Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(21)

Issue 2276043004: Add PrefMember<base::Time>

Created:
4 years, 3 months ago by markuso
Modified:
4 years, 3 months ago
Reviewers:
Bernhard Bauer
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add PrefMember<base::Time> There is a lot of code that stores a time-stamp as a preference, always explicitly converting the value. Adding this PrefMember template can help to simplify access to those preferences. R=bauerb@chromium.org

Patch Set 1 #

Patch Set 2 : Add PrefService::{Get,Set}Time(..., base::Time) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+43 lines, -0 lines) Patch
M components/prefs/pref_service.h View 1 3 chunks +3 lines, -0 lines 0 comments Download
M components/prefs/pref_service.cc View 1 2 chunks +21 lines, -0 lines 0 comments Download
M components/prefs/pref_service_unittest.cc View 1 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 8 (4 generated)
markuso
There is a lot of code that stores a time-stamp as a preference, always explicitly ...
4 years, 3 months ago (2016-08-25 07:14:15 UTC) #3
Bernhard Bauer
I don't think PrefMember is the right place for this: PrefMember itself is meant to ...
4 years, 3 months ago (2016-08-25 09:25:40 UTC) #6
markuso
On 2016/08/25 09:25:40, Bernhard Bauer wrote: > I don't think PrefMember is the right place ...
4 years, 3 months ago (2016-08-25 13:41:07 UTC) #7
Bernhard Bauer
4 years, 3 months ago (2016-08-25 14:00:48 UTC) #8
On 2016/08/25 13:41:07, markuso wrote:
> On 2016/08/25 09:25:40, Bernhard Bauer wrote:
> > I don't think PrefMember is the right place for this: PrefMember itself is
> meant
> > to be used to cache a pref value, either if it needs to be read *really*
> > frequently (e.g. in a repeated callback from the UI framework), or if it
needs
> > to be read on a different thread than the UI thread. (Sadly, there are a lot
> of
> > existing places in the code where people use PrefMember anyway, and it's
hard
> to
> > educate people away from that.)
> 
> IMHO it would help to document this intention in the header file.
> I am one of those who saw the PrefMember as a convenient way to access the
> values and to register the change-callback. Using the PrefService and
> PrefChangeRegistrar the same code becomes a bit more complex.
> I won't add a patch to add such documentation, so if you want to educate
people
> in that respect, you should do it yourself.

Yes, I need to do that at some point...

> > Why don't we start by adding SetTime() and GetTime() to PrefService, and
only
> > add the PrefMember specialization if it turns out to be needed?
> 
> That is at least a start. I can update this patch - first revert, then add
> PrefService::{Get,Set}Time() and consider if I need the PrefMember in the way
it
> is intended (and if the answer is yes, I'll re-add the PrefMember
> specialization).

Yes please.

Powered by Google App Engine
This is Rietveld 408576698