Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1415223002:
Add counts of User data to ProfileInfoCache (Closed)
DescriptionAdded functions for storing browsing statistics in ProfileInfoCache.
Also a few minor fixes reported by cpplint.
BUG=502346
Committed: https://crrev.com/e9be770cd1340b3a9668095d0785cc096a725bd5
Cr-Commit-Position: refs/heads/master@{#360576}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added unit test, fixed a bug, and a few minor edits #
Total comments: 6
Patch Set 3 : Respond to comments #Patch Set 4 : Changed statistics keys from UpperCase to lower_case #Patch Set 5 : Removed statistics accessors of base::DictionaryValue type #Patch Set 6 : Change GetStatistic from int(...) back to bool(..., int*) #
Total comments: 5
Patch Set 7 : Rewritten according to Mike's comment #Patch Set 8 : Fix: removed declarations and includes that should not be there #Patch Set 9 : Added methods in profile_statistics.cc to access ProfileInfoCache, fixed a few style errors #
Total comments: 15
Patch Set 10 : Respond to comments. #Patch Set 11 : Bug fix and refactoring utility methods in profilestatistics.* #Patch Set 12 : Bug fix. Removed a non-trivial style fix (will upload in another CL). #Messages
Total messages: 53 (22 generated)
Description was changed from ========== Added functions for storing statistics. BUG=502346 ========== to ========== Added functions for storing statistics. BUG=502346 ==========
lwchkg@gmail.com changed reviewers: + anthonyvd@chromium.org
Dear Anthony,
Please have a look and comment. Anyway, I'll add the tests soon (hopefully
by tomorrow).
Regards,
WC Leung.
Patchset #2 (id:2) has been deleted
Patchset #2 (id:30001) has been deleted
Dear Anthony, Unit tests are added in the second patch. Please have a look. Regards, WC Leung.
anthonyvd@chromium.org changed reviewers: + mlerman@chromium.org
Great work, thanks a lot :) only a few comments! I'm not the biggest fan of the "Statistics" naming but the closest I have to a suggestion is "Tally" and your other CL uses Statistics. +mlerman who's (I believe) great at naming things but I won't bikeshed hard on this. https://codereview.chromium.org/1415223002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1415223002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:63: const char kStatistics[] = "statistics"; nit: kStatisticsKey https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:133: bool ProfileAttributesEntry::GetStatistic(const std::string& category, I think this (and the ProfileInfoCache implementation) can just return the value directly and return 0 for items we don't have data for. After all, the count of an item we don't know about is still 0 and someone *really* interested in the counts that have associated data can use GetAllStatistics(). If you go down that road, please add a test around getting a count that's not in the PIC yet. https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:92: scoped_ptr<base::DictionaryValue> GetAllStatisticsAsDictionaryValue() const; Do you have a specific plan for GetAllStatisticsAsDictionaryValue() that can't be achieved with GetAllStatistics()? If you don't I would just not include it.
I'm not convinced this information belongs in the ProfileInfoCache. This cache is mainly used to drive Profile Selectors and various UI, and should stay that way. Also, the ProfileInfoCache needs to have certain information for the profile to be selectable, and that information does change much with usage. I think perhaps we should create a new object with local_state, that ends up looking kind of like the ProfileInfoCache but lets us store all things stats-y. Anthony, WDYT? https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:970: FOR_EACH_OBSERVER(ProfileInfoCacheObserver, Why does changing the stats set OnProfileSigninRequiredChanged?
Here's my reply. https://codereview.chromium.org/1415223002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1415223002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_info_cache.cc:63: const char kStatistics[] = "statistics"; On 2015/10/23 15:53:13, anthonyvd wrote: > nit: kStatisticsKey Acknowledged. https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:133: bool ProfileAttributesEntry::GetStatistic(const std::string& category, On 2015/10/23 15:53:14, anthonyvd wrote: > I think this (and the ProfileInfoCache implementation) can just return the value > directly and return 0 for items we don't have data for. After all, the count of > an item we don't know about is still 0 and someone *really* interested in the > counts that have associated data can use GetAllStatistics(). > I see. It's okay here because everything else in this file returns a default value in similar cases. > If you go down that road, please add a test around getting a count that's not in > the PIC yet. It's already there. The keys "One", "Two" and "Three" are tested every time we add an value to the cache. The same keys are also tested before the first value is added to the cache. https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:92: scoped_ptr<base::DictionaryValue> GetAllStatisticsAsDictionaryValue() const; On 2015/10/23 15:53:14, anthonyvd wrote: > Do you have a specific plan for GetAllStatisticsAsDictionaryValue() that can't > be achieved with GetAllStatistics()? If you don't I would just not include it. Actually this is the only method I'll use in 501916. The return in this function is simply relayed to Javascript (user_pod_row.js) without processing. https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... chrome/browser/profiles/profile_info_cache.cc:970: FOR_EACH_OBSERVER(ProfileInfoCacheObserver, On 2015/10/23 16:04:19, Mike Lerman wrote: > Why does changing the stats set OnProfileSigninRequiredChanged? Thanks for catching the mistake. We don't need an observer here, so I'll remove these lines.
On 2015/10/23 16:04:19, Mike Lerman wrote: > I'm not convinced this information belongs in the ProfileInfoCache. This cache > is mainly used to drive Profile Selectors and various UI, and should stay that > way. Also, the ProfileInfoCache needs to have certain information for the > profile to be selectable, and that information does change much with usage. > > I think perhaps we should create a new object with local_state, that ends up > looking kind of like the ProfileInfoCache but lets us store all things stats-y. > > Anthony, WDYT? I don't necessarily see a huge benefit in splitting it, although I might be missing something. I don't think we'll have a lot more stats-y things in the future and the PIC seems like the kind of place I would look for such info. There's also some code that would be duplicated in that split (GetInfoForProfileAtIndex()). I don't feel *super* strongly about this though, so happy either way. > > https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... > File chrome/browser/profiles/profile_info_cache.cc (right): > > https://codereview.chromium.org/1415223002/diff/50001/chrome/browser/profiles... > chrome/browser/profiles/profile_info_cache.cc:970: > FOR_EACH_OBSERVER(ProfileInfoCacheObserver, > Why does changing the stats set OnProfileSigninRequiredChanged?
Patchset #3 (id:70001) has been deleted
New patch uploaded to address comments. PTAL. On 2015/10/26 20:15:54, anthonyvd wrote: > I don't necessarily see a huge benefit in splitting it, although I might be > missing something. I don't think we'll have a lot more stats-y things in the > future and the PIC seems like the kind of place I would look for such info. > There's also some code that would be duplicated in that split > (GetInfoForProfileAtIndex()). > > I don't feel *super* strongly about this though, so happy either way. > Actually I think that ProfileInfoCache is not large considering that it has only 15-20 items (I'm not sure about the exact numbers.) But the code size is big anyway. BTW, I do feel that using a volatile number as the index in PIC is insane. (Any kind of fixed index, number or string, is okay to me.) Anyway, the new interface looks much more better. But I hope that we don't need to deal with the index even at the internals.
On 2015/10/28 13:49:50, lwchkg wrote: > New patch uploaded to address comments. PTAL. > > On 2015/10/26 20:15:54, anthonyvd wrote: > > I don't necessarily see a huge benefit in splitting it, although I might be > > missing something. I don't think we'll have a lot more stats-y things in the > > future and the PIC seems like the kind of place I would look for such info. > > There's also some code that would be duplicated in that split > > (GetInfoForProfileAtIndex()). > > > > I don't feel *super* strongly about this though, so happy either way. > > > > Actually I think that ProfileInfoCache is not large considering that it has only > 15-20 items (I'm not sure about the exact numbers.) But the code size is big > anyway. > > BTW, I do feel that using a volatile number as the index in PIC is insane. (Any > kind of fixed index, number or string, is okay to me.) Anyway, the new interface > looks much more better. But I hope that we don't need to deal with the index > even at the internals. I suggest the split for a couple of reasons, none big, and I'm certainly open to discussion. 1) Monica always suggested it should be done that way (although she's not a profiles owner anymore) 2) The number of methods on the ProfileInfoCacheInterface and the size of profile_info_cache.cc are getting ridiculously large. Statistics about Profiles (counts, usage, etc) are a whole class of data that could really bloat this in its current form. 3) The data needed to display Profiles and the statistics about Profile are different; they need to be loaded at different times, have different data types (stats are all ints), and may have different privacy implications (although Dominic's never cared much about storing stats in Local State). And you're right about using volatile index as insane ;) Thus why Anthony's diligently moving us to a Newer Better Version! One big reason I'd suggest against having a different ProfileStatsCache is that in the end, we're still going to store the data in LocalState, access it via preferences, and have a really similar interface. Though if that's the case, I don't know why you're making a special "Stats" key; either make all the individual stats elements members of the ProfileInfoCache as is, or make the stats outside of the ProfileInfoCache. There's no need to make them a Dictionary that needs additional parsing. Anthony, you're the active Profiles person, so I'll let you make the call :) Feel free to ping me if you want to chat more.
On 2015/10/29 17:23:14, Mike Lerman wrote: > On 2015/10/28 13:49:50, lwchkg wrote: > > New patch uploaded to address comments. PTAL. > > > > On 2015/10/26 20:15:54, anthonyvd wrote: > > > I don't necessarily see a huge benefit in splitting it, although I might be > > > missing something. I don't think we'll have a lot more stats-y things in the > > > future and the PIC seems like the kind of place I would look for such info. > > > There's also some code that would be duplicated in that split > > > (GetInfoForProfileAtIndex()). > > > > > > I don't feel *super* strongly about this though, so happy either way. > > > > > > > Actually I think that ProfileInfoCache is not large considering that it has > only > > 15-20 items (I'm not sure about the exact numbers.) But the code size is big > > anyway. > > > > BTW, I do feel that using a volatile number as the index in PIC is insane. > (Any > > kind of fixed index, number or string, is okay to me.) Anyway, the new > interface > > looks much more better. But I hope that we don't need to deal with the index > > even at the internals. > > I suggest the split for a couple of reasons, none big, and I'm certainly open to > discussion. > 1) Monica always suggested it should be done that way (although she's not a > profiles owner anymore) > 2) The number of methods on the ProfileInfoCacheInterface and the size of > profile_info_cache.cc are getting ridiculously large. Statistics about Profiles > (counts, usage, etc) are a whole class of data that could really bloat this in > its current form. > 3) The data needed to display Profiles and the statistics about Profile are > different; they need to be loaded at different times, have different data types > (stats are all ints), and may have different privacy implications (although > Dominic's never cared much about storing stats in Local State). > > And you're right about using volatile index as insane ;) Thus why Anthony's > diligently moving us to a Newer Better Version! > > One big reason I'd suggest against having a different ProfileStatsCache is that > in the end, we're still going to store the data in LocalState, access it via > preferences, and have a really similar interface. Though if that's the case, I > don't know why you're making a special "Stats" key; either make all the > individual stats elements members of the ProfileInfoCache as is, or make the > stats outside of the ProfileInfoCache. There's no need to make them a Dictionary > that needs additional parsing. I think it's useful to have them as a dictionary in this case because there's a use case to pass them directly to a javascript function but that's really just convenience. > > Anthony, you're the active Profiles person, so I'll let you make the call :) > Feel free to ping me if you want to chat more. Those are definitely all good points. I'm curious, what would you suggest instead of a ProfileStatsCache type thing? The only real option is to store those things in Local State and end up with a similar interface anyways (unless I'm missing something), isn't it? I also don't want to block lwchkg too much so if this can be implemented in the current PIC and moved to a new class as a subsequent step I'm fine with logging a bug and assigning it to myself.
> > I suggest the split for a couple of reasons, none big, and I'm certainly
open
> to
> > discussion.
> > 1) Monica always suggested it should be done that way (although she's not a
> > profiles owner anymore)
Curious. Apparently the refactoring was discussed and tried for more than 4
years without success. Why? (I don't see that task as difficult. So are there
some other constraints? (e.g. The issue is not receiving priority.)) Maybe I can
help here (of course, after rushing issue 501916 first).
> > 2) The number of methods on the ProfileInfoCacheInterface and the size of
> > profile_info_cache.cc are getting ridiculously large. Statistics about
> Profiles
> > (counts, usage, etc) are a whole class of data that could really bloat this
in
> > its current form.
Yes in the current form, but should be a no after refactoring ProfileInfoCache.
IMHO, ProfileInfoCache should only provide generic methods to read/write the
DictionaryValue involved. The accessors should be entirely contained in
ProfileAttributesEntry.
(P.S. I saw that something similar to what I suggest above is happening in
Sailesh's draft CL, but not in Monica's. Anything happened between that time?)
> > 3) The data needed to display Profiles and the statistics about Profile are
> > different; they need to be loaded at different times, have different data
> types
> > (stats are all ints), and may have different privacy implications (although
> > Dominic's never cared much about storing stats in Local State).
I'd like to know above the privacy implication here.
> > And you're right about using volatile index as insane ;) Thus why Anthony's
> > diligently moving us to a Newer Better Version!
> > One big reason I'd suggest against having a different ProfileStatsCache is
> that
> > in the end, we're still going to store the data in LocalState, access it via
> > preferences, and have a really similar interface. Though if that's the case,
I
> > don't know why you're making a special "Stats" key; either make all the
> > individual stats elements members of the ProfileInfoCache as is, or make the
> > stats outside of the ProfileInfoCache. There's no need to make them a
> Dictionary
> > that needs additional parsing.
Disagree that it needs additional parsing. Everything in Local State are stored
inside different DictionaryValue (it's written in JSON anyway), so the parsing
cost should be similar no matter it is located. (If you deep copy the values,
that's another story. But deep copies should be avoided as much as possible.)
> I think it's useful to have them as a dictionary in this case because there's
a
> use case to pass them directly to a javascript function but that's really just
> convenience.
In fact it is the ONLY use case. All the other read accessors are unused. I'll
upload a draft CL in one or two days, so both of you may have a look.
> Those are definitely all good points. I'm curious, what would you suggest
> instead of a ProfileStatsCache type thing? The only real option is to store
> those things in Local State and end up with a similar interface anyways
(unless
> I'm missing something), isn't it?
>
Agree. Both Local State and interface will be very similar. Also for
implementation (however, the new class will be what ProfileInfoCache should be
instead of what it is.)
> I also don't want to block lwchkg too much so if this can be implemented in
the
> current PIC and moved to a new class as a subsequent step I'm fine with
logging
> a bug and assigning it to myself.
Thanks a lot here. BTW, while the interface is not very important now, we should
finalize on the what's happening in the Local State file. With this CL this
looks like
"Profile 32": {
"active_time": 1446213310.40101,
"avatar_icon": "chrome://theme/IDR_PROFILE_AVATAR_26",
"background_apps": false,
"gaia_id": "",
"is_ephemeral": false,
"is_omitted_from_profile_list": false,
"is_using_default_avatar": true,
"is_using_default_name": true,
"managed_user_id": "",
"name": "Person 5",
"statistics": {
"Bookmarks": 0,
"BrowsingHistory": 1,
"Passwords": 0,
"Settings": 68
},
"user_name": ""
}
WDYT?
On 2015/10/30 17:44:17, lwchkg wrote:
> > > I suggest the split for a couple of reasons, none big, and I'm certainly
> open
> > to
> > > discussion.
> > > 1) Monica always suggested it should be done that way (although she's not
a
> > > profiles owner anymore)
>
> Curious. Apparently the refactoring was discussed and tried for more than 4
> years without success. Why? (I don't see that task as difficult. So are there
> some other constraints? (e.g. The issue is not receiving priority.)) Maybe I
can
> help here (of course, after rushing issue 501916 first).
Partially it takes time reason about the right interface, and then in large part
the ProfileInfoCache is used alllll over the place, so it's just a beast of a
job to implement the refactoring.
>
> > > 2) The number of methods on the ProfileInfoCacheInterface and the size of
> > > profile_info_cache.cc are getting ridiculously large. Statistics about
> > Profiles
> > > (counts, usage, etc) are a whole class of data that could really bloat
this
> in
> > > its current form.
>
> Yes in the current form, but should be a no after refactoring
ProfileInfoCache.
> IMHO, ProfileInfoCache should only provide generic methods to read/write the
> DictionaryValue involved. The accessors should be entirely contained in
> ProfileAttributesEntry.
>
> (P.S. I saw that something similar to what I suggest above is happening in
> Sailesh's draft CL, but not in Monica's. Anything happened between that time?)
Yes, after the refactoring, this won't be that big a deal.
Monica didn't land any major refactoring changes. She had a CL in the works, but
never landed anything.
>
> > > 3) The data needed to display Profiles and the statistics about Profile
are
> > > different; they need to be loaded at different times, have different data
> > types
> > > (stats are all ints), and may have different privacy implications
(although
> > > Dominic's never cared much about storing stats in Local State).
>
> I'd like to know above the privacy implication here.
It's likely minor. local_state is very easy to read off someone's disk, so
basically we're making it very easy to determine these aspects of a user's
browsing behaviour. But given the Profile directories aren't encrypted, and
we're assuming the vector already has disk access, there's really no difference
here (and Dominic has said as much previously).
>
> > > And you're right about using volatile index as insane ;) Thus why
Anthony's
> > > diligently moving us to a Newer Better Version!
>
> > > One big reason I'd suggest against having a different ProfileStatsCache is
> > that
> > > in the end, we're still going to store the data in LocalState, access it
via
> > > preferences, and have a really similar interface. Though if that's the
case,
> I
> > > don't know why you're making a special "Stats" key; either make all the
> > > individual stats elements members of the ProfileInfoCache as is, or make
the
> > > stats outside of the ProfileInfoCache. There's no need to make them a
> > Dictionary
> > > that needs additional parsing.
>
> Disagree that it needs additional parsing. Everything in Local State are
stored
> inside different DictionaryValue (it's written in JSON anyway), so the parsing
> cost should be similar no matter it is located. (If you deep copy the values,
> that's another story. But deep copies should be avoided as much as possible.)
By parsing I mean that all other values we access from the ProfileInfoCache,
even if they could be logically grouped, are returned as usable, primitive
values (strings, ints) rather than a data structure. Why is this field of the
ProfileInfoCache special?
>
> > I think it's useful to have them as a dictionary in this case because
there's
> a
> > use case to pass them directly to a javascript function but that's really
just
> > convenience.
Make a utility method, then. The ProfileInfoCache really should, IMO, be a
simple reader/writer around these non-structured preferences; otherwise we'll
start a trend that will make this class unwieldy.
>
> In fact it is the ONLY use case. All the other read accessors are unused. I'll
> upload a draft CL in one or two days, so both of you may have a look.
>
> > Those are definitely all good points. I'm curious, what would you suggest
> > instead of a ProfileStatsCache type thing? The only real option is to store
> > those things in Local State and end up with a similar interface anyways
> (unless
> > I'm missing something), isn't it?
> >
> Agree. Both Local State and interface will be very similar. Also for
> implementation (however, the new class will be what ProfileInfoCache should be
> instead of what it is.)
>
> > I also don't want to block lwchkg too much so if this can be implemented in
> the
> > current PIC and moved to a new class as a subsequent step I'm fine with
> logging
> > a bug and assigning it to myself.
>
> Thanks a lot here. BTW, while the interface is not very important now, we
should
> finalize on the what's happening in the Local State file. With this CL this
> looks like
>
> "Profile 32": {
> "active_time": 1446213310.40101,
> "avatar_icon": "chrome://theme/IDR_PROFILE_AVATAR_26",
> "background_apps": false,
> "gaia_id": "",
> "is_ephemeral": false,
> "is_omitted_from_profile_list": false,
> "is_using_default_avatar": true,
> "is_using_default_name": true,
> "managed_user_id": "",
> "name": "Person 5",
> "statistics": {
> "Bookmarks": 0,
> "BrowsingHistory": 1,
> "Passwords": 0,
> "Settings": 68
> },
> "user_name": ""
> }
>
> WDYT?
If you go with the dictionary approach, you should maintain the {lower_case}
format for the keys rather than {UpperCase} format.
> > Disagree that it needs additional parsing. Everything in Local State are
> stored
> > inside different DictionaryValue (it's written in JSON anyway), so the
parsing
> > cost should be similar no matter it is located. (If you deep copy the
values,
> > that's another story. But deep copies should be avoided as much as
possible.)
>
> By parsing I mean that all other values we access from the ProfileInfoCache,
> even if they could be logically grouped, are returned as usable, primitive
> values (strings, ints) rather than a data structure. Why is this field of the
> ProfileInfoCache special?
There's a concern returning ints in the statistics: the statistics *can fail*.
The current approach used by ProfileInfoCache do not consider the possibility of
failing, so statistics methods returns 0 for missing statistics. This is
unacceptable according to our past discussions in profile_statistics.cc.
So either some kind of failure handling (apparently not preferred by Anthony),
or returning a data structure (i.e. std::map or base::DictionaryValue, or both)
is necessary to know exactly which statistics are available.
> >
> > > I think it's useful to have them as a dictionary in this case because
> there's
> > a
> > > use case to pass them directly to a javascript function but that's really
> just
> > > convenience.
>
> Make a utility method, then. The ProfileInfoCache really should, IMO, be a
> simple reader/writer around these non-structured preferences; otherwise we'll
> start a trend that will make this class unwieldy.
>
Using base::DictionaryValue is a bit more efficient because one less conversion
is needed, but I'm okay with having only std::map and do the conversion in
user_manager_screen_handler.cc. Anyway, I'll let Anthony comment before making
the final judgment.
> > Thanks a lot here. BTW, while the interface is not very important now, we
> should
> > finalize on the what's happening in the Local State file. With this CL this
> > looks like
> >
> > "Profile 32": {
> > "active_time": 1446213310.40101,
> > "avatar_icon": "chrome://theme/IDR_PROFILE_AVATAR_26",
> > "background_apps": false,
> > "gaia_id": "",
> > "is_ephemeral": false,
> > "is_omitted_from_profile_list": false,
> > "is_using_default_avatar": true,
> > "is_using_default_name": true,
> > "managed_user_id": "",
> > "name": "Person 5",
> > "statistics": {
> > "Bookmarks": 0,
> > "BrowsingHistory": 1,
> > "Passwords": 0,
> > "Settings": 68
> > },
> > "user_name": ""
> > }
> >
> > WDYT?
>
> If you go with the dictionary approach, you should maintain the {lower_case}
> format for the keys rather than {UpperCase} format.
Updated the keys in profile_statistics.cc and user_pod_row.js in the new patch.
(Note: the keys in user_pod_row.js are also changed to {lower_case}. I don't
bother to have any kind of conversion here because it affects performance.)
On 2015/11/05 14:11:36, lwchkg wrote:
> > > Disagree that it needs additional parsing. Everything in Local State are
> > stored
> > > inside different DictionaryValue (it's written in JSON anyway), so the
> parsing
> > > cost should be similar no matter it is located. (If you deep copy the
> values,
> > > that's another story. But deep copies should be avoided as much as
> possible.)
> >
> > By parsing I mean that all other values we access from the ProfileInfoCache,
> > even if they could be logically grouped, are returned as usable, primitive
> > values (strings, ints) rather than a data structure. Why is this field of
the
> > ProfileInfoCache special?
>
> There's a concern returning ints in the statistics: the statistics *can fail*.
> The current approach used by ProfileInfoCache do not consider the possibility
of
> failing, so statistics methods returns 0 for missing statistics. This is
> unacceptable according to our past discussions in profile_statistics.cc.
>
> So either some kind of failure handling (apparently not preferred by Anthony),
> or returning a data structure (i.e. std::map or base::DictionaryValue, or
both)
> is necessary to know exactly which statistics are available.
I made this comment without knowing/remembering the discussion about why
returning 0 for unavailable stats was incorrect. A method that returns a bool
for failure/success like you had before is fine in that case. Sorry for the
confusion.
>
> > >
> > > > I think it's useful to have them as a dictionary in this case because
> > there's
> > > a
> > > > use case to pass them directly to a javascript function but that's
really
> > just
> > > > convenience.
> >
> > Make a utility method, then. The ProfileInfoCache really should, IMO, be a
> > simple reader/writer around these non-structured preferences; otherwise
we'll
> > start a trend that will make this class unwieldy.
> >
>
> Using base::DictionaryValue is a bit more efficient because one less
conversion
> is needed, but I'm okay with having only std::map and do the conversion in
> user_manager_screen_handler.cc. Anyway, I'll let Anthony comment before making
> the final judgment.
I agree with Mike here, returning a map is the right thing to do. If this is
consumed anywhere else the conversion will have to happen anyways and just
returning the DictionaryValue tightly couple the local_state file, the
ProfileInfoCache, and the UserManager's javascript together.
This isn't an operation that will happen often or in a time sensitive setting,
the code quality is probably worth more than the minuscule cost of converting :)
>
> > > Thanks a lot here. BTW, while the interface is not very important now, we
> > should
> > > finalize on the what's happening in the Local State file. With this CL
this
> > > looks like
> > >
> > > "Profile 32": {
> > > "active_time": 1446213310.40101,
> > > "avatar_icon": "chrome://theme/IDR_PROFILE_AVATAR_26",
> > > "background_apps": false,
> > > "gaia_id": "",
> > > "is_ephemeral": false,
> > > "is_omitted_from_profile_list": false,
> > > "is_using_default_avatar": true,
> > > "is_using_default_name": true,
> > > "managed_user_id": "",
> > > "name": "Person 5",
> > > "statistics": {
> > > "Bookmarks": 0,
> > > "BrowsingHistory": 1,
> > > "Passwords": 0,
> > > "Settings": 68
> > > },
> > > "user_name": ""
> > > }
> > >
> > > WDYT?
> >
> > If you go with the dictionary approach, you should maintain the {lower_case}
> > format for the keys rather than {UpperCase} format.
>
> Updated the keys in profile_statistics.cc and user_pod_row.js in the new
patch.
> (Note: the keys in user_pod_row.js are also changed to {lower_case}. I don't
> bother to have any kind of conversion here because it affects performance.)
On 2015/11/05 14:58:16, anthonyvd wrote: > > There's a concern returning ints in the statistics: the statistics *can fail*. > > The current approach used by ProfileInfoCache do not consider the possibility > of > > failing, so statistics methods returns 0 for missing statistics. This is > > unacceptable according to our past discussions in profile_statistics.cc. > > > > So either some kind of failure handling (apparently not preferred by Anthony), > > or returning a data structure (i.e. std::map or base::DictionaryValue, or > both) > > is necessary to know exactly which statistics are available. > > I made this comment without knowing/remembering the discussion about why > returning 0 for unavailable stats was incorrect. A method that returns a bool > for failure/success like you had before is fine in that case. Sorry for the > confusion. I see. > > > > > > > Using base::DictionaryValue is a bit more efficient because one less > conversion > > is needed, but I'm okay with having only std::map and do the conversion in > > user_manager_screen_handler.cc. Anyway, I'll let Anthony comment before making > > the final judgment. > > I agree with Mike here, returning a map is the right thing to do. If this is > consumed anywhere else the conversion will have to happen anyways and just > returning the DictionaryValue tightly couple the local_state file, the > ProfileInfoCache, and the UserManager's javascript together. > > This isn't an operation that will happen often or in a time sensitive setting, > the code quality is probably worth more than the minuscule cost of converting :) > Thanks. I'll upload another patch tonight.
Patchset #5 (id:130001) has been deleted
Patchset #5 (id:150001) has been deleted
Patch set #5 and #6 uploaded. Please see which one do you prefer. (Check the description of #6 for the difference.)
https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_entry.h:88: // the statistic of the |category| exists. Returns true if succeed. last sentence isn't grammatically correct. "Returns true if value exists". https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_entry.h:91: std::map<std::string, int> GetAllStatistics() const; I thought the ProfileInfoCache wasn't going to return a map? https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... File chrome/browser/profiles/profile_info_cache.h (right): https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.h:108: bool GetStatisticOfProfileAtIndex(size_t index, const std::string& category, All the other ProfileInfoCache methods just return their value directly, whether there's one stored or otherwise. Look at GAIAPicture as a way to test for existence, e.g. add two methods: One called bool HasStatisticOfProfile()... and the other int GetStatisticOfProfile...(). Which I guess means I prefer solution #5, with the addition of Has() methods if we need them. Also - I think this interface should continue the pattern of having one explicit method per datum, so we'd want a method for BookmarkCountStats, HistoryCountStats, etc... (feel free to tweak names) https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.h:110: std::map<std::string, int> GetAllStatisticsOfProfileAtIndex(size_t index) I thought the profileInfoCache wasn't going to return a map? https://codereview.chromium.org/1415223002/diff/190001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.h:154: void SetStatisticOfProfileAtIndex(size_t index, const std::string& category, comment please.
Mike: Thanks for you comment! I'll make another patch ASAP (it's 3am now, so I need to sleep right now.)
Another patch uploaded. This time the statistics are written as individual functions.
Patchset #8 (id:230001) has been deleted
Patchset #8 (id:250001) has been deleted
Another patch uploaded to add utility methods in profile_statistics.cc. Also followed the output of cpplint. (The comment below is one of them.) https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_storage_unittest.cc:109: static_cast<unsigned long>(storage()->GetNumberOfProfiles()); Should this be simply "size_t" without casting? If yes I'll change it in this CL.
I think this looks great! Very very good. Just a few minor comment-related nits. Anthony, do you have any comments? Otherwise I think this is good to go. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_entry.h:88: // Get the statistics of the profiles Browsing statistics of the profile. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_entry.h:116: // Set the statistics of the profiles nit: of the profile. (singular, and add a period) https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_storage_unittest.cc:109: static_cast<unsigned long>(storage()->GetNumberOfProfiles()); On 2015/11/15 19:40:06, lwchkg wrote: > Should this be simply "size_t" without casting? If yes I'll change it in this > CL. Yes, just use size_t where that's the defined type. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.cc:548: bool ProfileInfoCache::HasStatsSettingsOfProfileAtIndex(size_t index) nit: I think const can go on the line before. (and perhaps in a couple of the others)
Just a single nit. Other than that and Mike's comments, lgtm. Thanks a lot for doing this :) https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.cc:981: int value) { nit: alignment of the params here and the following 2 functions.
Description was changed from ========== Added functions for storing statistics. BUG=502346 ========== to ========== Added functions for storing browsing statistics in ProfileInfoCache. Also a few minor fixes reported by cpplint. BUG=502346 ==========
Thank you for all your comments. Now they're fixed. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_entry.h:8: #include <map> Unnecessary. Now removed. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_entry.h:88: // Get the statistics of the profiles On 2015/11/17 16:29:51, Mike Lerman wrote: > Browsing statistics of the profile. Done. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_entry.h:116: // Set the statistics of the profiles On 2015/11/17 16:29:51, Mike Lerman wrote: > nit: of the profile. (singular, and add a period) Done. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_attributes_storage_unittest.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_attributes_storage_unittest.cc:109: static_cast<unsigned long>(storage()->GetNumberOfProfiles()); On 2015/11/17 16:29:51, Mike Lerman wrote: > On 2015/11/15 19:40:06, lwchkg wrote: > > Should this be simply "size_t" without casting? If yes I'll change it in this > > CL. > > Yes, just use size_t where that's the defined type. Done. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.cc:548: bool ProfileInfoCache::HasStatsSettingsOfProfileAtIndex(size_t index) On 2015/11/17 16:29:51, Mike Lerman wrote: > nit: I think const can go on the line before. (and perhaps in a couple of the > others) Done. Six pairs replaced. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.cc:981: int value) { On 2015/11/17 19:03:30, anthonyvd wrote: > nit: alignment of the params here and the following 2 functions. Done. (Anyway, what's happening with me with those alignments? Embarrassing.) https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:275: g_browser_process->profile_manager()->GetProfileInfoCache(); Wrong indent. Done. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:311: g_browser_process->profile_manager()->GetProfileInfoCache(); Wrong indent. Done. https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics_unittest.cc (right): https://codereview.chromium.org/1415223002/diff/290001/chrome/browser/profile... chrome/browser/profiles/profile_statistics_unittest.cc:18: const std::map<std::string, int>& expected, Wrong indent here. Done.
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/1415223002/#ps310001 (title: "Respond to comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415223002/310001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415223002/310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/1415223002/#ps330001 (title: "Bug fix and refactoring utility methods in profilestatistics.*")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415223002/330001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415223002/330001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #12 (id:350001) has been deleted
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415223002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415223002/370001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/1415223002/#ps370001 (title: "Bug fix. Removed a non-trivial style fix (will upload in another CL).")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415223002/370001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415223002/370001
Message was sent while issue was closed.
Committed patchset #12 (id:370001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/e9be770cd1340b3a9668095d0785cc096a725bd5 Cr-Commit-Position: refs/heads/master@{#360576} |
