Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1248613003:
Issue 501916 : Add data type counts to profile deletion flow (Closed)
|
Created:
5 years, 5 months ago by lwchkg Modified:
5 years, 2 months ago Reviewers:
msramek, Roger Tawa OOO till Jul 10th, Dmitry Polukhin, Mike Lerman, anthonyvd, vabr (Chromium), achuithb CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, gcasto+watchlist_chromium.org, mkwst+watchlist-passwords_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIssue 501916: Add data type counts to profile deletion flow
The "remove user warning" dialog shows counts browsing history,
passwords, bookmarks and preferences. For synced account, only the total
of the four counts is displayed.
chrome/browser/profiles/profile_statistics.* :
New function GetProfileStatistics to find the counts.
chrome/app/generated_resources.grd,
ui/login/account_picker/user_pod_*.*,
chrome/browser/ui/webui/signin/user_manager_screen_handler.*,
chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc :
Adding the required functions to the UI.
components/history/core/browser/* :
Added helper functions to find the browsing history counts.
BUG=501916
TEST=
Testing procedures
==================
(See the section below for explanation of the starred steps.)
1. Obtain a user-dir with at least a non-synced user, a synced user and
a legacy supervised user. (You may need to use official Chrome to
create a legacy supervised user.)
2. COPY the user-dir to a new place.
3. Start Chromium with the copied user-dir (--user-data-dir=...).
HINT: CREATE A BATCH FILE / SHELL SCRIPT. OTHERWISE YOU MAY
ACCIDENTALLY DELETE YOUR OWN PROFILES!
4. Switch to a non-synced user. Close and reopen Chromium so only that
user's profile is open.
5*.Go to the user selection dialog. Do not switch users now.
6*.Try to delete a user and see the confirmation message. Do this for
all users to see if all the messages are correct. Statistics be
shown on the opened user only. (Note: we did't make a confirmation
dialog for legacy supervised users. Just test if the old one still
works.)
7. Do steps 4-6 again but switch to
(a) synced user,
(b) legacy supervised user
for step 4. 8.
After 7(a) and 7(b), delete all the users in the user selection dialog.
Deletion should be successful. (This step checks for regressions.)
Going to the user selection dialog (steps 5-6)
==============================================
(Step 5)
1. At any Chromium window, find the button at the right side of the
title bar. It can be a picture of a people, "Person ?" or some
initials.
2. Click the button.
3. Select "Switch Person".
(Step 6)
4. Make the mouse cursor hover on a user.
5. Click on the button with the downward arrow, which appears when you
hover on the user.
6. Click "Remove this person".
7. Now you will see the confirmation message.
To populate the categories
==========================
Bookmark:
To add an entry: click the empty star at the location bar. Then press
"Done".
To remove an entry: similar to add an entry, but press "Remove"
instead of "Done".
History:
To add an entry: type an URL in the location bar. If the same URL is
not visited in the same day, then a new entry is
made.
To remove an entry: go to chrome://history/ and delete the entry
there.
Password:
To add an entry: Go to http://codepen.io/anon/pen/JYPjdd . Type a
random username and password, then click submit.
(You may ignore the password criteria.) You will be
asked to save the password.
To remove an entry: Go to chrome://settings/passwords . Select an
unwanted entry and delete it.
Preferences:
Unknown. I have no idea how to add/remove an entry.
Committed: https://crrev.com/3bbd8033884cca5e08fc8618670608798e2e20f8
Cr-Commit-Position: refs/heads/master@{#354679}
Patch Set 1 : First draft #
Total comments: 20
Patch Set 2 : Second draft #Patch Set 3 : Third draft - removed all password manager code #
Total comments: 25
Patch Set 4 : Fourth draft #
Total comments: 1
Patch Set 5 : Fifth draft #Patch Set 6 : minor update (fix wrong initializer list sequence) #Patch Set 7 : minor update + remove unused draft code #Patch Set 8 : updated chromeos empty "localized" strings in signin_screen_handler.cc #
Total comments: 32
Patch Set 9 : Sixth draft #
Total comments: 19
Patch Set 10 : Seventh draft #
Total comments: 23
Patch Set 11 : Eighth draft - added "bool success" to all returns #
Total comments: 16
Patch Set 12 : Respond to Mike's comment #
Total comments: 3
Patch Set 13 : update: HTML message in generated_resouces.grd changed to plain text. HTML tags are generated by JS now. #
Total comments: 7
Patch Set 14 : Added warning messages with words "at least" which is shown when some stats failed. Also fixed the missing "will" in the messages. #Patch Set 15 : Ninth draft - singular/plural strings handled correctly #
Total comments: 18
Patch Set 16 : Rebase - history code already committed in another CL #Patch Set 17 : Rebase (again) and added strings for the case when statistics are loading #
Total comments: 5
Patch Set 18 : Respond to comments #Patch Set 19 : Rebased and modified GetHistoryCount call with reference to https://codereview.chromium.org/1370493002 #
Total comments: 28
Patch Set 20 : Respond to achuithb's comment #Messages
Total messages: 184 (52 generated)
lwchkg@gmail.com changed reviewers: + anthonyvd@chromium.org, mlerman@chromium.org
Dear Anthony and Mike, Please review my draft code and give comments. Thank you. Regards, WC Leung
Hi, a few initial comments (I'm on paternity leave, so will be sadly slow to respond). Two major things: 1) Please make modifications to the User Manager (see user_manager_screen_handler.cc), which is the screen you access by clicking "Switch Person", to also display counts when the profile is displayed. 2) I get early buy-in from owners of all the various classes you're changing, re: adding functions to Bookmarks, Passwords, etc... that's a big change and may not be desired by all parties. To know in advance, is it necessary, or can the ProfileStatisticsService get that information on its own? As I think of it - please change the name of ProfileStatisticsService. The suffix "Service" is used for BrowserContextKeyedServices which generally live for the lifetime of the Profile and have a factory object. Perhaps "counter" or "aggregator"? https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_service.cc (right): https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.cc:36: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB).get(), nit: indent 4 spaces whenever you line wrap. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.cc:104: if (bookmark_model) nit: use parenthesis anytime the statement block is more than one line (here and for the else) https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.cc:125: FindPreference(it.key()); indent only 4 spaces. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_service.h (right): https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:18: class ProfileStatisticsService { Please add a class comment about what this class does and how it's used. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:21: typedef std::vector<std::pair<std::string, int> > ProfileStatisticsValues; can you comment what is stored in ProfileStatisticsValues? https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:22: // Definition of the callback function. Note that a copy of nit: you wrote "a copy of" twice https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:42: ProfileStatisticsCallback callback_; this can be const. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:45: void init_(); remove _ from this and all other methods. Also, capitalize Init(). https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:49: void StatsticsCallback_(std::string category, int count); nit: mis-spelled Statistics. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:58: DISALLOW_IMPLICIT_CONSTRUCTORS(ProfileStatisticsService); Prefer DISALLOW_COPY_AND_ASSIGN.
> Hi, a few initial comments (I'm on paternity leave, so will be sadly slow to
> respond).
Nice to hear that. Congratulations.
I'll also have my new daughter soon and cannot expect to have such a long
paternity leave in Hong Kong. (Should I come to work in US instead?)
Anyway, thanks for the long reply.
>
> Two major things:
> 1) Please make modifications to the User Manager (see
> user_manager_screen_handler.cc), which is the screen you access by clicking
> "Switch Person", to also display counts when the profile is displayed.
Let's discuss this one after you come back to the office. It seems to be a
related feature that bears a lot of details.
We also need to consider issue 502346 for the implementation. Anyway, should we
file a new issue?
>
> 2) I get early buy-in from owners of all the various classes you're changing,
> re: adding functions to Bookmarks, Passwords, etc... that's a big change and
may
> not be desired by all parties. To know in advance, is it necessary, or can the
> ProfileStatisticsService get that information on its own?
Thanks for communicating with the owners. That is something very important to do
early, and it is hard for me to do so without your help.
Bookmark and preferences: The classes are NOT changed. ProfileStatisticsService
do the task on its own. Note that there is no change to existing bookmark code.
Anyway, please help me to determine the correct thread to post the counting
tasks.
History: As the history database is open with a journal, I need to query the
statistics with the opened database via HistoryService. To me there are two
possible approaches:
(1) Call GetAllVisitsInRange with a "infinite" date range and max no. of
entries.
There is a potential transfer of 100000s of records (and also 100000s of
queries in the url database) so this is very bad in performance.
(2) Add a function to do the counting in SqLite. This is what I have submitted
in
the code review. Unfortunately there is change in the History code.
(If (2) is better, then we need to get in touch with the History owner.)
Password: Also a database opened with a journal, so querying the PasswordStore
is
necessary.
(1) Call FillAutofillableLogins to get the list of all non-blacklisted logins.
This can be a good solution. (Any undesirable transfer of sensitive
information?)
(2) Add a function to PasswordStore. It is more efficient, but the code I have
inserted there is very ugly. (The change will be even bigger if it is
"beautified".)
So this should be considered only if (1) is unacceptable.
(As passwords are sensitive information, I hope to get an advice from the
PasswordStore
owner with either solution.)
>
> As I think of it - please change the name of ProfileStatisticsService. The
> suffix "Service" is used for BrowserContextKeyedServices which generally live
> for the lifetime of the Profile and have a factory object. Perhaps "counter"
or
> "aggregator"?
Yes. I'll get another name. Thinking of that now.
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_statistics_service.cc (right):
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.cc:36:
> BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB).get(),
> nit: indent 4 spaces whenever you line wrap.
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.cc:104: if (bookmark_model)
> nit: use parenthesis anytime the statement block is more than one line (here
and
> for the else)
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.cc:125:
> FindPreference(it.key());
> indent only 4 spaces.
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_statistics_service.h (right):
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.h:18: class
> ProfileStatisticsService {
> Please add a class comment about what this class does and how it's used.
>
Forgot to do this one. Embarrassing for me. Will add the comments soon.
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.h:21: typedef
> std::vector<std::pair<std::string, int> > ProfileStatisticsValues;
> can you comment what is stored in ProfileStatisticsValues?
>
The content is same as comment #9 of issue 501916:
i.e. Example: <"Browsing History", 912>, <"Passwords", 71>, <"Bookmarks", 120>,
<"Settings", 200>
A copy of the whole array will be passed back to the callback (eventually to
the .js file) whenever a single statistics is calculated.
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.h:22: // Definition of the
> callback function. Note that a copy of
> nit: you wrote "a copy of" twice
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.h:42:
> ProfileStatisticsCallback callback_;
> this can be const.
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.h:45: void init_();
> remove _ from this and all other methods.
>
> Also, capitalize Init().
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.h:49: void
> StatsticsCallback_(std::string category, int count);
> nit: mis-spelled Statistics.
>
>
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_statistics_service.h:58:
> DISALLOW_IMPLICIT_CONSTRUCTORS(ProfileStatisticsService);
> Prefer DISALLOW_COPY_AND_ASSIGN.
> > 2) I get early buy-in from owners of all the various classes you're
changing,
> > re: adding functions to Bookmarks, Passwords, etc... that's a big change and
> may
> > not be desired by all parties. To know in advance, is it necessary, or can
the
> > ProfileStatisticsService get that information on its own?
>
> Thanks for communicating with the owners. That is something very important to
do
> early, and it is hard for me to do so without your help.
>
> Bookmark and preferences: The classes are NOT changed.
ProfileStatisticsService
> do the task on its own. Note that there is no change to existing bookmark
code.
> Anyway, please help me to determine the correct thread to post the counting
> tasks.
>
> History: As the history database is open with a journal, I need to query the
> statistics with the opened database via HistoryService. To me there are two
> possible approaches:
>
> (1) Call GetAllVisitsInRange with a "infinite" date range and max no. of
> entries.
> There is a potential transfer of 100000s of records (and also 100000s of
> queries in the url database) so this is very bad in performance.
>
> (2) Add a function to do the counting in SqLite. This is what I have submitted
> in the code review. Unfortunately there is change in the History code.
>
> (If (2) is better, then we need to get in touch with the History owner.)
>
> Password: Also a database opened with a journal, so querying the PasswordStore
> is necessary.
>
> (1) Call FillAutofillableLogins to get the list of all non-blacklisted logins.
> This can be a good solution. (Any undesirable transfer of sensitive
> information?)
>
> (2) Add a function to PasswordStore. It is more efficient, but the code I have
> inserted there is very ugly. (The change will be even bigger if it is
> "beautified".)
> So this should be considered only if (1) is unacceptable.
>
> (As passwords are sensitive information, I hope to get an advice from the
> PasswordStore owner with either solution.)
>
Summary: History - prefer to add the new SQL query in the history module.
Password - prefer to revert all changes and use existing functions
(i.e. do it the same way as the password manager).
> >
> > As I think of it - please change the name of ProfileStatisticsService. The
> > suffix "Service" is used for BrowserContextKeyedServices which generally
live
> > for the lifetime of the Profile and have a factory object. Perhaps "counter"
> or
> > "aggregator"?
>
> Yes. I'll get another name. Thinking of that now.
>
After some dictionary search I agreed to use the word aggregator. So the class
is named ProfileStatisticsAggregator. I'll update the names of the files.
> Two major things: > 1) Please make modifications to the User Manager (see > user_manager_screen_handler.cc), which is the screen you access by clicking > "Switch Person", to also display counts when the profile is displayed. Sorry I did not identify the screenshot in comment #2 of issue 501916 correctly. Now I understand what is happening here and the code will land soon.
https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_service.cc (right): https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.cc:36: BrowserThread::GetMessageLoopProxyForThread(BrowserThread::DB).get(), On 2015/07/22 01:39:26, Mike Lerman (OOO through Aug4) wrote: > nit: indent 4 spaces whenever you line wrap. Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.cc:104: if (bookmark_model) On 2015/07/22 01:39:26, Mike Lerman (OOO through Aug4) wrote: > nit: use parenthesis anytime the statement block is more than one line (here and > for the else) Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.cc:125: FindPreference(it.key()); On 2015/07/22 01:39:26, Mike Lerman (OOO through Aug4) wrote: > indent only 4 spaces. Should I have "F" directly under "_"? https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_statistics_service.h (right): https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:18: class ProfileStatisticsService { On 2015/07/22 01:39:26, Mike Lerman (OOO through Aug4) wrote: > Please add a class comment about what this class does and how it's used. Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:21: typedef std::vector<std::pair<std::string, int> > ProfileStatisticsValues; On 2015/07/22 01:39:27, Mike Lerman (OOO through Aug4) wrote: > can you comment what is stored in ProfileStatisticsValues? Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:22: // Definition of the callback function. Note that a copy of On 2015/07/22 01:39:27, Mike Lerman (OOO through Aug4) wrote: > nit: you wrote "a copy of" twice Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:42: ProfileStatisticsCallback callback_; On 2015/07/22 01:39:27, Mike Lerman (OOO through Aug4) wrote: > this can be const. Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:45: void init_(); On 2015/07/22 01:39:27, Mike Lerman (OOO through Aug4) wrote: > remove _ from this and all other methods. > > Also, capitalize Init(). Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:49: void StatsticsCallback_(std::string category, int count); On 2015/07/22 01:39:26, Mike Lerman (OOO through Aug4) wrote: > nit: mis-spelled Statistics. Acknowledged. https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_statistics_service.h:58: DISALLOW_IMPLICIT_CONSTRUCTORS(ProfileStatisticsService); On 2015/07/22 01:39:27, Mike Lerman (OOO through Aug4) wrote: > Prefer DISALLOW_COPY_AND_ASSIGN. Should I disable the default constructor?
So stupid to leave the draft reply unsent for days. :-(
lwchkg@gmail.com changed reviewers: + msramek@chromium.org
Dear all, Here is the second draft of Issue 501916. Please help by having a look. Regards, Leung WC.
https://codereview.chromium.org/1248613003/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:14293: + This will permanently delete <span class="total-count">...</span> items from this computer. This cannot be undone! Please wrap the span in a placeholder (<ph><ex></ex></ph>), so it doesn't get lost in translation. I.e., your string will be "This will permanently delete X items...." and then in the code you'll replace X with "<span class="">number</span>". https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.cc:24: class ProfileStatisticsAggregator Please rename the file to match the class name. https://codereview.chromium.org/1248613003/diff/40001/components/history/core... File components/history/core/browser/visit_database.cc (right): https://codereview.chromium.org/1248613003/diff/40001/components/history/core... components/history/core/browser/visit_database.cc:539: int VisitDatabase::GetCountOfURLsWithVisibleVisit() { In the Clear Browsing Data dialog, we decided to go with what the user sees in chrome://history, which is "DISTINCT url FROM visits" per day. It would be better to do the same here for consistency. But I guess that this can be tweaked later, once the whole infrastructure is in place. https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:551: .action-box-remove-user-warning-text-sync2, Use more informative name than "sync2". https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:957: // * Gets action box menu, remove user warning text div. Please remove the unused code. https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2000: this.querySelector('.action-box-remove-user-warning-text-nonsync') These classes seem to refer to a particular element - shouldn't they be IDs then? Or if there are more elements with that class, querySelector will only pick the first one, which is probably not the desired result.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Dear msramek, Thanks for your review. I hope to finish a fully functional draft tomorrow. Anyway, should I also invite ro...@chromium.org? If yes please do that for me, because I cannot obtain his/her full email address. Regards, WC Leung. https://codereview.chromium.org/1248613003/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:14293: + This will permanently delete <span class="total-count">...</span> items from this computer. This cannot be undone! On 2015/07/29 13:55:01, msramek wrote: > Please wrap the span in a placeholder (<ph><ex></ex></ph>), so it doesn't get > lost in translation. > > I.e., your string will be "This will permanently delete X items...." and then in > the code you'll replace X with "<span class="">number</span>". Acknowledged. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.cc:24: class ProfileStatisticsAggregator On 2015/07/29 13:55:01, msramek wrote: > Please rename the file to match the class name. The class is not exposed to the outside. Only GetProfileStatistics in Line 188 get exposed. Should I really rename the file according to this private class? https://codereview.chromium.org/1248613003/diff/40001/components/history/core... File components/history/core/browser/history_service.h (right): https://codereview.chromium.org/1248613003/diff/40001/components/history/core... components/history/core/browser/history_service.h:341: base::CancelableTaskTracker::TaskId HistoryService::CountURLs( The extra "HistoryService::" is causing errors in trybot. Too bad this is NOT caught in VS2013. https://codereview.chromium.org/1248613003/diff/40001/components/history/core... File components/history/core/browser/visit_database.cc (right): https://codereview.chromium.org/1248613003/diff/40001/components/history/core... components/history/core/browser/visit_database.cc:539: int VisitDatabase::GetCountOfURLsWithVisibleVisit() { On 2015/07/29 13:55:01, msramek wrote: > In the Clear Browsing Data dialog, we decided to go with what the user sees in > chrome://history, which is "DISTINCT url FROM visits" per day. It would be > better to do the same here for consistency. > > But I guess that this can be tweaked later, once the whole infrastructure is in > place. I see. It means if "www.google.com" was visited in 3 different days, it should be counted 3 times. https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:551: .action-box-remove-user-warning-text-sync2, On 2015/07/29 13:55:01, msramek wrote: > Use more informative name than "sync2". Okay. I'll try "sync-part1" and "sync-part2" Anyway should I just select .action-box-remove-user-warning > div, .action-box-remove-user-warning > table instead for the css? https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:957: // * Gets action box menu, remove user warning text div. On 2015/07/29 13:55:01, msramek wrote: > Please remove the unused code. Oh it is embarrassing... https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2000: this.querySelector('.action-box-remove-user-warning-text-nonsync') On 2015/07/29 13:55:01, msramek wrote: > These classes seem to refer to a particular element - shouldn't they be IDs > then? Or if there are more elements with that class, querySelector will only > pick the first one, which is probably not the desired result. All tags in this file should NOT have IDs because the tags are duplicated once for each user in the profile selection screen. Actually I found that out in the hard way... (And OMG Chromium crash every time I reload chrome://user-manager and crash very often when I inspect any running JS in that url.)
I added some more comments. High-level architecture lg, but you'll need the code owners to approve the changes to their codebases. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.cc:24: class ProfileStatisticsAggregator On 2015/07/29 14:46:05, lwchkg wrote: > On 2015/07/29 13:55:01, msramek wrote: > > Please rename the file to match the class name. > > The class is not exposed to the outside. > Only GetProfileStatistics in Line 188 get exposed. > > Should I really rename the file according to this private class? Oh, sorry, I didn't realize it wasn't exposed. Then no. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:14: namespace profiles { Nit: Empty line after this line. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:19: typedef std::vector<std::pair<std::string, int> > ProfileStatisticsValues; Wouldn't std::map<std::string, int> be preferable? And actually, it's not ideal to key these items by strings. If this goes directly to the UI and only there, then I understand that it's convenient. But it's not really reusable, because all callsites must know the strings to extract the data they need. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:25: // This class collect statistical information about the profile and returns the Copy-pasted comment. Please update. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:28: void GetProfileStatistics(Profile* profile, Nit: formatting. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:551: .action-box-remove-user-warning-text-sync2, On 2015/07/29 14:46:05, lwchkg wrote: > On 2015/07/29 13:55:01, msramek wrote: > > Use more informative name than "sync2". > > Okay. I'll try "sync-part1" and "sync-part2" > > Anyway should I just select > .action-box-remove-user-warning > div, > .action-box-remove-user-warning > table > instead for the css? I would actually leave the first one, but name the other something like "action-box-remove-user-manage-link". Those strings are not just two parts of one sentence or paragraph; they have different purposes and that should be captured in the name. And yes, you can use .action-box-remove-user-warning > div. If the reason for styling is that those are divs inside .action-box-remove-user-warning, I don't see the necessity to name them all. https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2000: this.querySelector('.action-box-remove-user-warning-text-nonsync') On 2015/07/29 14:46:05, lwchkg wrote: > On 2015/07/29 13:55:01, msramek wrote: > > These classes seem to refer to a particular element - shouldn't they be IDs > > then? Or if there are more elements with that class, querySelector will only > > pick the first one, which is probably not the desired result. > > All tags in this file should NOT have IDs because the tags are duplicated once > for each user in the profile selection screen. > > Actually I found that out in the hard way... (And OMG Chromium crash every time > I reload chrome://user-manager and crash very often when I inspect any running > JS in that url.) If that's the case, then your selectors will always pick the first matching element, which means always show the data in the first user pod, no?
msramek@chromium.org changed reviewers: + jhawkins@chromium.org, nkostylev@chromium.org, sdefresne@chromium.org
+OWNERS of the affected files Please take a look.
Please ping back when try bots are green.
The CQ bit was checked by lwchkg@gmail.com to run a CQ dry run
The CQ bit was unchecked by lwchkg@gmail.com
The CQ bit was checked by lwchkg@gmail.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/60001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Dear all, I've just uploaded a new draft. Please have a look. Thank you. Note: the correct dialogs are shown this time for users with or without sync, and whether statistics are available (not available if profile is not opened). But I need some tips for testing for the UI in "legacy supervised user". How can I create such a user? Regards, WC Leung. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:14: namespace profiles { On 2015/07/29 18:10:25, msramek wrote: > Nit: Empty line after this line. Acknowledged. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:19: typedef std::vector<std::pair<std::string, int> > ProfileStatisticsValues; On 2015/07/29 18:10:25, msramek wrote: > Wouldn't std::map<std::string, int> be preferable? I agree, but that vector is the idea of Mike, so I'd also ask for his opinion. Anyway, the result is simply converted to a DictionaryValue (i.e. a Map) before transferring to JavaScript. (See comments #7, #9, #11 in issue 501916.) > And actually, it's not ideal to key these items by strings. If this goes > directly to the UI and only there, then I understand that it's convenient. But > it's not really reusable, because all callsites must know the strings to extract > the data they need. Not sure about this. Currently all existing and potential callsites are from JavaScript, but I don't know whether it will be processed by C++ code in the future. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:25: // This class collect statistical information about the profile and returns the On 2015/07/29 18:10:25, msramek wrote: > Copy-pasted comment. Please update. Acknowledged. https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_statistics.h:28: void GetProfileStatistics(Profile* profile, On 2015/07/29 18:10:25, msramek wrote: > Nit: formatting. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Calls Acknowledged. https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/40001/ui/login/account_picker... ui/login/account_picker/user_pod_row.css:551: .action-box-remove-user-warning-text-sync2, On 2015/07/29 18:10:25, msramek wrote: > On 2015/07/29 14:46:05, lwchkg wrote: > > On 2015/07/29 13:55:01, msramek wrote: > > > Use more informative name than "sync2". > > > > Okay. I'll try "sync-part1" and "sync-part2" > > > > Anyway should I just select > > .action-box-remove-user-warning > div, > > .action-box-remove-user-warning > table > > instead for the css? > > I would actually leave the first one, but name the other something like > "action-box-remove-user-manage-link". Those strings are not just two parts of > one sentence or paragraph; they have different purposes and that should be > captured in the name. > > And yes, you can use > > .action-box-remove-user-warning > div. > > If the reason for styling is that those are divs inside > .action-box-remove-user-warning, I don't see the necessity to name them all. Actually the divs are also used in the javascript to set the content of the text and to make some of the contents hidden. https://codereview.chromium.org/1248613003/diff/80001/ui/login/account_picker... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/80001/ui/login/account_picker... ui/login/account_picker/user_pod_row.js:2008: this.querySelector('.action-box-remove-user-warning-text-nonsync') nits: wrong indentation. Will update in the next patch.
Can you please update the CL description to just list the current state of the CL? It's very difficult to grok all of the deltas you have listed.
Patchset #4 (id:60001) has been deleted
On 2015/07/30 17:56:37, James Hawkins wrote: > Can you please update the CL description to just list the current state of the > CL? It's very difficult to grok all of the deltas you have listed. Thank you for your reminder. The CL description has been updated now.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Fourth draft (Backlog) User switching dialog (chrome://user-manager) finished. - Added: Messages when no statistics is available (i.e. when profile is not available). - Changed: Used css for showing/hiding different parts of the dialogs. Unused code deleted. Fixed: Wrong method declaration in history_service.h Minor changes: Updated profile_statistics.h according to comments. TODO: Change history database counting code (SQL). (I need to verify that time zones and DST are correctly used in the date calculation.)
Fifth draft Updated history calculation to per url chain per day Fixed profile_statistics.cc initialization order Indent fixed in user_pod_row.js Note: Please help to activate the try bots. P.S. Really hate that the errors (or warnings) are not fired in VS2013.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/07/31 15:40:37, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) profile_statistics.cc: fixed initialization sequence order "again". (It was not properly fixed.) Note: Please help to activate the try bots again. Many thanks.
The CQ bit was checked by jhawkins@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/120001
On 2015/07/31 08:48:10, lwchkg wrote: > On 2015/07/30 17:56:37, James Hawkins wrote: > > Can you please update the CL description to just list the current state of the > > CL? It's very difficult to grok all of the deltas you have listed. > > Thank you for your reminder. The CL description has been updated now. I think I wasn't specific enough: the CL description must include the high level change description you're making against trunk (not a previous patch set); diving into specific changes below the high level change is also important. It should be obvious from the CL description what the purpose of the change is and how it relates to the bug you're fixing. Here is an example: " chrome/browser/foobar: Update the sync service model to take into account time zone changes. Fixes an issue when the user travels between different time zones. * Added time zone tracking to SyncServiceModel * Updated existing clients of SyncServiceMOdel to handle time zone changes * Added tests for the new behavior "
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I've just submitted a new patch to fix the GCC compiling problems, and removed the draft code in manage-profile-overlay.* The CL description is updated. (Thanks James for help.) This time I've set up a Linux VM and compiled the source code in GCC, so the try bots should be green this time. (Again, please help me to activate the try bots.)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Updated empty translated strings in signin_screen_handler.cc (These strings are not used in Chrome OS, but needed to satisfy the i18n-content dependencies in user_pod_template.html). Again, please help to activate the try bots. Thanks.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/08/04 10:08:29, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. +jhawkins The trybots are green now.
Here are some first-pass-through comments. Just catching up now that I'm back from leave :) (And I'm in Canada not the US - the US is terrible for leave. Canada's pretty good :) https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:76: : parent_(parent) {} nit: add a newline below https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:81: private: newline above private. Also, DISALLOW_COPY_AND_ASSIGN. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:92: : profile_(profile), callback_(callback), tracker_(tracker), nit: 1 initialization per line https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:104: this, "Bookmarks")); all these string parameters to StatisticsCallback should be using constants that are defined... perhaps in this class's header file. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:116: StatisticsCallback("BrowsingHistory", 0); If there's no history service, does that necessarily mean there's 0 browsing history, or perhaps just that the service object hasn't been created yet? Perhaps just omit the entry in this case? (e.g. don't have an else condition) https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:120: // TODO(lwchkg): make password task cancellable. I think all TODOs should be assigned to @chromium addresses. Please put anthonyvd@. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:153: int ProfileStatisticsAggregator::CountURLs() const { since this method counts bookmarks, could we call it CountBookmarks? https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:27: // This fucntion collects statistical information about |profile| and returns nit: spelling (fucntion/function) https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:29: // preferences are counted. The callback function may be called more than once. I'd replace "may" with "will probably", before "be called more than once". https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:557: base::FilePath* profile_path = new base::FilePath(); Please create this on the stack rather the using new/delete (and then pass the reference to GetValueAsFilePath) https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.h:139: // The CalcelableTaskTracker is currently used by GetProfileStatistics spelling, missed an n on CaNcelableTaskTracker.
Acknowledged most of Mike's comments. And also asked a few questions. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:76: : parent_(parent) {} On 2015/08/05 19:26:09, Mike Lerman wrote: > nit: add a newline below Acknowledged. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:81: private: On 2015/08/05 19:26:09, Mike Lerman wrote: > newline above private. Also, DISALLOW_COPY_AND_ASSIGN. Acknowledged. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:92: : profile_(profile), callback_(callback), tracker_(tracker), On 2015/08/05 19:26:09, Mike Lerman wrote: > nit: 1 initialization per line Acknowledged. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:104: this, "Bookmarks")); On 2015/08/05 19:26:09, Mike Lerman wrote: > all these string parameters to StatisticsCallback should be using constants that > are defined... perhaps in this class's header file. Acknowledged. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:116: StatisticsCallback("BrowsingHistory", 0); On 2015/08/05 19:26:09, Mike Lerman wrote: > If there's no history service, does that necessarily mean there's 0 browsing > history, or perhaps just that the service object hasn't been created yet? > Perhaps just omit the entry in this case? (e.g. don't have an else condition) For chrome://user-manager the absence of history is an exception. Maybe a return value of "-1" is preferred. (Note: the same applies to the bookmarks and preferences.) However, I wonder if there is/will be a case where history is not present. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:120: // TODO(lwchkg): make password task cancellable. On 2015/08/05 19:26:09, Mike Lerman wrote: > I think all TODOs should be assigned to @chromium addresses. Please put > anthonyvd@. Acknowledged. Anyway, by design password tasks are cancellable. It is not here because the internal CancelableTaskTracker cannot be overridden. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:153: int ProfileStatisticsAggregator::CountURLs() const { On 2015/08/05 19:26:09, Mike Lerman wrote: > since this method counts bookmarks, could we call it CountBookmarks? Yes. Let me change the name. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:27: // This fucntion collects statistical information about |profile| and returns On 2015/08/05 19:26:09, Mike Lerman wrote: > nit: spelling (fucntion/function) Acknowledged. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:29: // preferences are counted. The callback function may be called more than once. On 2015/08/05 19:26:09, Mike Lerman wrote: > I'd replace "may" with "will probably", before "be called more than once". Acknowledged. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:557: base::FilePath* profile_path = new base::FilePath(); On 2015/08/05 19:26:09, Mike Lerman wrote: > Please create this on the stack rather the using new/delete (and then pass the > reference to GetValueAsFilePath) Acknowledged. I was worrying too much about copy assigning FilePath type in line 569. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.h (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.h:139: // The CalcelableTaskTracker is currently used by GetProfileStatistics On 2015/08/05 19:26:10, Mike Lerman wrote: > spelling, missed an n on CaNcelableTaskTracker. Acknowledged.
On 2015/08/05 19:26:10, Mike Lerman wrote: > Here are some first-pass-through comments. Just catching up now that I'm back > from leave :) (And I'm in Canada not the US - the US is terrible for leave. > Canada's pretty good :) > OIC.
Dear Mike, Just ask a few questions (see the marked lines below). Regards, WC Leung. P.S. Comments by others are also welcome. https://codereview.chromium.org/1248613003/diff/160001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/app/generated_r... chrome/app/generated_resources.grd:14392: + Manage your synced data on <a href="https://www.google.com/settings/chrome/sync">Google Dashboard</a>. Hyperlinks do not work with the profile switching dialog. Any alternative ideas? https://codereview.chromium.org/1248613003/diff/160001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1396: showRemoveWarning_: function() { I need some help to test for the "legacy supervised user" thing, just to know that I did not break things for this kind of user. How should I set up an account? https://codereview.chromium.org/1248613003/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1401: this.classList.toggle('has-no-stats', this.classList.contains('synced')); I've verified that this hack does not work, so we need to fix this one before committing. There are two options: (1) chrome.send something at showRemoveWarning_ in the desktop version to check for the presence of profile. Then execute the remainder of the code after it returns. In this way there may be a small slowdown. (2) chrome.send at update (see line 1987 below). Unnecessary calls may be made, but the dialog box can be displayed immediately in showRemoveWarning_. Note: checking the presence of the profile is cheap. The time used is mainly for waiting for the async tasks to execute. Which one do you prefer?
https://codereview.chromium.org/1248613003/diff/160001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/app/generated_r... chrome/app/generated_resources.grd:14392: + Manage your synced data on <a href="https://www.google.com/settings/chrome/sync">Google Dashboard</a>. On 2015/08/06 04:58:23, lwchkg wrote: > Hyperlinks do not work with the profile switching dialog. Any alternative ideas? Indeed they don't. I'd post something in the bug adding for input from UI folks, saying that we can't have URLs in the User Manager. https://codereview.chromium.org/1248613003/diff/160001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1396: showRemoveWarning_: function() { On 2015/08/06 04:58:23, lwchkg wrote: > I need some help to test for the "legacy supervised user" thing, just to know > that I did not break things for this kind of user. How should I set up an > account? Be signed in to your profile. Then: Chrome Settings -> Add Person -> "Control websites view by this user" will create a "supervised user". https://codereview.chromium.org/1248613003/diff/160001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1401: this.classList.toggle('has-no-stats', this.classList.contains('synced')); On 2015/08/06 04:58:23, lwchkg wrote: > I've verified that this hack does not work, so we need to fix this one before > committing. There are two options: > > (1) chrome.send something at showRemoveWarning_ in the desktop version to check > for the presence of profile. Then execute the remainder of the code after it > returns. In this way there may be a small slowdown. > > (2) chrome.send at update (see line 1987 below). Unnecessary calls may be made, > but the dialog box can be displayed immediately in showRemoveWarning_. > > Note: checking the presence of the profile is cheap. The time used is mainly for > waiting for the async tasks to execute. > > Which one do you prefer? Add something to UserManagerScreenHandler::SendUserList() that includes within the list that drives the UserManager a boolean of whether or not we have the Profile* loaded. We can then refer to it here, as this.user.isProfileLoaded (or similar). Should work?
> On 2015/08/06 04:58:23, lwchkg wrote: > > Hyperlinks do not work with the profile switching dialog. Any alternative > ideas? > > Indeed they don't. I'd post something in the bug adding for input from UI folks, > saying that we can't have URLs in the User Manager. Actually I had already posted in comment #23, but no one responded. Should we add someone into the bug? > > I need some help to test for the "legacy supervised user" thing, just to know > > that I did not break things for this kind of user. How should I set up an > > account? > > Be signed in to your profile. Then: Chrome Settings -> Add Person -> "Control > websites view by this user" will create a "supervised user". Thanks. Anyway, by bug 477048 I cannot test by adding the user in Chromium. Luckily adding in office Chrome and copying the whole data directory worked. > > Add something to UserManagerScreenHandler::SendUserList() that includes within > the list that drives the UserManager a boolean of whether or not we have the > Profile* loaded. We can then refer to it here, as this.user.isProfileLoaded (or > similar). Should work? Thanks a lot. This is simple and neat. I can also remove some useless code in the javascript as well.
Patchset #9 (id:180001) has been deleted
Please see the next draft, which mainly address the comments by Mike. Also fixed a bugs affecting "supervised users" which is caused by a typo in the css file. P.S. To Mike: Please check the two comments in chrome/browser/profiles/profile_statistics.cc . I need your reply/decision. P.P.S. Please activate the trybots for me. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:30: class ProfileStatisticsAggregator Should I move the class into to anonymous namespace? https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:128: StatisticsCallback(kProfileStatisticsBrowsingHistory, 0); On 2015/08/06 04:32:58, lwchkg wrote: > On 2015/08/05 19:26:09, Mike Lerman wrote: > > If there's no history service, does that necessarily mean there's 0 browsing > > history, or perhaps just that the service object hasn't been created yet? > > Perhaps just omit the entry in this case? (e.g. don't have an else condition) > > For chrome://user-manager the absence of history is an exception. Maybe a return > value of "-1" is preferred. > (Note: the same applies to the bookmarks and preferences.) > > However, I wonder if there is/will be a case where history is not present. Still waiting for reply here. Let's reach a decision before changes is made.
The CQ bit was checked by mlerman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/200001
Just a few more small comments for you. https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:116: StatisticsCallback("BrowsingHistory", 0); On 2015/08/06 04:32:58, lwchkg wrote: > On 2015/08/05 19:26:09, Mike Lerman wrote: > > If there's no history service, does that necessarily mean there's 0 browsing > > history, or perhaps just that the service object hasn't been created yet? > > Perhaps just omit the entry in this case? (e.g. don't have an else condition) > > For chrome://user-manager the absence of history is an exception. Maybe a return > value of "-1" is preferred. > (Note: the same applies to the bookmarks and preferences.) > > However, I wonder if there is/will be a case where history is not present. There are a variety of strange profiles - while you can't currently logically apply the ProfileStatisticsAggregator to the Guest Profile (since you can't explicitly delete it), it likely has no HistoryService. Also ephemeral profiles may not have something like a HistoryService, and could be explicitly deleteable. So yes, let's consider the case when the HistoryService isn't present. I would lean away from having to handle a special value ("-1" or otherwise). I'm okay with either leaving this else condition in if you'd like to keep it, or removing it. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:30: class ProfileStatisticsAggregator On 2015/08/07 10:43:48, lwchkg wrote: > Should I move the class into to anonymous namespace? Yes please. Anything not publicly exposed in the header file should be defined here in the anonymous namespace. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:32: // This class collect statistical information about the profile and returns nit: collect needs an s https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:53: // Private variables nit: don't need to specify private variables in a comment https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:148: int count) { nit: indent 3 spaces so that "int" is aligned with "std::string" https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:203: new ProfileStatisticsAggregator(profile, callback, tracker); what deletes this object? https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:577: &tracker_); Should we call tracker_.TryCancelAll() in this class' destructor? https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:817: profile_value->SetBoolean(kKeyIsProfileLoaded, Above line 782, can you declare a local reference to the ProfileManager? ProfileManager* profile_manager = g_browser_process->profile_manager(); Then here, you should be able to fit the whole ternary on one line. If you still can't, then please declare a local boolean for is_profile_loaded, for clarity. https://codereview.chromium.org/1248613003/diff/200001/components/history/cor... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/1248613003/diff/200001/components/history/cor... components/history/core/browser/history_backend.h:273: // Count the number of URLs in the browsing history nit: trailing period please.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:30: class ProfileStatisticsAggregator On 2015/08/07 19:37:12, Mike Lerman wrote: > On 2015/08/07 10:43:48, lwchkg wrote: > > Should I move the class into to anonymous namespace? > > Yes please. Anything not publicly exposed in the header file should be defined > here in the anonymous namespace. Acknowledged. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:32: // This class collect statistical information about the profile and returns On 2015/08/07 19:37:12, Mike Lerman wrote: > nit: collect needs an s Acknowledged. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:53: // Private variables On 2015/08/07 19:37:12, Mike Lerman wrote: > nit: don't need to specify private variables in a comment Acknowledged. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:148: int count) { On 2015/08/07 19:37:12, Mike Lerman wrote: > nit: indent 3 spaces so that "int" is aligned with "std::string" Acknowledged. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:203: new ProfileStatisticsAggregator(profile, callback, tracker); This object is now RefcountedThreadSafe. This is needed for CancelableTaskTracker to function properly. I'll put it in a scoped_refptr (sorry for bad design here) so the object is refcounted at least once. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:577: &tracker_); On 2015/08/07 19:37:12, Mike Lerman wrote: > Should we call tracker_.TryCancelAll() in this class' destructor? No. This is already called by the destructor of CancelableTaskTracker. See https://code.google.com/p/chromium/codesearch#chromium/src/base/task/cancelab... https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:817: profile_value->SetBoolean(kKeyIsProfileLoaded, On 2015/08/07 19:37:12, Mike Lerman wrote: > Above line 782, can you declare a local reference to the ProfileManager? > ProfileManager* profile_manager = g_browser_process->profile_manager(); > > Then here, you should be able to fit the whole ternary on one line. If you still > can't, then please declare a local boolean for is_profile_loaded, for clarity. Acknowledged. https://codereview.chromium.org/1248613003/diff/200001/components/history/cor... File components/history/core/browser/history_backend.h (right): https://codereview.chromium.org/1248613003/diff/200001/components/history/cor... components/history/core/browser/history_backend.h:273: // Count the number of URLs in the browsing history On 2015/08/07 19:37:12, Mike Lerman wrote: > nit: trailing period please. Acknowledged. Anyway we need to discuss the naming of this function and the other two new functions in the history component, because the function names do not match what they are actually doing now. To make discussion easy I'll post the details in the next draft (should be ready tomorrow). We'll also need to include James in the decision.
Seventh draft
=============
Updated messages to match issue 501916 comment #31 to #34
Moved ProfileStatisticsAggregator to anonymous namespace
profile_statistics.cc:
fixed - missing error check in password_store
- GetProfileStatistics now saved in a scoped_refptr
Other updates according to Mike's comment
https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:116: StatisticsCallback("BrowsingHistory", 0); > There are a variety of strange profiles - while you can't currently logically > apply the ProfileStatisticsAggregator to the Guest Profile (since you can't > explicitly delete it), it likely has no HistoryService. Also ephemeral profiles > may not have something like a HistoryService, and could be explicitly > deleteable. So yes, let's consider the case when the HistoryService isn't > present. > > I would lean away from having to handle a special value ("-1" or otherwise). I'm > okay with either leaving this else condition in if you'd like to keep it, or > removing it. I see. Then I'd like to keep it here (and everywhere else in other statistics.) There is a consideration: the callback is called async. If a return of some irrelevant statistics is made, the caller is safe to ignore that value. But if the caller is expecting certain statistics which is silently dropped, then the caller can only wait indefinitely or resort to timeout. I also want to consider future cases that ProfileStatisticsAggregator is not used for deletion, e.g. having the values passed to profileInfoCache. In this case there is an advantage using special values (e.g. -1 for error): nonexistent statistics values can be dropped if return < 0. If "0" is used there is no way for the caller to tell the statistics exists or not.
https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... components/history/core/browser/history_backend.cc:1060: if (!db_) Also suggest to rename to GetHistoryCount. Mike: note the "return 0" here. We have discussed about this before. https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... components/history/core/browser/history_service.cc:660: const CountURLsCallback& callback, Suggest to rename to GetHistoryCount. Mike and James. Any comments? https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... File components/history/core/browser/visit_database.cc (right): https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... components/history/core/browser/visit_database.cc:539: int VisitDatabase::GetCountOfURLsWithVisibleVisit() { Please discuss the naming of this function. It should count in the same way in chrome://history/ , with dates set to infinity. I'd suggest GetHistoryCount because it is difficult to write down the URL-days concept in the function name. I'll update the function names and related comments once the names are decided. Mike and James: is the above proposed name okay? If not please suggest some other names.
lwchkg@gmail.com changed reviewers: + dpolukhin@chromium.org, sky@chromium.org
lwchkg@gmail.com changed reviewers: + vabr@chromium.org
+owners of affected directories @sdefrense and @sky: For performance reasons I have added a set of functions to query the total number of history items. Please see if this is acceptable. If yes then please suggest the function names (I suggest the name "GetHistoryCount" for all the 3 functions.) @vabr: No code in your directory is changed, may added you in case you want to check if profile_statistics.cc query the password store correctly. Regards, WC Leung. https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... File components/history/core/browser/visit_database.cc (right): https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... components/history/core/browser/visit_database.cc:539: int VisitDatabase::GetCountOfURLsWithVisibleVisit() { On 2015/08/11 15:54:51, lwchkg wrote: > Please discuss the naming of this function. It should count in the same way in > chrome://history/ , with dates set to infinity. > > I'd suggest GetHistoryCount because it is difficult to write down the URL-days > concept in the function name. I'll update the function names and related > comments once the names are decided. > @sdefrense and @sky: Please check the code here. Thanks.
Hi WC Leung, On 2015/08/12 17:47:30, lwchkg wrote: > @vabr: No code in your directory is changed, may added you in case you want to > check if profile_statistics.cc query the password store correctly. Your use of the password store LGTM. Thanks for looping me in. Vaclav
Next round :) Nikita/Dmitry, this CL's pretty stable for your sections of the code. Feel free to review. Thanks! https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:116: StatisticsCallback("BrowsingHistory", 0); On 2015/08/10 12:03:37, lwchkg wrote: > > There are a variety of strange profiles - while you can't currently logically > > apply the ProfileStatisticsAggregator to the Guest Profile (since you can't > > explicitly delete it), it likely has no HistoryService. Also ephemeral > profiles > > may not have something like a HistoryService, and could be explicitly > > deleteable. So yes, let's consider the case when the HistoryService isn't > > present. > > > > I would lean away from having to handle a special value ("-1" or otherwise). > I'm > > okay with either leaving this else condition in if you'd like to keep it, or > > removing it. > > I see. Then I'd like to keep it here (and everywhere else in other statistics.) > There is a consideration: the callback is called async. If a return of some > irrelevant statistics is made, the caller is safe to ignore that value. But if > the caller is expecting certain statistics which is silently dropped, then the > caller can only wait indefinitely or resort to timeout. > > I also want to consider future cases that ProfileStatisticsAggregator is not > used for deletion, e.g. having the values passed to profileInfoCache. In this > case there is an advantage using special values (e.g. -1 for error): nonexistent > statistics values can be dropped if return < 0. If "0" is used there is no way > for the caller to tell the statistics exists or not. My preference, rather than using special values, would be to add a parameter to StatisticsCallback that conveys the intended meaning, or to change the parameter type to a struct (containing an int and a bool). In this case, a parameter "bool success" would likely accomplish our needs, but if we need an enum or other construct/parameters that's fine too. I acknowledge this is more verbose and cumbersome, but special values require constant handling of special cases and attention, and it's easy for future programmers to make mistakes. https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/200001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:577: &tracker_); On 2015/08/09 16:11:49, lwchkg wrote: > On 2015/08/07 19:37:12, Mike Lerman wrote: > > Should we call tracker_.TryCancelAll() in this class' destructor? > > No. This is already called by the destructor of CancelableTaskTracker. > See > https://code.google.com/p/chromium/codesearch#chromium/src/base/task/cancelab... Acknowledged. https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:212: scoped_refptr<ProfileStatisticsAggregator> aggregator = This means, I think, that the reference will be deleted as soon as the constructor finishes executing, even though there are pending tasks. I don't see anywhere that the tracker holds a scoped_refptr reference to the ProfileStatisticsAggregator object. I feel like you need to either have a singleton type reference to a ProfileStatisticsAggregator (otherwise, each time you attempt to delete a Profile you'll allocate an object and never release it, although this may be hard if I call this on multiple Profiles in rapid succession), or you need to count calls to StatisticsCallback and delete it after the 4th call, or get the calling UserManagerScreenHandler involved in tracking the objects. https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:819: profile_value->SetBoolean(kKeyIsProfileLoaded, profile ? true : false); (optional) the condition can written more simply as profile != nullptr https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... File components/history/core/browser/history_backend.cc (right): https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... components/history/core/browser/history_backend.cc:1060: if (!db_) On 2015/08/11 15:54:51, lwchkg wrote: > Also suggest to rename to GetHistoryCount. > > Mike: note the "return 0" here. We have discussed about this before. GetHistoryCount or GetCountHistoryEntries sound good. https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... File components/history/core/browser/history_service.cc (right): https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... components/history/core/browser/history_service.cc:660: const CountURLsCallback& callback, On 2015/08/11 15:54:51, lwchkg wrote: > Suggest to rename to GetHistoryCount. Mike and James. Any comments? GetHistoryCount or GetCountHistoryEntries (or similar) are my leanings. https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... File components/history/core/browser/visit_database.cc (right): https://codereview.chromium.org/1248613003/diff/220001/components/history/cor... components/history/core/browser/visit_database.cc:539: int VisitDatabase::GetCountOfURLsWithVisibleVisit() { On 2015/08/11 15:54:51, lwchkg wrote: > Please discuss the naming of this function. It should count in the same way in > chrome://history/ , with dates set to infinity. > > I'd suggest GetHistoryCount because it is difficult to write down the URL-days > concept in the function name. I'll update the function names and related > comments once the names are decided. > > Mike and James: is the above proposed name okay? If not please suggest some > other names. I think GetCountHistoryEntries extends well to this function?
On 2015/08/13 14:56:33, Mike Lerman wrote: > Nikita/Dmitry, this CL's pretty stable for your sections of the code. Feel free > to review. Thanks! I'll let Dmitry review since I've switched to another project.
I can review Chrome OS side of this CL but you also need reviewer who works on desktop user manager code. https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14413: + This will permanently delete <ph name="TOTAL_COUNT"><span class="total-count">...</span><ex>1234</ex></ph> items from this device. I think we should use plain text messages and avoid setting innerHTML. This text will be translated to other languages too. https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:574: /* Hide dialogs not in correct category */ I would prefer to show it when it is need instead of hiding it when it is not needed. https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1995: .innerHTML = loadTimeData.getString( For security reasons I prefer use textContent here.
https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/160001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:116: StatisticsCallback("BrowsingHistory", 0); > My preference, rather than using special values, would be to add a parameter to > StatisticsCallback that conveys the intended meaning, or to change the parameter > type to a struct (containing an int and a bool). In this case, a parameter "bool > success" would likely accomplish our needs, but if we need an enum or other > construct/parameters that's fine too. > > I acknowledge this is more verbose and cumbersome, but special values require > constant handling of special cases and attention, and it's easy for future > programmers to make mistakes. Sounds good to me. I'll submit a revision tomorrow. https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:212: scoped_refptr<ProfileStatisticsAggregator> aggregator = On 2015/08/13 14:56:33, Mike Lerman wrote: > This means, I think, that the reference will be deleted as soon as the > constructor finishes executing, even though there are pending tasks. I don't see > anywhere that the tracker holds a scoped_refptr reference to the > ProfileStatisticsAggregator object. > > I feel like you need to either have a singleton type reference to a > ProfileStatisticsAggregator (otherwise, each time you attempt to delete a > Profile you'll allocate an object and never release it, although this may be > hard if I call this on multiple Profiles in rapid succession), or you need to > count calls to StatisticsCallback and delete it after the 4th call, or get the > calling UserManagerScreenHandler involved in tracking the objects. Correct me if I'm wrong here. As in /src/base/callback.h, the Chromium version of callback supports refcounting, and it is different from Google callback. The callback closure will do the refcounting by itself, i.e. "this" in base::Bind(..., this, ...) creates a scoped_refptr<PSA> that adds to the count. By now there are six such closures created, so it should be counted six times. These closures are repackaged by CancelableTaskTracker (see /src/base/task/cancelable_task_tracker.cc line 100), passed by references several times, finally stored in a PostTaskAndReplyRelay in line 78 of /src/base/threading/post_task_and_reply_impl.cc. That relay will be deleted either in post_task_and_reply_impl.cc line 82 (failure) or line 61 (success), and the refcount will be released by the deletion. That's why when all the PostTask finish all the task ProfileStatisticsAggregator gets deleted automatically. Anyway, if a refcounted class is never counted, then it is unable to delete itself. So I've add the temporary scoped_refptr to force the class to be refcounted at least once. Not sure if the smart compilers will defeat my effort here. BTW, I have some concern about possible dangling reference in the code, in particular, Profile* and BookmarkModel* may be deleted before or during its use. Is there any way to make sure that the underlying object is alive? https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:819: profile_value->SetBoolean(kKeyIsProfileLoaded, profile ? true : false); On 2015/08/13 14:56:33, Mike Lerman wrote: > (optional) the condition can written more simply as profile != nullptr Acknowledged.
> and the refcount will be released by the deletion. That's why when all the > PostTask finish all the task ProfileStatisticsAggregator gets deleted > automatically. > Sorry for grammatical mistakes. The corrected version is here: That's why when all the PostTasks finish ProfileStatisticsAggregator gets deleted automatically.
Eighth draft
============
- Respond to Mike's comment. (Not Dmitry's comment yet... sorry I didn't see
that when I uploaded this patch.)
- Return values of GetProfileStatistics is std::vector<{std::string category,
int count, bool success}>. (Anyway, should I change to std::map? See line 19 in
https://codereview.chromium.org/1248613003/diff/40001/chrome/browser/profiles...
)
- Related changed to profile_statistics.* and other files.
- All history functions renamed to GetHistoryCount. The return is a
HistoryCountResult defined in history_types.h. This should be more coherent to
the style in history directory.
- The web UI part returns "false" to Javascript if (success == false). (Should I
change to {count, success} pair instead?)
P.S. I want to try executing the JS code in my computer, but my Chromium build
do not display any web page properly (a blank screen occur for every page). So I
do not guarantee that the JS code run properly.
Dmirty, I'm an owner of the desktop UserManager/user_pod code, and will review it accordingly. https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:212: scoped_refptr<ProfileStatisticsAggregator> aggregator = On 2015/08/13 16:20:17, lwchkg wrote: > On 2015/08/13 14:56:33, Mike Lerman wrote: > > This means, I think, that the reference will be deleted as soon as the > > constructor finishes executing, even though there are pending tasks. I don't > see > > anywhere that the tracker holds a scoped_refptr reference to the > > ProfileStatisticsAggregator object. > > > > I feel like you need to either have a singleton type reference to a > > ProfileStatisticsAggregator (otherwise, each time you attempt to delete a > > Profile you'll allocate an object and never release it, although this may be > > hard if I call this on multiple Profiles in rapid succession), or you need to > > count calls to StatisticsCallback and delete it after the 4th call, or get the > > calling UserManagerScreenHandler involved in tracking the objects. > > Correct me if I'm wrong here. As in /src/base/callback.h, the Chromium version > of callback supports refcounting, and it is different from Google callback. > > The callback closure will do the refcounting by itself, i.e. "this" in > base::Bind(..., this, ...) creates a scoped_refptr<PSA> that adds to the count. > By now there are six such closures created, so it should be counted six times. > > These closures are repackaged by CancelableTaskTracker (see > /src/base/task/cancelable_task_tracker.cc line 100), passed by references > several times, finally stored in a PostTaskAndReplyRelay in line 78 of > /src/base/threading/post_task_and_reply_impl.cc. That relay will be deleted > either in post_task_and_reply_impl.cc line 82 (failure) or line 61 (success), > and the refcount will be released by the deletion. That's why when all the > PostTask finish all the task ProfileStatisticsAggregator gets deleted > automatically. > > Anyway, if a refcounted class is never counted, then it is unable to delete > itself. So I've add the temporary scoped_refptr to force the class to be > refcounted at least once. Not sure if the smart compilers will defeat my effort > here. > > BTW, I have some concern about possible dangling reference in the code, in > particular, Profile* and BookmarkModel* may be deleted before or during its use. > Is there any way to make sure that the underlying object is alive? It is assumed that the Profile* is never deleted. BookmarkModel* will, I think, consequently never be deleted. https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:65: // Normal callback period at the end. https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:66: void StatisticsCallback(std::string category, ProfileStatisticsValue result); If you're going to call the other method "StatisticsCallbackFailure" this should be called "StatisticsCallbackSuccess". Also, if you call it $Success, then you can change the 2nd parameter from a ProfileStatisticsValue to an int, and make the callers of this method simpler. https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:74: void StatisticsCallbackImpl( I'm not a fan of the suffix Impl, it seems odd given the other methods are the same visibility in the same class. Once StatisticsCallback is called StatisticsCallbackSuccess, this can perhaps just be called StatisticsCallback? https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:26: struct ProfileStatisticsDatum { I am highly entertained by your use of Datum! Sadly, it doesn't convey any meaning (it's like labeling a variable "data"). You could go with: struct ProfileCategoryStat and typedef ProfileCategoryStats or struct ProfileCategoryStatistic and typedef ProfileCategoryStatistics. https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:597: return_value.SetBooleanWithoutPathExpansion(item.category, false); This is a little ugly, to be placing a value that might be a boolean and might be an integer. But I don't have a nicer idea.
Now response to Dmitry's review. > I can review Chrome OS side of this CL but you also need > reviewer who works on desktop user manager code. Thanks a lot for the review! BTW it means this CL won't affect you at all (except a few extra lines in signin_screen_handler.cc), since all changed code affect desktop Chromium only. https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14413: + This will permanently delete <ph name="TOTAL_COUNT"><span class="total-count">...</span><ex>1234</ex></ph> items from this device. On 2015/08/13 15:34:16, Dmitry Polukhin wrote: > I think we should use plain text messages and avoid setting innerHTML. This text > will be translated to other languages too. I see. I still need an HTML tag here. So I'll just change the tag into "$1" here and add the tag via DOM manipulation. (Or should I simply give up on that formatting?) https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:574: /* Hide dialogs not in correct category */ On 2015/08/13 15:34:17, Dmitry Polukhin wrote: > I would prefer to show it when it is need instead of hiding it when it is not > needed. I'd also like to do it, but it mean we need to hide everything first and counter that effect with another rule. Sadly there is no generic opposite to "display: none" in CSS. All you have is "display: block", "display: table" and we need to write a rule for each different tag. https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1995: .innerHTML = loadTimeData.getString( On 2015/08/13 15:34:17, Dmitry Polukhin wrote: > For security reasons I prefer use textContent here. I see. So I'll try other methods.
https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14413: + This will permanently delete <ph name="TOTAL_COUNT"><span class="total-count">...</span><ex>1234</ex></ph> items from this device. On 2015/08/17 17:36:32, lwchkg wrote: > On 2015/08/13 15:34:16, Dmitry Polukhin wrote: > > I think we should use plain text messages and avoid setting innerHTML. This > text > > will be translated to other languages too. > > I see. I still need an HTML tag here. So I'll just change the tag into "$1" here > and add the tag via DOM manipulation. (Or should I simply give up on that > formatting?) IMHO, I would give up with formating here but UX may strong object so you may also ask them to change text to remove number form the phrase and just show total at the end of list. https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:574: /* Hide dialogs not in correct category */ On 2015/08/17 17:36:32, lwchkg wrote: > On 2015/08/13 15:34:17, Dmitry Polukhin wrote: > > I would prefer to show it when it is need instead of hiding it when it is not > > needed. > > I'd also like to do it, but it mean we need to hide everything first and counter > that effect with another rule. Sadly there is no generic opposite to "display: > none" in CSS. All you have is "display: block", "display: table" and we need to > write a rule for each different tag. Not sure that I understand the problem. You can add some parent div and control its visibility (visibility:hidden).
Just confirmed with the newest build that the JS code works with patch #11, so I'll be able continue to update and respond to comments today.
Now my daughter is finally born. :-) So I'll be slower to update the code. Won't be too slow anyway. https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:212: scoped_refptr<ProfileStatisticsAggregator> aggregator = > It is assumed that the Profile* is never deleted. BookmarkModel* will, I think, > consequently never be deleted. Got it. Thanks! https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:66: void StatisticsCallback(std::string category, ProfileStatisticsValue result); On 2015/08/17 14:23:38, Mike Lerman wrote: > If you're going to call the other method "StatisticsCallbackFailure" this should > be called "StatisticsCallbackSuccess". Also, if you call it $Success, then you > can change the 2nd parameter from a ProfileStatisticsValue to an int, and make > the callers of this method simpler. Good idea. This will be used in the password code. BTW I'll still keep the original two parameter function because there is no way for PostTaskAndReplyWithResult to choose the correct callback function (i.e. success/failure). https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:74: void StatisticsCallbackImpl( On 2015/08/17 14:23:38, Mike Lerman wrote: > I'm not a fan of the suffix Impl, it seems odd given the other methods are the > same visibility in the same class. Once StatisticsCallback is called > StatisticsCallbackSuccess, this can perhaps just be called StatisticsCallback? We still need to have a generic method to handle both success and failure, as PostTaskAndReplyWithResult cannot choose the function to run. BTW, any other suffix to suggest? https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:26: struct ProfileStatisticsDatum { On 2015/08/17 14:23:38, Mike Lerman wrote: > I am highly entertained by your use of Datum! Sadly, it doesn't convey any > meaning (it's like labeling a variable "data"). > > You could go with: struct ProfileCategoryStat and typedef ProfileCategoryStats > or struct ProfileCategoryStatistic and typedef ProfileCategoryStatistics. Acknowledged. https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:597: return_value.SetBooleanWithoutPathExpansion(item.category, false); On 2015/08/17 14:23:38, Mike Lerman wrote: > This is a little ugly, to be placing a value that might be a boolean and might > be an integer. But I don't have a nicer idea. Two options here: 1. Leave boolean "false" here. 2. Write JSON values like {count: 123, success: true} Which one do you prefer?
https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:74: void StatisticsCallbackImpl( On 2015/08/19 06:19:45, lwchkg wrote: > On 2015/08/17 14:23:38, Mike Lerman wrote: > > I'm not a fan of the suffix Impl, it seems odd given the other methods are the > > same visibility in the same class. Once StatisticsCallback is called > > StatisticsCallbackSuccess, this can perhaps just be called StatisticsCallback? > > We still need to have a generic method to handle both success and failure, as > PostTaskAndReplyWithResult cannot choose the function to run. BTW, any other > suffix to suggest? If this were a class I'd go with StatisticsCallbackBase. Sure - go with Impl. https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:597: return_value.SetBooleanWithoutPathExpansion(item.category, false); On 2015/08/19 06:19:45, lwchkg wrote: > On 2015/08/17 14:23:38, Mike Lerman wrote: > > This is a little ugly, to be placing a value that might be a boolean and might > > be an integer. But I don't have a nicer idea. > > Two options here: > 1. Leave boolean "false" here. > 2. Write JSON values like {count: 123, success: true} > > Which one do you prefer? Try the json thing; I expect it will yield very readable javascript. If not, then we can go with this hack :S
https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:74: void StatisticsCallbackImpl( Base sounds good. BTW, I've got a new idea. We can change the argument type from <std::string> into <const char *>, since the arguments passed are constants in <const char []>. In this way we don't need a ...Base function to handle reference passing. https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:597: return_value.SetBooleanWithoutPathExpansion(item.category, false); > Try the json thing; I expect it will yield very readable javascript. If not, > then we can go with this hack :S Good. Anyway, we are already passing a json already, so why not doing it to the fullest? :-) Now we won't have to worry about another novice programmer using "==" instead of "===" for comparison.
https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:74: void StatisticsCallbackImpl( On 2015/08/20 16:06:24, lwchkg wrote: > Base sounds good. > > BTW, I've got a new idea. We can change the argument type from <std::string> > into <const char *>, since the arguments passed are constants in <const char > []>. In this way we don't need a ...Base function to handle reference passing. sure, sgtm.
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/1248613003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/260001
New patch just uploaded to address Mike's comment. (Not enough time to address Dmitry's - have to sleep now.) https://codereview.chromium.org/1248613003/diff/260001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:72: // Callbacks for history. Typo (callback instead of callbacks). Will update in next patch. https://codereview.chromium.org/1248613003/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:243: result.success = true; Do you prefer the current style or to use braced initializer list (i.e. return {count, true};)?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Just uploaded a new patch that adds the HTML tags by JS rather than write them in generated_resources.grd. https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/220001/chrome/app/generated_r... chrome/app/generated_resources.grd:14413: + This will permanently delete <ph name="TOTAL_COUNT"><span class="total-count">...</span><ex>1234</ex></ph> items from this device. I see. Anyway I've just uploaded another patch which add the tags with DOM manipulation. https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.css (right): https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke... ui/login/account_picker/user_pod_row.css:574: /* Hide dialogs not in correct category */ Just confirmed that (visibility: hidden; position: absolute) works, so technically this is feasible. @Mike: Should I change the CSS here to "show it when it is need instead of hiding it when it is not needed"?
LGTM for chromeos files
https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:597: return_value.SetBooleanWithoutPathExpansion(item.category, false); On 2015/08/20 16:06:24, lwchkg wrote: > > Try the json thing; I expect it will yield very readable javascript. If not, > > then we can go with this hack :S > > Good. Anyway, we are already passing a json already, so why not doing it to the > fullest? :-) > > Now we won't have to worry about another novice programmer using "==" instead of > "===" for comparison. Updated on patch #13 already. I've forgot to announce this part of the update yesterday.
I really happy with how this looks! A few more tidy things to do, but I'm happy to put my LGTM on this now :) Thanks for all this work! Other reviewers: PTAL! https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/240001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:597: return_value.SetBooleanWithoutPathExpansion(item.category, false); On 2015/08/22 15:35:29, lwchkg wrote: > On 2015/08/20 16:06:24, lwchkg wrote: > > > Try the json thing; I expect it will yield very readable javascript. If not, > > > then we can go with this hack :S > > > > Good. Anyway, we are already passing a json already, so why not doing it to > the > > fullest? :-) > > > > Now we won't have to worry about another novice programmer using "==" instead > of > > "===" for comparison. > > Updated on patch #13 already. I've forgot to announce this part of the update > yesterday. This looks really good! https://codereview.chromium.org/1248613003/diff/260001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/260001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:243: result.success = true; On 2015/08/20 17:40:27, lwchkg wrote: > Do you prefer the current style or to use braced initializer list (i.e. return > {count, true};)? this is fine. https://codereview.chromium.org/1248613003/diff/280001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/280001/chrome/app/generated_r... chrome/app/generated_resources.grd:14104: + This will permanently delete <ph name="TOTAL_COUNT">$1<ex>1234</ex></ph> items from this device but not clear synced items from other devices. as per ainslie@: missing "will" in "but... not clear" https://codereview.chromium.org/1248613003/diff/280001/chrome/app/generated_r... chrome/app/generated_resources.grd:14107: + This will permanently delete your browsing data from this device but not clear synced items from other devices. as per ainslie@: missing "will" in "but... not clear" https://codereview.chromium.org/1248613003/diff/280001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/280001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1444: var count = profileStats[key].success ? profileStats[key].count : 0; I'm not sure if an error in counting means we want to list "0". I posted a question in the bug for the UI folks, to see their answer.
lwchkg: please remove un-needed reviewers (such as Nikita and Anthony), just leave the ones who you actually need to look at this (the LGTMs you already have, + sky/sdefresne)
The CQ bit was checked by mlerman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/280001
On 2015/08/24 14:47:12, Mike Lerman wrote: > lwchkg: please remove un-needed reviewers (such as Nikita and Anthony), just > leave the ones who you actually need to look at this (the LGTMs you already > have, + sky/sdefresne) Also: Can you please add a TEST= section below BUG= in the CL description? TEST=<text> should provide an (english) description to our testers about how to go about testing this patch.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lwchkg@gmail.com changed reviewers: - anthonyvd@chromium.org, jhawkins@chromium.org, msramek@chromium.org, nkostylev@chromium.org
On 2015/08/24 14:49:43, Mike Lerman wrote: > Also: Can you please add a TEST= section below BUG= in the CL description? > TEST=<text> should provide an (english) description to our testers about how to > go about testing this patch. Okay I just tried. Since I cannot find any example, please don't laugh at me if I made this wrong.
On 2015/08/24 17:47:58, lwchkg wrote: > On 2015/08/24 14:49:43, Mike Lerman wrote: > > Also: Can you please add a TEST= section below BUG= in the CL description? > > TEST=<text> should provide an (english) description to our testers about how > to > > go about testing this patch. > > Okay I just tried. Since I cannot find any example, please don't laugh at me if > I made this wrong. Can you focus on two things? 1) How do you " Go to the user selection dialog."? and 2) Directions to populate the categories (e.g. bookmark some things! save some passwords!) and then verify that you get some counts that kinda correspond.
@Mike: Just added a response to #40 in the bug. I "replied" here because I need your opinion. https://codereview.chromium.org/1248613003/diff/280001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/280001/chrome/app/generated_r... chrome/app/generated_resources.grd:14104: + This will permanently delete <ph name="TOTAL_COUNT">$1<ex>1234</ex></ph> items from this device but not clear synced items from other devices. On 2015/08/24 14:45:40, Mike Lerman wrote: > as per ainslie@: missing "will" in "but... not clear" Acknowledged. https://codereview.chromium.org/1248613003/diff/280001/chrome/app/generated_r... chrome/app/generated_resources.grd:14107: + This will permanently delete your browsing data from this device but not clear synced items from other devices. On 2015/08/24 14:45:40, Mike Lerman wrote: > as per ainslie@: missing "will" in "but... not clear" Acknowledged. https://codereview.chromium.org/1248613003/diff/280001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/280001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1444: var count = profileStats[key].success ? profileStats[key].count : 0; On 2015/08/24 14:45:40, Mike Lerman wrote: > I'm not sure if an error in counting means we want to list "0". I posted a > question in the bug for the UI folks, to see their answer. By rolfe: (actually referring to #39 in the bug) > No. 38: Try "This will permanently delete at least 64 items..." > or "This will permanently delete the following items... How often will these failure cases happen? If it's rare I think it is not worth to add 2 more translatable strings, with extra css and html to handle the strings. While these cases can be easy to code, it adds complexity to future maintenance, and is also very difficult to test. I prefer to revert to the "no-stats" version of the dialog because it only requires a few lines of JS code. @Mike, what is your opinion? (You may post on the bug directly.)
On 2015/08/24 17:51:59, Mike Lerman wrote: > Can you focus on two things? 1) How do you " Go to the user selection dialog."? > and 2) Directions to populate the categories (e.g. bookmark some things! save > some passwords!) and then verify that you get some counts that kinda correspond. I've add a section for each of the above. Now the description is really big. (Sad. Any idea to simplify it?) Anyway, I have no idea how to populate the preferences. And once a new profile is made I have already about 30 preferences coming from nowhere. (Refer to comments #19 and #20 in the bug for the calculation.)
I've never seen someone complain for the description on the CL being too long :) https://codereview.chromium.org/1248613003/diff/280001/ui/login/account_picke... File ui/login/account_picker/user_pod_row.js (right): https://codereview.chromium.org/1248613003/diff/280001/ui/login/account_picke... ui/login/account_picker/user_pod_row.js:1444: var count = profileStats[key].success ? profileStats[key].count : 0; On 2015/08/26 14:43:12, lwchkg wrote: > On 2015/08/24 14:45:40, Mike Lerman wrote: > > I'm not sure if an error in counting means we want to list "0". I posted a > > question in the bug for the UI folks, to see their answer. > > By rolfe: (actually referring to #39 in the bug) > > No. 38: Try "This will permanently delete at least 64 items..." > > or "This will permanently delete the following items... > > How often will these failure cases happen? If it's rare I think it is not worth > to add 2 more translatable strings, with extra css and html to handle the > strings. While these cases can be easy to code, it adds complexity to future > maintenance, and is also very difficult to test. > > I prefer to revert to the "no-stats" version of the dialog because it only > requires a few lines of JS code. > > @Mike, what is your opinion? (You may post on the bug directly.) Having asked the question, I prefer to implement the answer given :) I don't think code need become significantly more complex. Track an additional variable, declared near line 1440, called "any_stat_failed" or similar. Around line 1443, if profileState[key].success == false, skip setting the textContent and other items, and instead set any_stat_failed to true, and then have some javascript to write the additional string. This seems reasonable in terms of effort. I don't know how often these cases will arise, but as I've learned with Chrome, even very small percentages are still large numbers of users.
> > @Mike, what is your opinion? (You may post on the bug directly.) > > Having asked the question, I prefer to implement the answer given :) I don't > think code need become significantly more complex. Track an additional variable, > declared near line 1440, called "any_stat_failed" or similar. Around line 1443, > if profileState[key].success == false, skip setting the textContent and other > items, and instead set any_stat_failed to true, and then have some javascript to > write the additional string. This seems reasonable in terms of effort. I don't > know how often these cases will arise, but as I've learned with Chrome, even > very small percentages are still large numbers of users. I see. Then I'll implement those extra strings in full. Anyway, should we also use UMAHistogram or some other means to track the failure rate?
On 2015/08/28 13:07:02, lwchkg wrote: > > > @Mike, what is your opinion? (You may post on the bug directly.) > > > > Having asked the question, I prefer to implement the answer given :) I don't > > think code need become significantly more complex. Track an additional > variable, > > declared near line 1440, called "any_stat_failed" or similar. Around line > 1443, > > if profileState[key].success == false, skip setting the textContent and other > > items, and instead set any_stat_failed to true, and then have some javascript > to > > write the additional string. This seems reasonable in terms of effort. I don't > > know how often these cases will arise, but as I've learned with Chrome, even > > very small percentages are still large numbers of users. > > I see. Then I'll implement those extra strings in full. Anyway, should we also > use UMAHistogram or some other means to track the failure rate? I don't think so. What decision would an UMAHistogram help us inform?
Just uploaded patch set 14 : Added warning messages with words "at least" which is shown when some stats failed. Also fixed the missing "will" in the messages.
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/1248613003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dear Mike,
Please see the latest patch. This patch handles singular/plural in strings
correctly. Sadly that there are >200 lines of changes, so you may need to review
the affected files again.
Regards,
WC Leung.
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/1248613003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Still looks pretty good, although I'm not super familiar with the pluralization pattern. Can you ask the person who suggested that pattern to review your implementation? https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:611: std::string profile_path(""); nit: No need to specify empty-string initialization, I don't think. https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:620: int total_count = 1234; // number for plural formating if |has_count| is false Please don't initialize to a random number. If the compiler requires initialization, just use 0. https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:628: (has_errors? space before ? https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:635: message = base::StringValue( just define message as base::StringValue here, on the same line you assign the value.
lwchkg@gmail.com changed reviewers: + msramek@chromium.org
+msramek for reviewing about pluralization. Dear msramek, Thank you for your help for pluralization and we need your help reviewing related code. (Basically the file linked in this comment/email and also generate_resources.grd) Regards, WC Leung. https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:611: std::string profile_path(""); On 2015/09/14 16:28:48, Mike Lerman wrote: > nit: No need to specify empty-string initialization, I don't think. Acknowledged. https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:620: int total_count = 1234; // number for plural formating if |has_count| is false On 2015/09/14 16:28:48, Mike Lerman wrote: > Please don't initialize to a random number. If the compiler requires > initialization, just use 0. Thanks for pointing out. Looks like I've missing something here. Anyway this is the value for pluralization when the argument is a placeholder (currently "...") when the value is still being calculated). Just checked https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_an... . Appears that 0 and 1234 are both unacceptable. I'll ask for more help in the bug. https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:628: (has_errors? On 2015/09/14 16:28:49, Mike Lerman wrote: > space before ? Acknowledged. And this is embarrassing. https://codereview.chromium.org/1248613003/diff/320001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:635: message = base::StringValue( On 2015/09/14 16:28:48, Mike Lerman wrote: > just define message as base::StringValue here, on the same line you assign the > value. Acknowledged.
https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14084: + =1 {This will permanently delete $1 item from this device.} $1, $2, etc. are used when you're substituting value for placeholders in a regular string In these plural constructions, the placeholder for the number in question is represented by "#". (search for other plural strings in this file to see an example) https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14087: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_NONSYNC_WITH_ERRORS" desc="Main text shown as a warning when attempting to remove an user (with some statistics failed to load)."> nit: s/with/when ? https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14092: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_NONSYNC_NOSTATS" desc="Main text shown as a warning when attempting to remove an user."> Please change the description to explain what is the difference from IDS_LOGIN_POD_USER_REMOVE_WARNING_NONSYNC. https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14095: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_HISTORY" desc="One of the statistics shown below the warning when attempting to remove an user. This is the category, and a number is displayed next to it."> Please customize the string descriptions: "Title of the statistic ... for browsing history ..." https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14107: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_CALCULATING" desc="This is used in '...' of the warning message 'This will permanently delete ... items from the device.', which is shown when attempting to remove an user. Also put next to any loading statistics below this warning. All occurences of '...' will be eventually replaced by numbers when the statistics are loaded."> As you noted yourself on the bug, it's probably better to have this sentence translated separately. One of the reasons is that the plural strings work with the number itself (# parameter), they won't accept any string for substitution (you can't do $1 = '...').
Respond to msramek's comment. https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14084: + =1 {This will permanently delete $1 item from this device.} On 2015/09/15 15:33:09, msramek wrote: > $1, $2, etc. are used when you're substituting value for placeholders in a > regular string > > In these plural constructions, the placeholder for the number in question is > represented by "#". > > (search for other plural strings in this file to see an example) The $1 is substituted in my JS code with the number put inside a <span> tag. I admit that this is ugly. I've previously tried to put the HTML tag here and set innerHTML in the JS code, but Dmitry has opposed the idea in https://codereview.chromium.org/1248613003/diff/220001/ui/login/account_picke.... Anyway, see the attached picture in the description of bug 501916 for an idea why an HTML tag is needed. https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14087: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_NONSYNC_WITH_ERRORS" desc="Main text shown as a warning when attempting to remove an user (with some statistics failed to load)."> On 2015/09/15 15:33:08, msramek wrote: > nit: s/with/when ? Acknowledged and thanks. https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14092: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_NONSYNC_NOSTATS" desc="Main text shown as a warning when attempting to remove an user."> On 2015/09/15 15:33:08, msramek wrote: > Please change the description to explain what is the difference from > IDS_LOGIN_POD_USER_REMOVE_WARNING_NONSYNC. Acknowledged. https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14095: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_HISTORY" desc="One of the statistics shown below the warning when attempting to remove an user. This is the category, and a number is displayed next to it."> On 2015/09/15 15:33:08, msramek wrote: > Please customize the string descriptions: "Title of the statistic ... for > browsing history ..." Acknowledged. https://codereview.chromium.org/1248613003/diff/320001/chrome/app/generated_r... chrome/app/generated_resources.grd:14107: + <message name="IDS_LOGIN_POD_USER_REMOVE_WARNING_CALCULATING" desc="This is used in '...' of the warning message 'This will permanently delete ... items from the device.', which is shown when attempting to remove an user. Also put next to any loading statistics below this warning. All occurences of '...' will be eventually replaced by numbers when the statistics are loaded."> On 2015/09/15 15:33:08, msramek wrote: > As you noted yourself on the bug, it's probably better to have this sentence > translated separately. > > One of the reasons is that the plural strings work with the number itself (# > parameter), they won't accept any string for substitution (you can't do $1 = > '...'). # is not okay, but $1 is possible with my own JS code. But I'll in for a separate string because this is less error-prone.
Hoping that I can continue working from the rebase today, but that's just too optimistic. I have pulled a build that did not compile. Any tips for getting good builds from git+gclient?
On 2015/09/17 15:53:38, lwchkg wrote:
> Hoping that I can continue working from the rebase today, but that's just too
> optimistic. I have pulled a build that did not compile. Any tips for getting
> good builds from git+gclient?
Ideally your current local git setup is such that you have a branch, called
"master" or equivalent, that you pulled from the remote origin, and then you
have a local sub_branch in which you're doing all your work. In that scenario:
1) Checkout your master branch
a) Do git pull and rebase. My usual syntax is "git pull --rebase origin
master", but there are many variants that work.
b) Do a "gclient sync" to update all 3rd party dependencies, regenerate GYP
files, etc. It won't compile until you do that.
2) Rebase your working branch onto your local master.
Forge ahead!
lwchkg@gmail.com changed reviewers: - sdefresne@chromium.org, sky@chromium.org
-sky, -sdefrense because the files they own are already reviewed and commited in another CL. Finally I found that I cannot build because of the reason stated in https://groups.google.com/a/chromium.org/forum/#!msg/chromium-reviews/UF13QnI... , and the solution is to delete out/Debug/gen/ui/gfx/* (or clobber the build.) I just have prepared #17 which addressed the comments. In the patch, the case with "..." used as placeholder of the number are added as separate strings. Now I'm going to test the correctness of the JS code and prepare screenshots. (There are totally 12 of them, so it takes some time.) You may review after I've posted the screenshots to the bug.
Patchset #17 (id:360001) has been deleted
Re-uploaded patch #17 because two bugs were discovered during testing. The
screenshots are taken and will be posted to the bug shortly.
To test the singular failure cases add the following code (modify it to the case
you need) before line 1446 (the beginning of updateRemoveWarningDialog_) of
user_pod_row.js
if (profileStats.length < 4)
return;
profileStats = {
'BrowsingHistory': {count: 50, success: false},
'Passwords': {count: 5, success: true},
'Bookmarks': {count: 500, success: true},
'Settings': {count: 0, success: false}
};
The case above is a loading failure with a total count of 505. (The 50 with
failure should not be counted.)
https://codereview.chromium.org/1248613003/diff/380001/chrome/app/generated_r...
File chrome/app/generated_resources.grd (right):
https://codereview.chromium.org/1248613003/diff/380001/chrome/app/generated_r...
chrome/app/generated_resources.grd:13838: + <!-- Warning messages shown
when attempting to remove an user. Synced versions has the extra words "but will
not clear synced items from other devices." Versions with loading errors has the
extra words "at least." "Calculating" versions are non-failure versions with the
number missing, which can take different plural forms when translated. -->
Not sure whether I should write the comment here or inside the messages. Please
comment.
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/1248613003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1248613003/diff/380001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/380001/chrome/app/generated_r... chrome/app/generated_resources.grd:13838: + <!-- Warning messages shown when attempting to remove an user. Synced versions has the extra words "but will not clear synced items from other devices." Versions with loading errors has the extra words "at least." "Calculating" versions are non-failure versions with the number missing, which can take different plural forms when translated. --> On 2015/09/20 17:29:08, lwchkg wrote: > Not sure whether I should write the comment here or inside the messages. Please > comment. What does inside the description is sent to the translation team to help inform them about how to translate the content. I think you've already included this description in each message. I'd be concerned that his comment may not be maintained down the road, so I'm a little hesitant to include it. The descriptions should be enough comment. https://codereview.chromium.org/1248613003/diff/380001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/380001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:617: bool is_synced_user = false, has_errors = false; please only declare one variable per line. no need to initialize them, they'll all be assigned values with GetBoolean below.
Response to comment. Patch will be submitted soon. https://codereview.chromium.org/1248613003/diff/380001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1248613003/diff/380001/chrome/app/generated_r... chrome/app/generated_resources.grd:13838: + <!-- Warning messages shown when attempting to remove an user. Synced versions has the extra words "but will not clear synced items from other devices." Versions with loading errors has the extra words "at least." "Calculating" versions are non-failure versions with the number missing, which can take different plural forms when translated. --> On 2015/09/21 19:24:51, Mike Lerman wrote: > On 2015/09/20 17:29:08, lwchkg wrote: > > Not sure whether I should write the comment here or inside the messages. > Please > > comment. > > What does inside the description is sent to the translation team to help inform > them about how to translate the content. I think you've already included this > description in each message. I'd be concerned that his comment may not be > maintained down the road, so I'm a little hesitant to include it. The > descriptions should be enough comment. Agree. So I'll remove this comment. https://codereview.chromium.org/1248613003/diff/380001/chrome/browser/ui/webu... File chrome/browser/ui/webui/signin/user_manager_screen_handler.cc (right): https://codereview.chromium.org/1248613003/diff/380001/chrome/browser/ui/webu... chrome/browser/ui/webui/signin/user_manager_screen_handler.cc:617: bool is_synced_user = false, has_errors = false; On 2015/09/21 19:24:51, Mike Lerman wrote: > please only declare one variable per line. no need to initialize them, they'll > all be assigned values with GetBoolean below. Acknowledged for one variable per line. But I must initialize them because compiler warnings will be thrown otherwise. e:\chromium\src\chrome\browser\ui\webui\signin\user_manager_screen_handler.cc(633) : warning C4701: potentially uninitialized local variable 'is_synced_user' used e:\chromium\src\chrome\browser\ui\webui\signin\user_manager_screen_handler.cc(633) : warning C4701: potentially uninitialized local variable 'has_errors' used
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/1248613003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/400001
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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/420001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
I've just uploaded a patch after a rebase, hopefully the last one before landing the CL. The only change is a three-line modification to profile_statistics.cc with reference to the changes in https://codereview.chromium.org/1370493002 Mike: If no issue is outstanding then I'll commit the CL. BTW, we need to discuss on two things: (any ideas?) 1. Storing/using the statistics in ProfileInfoCache. 2. Adding tests. I'll thinking about test ideas, e.g. test for the correctness of bookmark counting. BTW, are there any existing unit tests/browser tests about deleting profiles?
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/1248613003/420001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/420001
On 2015/10/01 19:09:58, lwchkg wrote: > I've just uploaded a patch after a rebase, hopefully the last one before landing > the CL. The only change is a three-line modification to profile_statistics.cc > with reference to the changes in https://codereview.chromium.org/1370493002 > > Mike: If no issue is outstanding then I'll commit the CL. BTW, we need to > discuss on two things: (any ideas?) > 1. Storing/using the statistics in ProfileInfoCache. > 2. Adding tests. I'll thinking about test ideas, e.g. test for the correctness > of bookmark counting. BTW, are there any existing unit tests/browser tests about > deleting profiles? You're missing an owner; please add achuith@ to the review. (I think because dpoluhkin@ moved off the project, but none the less, should get the review of an active owner). Re: 1) You should ping anthonyvd@ who is thinking about changes to the ProfileInfoCache and its API. Re: 2) There are some examples of profile deletion in profile_manager_unittest.cc: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pro...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lwchkg@gmail.com changed reviewers: + achuith@chromium.org
+achuith@ as owner of userpod and ChromeOS code. Dear achuith@, Please take a look. Thanks! (Dmitry reviewed the CL before, but he has left the project. So I need your help to review.) Regards, WC Leung.
https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:26: int count; Add a ctor like ProfileStatValue(int count, bool success) : count(count), success(success) {} https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:43: // Constructor drop this comment https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:53: Profile* profile_; I think it's more common to have data members follow methods. https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:58: const profiles::ProfileStatisticsCallback callback_; It's a bit strange to have a const data member. We usually don't do this. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:62: // Initialization. Called by constructors. Drop this comment https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:63: void Init(); What's the point of this method? https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:77: static int CountBookmarksFromNode(const bookmarks::BookmarkNode* node); Could you move this out of this class into the top level anonymous namespace? https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:84: class PasswordStoreConsumerHelper What's the point of this class? Why not have ProfileStatisticsAggregator inherit from password_manager::PasswordStoreConsumer https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:97: ProfileStatisticsAggregator* parent_; parent_ = nullptr; https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:130: if (history_service) { When can this be null? https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:33: typedef std::vector<ProfileCategoryStat> ProfileCategoryStats; nit c++11 way is: using ProfileCategoryStats = std::vector<ProfileCategoryStat>; https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:37: typedef base::Callback<void(ProfileCategoryStats)> use using
achuith@chromium.org changed reviewers: + rogerta@chromium.org
I think you want a review from Roger
achuithb@ Thanks for your comments. I'll upload another patch tomorrow. (Sorry for slow reply because I was occupied by moving house.) BTW, what the CL needs is an re-review of userpod and ChromeOS code, since the previous reviewer left the project. The other code are already reviewed by Mike. Since Roger is not an owner of userpod code, I doubt whether his review is needed. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:26: int count; On 2015/10/02 23:48:36, achuithb wrote: > Add a ctor like > ProfileStatValue(int count, bool success) : count(count), success(success) {} Mike and I agreed to use ProfileStatValue in the following way before. ProfileStatValue result; result.count = 0; result.success = false; See https://codereview.chromium.org/1248613003/diff/260001/chrome/browser/profile... Adding the ctor means we need to change this into ProfileStatValue(0, false) We prefer to use the first one, because the code easier to read (but at the expense of possibility of forgetting to assign values to members). https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:43: // Constructor On 2015/10/02 23:48:36, achuithb wrote: > drop this comment Acknowledged. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:53: Profile* profile_; On 2015/10/02 23:48:36, achuithb wrote: > I think it's more common to have data members follow methods. > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_... Acknowledged. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:58: const profiles::ProfileStatisticsCallback callback_; On 2015/10/02 23:48:36, achuithb wrote: > It's a bit strange to have a const data member. We usually don't do this. Agree. mlerman@: how do you think about this? ("const" was added as a response to a comment in patch #1). See https://codereview.chromium.org/1248613003/diff/1/chrome/browser/profiles/pro... https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:62: // Initialization. Called by constructors. On 2015/10/02 23:48:36, achuithb wrote: > Drop this comment Acknowledged. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:63: void Init(); On 2015/10/02 23:48:36, achuithb wrote: > What's the point of this method? So far we have only one constructor, so this method looks redundant. But I don't know whether we will have another constructor (e.g. a specially crafted constructor for unit tests). With Init() method these kind of changes should be easy. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:77: static int CountBookmarksFromNode(const bookmarks::BookmarkNode* node); On 2015/10/02 23:48:36, achuithb wrote: > Could you move this out of this class into the top level anonymous namespace? Sounds good. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:84: class PasswordStoreConsumerHelper On 2015/10/02 23:48:36, achuithb wrote: > What's the point of this class? Why not have ProfileStatisticsAggregator inherit > from password_manager::PasswordStoreConsumer I don't want PasswordStoreConsumer.* to pollute ProfileStatisticsAggregator. Inheriting is convenient, but it looks like "using namespace std" to me, so I avoid inheriting another class unless strictly necessary. This may also may cause problems if PasswordStoreConsumer is modified to use a method/data member of the same names.. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:97: ProfileStatisticsAggregator* parent_; On 2015/10/02 23:48:36, achuithb wrote: > parent_ = nullptr; Acknowledged. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:130: if (history_service) { On 2015/10/02 23:48:36, achuithb wrote: > When can this be null? If GetForProfileWithoutCreating fails this is null. I'm not sure when this happens, but for safety I don't assume that any resource allocation is an automatic success. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.h (right): https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:33: typedef std::vector<ProfileCategoryStat> ProfileCategoryStats; On 2015/10/02 23:48:36, achuithb wrote: > nit c++11 way is: > using ProfileCategoryStats = std::vector<ProfileCategoryStat>; Acknowledged. I'll switch to using c++11 features because this file is new. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.h:37: typedef base::Callback<void(ProfileCategoryStats)> On 2015/10/02 23:48:36, achuithb wrote: > use using Acknowledged.
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/1248613003/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/440001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2015/10/11 20:16:42, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run. Taking a look, will try to patch in to my machine and play with it.
rogerta@chromium.org changed reviewers: + anthonyvd@chromium.org
On 2015/10/13 15:10:45, Roger Tawa wrote: > On 2015/10/11 20:16:42, commit-bot: I haz the power wrote: > > Dry run: This issue passed the CQ dry run. > > Taking a look, will try to patch in to my machine and play with it. This CL looks good. I noticed a couple of things after playing with it: 1/ Removing a profile from the settings page never shows counts. However, the plan is to remove this part of settings anyway, so I don't think it's a big deal. 2/ Counts are not shown for profiles that are not already opened. This is confusing to the user, they'll have a difficult time understanding why counts show sometimes and not other times. I see that bug crbug.com/502346 was opened to track this. I'm OK with committing this CL as is right now, if we have a commitment to fix 502346 by M48 branch point (Nov 13th). lwchkg: are you still available to fix 502346 by Nov 13th?
On 2015/10/13 15:49:26, Roger Tawa wrote: > On 2015/10/13 15:10:45, Roger Tawa wrote: > > On 2015/10/11 20:16:42, commit-bot: I haz the power wrote: > > > Dry run: This issue passed the CQ dry run. > > > > Taking a look, will try to patch in to my machine and play with it. > > This CL looks good. I noticed a couple of things after playing with it: > > 1/ Removing a profile from the settings page never shows counts. However, the > plan is to remove this part of settings anyway, so I don't think it's a big > deal. > Thanks for telling. I've been puzzled why Mike didn't ask me to do anything in the settings page. Now I understands. > 2/ Counts are not shown for profiles that are not already opened. This is > confusing to the user, they'll have a difficult time understanding why counts > show sometimes and not other times. I see that bug crbug.com/502346 was opened > to track this. I'm OK with committing this CL as is right now, if we have a > commitment to fix 502346 by M48 branch point (Nov 13th). > > lwchkg: are you still available to fix 502346 by Nov 13th? Yes. I'll start working on this issue in the weekend. (I'm busy with my full-time job (not an IT job) for two more days. After that things should improve.) BTW, (1) do you have any news about the changes of ProfileInfoCache by anthonyvd@? (2) Bug 502346 said "These should be stored to ProfileInfoCache when closing a profile." To me "closing a profile" means browser shutdown, which is when resources are deallocated. Is there a way to ensure that the resources used by GetProfileStatistics are not deallocated before the async counts finish?
Anyway, I still need the l-g-t-m from achuithb for commiting this CL.
On 2015/10/14 16:55:29, lwchkg wrote: > Anyway, I still need the l-g-t-m from achuithb for commiting this CL. I'll be happy to lg2m if Roger is satisfied.
lgtm Awesome lwchkg! I also discussed this with Eli, and he's good with committing as is and fixing 502346 in a follow up CL. I'll let Anthony response to question (1). For (2), closing a profile can be just closing the last window open for that profile. The profile remains in memory though. I'm not sure how to best block chrome shutdown. I think you can post to a thread with a flag to block shutdown, I'll try to find out. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:40: // The class is used internally by GetProfileStatistics function. Put this comment above class declaration at line 30. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:46: base::CancelableTaskTracker* tracker); Don't need explicit for ctor wirh more than one arg.
On 2015/10/16 12:22:35, Roger Tawa wrote: > lgtm > > Awesome lwchkg! I also discussed this with Eli, and he's good with committing > as is and fixing 502346 in a follow up CL. > > I'll let Anthony response to question (1). > > For (2), closing a profile can be just closing the last window open for that > profile. The profile remains in memory though. I'm not sure how to best block > chrome shutdown. I think you can post to a thread with a flag to block > shutdown, I'll try to find out. > > https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... > File chrome/browser/profiles/profile_statistics.cc (right): > > https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... > chrome/browser/profiles/profile_statistics.cc:40: // The class is used > internally by GetProfileStatistics function. > Put this comment above class declaration at line 30. > > https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... > chrome/browser/profiles/profile_statistics.cc:46: base::CancelableTaskTracker* > tracker); > Don't need explicit for ctor wirh more than one arg. To make sure a task finishes before the browser quits, you need to post a task with BLOCK__SHUTDOWN. See here for an example: https://code.google.com/p/chromium/codesearch#chromium/src/base/prefs/json_pr...
lgtm
On 2015/10/14 16:51:50, lwchkg wrote: > On 2015/10/13 15:49:26, Roger Tawa wrote: > > On 2015/10/13 15:10:45, Roger Tawa wrote: > > > On 2015/10/11 20:16:42, commit-bot: I haz the power wrote: > > > > Dry run: This issue passed the CQ dry run. > > > > > > Taking a look, will try to patch in to my machine and play with it. > > > > This CL looks good. I noticed a couple of things after playing with it: > > > > 1/ Removing a profile from the settings page never shows counts. However, the > > plan is to remove this part of settings anyway, so I don't think it's a big > > deal. > > > Thanks for telling. I've been puzzled why Mike didn't ask me to do anything in > the settings page. Now I understands. > > > 2/ Counts are not shown for profiles that are not already opened. This is > > confusing to the user, they'll have a difficult time understanding why counts > > show sometimes and not other times. I see that bug crbug.com/502346 was > opened > > to track this. I'm OK with committing this CL as is right now, if we have a > > commitment to fix 502346 by M48 branch point (Nov 13th). > > > > lwchkg: are you still available to fix 502346 by Nov 13th? > > Yes. I'll start working on this issue in the weekend. (I'm busy with my > full-time job (not an IT job) for two more days. After that things should > improve.) > > BTW, > (1) do you have any news about the changes of ProfileInfoCache by anthonyvd@? My changes basically introduce a more sensible interface to the ProfileInfoCache. It's landed and you can see the review here: https://codereview.chromium.org/1214483002/ . If your code ends up talking to the ProfileInfoCache, it should do it using the ProfileAttributesStorage interface instead of the ProfileInfoCache directly. > (2) Bug 502346 said "These should be stored to ProfileInfoCache when closing a > profile." To me "closing a profile" means browser shutdown, which is when > resources are deallocated. Is there a way to ensure that the resources used by > GetProfileStatistics are not deallocated before the async counts finish?
Thank you everyone. Now I'll land and continue with the ProfileInfoCache part of 501916/502346.
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org, dpolukhin@chromium.org, mlerman@chromium.org Link to the patchset: https://codereview.chromium.org/1248613003/#ps440001 (title: "Respond to achuithb's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248613003/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248613003/440001
Message was sent while issue was closed.
Committed patchset #20 (id:440001)
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/3bbd8033884cca5e08fc8618670608798e2e20f8 Cr-Commit-Position: refs/heads/master@{#354679}
Message was sent while issue was closed.
Forgot to follow the comments of Roger. :-( I'll fix them in a follow-up CL. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... File chrome/browser/profiles/profile_statistics.cc (right): https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:40: // The class is used internally by GetProfileStatistics function. On 2015/10/16 12:22:35, Roger Tawa wrote: > Put this comment above class declaration at line 30. Acknowledged. https://codereview.chromium.org/1248613003/diff/420001/chrome/browser/profile... chrome/browser/profiles/profile_statistics.cc:46: base::CancelableTaskTracker* tracker); On 2015/10/16 12:22:35, Roger Tawa wrote: > Don't need explicit for ctor wirh more than one arg. Acknowledged. |
