Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1540543002:
Uniquify profile shortcut name. (Closed)
|
Created:
5 years ago by Michael K. (Yandex Team) Modified:
4 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUniquify profile shortcut name.
Profile desktop shortcut can have a name that conflicts with existing
shortcuts. This commit tries to fix it by uniquifying shortcut name: by
appending "(2)", "(3)" ... suffix if there is a conflict.
If there are two or more profiles with identical logical names they are all
able to have desktop shortcuts at the same time now.
R=erg@chromium.org
BUG=572593
Committed: https://crrev.com/915cc7306b0899ed017f7656dd82de8a4e4184ec
Cr-Commit-Position: refs/heads/master@{#370625}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fix review notes. #
Total comments: 10
Patch Set 3 : Fix review notes #2. #
Total comments: 6
Patch Set 4 : Fix review notes #3 #
Messages
Total messages: 44 (15 generated)
https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc (right): https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_unittest_win.cc:241: profile_info_cache_->GetIndexOfProfileWithPath(profile_path); This was actually a bug.
Description was changed from ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG= ========== to ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG= ==========
mkolom@yandex-team.ru changed reviewers: + asvitkine@chromium.org, calamity@chromium.org - erg@chromium.org
mkolom@yandex-team.ru changed reviewers: + anthonyvd@chromium.org
Can you associate this CL with a crbug via BUG= ?
On 2015/12/22 16:30:26, asvitkine (OOO Dec24 - Jan4) wrote: > Can you associate this CL with a crbug via BUG= ? I've just made a search through http://crbug.com/ and can't find any issue related to this commit. Should I create a new one?
Yes, please create one. Thanks for the fix, by the way! However, I likely won't have time to review this until after my vacation on Jan 4th, so please be patient. I do plan to look at it thoroughly when I'm back. On Wed, Dec 23, 2015 at 2:58 AM, <mkolom@yandex-team.ru> wrote: > On 2015/12/22 16:30:26, asvitkine (OOO Dec24 - Jan4) wrote: > >> Can you associate this CL with a crbug via BUG= ? >> > > I've just made a search through http://crbug.com/ and can't find any issue > related to this commit. > Should I create a new one? > > > https://codereview.chromium.org/1540543002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG= ========== to ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG=572593 ==========
I created the issue at crbug.com and made an association with BUG=. PTAL
Hi, thanks for this CL! Just a few comments. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:591: const auto shortcuts = ListUserDesktopContents(filter); nit: Can you write the actual return type when calling those ListUserDesktopContents and GetShortcutUniqueFilenameForProfile? I think it's relevant when reading the functions. Of course, you can leave auto in range-based for loops and for iterators. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:721: for (int uniquifier = 2; excludes_names.count(name) > 0; ++uniquifier) Little bikeshedding: I think this should start at "(1)" since that's seems more consistent with the Chrome Download manager (which goes "download.ext", "download (1).ext", etc) and Windows itself (which goes "file.ext", "file - Copy.ext", "file - Copy (2).ext", etc). In both those cases, the original file is index 0. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.h (right): https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.h:30: // The same as above but uniqueness is guaranteed. nit: maybe name the function directly so this comment isn't made invalid by something being inserted between the 2 functions.
asvitkine@chromium.org changed reviewers: + gab@chromium.org
+gab who's also a good reviewer for this code https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:295: // filtered by predicate |p|. Nit: Wrapping is off for this comment. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:296: template <class Filter> Templates are not commonly used in Chromium (outside of base/ utilities like callback.h). In this case, it seems the only reason you're using it so you can pass in a filter that always returns true as a closure. If that's the only use case, I don't think its worth bloating the binary with two machine codes for this function (as a result of the template). Instead, pass the filter as a const ShortcutFilenameMatcher* and allow it to be nullptr, in which case it wouldn't be used. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:309: if (p(path)) p(path) doesn't sound very readable. Name the parameter more meaningfully. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:508: std::inserter(shortcuts, shortcuts.begin()), filter); Why copy_if here instead of passing the filter directly to ListUserDesktopContents()? If there's a good reason, it should at least have a comment. But it's not obvious to me. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:755: !shortcut_suffix.ends_with(L")")) Nit: {}'s https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.cc:758: [](wchar_t c) { return c >= L'0' && c <= L'9'; }); Can this be isdigit? https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_shortcut_manager_win.h (right): https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.h:31: base::string16 GetShortcutUniqueFilenameForProfile( Nit: GetUniqueShortcutFilenameForProfile(). https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.h:36: class ShortcutFilenameMatcher { Add a comment about the purpose of this class. https://codereview.chromium.org/1540543002/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_shortcut_manager_win.h:49: }; Nit: DISALLOW_COPY_AND_ASSIGN()
PTAL. All review notes have been fixed.
Driving-by. Do we even want to support profiles with the same name? That just sounds confusing... Maybe we should simply disallow profiles with the same name (i.e. uniquify profile names on creation for accounts with the same name). I know for myself I have multiple accounts which default to the same name (mine) on first sign-in but I've long ago manually fixed them up to have explicitly different names or it's just confusing.
On 2016/01/11 18:21:15, gab wrote: > Driving-by. > > Do we even want to support profiles with the same name? That just sounds > confusing... Maybe we should simply disallow profiles with the same name (i.e. > uniquify profile names on creation for accounts with the same name). > > I know for myself I have multiple accounts which default to the same name (mine) > on first sign-in but I've long ago manually fixed them up to have explicitly > different names or it's just confusing. That decision was taken before my time but looking at bugs about this, such as https://code.google.com/p/chromium/issues/detail?id=532816 it seems like it was a conscious choice. Personally I think we want to support it. For example, I use two profiles with the same name but differentiate between them based on the Avatar and Theme. I think preventing things like this might feel a little arbitrary as well as surface poorly in the UI. An example is you have a profile named "Foo" and sign in with a Google Account for which the profile name is also "Foo". One of them will have to be uniquified, which will be sync'ed to all your other machines (which don't necessarily have 2 profiles named "Foo").
Ok sgtm then, I don't feel strongly just wanted to make sure we agreed on the premise of the CL before reviewing it. Le lun. 11 janv. 2016 14:19, <anthonyvd@chromium.org> a écrit : > On 2016/01/11 18:21:15, gab wrote: > > Driving-by. > > > Do we even want to support profiles with the same name? That just sounds > > confusing... Maybe we should simply disallow profiles with the same name > > (i.e. > > uniquify profile names on creation for accounts with the same name). > > > I know for myself I have multiple accounts which default to the same name > (mine) > > on first sign-in but I've long ago manually fixed them up to have > > explicitly > > different names or it's just confusing. > > That decision was taken before my time but looking at bugs about this, such > as > https://code.google.com/p/chromium/issues/detail?id=532816 it seems like > it > was > a conscious choice. Personally I think we want to support it. For example, > I use > two profiles with the same name but differentiate between them based on the > Avatar and Theme. > > I think preventing things like this might feel a little arbitrary as well > as > surface poorly in the UI. An example is you have a profile named "Foo" and > sign > in with a Google Account for which the profile name is also "Foo". One of > them > will have to be uniquified, which will be sync'ed to all your other > machines > (which don't necessarily have 2 profiles named "Foo"). > > https://codereview.chromium.org/1540543002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
PS: Please let me know if you need you need my eyes for a specific section, otherwise I don't plan to take a very close look. Le lun. 11 janv. 2016 15:53, Gabriel Charette <gab@google.com> a écrit : > Ok sgtm then, I don't feel strongly just wanted to make sure we agreed on > the premise of the CL before reviewing it. > > Le lun. 11 janv. 2016 14:19, <anthonyvd@chromium.org> a écrit : > >> On 2016/01/11 18:21:15, gab wrote: >> > Driving-by. >> >> > Do we even want to support profiles with the same name? That just sounds >> > confusing... Maybe we should simply disallow profiles with the same name >> > (i.e. >> > uniquify profile names on creation for accounts with the same name). >> >> > I know for myself I have multiple accounts which default to the same >> name >> (mine) >> > on first sign-in but I've long ago manually fixed them up to have >> > explicitly >> > different names or it's just confusing. >> >> That decision was taken before my time but looking at bugs about this, >> such >> as >> https://code.google.com/p/chromium/issues/detail?id=532816 it seems like >> it >> was >> a conscious choice. Personally I think we want to support it. For example, >> I use >> two profiles with the same name but differentiate between them based on >> the >> Avatar and Theme. >> >> I think preventing things like this might feel a little arbitrary as well >> as >> surface poorly in the UI. An example is you have a profile named "Foo" and >> sign >> in with a Google Account for which the profile name is also "Foo". One of >> them >> will have to be uniquified, which will be sync'ed to all your other >> machines >> (which don't necessarily have 2 profiles named "Foo"). >> >> https://codereview.chromium.org/1540543002/ >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Yes, profiles' names are synced. Thus it's difficult to control uniqueness. But there is another problem (and I told about it in crbug.com/572593): what if there is already smb`s (not Chrome) desktop shortcut or just text file named "User - Chrome.lnk"? This CL fixes it too.
The CQ bit was checked by mkolom@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1540543002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1540543002/20001
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.
The CQ bit was unchecked by mkolom@yandex-team.ru
https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:296: // Chrome desktop shortcuts with empty command lines will also be included. This comment doesn't seem to be applicable to the struct it's above. Please fix. https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:329: std::set<base::FilePath> res; Nit: result https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:362: // FILE thread. Document the new params. https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:724: for (int uniquifier = 1; excludes_names.count(name) > 0; ++uniquifier) Nit: {}'s https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:725: name = base::FilePath(base_name) Move the FilePath outside the loop.
PTAL https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:296: // Chrome desktop shortcuts with empty command lines will also be included. On 2016/01/15 18:01:38, asvitkine (OOO until Jan18) wrote: > This comment doesn't seem to be applicable to the struct it's above. Please fix. I've looked through the code and it seems that comment is ok. Examples: docked_window_layout_manager.cc (CompareWindowPos), utf_offset_string_conversions.h (LimitOffset)... ChromeCommandLineFilter is a typical functor/predicate and it is difficult to make separate comments for constructor, operator() and struct itself. https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:329: std::set<base::FilePath> res; On 2016/01/15 18:01:38, asvitkine (OOO until Jan18) wrote: > Nit: result Done. https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:362: // FILE thread. On 2016/01/15 18:01:38, asvitkine (OOO until Jan18) wrote: > Document the new params. Done. https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:724: for (int uniquifier = 1; excludes_names.count(name) > 0; ++uniquifier) On 2016/01/15 18:01:38, asvitkine (OOO until Jan18) wrote: > Nit: {}'s Done. https://codereview.chromium.org/1540543002/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:725: name = base::FilePath(base_name) On 2016/01/15 18:01:38, asvitkine (OOO until Jan18) wrote: > Move the FilePath outside the loop. Acknowledged.
lgtm % comments Thanks! https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.cc (right): https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:364: // that profile (there can be several shortcuts for profile). Nit: "are Chrome desktop shortcuts for the profile (there can be several)." https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:396: // From all profile_shortcuts choose only with known (canonical) name. Nit: "choose the one with a known (canonical) name." https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:650: const std::set<base::FilePath> shortcuts = ListUserDesktopContents(&filter); Nit: combine this with the next line, no need for a local var https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.cc:735: .value(); Nit: const auto suffix = base::StringPrintf(" (%d)", uniquifier); name = base_path.InsertBeforeExtensionASCII(suffix).value(); https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... File chrome/browser/profiles/profile_shortcut_manager_win.h (right): https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.h:30: // The same as GetShortcutFilenameForProfile but uniqueness is guaranteed. Document |excludes| please. https://codereview.chromium.org/1540543002/diff/40001/chrome/browser/profiles... chrome/browser/profiles/profile_shortcut_manager_win.h:33: BrowserDistribution* distribution, Nit: Non-const params should be last.
PTAL Fixed all review notes.
The CQ bit was checked by mkolom@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1540543002/#ps60001 (title: "Fix review notes #3")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1540543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1540543002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
anthonyvd, PTAL. (There should be LGTM from OWNER to satisfy chromium_presubmit)
Sorry for the delay. lgtm
The CQ bit was checked by mkolom@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1540543002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1540543002/60001
Message was sent while issue was closed.
Description was changed from ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG=572593 ========== to ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG=572593 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG=572593 ========== to ========== Uniquify profile shortcut name. Profile desktop shortcut can have a name that conflicts with existing shortcuts. This commit tries to fix it by uniquifying shortcut name: by appending "(2)", "(3)" ... suffix if there is a conflict. If there are two or more profiles with identical logical names they are all able to have desktop shortcuts at the same time now. R=erg@chromium.org BUG=572593 Committed: https://crrev.com/915cc7306b0899ed017f7656dd82de8a4e4184ec Cr-Commit-Position: refs/heads/master@{#370625} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/915cc7306b0899ed017f7656dd82de8a4e4184ec Cr-Commit-Position: refs/heads/master@{#370625}
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
Looks like this broke tests on the official bots, filed https://code.google.com/p/chromium/issues/detail?id=580073
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1614943004/ by asvitkine@chromium.org. The reason for reverting is: Reverting since it broke tests on official bots: https://code.google.com/p/chromium/issues/detail?id=580073. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
