Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7138002:
Add profiles to wrench menu (Closed)
|
Created:
9 years, 6 months ago by sail Modified:
9 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, rginda+watch_chromium.org, achuith+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd profiles to wrench menu
This change adds a profile menu to the wrench menu.
If the user has one profile then the wrench menu looks like this:
Wrench Menu >
...
Downloads
-------
New Browsing Profile...
-------
...
If the user has two or more profile then the wrench menu looks like this:
Wrench Menu >
...
Downloads
-------
Browsing Profiles >
Profile 1
Profile 2
--------
New Browsing Profile...
-------
...
BUG=None
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88942
Patch Set 1 #Patch Set 2 : Opening windows #
Total comments: 33
Patch Set 3 : Removed dynamic menu for now #Patch Set 4 : Fixed menu item #
Total comments: 2
Patch Set 5 : Addressed review comments #
Total comments: 6
Patch Set 6 : Add profiles to wrench menu #
Total comments: 5
Patch Set 7 : '' #Patch Set 8 : '' #
Messages
Total messages: 21 (0 generated)
http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... ui/base/models/simple_menu_model.cc:183: void SimpleMenuModel::RemoveItemAt(int index) { This change isn't ready yet but I'm stuck on this part. I need to be able to change the menu as the user adds and removes profiles. I tried changing the menu model but this ends up crashing in the UI code. It looks like the UI code doesn't realize that the model has changed and it starts to dereference stale pointers. Is there a good way to do this? Thanks!
http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... ui/base/models/simple_menu_model.cc:183: void SimpleMenuModel::RemoveItemAt(int index) { On 2011/06/10 06:57:31, sail wrote: > This change isn't ready yet but I'm stuck on this part. > I need to be able to change the menu as the user adds and removes profiles. > I tried changing the menu model but this ends up crashing in the UI code. It > looks like the UI code doesn't realize that the model has changed and it starts > to dereference stale pointers. > > Is there a good way to do this? Thanks! Several SimpleMenuModel clients create a new ui menu from the model every time the menu is displayed. That sounds like a simple way to make this work. Which platform are you seeing this problem on?
On 2011/06/10 14:34:23, Nico wrote: > http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... > File ui/base/models/simple_menu_model.cc (right): > > http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... > ui/base/models/simple_menu_model.cc:183: void SimpleMenuModel::RemoveItemAt(int > index) { > On 2011/06/10 06:57:31, sail wrote: > > This change isn't ready yet but I'm stuck on this part. > > I need to be able to change the menu as the user adds and removes profiles. > > I tried changing the menu model but this ends up crashing in the UI code. It > > looks like the UI code doesn't realize that the model has changed and it > starts > > to dereference stale pointers. > > > > Is there a good way to do this? Thanks! > > Several SimpleMenuModel clients create a new ui menu from the model every time > the menu is displayed. That sounds like a simple way to make this work. Which > platform are you seeing this problem on? ++ to thakis -- I think that if you want remove functionality here, you probably want to use a more sophisticated MenuModel (though I would just do as thakis suggests, and rebuild from scratch).
http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... ui/base/models/simple_menu_model.cc:183: void SimpleMenuModel::RemoveItemAt(int index) { On 2011/06/10 14:34:23, Nico wrote: > On 2011/06/10 06:57:31, sail wrote: > > This change isn't ready yet but I'm stuck on this part. > > I need to be able to change the menu as the user adds and removes profiles. > > I tried changing the menu model but this ends up crashing in the UI code. It > > looks like the UI code doesn't realize that the model has changed and it > starts > > to dereference stale pointers. > > > > Is there a good way to do this? Thanks! > > Several SimpleMenuModel clients create a new ui menu from the model every time > the menu is displayed. That sounds like a simple way to make this work. Which > platform are you seeing this problem on? I'm seeing this on mac. So do I change wrench menu controller to build a new menu each time? This will mean that I have to move the edit and zoom buttons into a separate nib. Is that ok?
http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:445: if (path_map) nit: return path_map ? path_map->size() : 0; http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:479: // Pending, need to insert it alphabetically. Pending -> TODO http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.h:68: FilePath GetFilePathOfProfileAtIndex(size_t index, FilePath user_data_dir); const FilePath& http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.h:181: std::vector<std::pair<FilePath, string16> > How about a typedef. And document what the string16 is. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:226: void ProfilesSubMenuModel::Build() { Rename this Init and have the caller invoke it (http://www.corp.google.com/eng/doc/cppguide.xml#Doing_Work_in_Constructors) http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:235: AddItemWithStringId(IDC_CREATE_NEW_PROFILE, Are you sure you don't want this first? Having the new menu first then the list of profiles better matches the bookmarks menu. The bookmarks menu has operations related to the bookmarks first, then the bookmarks. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:263: ProfileManager* profile_manager = g_browser_process->profile_manager(); Is ProfileManager, Profile and all the classes that Profile is going to create really thread safe? http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:280: Browser* browser = BrowserList::FindAnyBrowser(profile, false); Are you sure you don't want FindTabbedBrowser(profile, true)? http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:506: case NotificationType::PROFILE_ADDED: { This will cause problems if the menu is showing. Seems like it would be better to rebuild the model before showing and have the ProfilesMenuModel cache the menu items. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:691: if (g_browser_process->profile_manager()->GetNumberOfProfiles() > 1) { Seems weird to me to change the type based on contents. Did you run this by Glen and company? http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.h (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.h:85: class ProfilesSubMenuModel : public ui::SimpleMenuModel, Does this really need to be in the header? http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.h:104: void GetProfileOnIOThread(FilePath profile_path); const FilePath& (and on next line). Also, add descriptions. http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... ui/base/models/simple_menu_model.cc:183: void SimpleMenuModel::RemoveItemAt(int index) { On 2011/06/10 14:34:23, Nico wrote: > On 2011/06/10 06:57:31, sail wrote: > > This change isn't ready yet but I'm stuck on this part. > > I need to be able to change the menu as the user adds and removes profiles. > > I tried changing the menu model but this ends up crashing in the UI code. It > > looks like the UI code doesn't realize that the model has changed and it > starts > > to dereference stale pointers. > > > > Is there a good way to do this? Thanks! > > Several SimpleMenuModel clients create a new ui menu from the model every time > the menu is displayed. That sounds like a simple way to make this work. Which > platform are you seeing this problem on? The windows side does this. Of course you would have to take care not to modify the model while the menu is showing, otherwise bad things would happen.
http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... File ui/base/models/simple_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/ui/base/models/simple_menu_m... ui/base/models/simple_menu_model.cc:183: void SimpleMenuModel::RemoveItemAt(int index) { On 2011/06/10 14:52:59, sail wrote: > On 2011/06/10 14:34:23, Nico wrote: > > On 2011/06/10 06:57:31, sail wrote: > > > This change isn't ready yet but I'm stuck on this part. > > > I need to be able to change the menu as the user adds and removes profiles. > > > I tried changing the menu model but this ends up crashing in the UI code. It > > > looks like the UI code doesn't realize that the model has changed and it > > starts > > > to dereference stale pointers. > > > > > > Is there a good way to do this? Thanks! > > > > Several SimpleMenuModel clients create a new ui menu from the model every time > > the menu is displayed. That sounds like a simple way to make this work. Which > > platform are you seeing this problem on? > > I'm seeing this on mac. > So do I change wrench menu controller to build a new menu each time? This will > mean that I have to move the edit and zoom buttons into a separate nib. Is that > ok? In general, MenuModel is immutable after creation. There are no delegate notifications for when the Model changes and all the clients (at least on Mac) do not expect for the model to mutate. I may be mis-remembering, so take this with a grain of salt, but I think moving views between menu items did not work in the past. So, you would need to move the objects into a new xib and re-load it each time. Unfortunately, the current architecture of the WMC doesn't really expect this. If you need me to help at all with this, let me know. The other option, which I think would be all-around preferable, would be to make a MutableMenuModel which would broadcast change notifications to its delegate. This has been something that I've been meaning to do for a while but haven't had the time to pick up. Then the model wouldn't have to be rebuilt each time and items could be added/inserted/mutated at will.
> The other option, which I think would be all-around preferable, would be > to make a MutableMenuModel which would broadcast change notifications to > its delegate. This has been something that I've been meaning to do for a > while but haven't had the time to pick up. Then the model wouldn't have > to be rebuilt each time and items could be added/inserted/mutated at > will. I don't have a strong opinion on this, but recreating the menu before display every time instead of having observer code seems a lot simpler to me (no observer code necessary; menus which could easily do with 'recreate before display' won't be lured into complexity by the tempting name 'MutableMenuModel'; etc). So I contest the "all-around" preferable :-) I'd leave it up to sail to decide what's the best approach in this case.
On Fri, Jun 10, 2011 at 10:06 AM, Nico Weber <thakis@chromium.org> wrote: >> The other option, which I think would be all-around preferable, would be >> to make a MutableMenuModel which would broadcast change notifications to >> its delegate. This has been something that I've been meaning to do for a >> while but haven't had the time to pick up. Then the model wouldn't have >> to be rebuilt each time and items could be added/inserted/mutated at >> will. > > I don't have a strong opinion on this, but recreating the menu before > display every time instead of having observer code seems a lot simpler > to me (no observer code necessary; menus which could easily do with > 'recreate before display' won't be lured into complexity by the > tempting name 'MutableMenuModel'; etc). So I contest the "all-around" > preferable :-) +1 -Scott
sky's a sufficiently good reviewer of this stuff that I'm going to take myself off the list.
I removed the dynamic menu code for now. That's sufficiently complicated to deserve its own CL. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:445: if (path_map) On 2011/06/10 16:24:14, sky wrote: > nit: return path_map ? path_map->size() : 0; Done. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:479: // Pending, need to insert it alphabetically. On 2011/06/10 16:24:14, sky wrote: > Pending -> TODO Removed and added sorting. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.h:68: FilePath GetFilePathOfProfileAtIndex(size_t index, FilePath user_data_dir); On 2011/06/10 16:24:14, sky wrote: > const FilePath& Done. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.h:181: std::vector<std::pair<FilePath, string16> > On 2011/06/10 16:24:14, sky wrote: > How about a typedef. And document what the string16 is. Done. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:226: void ProfilesSubMenuModel::Build() { On 2011/06/10 16:24:14, sky wrote: > Rename this Init and have the caller invoke it > (http://www.corp.google.com/eng/doc/cppguide.xml#Doing_Work_in_Constructors) I agree but all the other classes in this file do the same thing so doing it different here would be confusing. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:235: AddItemWithStringId(IDC_CREATE_NEW_PROFILE, On 2011/06/10 16:24:14, sky wrote: > Are you sure you don't want this first? Having the new menu first then the list > of profiles better matches the bookmarks menu. The bookmarks menu has operations > related to the bookmarks first, then the bookmarks. This menu will mainly be used for switching between profiles so it makes sense to put the "create new profile" command last. For example, you look through all the profiles, if by the time you reach the end and you haven't found the one you want yet then you choose "create new profile". http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:263: ProfileManager* profile_manager = g_browser_process->profile_manager(); On 2011/06/10 16:24:14, sky wrote: > Is ProfileManager, Profile and all the classes that Profile is going to create > really thread safe? The GetProfile() function is meant to be called on a IO thread. Miranda would know more. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:280: Browser* browser = BrowserList::FindAnyBrowser(profile, false); On 2011/06/10 16:24:14, sky wrote: > Are you sure you don't want FindTabbedBrowser(profile, true)? Ahh, right. Switched to FindTabbedBrowser(). I don't think we want to match incognito windows. Incognito windows will have a different avatar and it won't be clear that you actually switched to a different profile. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:506: case NotificationType::PROFILE_ADDED: { On 2011/06/10 16:24:14, sky wrote: > This will cause problems if the menu is showing. Seems like it would be better > to rebuild the model before showing and have the ProfilesMenuModel cache the > menu items. Agreed, removed. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:691: if (g_browser_process->profile_manager()->GetNumberOfProfiles() > 1) { On 2011/06/10 16:24:14, sky wrote: > Seems weird to me to change the type based on contents. Did you run this by Glen > and company? Yea, we went through this UI in the profile meeting last week. We can definitely tweak this once it's checked in.
A few comments; also, right now the submenu activation doesn't work in Windows. I also crashed the browser every time I tried to load a profile with no browser window open from the profile menu -- I'm betting that's because of the load on IOThread issue. There are other issues (existing wrench menus don't change in response to a profile added in another wrench menu, for example), but as this is the first cut, I assume you are going to address most of these in other CLs, so that's fine. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:254: profile_path)); You don't want to do this on the IO Thread -- see below. What you want is something that launches an existing profile asynchronously, such that the stuff that touches the File System in profile creation takes place on the file thread. This will be very similar to ProfileManager::CreateMultiProfileAsync -- except that instead of observing with a NewProfileLauncher that opens the sync signin dialog after launch, observe with some other kind of launcher that calls back to the OpenWindowWithProfile task. See my comment below on GetProfileOnIOThread. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:263: ProfileManager* profile_manager = g_browser_process->profile_manager(); On 2011/06/11 01:27:37, sail wrote: > On 2011/06/10 16:24:14, sky wrote: > > Is ProfileManager, Profile and all the classes that Profile is going to create > > really thread safe? > > The GetProfile() function is meant to be called on a IO thread. Miranda would > know more. I am trying to remember a discussion about this, but I don't remember this -- could you remind me if we ever decided this was going to be the case? Certainly at the moment GetProfile() should not be called on the IO Thread -- Scott is correct that this is probably not thread safe. See comment above for a suggestion on how to launch an existing profile that's not currently loaded using asynchronous File Thread methods (and feel free to steal from the code I wrote to do this from my menu CL from last week if you want -- the LaunchExistingProfile code). http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:280: Browser* browser = BrowserList::FindAnyBrowser(profile, false); On 2011/06/11 01:27:37, sail wrote: > On 2011/06/10 16:24:14, sky wrote: > > Are you sure you don't want FindTabbedBrowser(profile, true)? > > Ahh, right. Switched to FindTabbedBrowser(). > I don't think we want to match incognito windows. Incognito windows will have a > different avatar and it won't be clear that you actually switched to a different > profile. Could you rename this FindAnyBrowserWithProfile, to make absolutely clear what's going on? http://codereview.chromium.org/7138002/diff/5006/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/5006/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:483: FilePath file_path(*it); I think you're going to need to handle Windows differently from Posix here; Win needs a wide type for FilePath.
http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:280: Browser* browser = BrowserList::FindAnyBrowser(profile, false); On 2011/06/11 15:09:08, Miranda Callahan wrote: > On 2011/06/11 01:27:37, sail wrote: > > On 2011/06/10 16:24:14, sky wrote: > > > Are you sure you don't want FindTabbedBrowser(profile, true)? > > > > Ahh, right. Switched to FindTabbedBrowser(). > > I don't think we want to match incognito windows. Incognito windows will have > a > > different avatar and it won't be clear that you actually switched to a > different > > profile. > > Could you rename this FindAnyBrowserWithProfile, to make absolutely clear what's > going on? nm -- I see that these are existing methods in BrowserList, and that you've changed to FindTabbedProfile. Not a fan, but any change belongs in a different CL.
Thanks for the review! I think this version should work on Windows. Checking right now. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:254: profile_path)); On 2011/06/11 15:09:08, Miranda Callahan wrote: > You don't want to do this on the IO Thread -- see below. What you want is > something that launches an existing profile asynchronously, such that the stuff > that touches the File System in profile creation takes place on the file thread. > This will be very similar to ProfileManager::CreateMultiProfileAsync -- except > that instead of observing with a NewProfileLauncher that opens the sync signin > dialog after launch, observe with some other kind of launcher that calls back to > the OpenWindowWithProfile task. > > See my comment below on GetProfileOnIOThread. Done. http://codereview.chromium.org/7138002/diff/3001/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:263: ProfileManager* profile_manager = g_browser_process->profile_manager(); On 2011/06/11 15:09:08, Miranda Callahan wrote: > On 2011/06/11 01:27:37, sail wrote: > > On 2011/06/10 16:24:14, sky wrote: > > > Is ProfileManager, Profile and all the classes that Profile is going to > create > > > really thread safe? > > > > The GetProfile() function is meant to be called on a IO thread. Miranda would > > know more. > > I am trying to remember a discussion about this, but I don't remember this -- > could you remind me if we ever decided this was going to be the case? Certainly > at the moment GetProfile() should not be called on the IO Thread -- Scott is > correct that this is probably not thread safe. > > See comment above for a suggestion on how to launch an existing profile that's > not currently loaded using asynchronous File Thread methods (and feel free to > steal from the code I wrote to do this from my menu CL from last week if you > want -- the LaunchExistingProfile code). Hey Miranda, we didn't discuss this. I just confused about what ProfileManager::GetProfile() does. I switched the code to use CreateProfileAsync() and it seems to work well. I'm testing on Windows right now. http://codereview.chromium.org/7138002/diff/5006/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/5006/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:483: FilePath file_path(*it); On 2011/06/11 15:09:08, Miranda Callahan wrote: > I think you're going to need to handle Windows differently from Posix here; Win > needs a wide type for FilePath. Ouch, fixed. Testing on windows right now. Also, quick question. Are we planning to suppor unicode profile names or unicode profile paths? I'm guessing unicode profile paths is not necessary since we just generate them ourselves. Unicode profile names might be useful. http://codereview.chromium.org/7138002/diff/7010/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:256: // Profile is being created. Add observer to list. I think this was a leak in the old code. Let me know if this make sense.
Looks good -- a couple more questions. http://codereview.chromium.org/7138002/diff/7010/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/7010/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:256: delete observer; Ah, good catch -- yes, this was skipped before. http://codereview.chromium.org/7138002/diff/7010/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:469: ProfileManager::GetSortedProfilesFromDirectoryMap() { Is there any reason to not keep these in sorted order, if you always want to refer to them in sorted order? Would it make more sense to just sort on insert, and not have to call the sort function every time we refer to the set of profiles? http://codereview.chromium.org/7138002/diff/7010/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/7010/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:273: bool ProfilesSubMenuModel::IsCommandIdChecked(int command_id) const { Are these really going to be checkboxed menu items? If so, could you add a comment to the .h file explaining exactly which menu items will have checkboxes?
Fixed windows build, addressed review comments. http://codereview.chromium.org/7138002/diff/7010/chrome/browser/profiles/prof... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/7010/chrome/browser/profiles/prof... chrome/browser/profiles/profile_manager.cc:469: ProfileManager::GetSortedProfilesFromDirectoryMap() { On 2011/06/13 22:08:11, Miranda Callahan wrote: > Is there any reason to not keep these in sorted order, if you always want to > refer to them in sorted order? Would it make more sense to just sort on insert, > and not have to call the sort function every time we refer to the set of > profiles? Do you mean keeping a cache of the profiles as a member variable? I was worried about the extra complexity of keeping the cache in sync. Currently this is very inexpensive since we don't have many profiles. Using a cache would definitely be something we could do later. http://codereview.chromium.org/7138002/diff/7010/chrome/browser/ui/toolbar/wr... File chrome/browser/ui/toolbar/wrench_menu_model.cc (right): http://codereview.chromium.org/7138002/diff/7010/chrome/browser/ui/toolbar/wr... chrome/browser/ui/toolbar/wrench_menu_model.cc:273: bool ProfilesSubMenuModel::IsCommandIdChecked(int command_id) const { On 2011/06/13 22:08:11, Miranda Callahan wrote: > Are these really going to be checkboxed menu items? If so, could you add a > comment to the .h file explaining exactly which menu items will have checkboxes? I found that the checkmark helped when switching between profiles. We can tweak this as we start using the menu more. I added a comment explaining this in the header file.
LGTM http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:450: size_t index, const FilePath& user_data_dir) { each param on its own line. http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:67: string16 GetNameOfProfileAtIndex(size_t index); const string16&
http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.cc (right): http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.cc:450: size_t index, const FilePath& user_data_dir) { On 2011/06/13 23:15:15, sky wrote: > each param on its own line. Done. http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:67: string16 GetNameOfProfileAtIndex(size_t index); On 2011/06/13 23:15:15, sky wrote: > const string16& Hm.. in this case the name of the profile is stored in a temp variable. Is returning a reference to the temp variable ok?
http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_manager.h (right): http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... chrome/browser/profiles/profile_manager.h:67: string16 GetNameOfProfileAtIndex(size_t index); 2011/06/13 23:26:12, sail wrote: > On 2011/06/13 23:15:15, sky wrote: > > const string16& > > Hm.. in this case the name of the profile is stored in a temp variable. Is > returning a reference to the temp variable ok? Yes, you're right. Definitely don't use const string16& then!
LGTM, thanks! On 2011/06/13 23:59:39, sky wrote: > http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... > File chrome/browser/profiles/profile_manager.h (right): > > http://codereview.chromium.org/7138002/diff/14001/chrome/browser/profiles/pro... > chrome/browser/profiles/profile_manager.h:67: string16 > GetNameOfProfileAtIndex(size_t index); > 2011/06/13 23:26:12, sail wrote: > > On 2011/06/13 23:15:15, sky wrote: > > > const string16& > > > > Hm.. in this case the name of the profile is stored in a temp variable. Is > > returning a reference to the temp variable ok? > > Yes, you're right. Definitely don't use const string16& then! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
