Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1631373003:
Refactor ProfileInfoCache in c/b/ui/app_list (Closed)
DescriptionRefactor ProfileInfoCache in c/b/ui/app_list
ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See
https://codereview.chromium.org/1599013002/ for details.
This CL adds an comparator for sorting ProfileAttributesEntry, and also
converts the calls in c/b/ui/app_list to use the new interface.
BUG=305720
Committed: https://crrev.com/b7904c220a3c17b729628c8ca9e97b35c6233d37
Cr-Commit-Position: refs/heads/master@{#374726}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Respond to comments, add sorting of ProfileAttributesEntry #
Total comments: 20
Patch Set 3 : Changed sorting API, respond to comments. #Patch Set 4 : Change to std::cref #
Total comments: 2
Patch Set 5 : Fix errors #
Total comments: 7
Patch Set 6 : Rebased, moved sorting code to GetAllProfilesAttributesSortedByName #
Total comments: 3
Patch Set 7 : Rebase, respond to comments #Messages
Total messages: 37 (13 generated)
Description was changed from ========== Fix bugs Refactor ProfileInfoCache in c/b/ui/app_list BUG=305720 ========== to ========== Refactor ProfileInfoCache in c/b/ui/app_list BUG=305720 ==========
lwchkg@gmail.com changed reviewers: + anthonyvd@chromium.org, mlerman@chromium.org
Dear Anthony and Mike, PTAL and comment. I've marked a few places that need special attention. Regards, WC Leung https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_attributes_storage.h (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_attributes_storage.h:15: #include "chrome/browser/profiles/profile_attributes_storage_observer.h" I looking into forward declaring this class, but currently it's impossible with the "class" being a typedef. It will be possible in a later stage of the refactoring. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_attributes_storage_observer.h (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_attributes_storage_observer.h:10: using ProfileAttributesStorageObserver = ProfileInfoCacheObserver; Name of new class: please comment as usual. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/app_list_view_delegate.cc:112: std::vector<ProfileAttributesEntry*> entries = g_browser_process-> const possible. Will update in the next patch. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/app_list_view_delegate.cc:114: GetAllProfilesAttributes(); Sorting is needed. (Anthony, I need something like your controversial LessThan.) Anyway, I think that all calls to GetAllProfilesAttributes() needs the items to be sorted in the same way, so it should be a good idea to do the sorting inside that function. (If we somehow need the entries unsorted, we can also make another function to get the entries that way.) About the sorting algorithm: the current algorithm in ProfileInfoCache is incorrect. Sorting of names should be locale aware. (See https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar... for an example.) If profile names are equal, then profile paths are best compared by length of string and then string contents, so we have "Profile 11" > "Profile 10" > "Profile 9", etc. WDYT?
Great work, thanks! Maybe explain a little more in the CL description why we're making this change. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_attributes_storage_observer.h (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_attributes_storage_observer.h:10: using ProfileAttributesStorageObserver = ProfileInfoCacheObserver; On 2016/01/27 22:37:19, lwchkg wrote: > Name of new class: please comment as usual. I would prefer not adding this file, include profile_info_cache_observer.h in profile_attributes_storage.h, and do using Observer = ProfileInfoCacheObserver; inside the ProfileAttributesStorage class declaration. Then when the PIC is removed, we can also fix the declaration of course. Most Observers are implemented as nested classes in the codebase at the moment, and it's almost never necessary to use the Observer without the Observee. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/app_list_view_delegate.cc:114: GetAllProfilesAttributes(); On 2016/01/27 22:37:19, lwchkg wrote: > Sorting is needed. (Anthony, I need something like your controversial LessThan.) > > Anyway, I think that all calls to GetAllProfilesAttributes() needs the items to > be sorted in the same way, so it should be a good idea to do the sorting inside > that function. (If we somehow need the entries unsorted, we can also make > another function to get the entries that way.) I think you're right that all the places that need them sorted do need them sorted the same way, although it's not impossible that some caller doesn't care about the order. Part of the goals of the refactor is to 1) remove the sorting cost when it's not needed and 2) get out of the situation where callers have this implicit contract with the PIC that the order is a certain way. Because of this, I wouldn't sort in GetAllProfileAttributes(). I could see a case being made for adding a GetAllProfileAttributesSorted() or something if the proper ordering logic is complex but I'm still not sold on that. > > About the sorting algorithm: the current algorithm in ProfileInfoCache is > incorrect. Sorting of names should be locale aware. > > (See > https://code.google.com/p/chromium/codesearch#chromium/src/components/bookmar... > for an example.) Interesting, I don't know if we have a common comparator (ie: not in bookmarks) that does something like this that we could use. > > If profile names are equal, then profile paths are best compared by length of > string and then string contents, so we have "Profile 11" > "Profile 10" > > "Profile 9", etc. Most places I've seen only care about name order since it's what's displayed so that sounds good but it could be more nuanced in some parts of the code. > > WDYT?
Just some replies. I'll make a patch tomorrow. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... File chrome/browser/profiles/profile_attributes_storage_observer.h (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/profiles/pro... chrome/browser/profiles/profile_attributes_storage_observer.h:10: using ProfileAttributesStorageObserver = ProfileInfoCacheObserver; On 2016/01/29 15:16:34, anthonyvd wrote: > On 2016/01/27 22:37:19, lwchkg wrote: > > Name of new class: please comment as usual. > > I would prefer not adding this file, include profile_info_cache_observer.h in > profile_attributes_storage.h, and do using Observer = ProfileInfoCacheObserver; > inside the ProfileAttributesStorage class declaration. Then when the PIC is > removed, we can also fix the declaration of course. > > Most Observers are implemented as nested classes in the codebase at the moment, > and it's almost never necessary to use the Observer without the Observee. Acknowledged. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/app_list_view_delegate.cc:114: GetAllProfilesAttributes(); On 2016/01/29 15:16:34, anthonyvd wrote: > I think you're right that all the places that need them sorted do need them > sorted the same way, although it's not impossible that some caller doesn't care > about the order. Part of the goals of the refactor is to 1) remove the sorting > cost when it's not needed and 2) get out of the situation where callers have > this implicit contract with the PIC that the order is a certain way. Because of > this, I wouldn't sort in GetAllProfileAttributes(). > > I could see a case being made for adding a GetAllProfileAttributesSorted() or > something if the proper ordering logic is complex but I'm still not sold on > that. > I see. Now I do see why you make a LessThan in your CL. I'll make a similar function. Anyway, sorting strings in non-English languages is indeed very complex. In Chromium our best bet is to sort the string using ICU functions. We need to hope that ICU collation is already set up properly in Chromium. (I have doubts in it though.)
Description was changed from ========== Refactor ProfileInfoCache in c/b/ui/app_list BUG=305720 ========== to ========== Refactor ProfileInfoCache in c/b/ui/app_list ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. This CL adds an comparator for sorting ProfileAttributesEntry, and also converts the calls in c/b/ui/app_list to use the new interface. BUG=305720 ==========
New patch updated. If things looks good I'll add the unit test for sorting, and also ask the owners of c/b/ui/app_list for review. https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/1631373003/diff/1/chrome/browser/ui/app_list/... chrome/browser/ui/app_list/app_list_view_delegate.cc:114: GetAllProfilesAttributes(); On 2016/01/29 15:16:34, anthonyvd wrote: > Interesting, I don't know if we have a common comparator (ie: not in bookmarks) > that does something like this that we could use. I don't know of one here. Anyway, each comparator looks a little bit different (the strings are compared the same way, but the secondary criteria are different). > > Most places I've seen only care about name order since it's what's displayed so > that sounds good but it could be more nuanced in some parts of the code. > Not sure, and we gotta be careful in every callsite that use GetAllProfileAttributes (maybe we should ask explicitly if you're not the owner).
https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_view_delegate.cc:120: for (const auto& entry : entries) { Should remove the & because the underlying data are pointers. Will be done in the next patch.
tapted@chromium.org changed reviewers: + tapted@chromium.org
(drive-by, but I think you'll need an app_list OWNER anyway :). I actually just started when I noticed the IsSigninRequired thing, but decided to do a pass over all the files) https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:24: return false; // If collator is absent sorting fails with undefined result. can this actually happen? I think this should DCHECK. A lot of sorting algorithms will actually crash if the comparator doesn't provide "strict weak" ordering, so better to crash sooner. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:38: const icu::Collator* const collator_; The ownership in this class is not right. You shouldn't instantiate a class with scoped_ptr::get() -- it makes the API very easy to abuse. I think the SortComparator() constructor shouldn't take an argument and |collator_| should be scoped_ptr. (GetCollator() isn't needed). If you need more flexibility in future, the SortComparator constructor should take a scoped_ptr and use std::move or scoped_ptr::swap (previously we would use scoped_ptr::Pass() but std::move is the new vogue). https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_service_mac.mm (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_service_mac.mm:396: if (has_entry && entry->IsSigninRequired() && ReadyToShow()) drive-by: I think you're missing a `!` before entry->IsSigninRequired() https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_view_delegate.cc:9: #include <algorithm> but you should keep std::vector here (as well as algorithm) since <vector> is used to implement methods as well as implement method override signatures https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_view_delegate.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_view_delegate.h:11: #include <vector> nit: also not required - see below https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/profile_store.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/profile_store.h:8: #include <string> nit: not needed https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/test/fake_profile_store.cc (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/test/fake_profile_store.cc:7: #include <string> nit: and this is only needed to implement the header, so also not needed. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/test/fake_profile_store.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/test/fake_profile_store.h:9: #include <string> nit: I think typically we say, if you need a type only for method overrides, no need to IWYU for it. So this can be removed.
tapted: Thanks for review! (You're in the CC list right at the start, so it doesn't count as a drive-by!) https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:24: return false; // If collator is absent sorting fails with undefined result. On 2016/01/31 23:04:37, tapted wrote: > can this actually happen? I think this should DCHECK. A lot of sorting > algorithms will actually crash if the comparator doesn't provide "strict weak" > ordering, so better to crash sooner. The constructor has already DCHECKed. This is only to prevent release version from crashing. Anyway, thanks for the reminder of strict weak ordering. So I'll provide a backup implementation (e.g. a default collation) that guarantees to finish sorting. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:38: const icu::Collator* const collator_; On 2016/01/31 23:04:37, tapted wrote: > The ownership in this class is not right. You shouldn't instantiate a class with > scoped_ptr::get() -- it makes the API very easy to abuse. > > I think the SortComparator() constructor shouldn't take an argument and > |collator_| should be scoped_ptr. (GetCollator() isn't needed). > > If you need more flexibility in future, the SortComparator constructor should > take a scoped_ptr and use std::move or scoped_ptr::swap (previously we would use > scoped_ptr::Pass() but std::move is the new vogue). Before coming to this version, actually I tried what you say and the compiler failed to compile, due to copy constructing in std::sort with scoped_ptr. BTW, I'll try again by enforcing Pred = SortComparator& instead of Pred = SortComparator in std::sort. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_service_mac.mm (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_service_mac.mm:396: if (has_entry && entry->IsSigninRequired() && ReadyToShow()) On 2016/01/31 23:04:37, tapted wrote: > drive-by: I think you're missing a `!` before entry->IsSigninRequired() Thanks for spotting! I don't have a Mac myself, so it's very easy to make mistakes! Anyway I have a question: do you think that this type of error will be caught in tests? https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/test/fake_profile_store.cc (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/test/fake_profile_store.cc:7: #include <string> On 2016/01/31 23:04:37, tapted wrote: > nit: and this is only needed to implement the header, so also not needed. Acknowledged. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/test/fake_profile_store.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/test/fake_profile_store.h:9: #include <string> On 2016/01/31 23:04:37, tapted wrote: > nit: I think typically we say, if you need a type only for method overrides, no > need to IWYU for it. So this can be removed. Thanks. Your point is really good! Anyway I added this because cpplint has thrown a warning. Maybe I should start a thread in chromium-dev and ask for a rule change in cpplint.
https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:24: return false; // If collator is absent sorting fails with undefined result. On 2016/02/01 06:31:20, lwchkg wrote: > On 2016/01/31 23:04:37, tapted wrote: > > can this actually happen? I think this should DCHECK. A lot of sorting > > algorithms will actually crash if the comparator doesn't provide "strict weak" > > ordering, so better to crash sooner. > > The constructor has already DCHECKed. This is only to prevent release version > from crashing. Anyway, thanks for the reminder of strict weak ordering. So I'll > provide a backup implementation (e.g. a default collation) that guarantees to > finish sorting. That's not normally what we use DCHECK for. DCHECKs should never happen, and you should never write code that considers that they could fail. See https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... If there's a real possibility that Collator::createInstance could fail, it should be handled and tested in debug builds. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:17: #include "third_party/icu/source/i18n/unicode/coll.h" this doesn't compile for me on mac: unicode/utypes.h' file not found - not sure why https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:38: const icu::Collator* const collator_; On 2016/02/01 06:31:20, lwchkg wrote: > On 2016/01/31 23:04:37, tapted wrote: > > The ownership in this class is not right. You shouldn't instantiate a class > with > > scoped_ptr::get() -- it makes the API very easy to abuse. > > > > I think the SortComparator() constructor shouldn't take an argument and > > |collator_| should be scoped_ptr. (GetCollator() isn't needed). > > > > If you need more flexibility in future, the SortComparator constructor should > > take a scoped_ptr and use std::move or scoped_ptr::swap (previously we would > use > > scoped_ptr::Pass() but std::move is the new vogue). > > Before coming to this version, actually I tried what you say and the compiler > failed to compile, due to copy constructing in std::sort with scoped_ptr. BTW, > I'll try again by enforcing Pred = SortComparator& instead of Pred = > SortComparator in std::sort. I think you just need to add a move constructor to SortComparator, and std::sort will accept it via std::move. e.g. something like class SortComparator { public: SortComparator() SortComparator(SortComparator&&) = default; bool operator()(... ... private: scoped_ptr<icu::Collator*> collator_; DISALLOW_COPY_AND_ASSIGN(..) }; Failing that, I'd suggest a simplified version of SortVectorWithStringKey from ui/base/l10n/l10n_util_collator.h so that instances of the comparator don't escape the API. (perhaps in app_list_view_delegate.cc unless you think something else might need it). Sadly, I don't think you can use SortVectorWithStringKey directly since the vector contains pointers, not values. You could also change SortVectorWithStringKey to replace the calls like `lhs.GetStringKey();` to GetStringKey(lhs), then provide a template specialization for ProfileAttributesEntry* that concatenates the name and path. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_service_mac.mm (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_service_mac.mm:396: if (has_entry && entry->IsSigninRequired() && ReadyToShow()) On 2016/02/01 06:31:20, lwchkg wrote: > On 2016/01/31 23:04:37, tapted wrote: > > drive-by: I think you're missing a `!` before entry->IsSigninRequired() > > Thanks for spotting! I don't have a Mac myself, so it's very easy to make > mistakes! > > Anyway I have a question: do you think that this type of error will be caught in > tests? Not sure - this path is used to improve response times, rather than change functional behaviour, so it's likely not tested.
Spent more than one day trying solutions and... failed a lot. And finally got two working solutions (patches #3 and #4). Anyway, can you help me to start a CQ dry run? Thanks! Regards, WC Leung. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:17: #include "third_party/icu/source/i18n/unicode/coll.h" On 2016/02/02 00:20:55, tapted wrote: > this doesn't compile for me on mac: unicode/utypes.h' file not found - not sure > why Oh. Can you start the try bot about Mac for me? It's easier to troubleshoot with the log. (Anyway, missing include path? If yes we probably need to shout for help in chromium-dev.) Anthony: looks like I need to apply for try bot access. https://codereview.chromium.org/1631373003/diff/60001/chrome/browser/ui/app_l... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/1631373003/diff/60001/chrome/browser/ui/app_l... chrome/browser/ui/app_list/app_list_view_delegate.cc:120: std::sort(entries.begin(), entries.end(), std::cref(comp)); Tried a few methods for one day... most failing except for the one uploaded here: (1) making a default move constructor... VC++ rejected by refusing to compile. I don't know why it fails. Anyway... why a move constructor? the scoped_ptr needs to be moved back in the inner function calls! (2) enforcing std::sort<..., ..., ...&>. The outer call is enforced, but std::sort loses the & when passing ...& to inner calls. I've checked that VC++ and GCC does the same thing. (3) std::bind : it copy constructs the class. The correct solution is very simple... wrap the comparator (needs to be a lvalue) in a reference_wrapper, i.e. std::cref(comparator). I'm not sure whether the use of reference wrappers is allowed in Chromium though. The Google C++ style and Chromium style says differently. Anyway, if not we need a hacky implementation in patch #3, which is inefficient and also smells. Maybe we can use the two patches for discussion in chromium-dev.
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/1631373003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631373003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Finally fixed the build errors. Can you please start another CQ dry run for me? Thanks. https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/20001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:17: #include "third_party/icu/source/i18n/unicode/coll.h" On 2016/02/03 17:18:38, lwchkg wrote: > On 2016/02/02 00:20:55, tapted wrote: > > this doesn't compile for me on mac: unicode/utypes.h' file not found - not > sure > > why > > Oh. Can you start the try bot about Mac for me? It's easier to troubleshoot with > the log. (Anyway, missing include path? If yes we probably need to shout for > help in chromium-dev.) > > Anthony: looks like I need to apply for try bot access. Find the cause of the error. There's a new dependency in test_support_ui when I include a file from icu.
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/1631373003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631373003/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
looks like you need to rebase as well https://codereview.chromium.org/1631373003/diff/60001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.cc (right): https://codereview.chromium.org/1631373003/diff/60001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.cc:15: DCHECK(U_SUCCESS(error_code)); After looking at SortVectorWithStringKey from ui/base/l10n/l10n_util_collator.h I think you really do need to handle this case. The strategy in SortVectorWithStringKey is to collator_.reset() on failure here and do an ASCII string comparison in operator()(..) if the collator is null. https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:17: #include "third_party/icu/source/common/unicode/uversion.h" Can this be moved to the.cc? That might remove the need for the .gyp changes. Doesn't seem right to force the test support library to depend on this, since the file that `owns` the dependency isn't compiled as part of test support https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:22: namespace U_ICU_NAMESPACE { does `icu` work in place of U_ICU_NAMESPACE ? That should allow the uversion.h include to be moved, and it's weird that you would need U_ICU_NAMESPACE here and not when declaring the scoped_ptr member. https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:40: }; nit: DISALLOW_COPY_AND_ASSIGN(..)
Rebased and compiling (will take a few hours). Maybe I should buy a SSD ASAP. https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:17: #include "third_party/icu/source/common/unicode/uversion.h" On 2016/02/04 23:10:35, tapted wrote: > Can this be moved to the.cc? That might remove the need for the .gyp changes. > Doesn't seem right to force the test support library to depend on this, since > the file that `owns` the dependency isn't compiled as part of test support I guess it simply means that I did not add a proper test for SortComparator. That comparator is complex and deserves some testing. Anthony: If we provide ProfileAttributesStorage::SortEntries(std::vector<...>) instead of ProfileAttributesEntry::SortComparator, then we don't need this file here (and the gyp changes). I guess you still want sorting implemented in the current way. Anyway, since you're the owner I'll wait for your decision. https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:22: namespace U_ICU_NAMESPACE { On 2016/02/04 23:10:36, tapted wrote: > does `icu` work in place of U_ICU_NAMESPACE ? That should allow the uversion.h > include to be moved, and it's weird that you would need U_ICU_NAMESPACE here and > not when declaring the scoped_ptr member. No. The namespace icu is alias to U_ICU_NAMESPACE (which currently resolves to icu54, but it may be changed when we upgrade icu). Sadly U_ICU_NAMESPACE is defined in uversions.h. And those gyp/gn changes is still needed. The files included by uversions.h should be only a small subset of "unicode/coll.h". https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:40: }; On 2016/02/04 23:10:36, tapted wrote: > nit: DISALLOW_COPY_AND_ASSIGN(..) Thanks. Forgot to add it back.
https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... File chrome/browser/profiles/profile_attributes_entry.h (right): https://codereview.chromium.org/1631373003/diff/80001/chrome/browser/profiles... chrome/browser/profiles/profile_attributes_entry.h:17: #include "third_party/icu/source/common/unicode/uversion.h" On 2016/02/05 00:56:22, lwchkg wrote: > On 2016/02/04 23:10:35, tapted wrote: > > Can this be moved to the.cc? That might remove the need for the .gyp changes. > > Doesn't seem right to force the test support library to depend on this, since > > the file that `owns` the dependency isn't compiled as part of test support > > I guess it simply means that I did not add a proper test for SortComparator. > That comparator is complex and deserves some testing. > > Anthony: If we provide ProfileAttributesStorage::SortEntries(std::vector<...>) > instead of ProfileAttributesEntry::SortComparator, then we don't need this file > here (and the gyp changes). I guess you still want sorting implemented in the > current way. Anyway, since you're the owner I'll wait for your decision. I'm fine with having a function in ProfileAttributesStorage to get the entries as a sorted vector. Something like GetAllProfileAttributesSortedByName or similar. It now looks like it's the better thing to do.
Patchset #6 (id:100001) has been deleted
Dear all, New patch uploaded. PTAL. (As usual, can you help by starting a CQ dry run?) Regards, WC Leung.
c/b/profiles/* lgtm % small nit https://codereview.chromium.org/1631373003/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1631373003/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.cc:189: if (a_path.length() != b_path.length()) I think this check isn't needed. base::FilePath::StringType is a string type (std::string/std::wstring) and their string comparison functions already handle the different length cases that way. For example, std::string::compare (called by std::string::operator<) considers a string smaller if "Either the value of the first character that does not match is lower in the compared string, or all compared characters match but the compared string is shorter."
c/b/ui/app_list/* lgtm https://codereview.chromium.org/1631373003/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1631373003/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.cc:189: if (a_path.length() != b_path.length()) On 2016/02/08 21:58:42, anthonyvd wrote: > I think this check isn't needed. base::FilePath::StringType is a string type > (std::string/std::wstring) and their string comparison functions already handle > the different length cases that way. For example, std::string::compare (called > by std::string::operator<) considers a string smaller if "Either the value of > the first character that does not match is lower in the compared string, or all > compared characters match but the compared string is shorter." I read this as solving the issue that for profiles named "Default", "Profile 1", "Profile 2", ...., "Profile 10", then "Profile 10" would sort before "Profile 2". With this extra check, "Profile 10" will sort after "Profile 9" since the string is longer. But if someone has 11 profiles that they've all given the same name, they are maybe an extreme example and can deal with it themselves.
The CQ bit was checked by lwchkg@gmail.com
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, anthonyvd@chromium.org Link to the patchset: https://codereview.chromium.org/1631373003/#ps140001 (title: "Rebase, respond to comments")
https://codereview.chromium.org/1631373003/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_info_cache.cc (right): https://codereview.chromium.org/1631373003/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_info_cache.cc:189: if (a_path.length() != b_path.length()) On 2016/02/08 22:33:36, tapted wrote: > On 2016/02/08 21:58:42, anthonyvd wrote: > > I think this check isn't needed. base::FilePath::StringType is a string type > > (std::string/std::wstring) and their string comparison functions already > handle > > the different length cases that way. For example, std::string::compare (called > > by std::string::operator<) considers a string smaller if "Either the value of > > the first character that does not match is lower in the compared string, or > all > > compared characters match but the compared string is shorter." > > I read this as solving the issue that for profiles named "Default", "Profile 1", > "Profile 2", ...., "Profile 10", then "Profile 10" would sort before "Profile > 2". With this extra check, "Profile 10" will sort after "Profile 9" since the > string is longer. > > But if someone has 11 profiles that they've all given the same name, they are > maybe an extreme example and can deal with it themselves. You're right. If the profile names are the same, the user deliberately confuses him/herself, and can probably deal with any sorting results. So I'm removing the length check before commit.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1631373003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1631373003/140001
Message was sent while issue was closed.
Description was changed from ========== Refactor ProfileInfoCache in c/b/ui/app_list ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. This CL adds an comparator for sorting ProfileAttributesEntry, and also converts the calls in c/b/ui/app_list to use the new interface. BUG=305720 ========== to ========== Refactor ProfileInfoCache in c/b/ui/app_list ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. This CL adds an comparator for sorting ProfileAttributesEntry, and also converts the calls in c/b/ui/app_list to use the new interface. BUG=305720 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Refactor ProfileInfoCache in c/b/ui/app_list ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. This CL adds an comparator for sorting ProfileAttributesEntry, and also converts the calls in c/b/ui/app_list to use the new interface. BUG=305720 ========== to ========== Refactor ProfileInfoCache in c/b/ui/app_list ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical indices in the interface. See https://codereview.chromium.org/1599013002/ for details. This CL adds an comparator for sorting ProfileAttributesEntry, and also converts the calls in c/b/ui/app_list to use the new interface. BUG=305720 Committed: https://crrev.com/b7904c220a3c17b729628c8ca9e97b35c6233d37 Cr-Commit-Position: refs/heads/master@{#374726} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/b7904c220a3c17b729628c8ca9e97b35c6233d37 Cr-Commit-Position: refs/heads/master@{#374726} |
