Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2352353002:
Add AppLauncherId wrapper for items shown in shelf (Closed)
|
Created:
4 years, 3 months ago by Andra Paraschiv Modified:
4 years, 2 months ago CC:
chromium-reviews, kalyank, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd AppLauncherId wrapper for items shown in shelf
Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a
unique chrome launcher id for a shelf item.
The "AppLauncherId(const std::string& app_id, const std::string& launch_id)"
constructor is currently unused, but will be used in a follow-up CL.
Based on https://codereview.chromium.org/2290603002
Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>;
BUG=610299
TEST=LauncherPlatformAppBrowserTest
Committed: https://crrev.com/f9a980f3718e5d262ee429032fb7083caa9ba343
Cr-Commit-Position: refs/heads/master@{#426145}
Patch Set 1 #Patch Set 2 : Include AppLauncherId in prefs code v1 #Patch Set 3 : Include AppLauncherId in prefs code v2 #
Total comments: 11
Patch Set 4 : Review v2 #Patch Set 5 : Rebase #
Total comments: 8
Patch Set 6 : Review v3 #
Total comments: 12
Patch Set 7 : Review v4 #
Total comments: 16
Patch Set 8 : Review v5 #
Total comments: 4
Patch Set 9 : Review v6 #
Total comments: 2
Patch Set 10 : Review v7 #Patch Set 11 : Rebase #
Total comments: 13
Patch Set 12 : Review v8 #Patch Set 13 : Rebase #Patch Set 14 : Rebase Fixes #Patch Set 15 : Review v9 #
Total comments: 2
Patch Set 16 : Review v10 #
Messages
Total messages: 42 (12 generated)
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper that includes a unique chrome launcher id for a shelf item. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610229 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper that includes a unique chrome launcher id for a shelf item. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610229 TEST=LauncherPlatformAppBrowserTest ==========
andra.paraschiv@intel.com changed reviewers: + jamescook@chromium.org, reillyg@chromium.org, skuhne@chromium.org, stevenjb@chromium.org
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper that includes a unique chrome launcher id for a shelf item. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610229 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610229 TEST=LauncherPlatformAppBrowserTest ==========
I only reviewed some of this. I think this CL will make more sense and be easier to review if we include the 'launch_id' in AppLauncherId, even though we won't use the 'launch_id' parameter anywhere (even though I suggested differently earlier). Go ahead and include the two argument constructor, but add a comment that it is currently unused but will be in a follow-up CL. https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:46: apps->Append(CreateAppDict(app_launcher_id)); This is leaky! Just do: apps->Append(CreateAppDict(AppLauncherId(kDefaultPinnedApps[i]))) (And use const& in CreateAppDict). https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:216: std::string app_launcher_id; This should be an AppLauncherId. Only use the string version when necessary, e.g. when storing the pref. https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.h:60: const std::string& GetAppLauncherID() const { return app_launcher_id_; } This is confusing, it looks like it should return an AppLauncherId, i.e. just 'this'. Instead name this GetAsString(). https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.h:64: } Since neither of these methods are simple getters or accessors, and will become less trivial in the follow-up CL, lets go ahead and implement them in the .cc file with this CL. https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.h:76: base::DictionaryValue* CreateAppDict(AppLauncherId* app_launcher_id); Generally prefer const AppLauncherId& for input parameters throughout.
Thank you, Steven, for review. I added the constructor that includes the launch_id and uploaded the CL changes according to the feedback. https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:46: apps->Append(CreateAppDict(app_launcher_id)); On 2016/09/21 16:06:40, stevenjb wrote: > This is leaky! > > Just do: > apps->Append(CreateAppDict(AppLauncherId(kDefaultPinnedApps[i]))) > > (And use const& in CreateAppDict). Done. https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:216: std::string app_launcher_id; On 2016/09/21 16:06:40, stevenjb wrote: > This should be an AppLauncherId. Only use the string version when necessary, > e.g. when storing the pref. If we change it to AppLauncherId, we should add an assignment operator overload function for AppLauncherId class and make app_launcher_id string attribute (from the AppLauncherId class) non-const. Should we include these changes too? https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.h:60: const std::string& GetAppLauncherID() const { return app_launcher_id_; } On 2016/09/21 16:06:40, stevenjb wrote: > This is confusing, it looks like it should return an AppLauncherId, i.e. just > 'this'. Instead name this GetAsString(). Done. https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.h:64: } On 2016/09/21 16:06:40, stevenjb wrote: > Since neither of these methods are simple getters or accessors, and will become > less trivial in the follow-up CL, lets go ahead and implement them in the .cc > file with this CL. Done. https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.h:76: base::DictionaryValue* CreateAppDict(AppLauncherId* app_launcher_id); On 2016/09/21 16:06:40, stevenjb wrote: > Generally prefer const AppLauncherId& for input parameters throughout. Done.
On 2016/09/22 09:23:41, Andra Paraschiv wrote: > Thank you, Steven, for review. > > I added the constructor that includes the launch_id and uploaded the CL changes > according to the feedback. > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:46: > apps->Append(CreateAppDict(app_launcher_id)); > On 2016/09/21 16:06:40, stevenjb wrote: > > This is leaky! > > > > Just do: > > apps->Append(CreateAppDict(AppLauncherId(kDefaultPinnedApps[i]))) > > > > (And use const& in CreateAppDict). > > Done. > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:216: std::string app_launcher_id; > On 2016/09/21 16:06:40, stevenjb wrote: > > This should be an AppLauncherId. Only use the string version when necessary, > > e.g. when storing the pref. > > If we change it to AppLauncherId, we should add an assignment operator overload > function for AppLauncherId class and make app_launcher_id string attribute (from > the AppLauncherId class) non-const. Should we include these changes too? > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.h:60: const std::string& > GetAppLauncherID() const { return app_launcher_id_; } > On 2016/09/21 16:06:40, stevenjb wrote: > > This is confusing, it looks like it should return an AppLauncherId, i.e. just > > 'this'. Instead name this GetAsString(). > > Done. > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.h:64: } > On 2016/09/21 16:06:40, stevenjb wrote: > > Since neither of these methods are simple getters or accessors, and will > become > > less trivial in the follow-up CL, lets go ahead and implement them in the .cc > > file with this CL. > > Done. > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.h:76: base::DictionaryValue* > CreateAppDict(AppLauncherId* app_launcher_id); > On 2016/09/21 16:06:40, stevenjb wrote: > > Generally prefer const AppLauncherId& for input parameters throughout. > > Done. I added a rebase patch set that also includes a few changes in the codebase that is covered by this CL e.g. SetPinPosition has a 4th vector parameter instead of a single object. Thank you!
https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:216: std::string app_launcher_id; On 2016/09/22 09:23:41, Andra Paraschiv wrote: > On 2016/09/21 16:06:40, stevenjb wrote: > > This should be an AppLauncherId. Only use the string version when necessary, > > e.g. when storing the pref. > > If we change it to AppLauncherId, we should add an assignment operator overload > function for AppLauncherId class and make app_launcher_id string attribute (from > the AppLauncherId class) non-const. Should we include these changes too? I'm not exactly sure what you are asking, but we should be able to store and copy AppLauncherId. https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:268: const std::vector<std::string>& app_list() const { return app_list_; } These should both store AppLauncherId instead of a string. The whole point of introducing AppLauncherId is to make it clear in places like this that we are storing a "launcher id". https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:734: } These should match the order in the header (i.e before RegisterChromeLauncherUserPrefs). https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:397: profile_, ash::launcher::AppLauncherId(GetAppIDForShelfID(id))); {} around multi-line if bodies. https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1200: new ash::launcher::AppLauncherId(app_id_after))); app_launcher_ids_after.push_back(base::MakeUnique<ash::launcher::AppLauncherId>(app_id_after));
On 2016/09/27 16:42:36, stevenjb wrote: > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): > > https://codereview.chromium.org/2352353002/diff/40001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:216: std::string app_launcher_id; > On 2016/09/22 09:23:41, Andra Paraschiv wrote: > > On 2016/09/21 16:06:40, stevenjb wrote: > > > This should be an AppLauncherId. Only use the string version when necessary, > > > e.g. when storing the pref. > > > > If we change it to AppLauncherId, we should add an assignment operator > overload > > function for AppLauncherId class and make app_launcher_id string attribute > (from > > the AppLauncherId class) non-const. Should we include these changes too? > > I'm not exactly sure what you are asking, but we should be able to store and > copy AppLauncherId. > > https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... > File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): > > https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:268: const > std::vector<std::string>& app_list() const { return app_list_; } > These should both store AppLauncherId instead of a string. The whole point of > introducing AppLauncherId is to make it clear in places like this that we are > storing a "launcher id". > > https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:734: } > These should match the order in the header (i.e before > RegisterChromeLauncherUserPrefs). > > https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... > File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): > > https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:397: profile_, > ash::launcher::AppLauncherId(GetAppIDForShelfID(id))); > {} around multi-line if bodies. > > https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... > chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1200: new > ash::launcher::AppLauncherId(app_id_after))); > app_launcher_ids_after.push_back(base::MakeUnique<ash::launcher::AppLauncherId>(app_id_after)); BTW, I am OOO from this Thursday until 10/13. Once we get this CL through the rest should hopefully be reasonably straightforward and hopefully jamescooh@ and skuhne@ will be able to do the follow-up reviews.
Thank you for the notification and feedback. I added another patch set that includes storing AppLauncherId instead of string. https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:268: const std::vector<std::string>& app_list() const { return app_list_; } On 2016/09/27 16:42:36, stevenjb (OOO 9-28 - 10-12) wrote: > These should both store AppLauncherId instead of a string. The whole point of > introducing AppLauncherId is to make it clear in places like this that we are > storing a "launcher id". Done. https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_launcher_prefs.cc:734: } On 2016/09/27 16:42:36, stevenjb (OOO 9-28 - 10-12) wrote: > These should match the order in the header (i.e before > RegisterChromeLauncherUserPrefs). Done. https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:397: profile_, ash::launcher::AppLauncherId(GetAppIDForShelfID(id))); On 2016/09/27 16:42:36, stevenjb (OOO 9-28 - 10-12) wrote: > {} around multi-line if bodies. Done. https://codereview.chromium.org/2352353002/diff/80001/chrome/browser/ui/ash/l... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1200: new ash::launcher::AppLauncherId(app_id_after))); On 2016/09/27 16:42:36, stevenjb (OOO 9-28 - 10-12) wrote: > app_launcher_ids_after.push_back(base::MakeUnique<ash::launcher::AppLauncherId>(app_id_after)); Done.
I took over for stevenjb because he's on vacation. I'll ask skuhne if he wants to be the primary reviewer or if he wants me to continue. (I am not in the OWNERS file for this directory.) https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:297: return app_launcher_id_ == app_launcher_id.GetAsString(); You don't have to call the method on the other object. A more common way to write operator functions is: bool AppLauncherId::operator==(const AppLauncherId& other) const { return app_launcher_id_ == other.app_launcher_id_; } Grep the codebase for "operator==" for examples. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:605: std::vector<std::string> GetPinnedAppsFromPrefs( I think this function should return a vector<AppLauncherId> as well. If you can't do that in this CL, then there needs to be a comment in the header with a http://crbug.com/123456 reference in it to track fixing that. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:59: AppLauncherId(const std::string& app_id, const std::string& launch_id); Document here (or in the class comment above) about the meanings of |app_id|, |launch_id| and why this class has to exist. (You might be able to reuse the comments from one of the other launch-id related patches. This is just a good place to centralize the explanation.) https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:60: ~AppLauncherId(); What about copy-construction and assignment? I think you should specify those operators, perhaps with "= default" to be explicit about them. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: bool operator!=(const AppLauncherId& app_launcher_id) const; Do you really need all of these operators? I would guess that you only need operator== for comparison and operator< for storing in a set.
Also, the BUG= line in the CL description is wrong. I think you meant 610299
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610229 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ==========
On 2016/10/03 23:03:19, James Cook wrote: > Also, the BUG= line in the CL description is wrong. I think you meant 610299 Yes, thanks for this, I modified the bug number in the description.
Ok, James, thank you for the review. I updated the CL to include the codebase fixes. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:297: return app_launcher_id_ == app_launcher_id.GetAsString(); On 2016/10/03 21:14:36, James Cook wrote: > You don't have to call the method on the other object. A more common way to > write operator functions is: > > bool AppLauncherId::operator==(const AppLauncherId& other) const { > return app_launcher_id_ == other.app_launcher_id_; > } > > Grep the codebase for "operator==" for examples. Done. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:605: std::vector<std::string> GetPinnedAppsFromPrefs( On 2016/10/03 21:14:36, James Cook wrote: > I think this function should return a vector<AppLauncherId> as well. If you > can't do that in this CL, then there needs to be a comment in the header with a > http://crbug.com/123456 reference in it to track fixing that. I updated this method to return an AppLauncherId vector. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:59: AppLauncherId(const std::string& app_id, const std::string& launch_id); On 2016/10/03 21:14:36, James Cook wrote: > Document here (or in the class comment above) about the meanings of |app_id|, > |launch_id| and why this class has to exist. (You might be able to reuse the > comments from one of the other launch-id related patches. This is just a good > place to centralize the explanation.) Done. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:60: ~AppLauncherId(); On 2016/10/03 21:14:36, James Cook wrote: > What about copy-construction and assignment? I think you should specify those > operators, perhaps with "= default" to be explicit about them. Done. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: bool operator!=(const AppLauncherId& app_launcher_id) const; On 2016/10/03 21:14:36, James Cook wrote: > Do you really need all of these operators? I would guess that you only need > operator== for comparison and operator< for storing in a set. Yes, you are right, we currently need only these comparison operators. Should we not include the other ones for now?
LGTM with nits https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: bool operator!=(const AppLauncherId& app_launcher_id) const; On 2016/10/04 11:39:00, Andra Paraschiv wrote: > On 2016/10/03 21:14:36, James Cook wrote: > > Do you really need all of these operators? I would guess that you only need > > operator== for comparison and operator< for storing in a set. > > Yes, you are right, we currently need only these comparison operators. Should we > not include the other ones for now? Yes, only include the ones you need. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:60: // has the same |app_id|. Nice docs, btw. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: AppLauncherId(const AppLauncherId& app_launcher_id) = default; super nit: Put copy constructor and operator= together, after constructors and destructors: Foo() ~Foo() Foo(const Foo& foo) = default Foo& operator=(const Foo& foo) = default like base/weak_reference.h optional: You may want move-construction as well (Foo&&) https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_mash_shelf_controller.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_mash_shelf_controller.cc:115: item->app_id = app_launcher_id.GetAsString(); nit: Given that you use app_launcher_id.GetAsString() 7 times, I would store the result in a local variable (like |app_id| or something) and use that. It'll make the line wrapping better.
msw@chromium.org changed reviewers: + msw@chromium.org
https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:292: : app_launcher_id_(app_id + launch_id) {} Why concatenate these internally? Then there's no way to reconstitute the |app_id| and |launcher_id| elements from an AppLauncherId object. It would be more useful if this single id could replace various uses of the other two, but losing the constituent id information means that we'll need to keep using the other ids in more places. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:296: bool AppLauncherId::operator==(const AppLauncherId& other) const { I see that some places DCHECK_NE(kPinnedAppsPlaceholder, app_launcher_id.GetAsString());, but where do we ever actually compare two AppLauncherId instances? https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:304: bool AppLauncherId::operator<(const AppLauncherId& other) const { Why are all these comparison functions needed? Could callers just compare the GetAsString values? https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:64: // This constructor is currently unused, but will be used in a follow-up CL. This would better as a code review comment, not a comment in the code itself.
https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:304: bool AppLauncherId::operator<(const AppLauncherId& other) const { On 2016/10/04 17:09:24, msw wrote: > Why are all these comparison functions needed? Could callers just compare the > GetAsString values? I think this one is needed to store AppLauncherId in a std::set<>. Most of the others are probably not needed.
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ==========
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ==========
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ==========
On 2016/10/04 17:18:44, James Cook wrote: > https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... > File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): > > https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... > chrome/browser/ui/ash/chrome_launcher_prefs.cc:304: bool > AppLauncherId::operator<(const AppLauncherId& other) const { > On 2016/10/04 17:09:24, msw wrote: > > Why are all these comparison functions needed? Could callers just compare the > > GetAsString values? > > I think this one is needed to store AppLauncherId in a std::set<>. Most of the > others are probably not needed. Yes, this one is needed for the set storing. I will not include the other comparison operators in the next patch set.
Thank you for feedback, I added the nits to the new patch set. https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/100001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: bool operator!=(const AppLauncherId& app_launcher_id) const; On 2016/10/04 16:08:37, James Cook wrote: > On 2016/10/04 11:39:00, Andra Paraschiv wrote: > > On 2016/10/03 21:14:36, James Cook wrote: > > > Do you really need all of these operators? I would guess that you only need > > > operator== for comparison and operator< for storing in a set. > > > > Yes, you are right, we currently need only these comparison operators. Should > we > > not include the other ones for now? > > Yes, only include the ones you need. Done. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:292: : app_launcher_id_(app_id + launch_id) {} On 2016/10/04 17:09:24, msw wrote: > Why concatenate these internally? Then there's no way to reconstitute the > |app_id| and |launcher_id| elements from an AppLauncherId object. It would be > more useful if this single id could replace various uses of the other two, but > losing the constituent id information means that we'll need to keep using the > other ids in more places. I added app_id and launch_id as class members. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:296: bool AppLauncherId::operator==(const AppLauncherId& other) const { On 2016/10/04 17:09:24, msw wrote: > I see that some places DCHECK_NE(kPinnedAppsPlaceholder, > app_launcher_id.GetAsString());, but where do we ever actually compare two > AppLauncherId instances? I included in the last patch set only the operators that are currently necessary. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:64: // This constructor is currently unused, but will be used in a follow-up CL. On 2016/10/04 17:09:24, msw wrote: > This would better as a code review comment, not a comment in the code itself. Done. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: AppLauncherId(const AppLauncherId& app_launcher_id) = default; On 2016/10/04 16:08:37, James Cook wrote: > super nit: Put copy constructor and operator= together, after constructors and > destructors: > > Foo() > ~Foo() > > Foo(const Foo& foo) = default > Foo& operator=(const Foo& foo) = default > > like base/weak_reference.h > > optional: You may want move-construction as well (Foo&&) Done. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_mash_shelf_controller.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_mash_shelf_controller.cc:115: item->app_id = app_launcher_id.GetAsString(); On 2016/10/04 16:08:37, James Cook wrote: > nit: Given that you use app_launcher_id.GetAsString() 7 times, I would store the > result in a local variable (like |app_id| or something) and use that. It'll make > the line wrapping better. Done.
still lgtm https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:292: : app_launcher_id_(app_id + launch_id) {} On 2016/10/05 11:21:58, Andra Paraschiv wrote: > On 2016/10/04 17:09:24, msw wrote: > > Why concatenate these internally? Then there's no way to reconstitute the > > |app_id| and |launcher_id| elements from an AppLauncherId object. It would be > > more useful if this single id could replace various uses of the other two, but > > losing the constituent id information means that we'll need to keep using the > > other ids in more places. > > I added app_id and launch_id as class members. I suspect msw meant to replace app_launcher_id_ with just app_id_ and launch_id_ and concatenate them in GetAsString, but I'll leave that up to him. https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:288: : app_launcher_id_(app_id), app_id_(app_id), launch_id_("") {} nit: no need for "" (or even for listing launch_id_ in the initializer list)
Also, you can probably remove stevenjb and reillyg from the reviewer line. Generally we only put people there that we really want to review the CL, and a CL this size doesn't need 4 reviewers. Feel free to add whoever you want to the CC list, of course.
lgtm with a nit and minor comment. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:292: : app_launcher_id_(app_id + launch_id) {} On 2016/10/05 15:33:10, James Cook wrote: > On 2016/10/05 11:21:58, Andra Paraschiv wrote: > > On 2016/10/04 17:09:24, msw wrote: > > > Why concatenate these internally? Then there's no way to reconstitute the > > > |app_id| and |launcher_id| elements from an AppLauncherId object. It would > be > > > more useful if this single id could replace various uses of the other two, > but > > > losing the constituent id information means that we'll need to keep using > the > > > other ids in more places. > > > > I added app_id and launch_id as class members. > > I suspect msw meant to replace app_launcher_id_ with just app_id_ and launch_id_ > and concatenate them in GetAsString, but I'll leave that up to him. Indeed, |app_launcher_id_| isn't needed anymore; just return a concatenated value in GetAsString, and define the comparison operator as "return GetAsString() < other.GetAsString();". It trades performance for memory, but that seems like the right tradeoff here. https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:71: const std::string& GetAsString() const { return app_launcher_id_; } optional nit: rename as ToString
lgtm
Thank you all for review, I modified the method name and definition. https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/120001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:292: : app_launcher_id_(app_id + launch_id) {} On 2016/10/05 17:14:28, msw wrote: > On 2016/10/05 15:33:10, James Cook wrote: > > On 2016/10/05 11:21:58, Andra Paraschiv wrote: > > > On 2016/10/04 17:09:24, msw wrote: > > > > Why concatenate these internally? Then there's no way to reconstitute the > > > > |app_id| and |launcher_id| elements from an AppLauncherId object. It would > > be > > > > more useful if this single id could replace various uses of the other two, > > but > > > > losing the constituent id information means that we'll need to keep using > > the > > > > other ids in more places. > > > > > > I added app_id and launch_id as class members. > > > > I suspect msw meant to replace app_launcher_id_ with just app_id_ and > launch_id_ > > and concatenate them in GetAsString, but I'll leave that up to him. > > Indeed, |app_launcher_id_| isn't needed anymore; just return a concatenated > value in GetAsString, and define the comparison operator as "return > GetAsString() < other.GetAsString();". It trades performance for memory, but > that seems like the right tradeoff here. Done. https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:288: : app_launcher_id_(app_id), app_id_(app_id), launch_id_("") {} On 2016/10/05 15:33:10, James Cook wrote: > nit: no need for "" (or even for listing launch_id_ in the initializer list) Done. https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/140001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:71: const std::string& GetAsString() const { return app_launcher_id_; } On 2016/10/05 17:14:28, msw wrote: > optional nit: rename as ToString Done.
still lgtm with an optional nit https://codereview.chromium.org/2352353002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:71: const std::string ToString() const { return app_id_ + launch_id_; } optional nit: the return value does not need to be const
Thanks. https://codereview.chromium.org/2352353002/diff/160001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/160001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:71: const std::string ToString() const { return app_id_ + launch_id_; } On 2016/10/06 16:46:52, msw wrote: > optional nit: the return value does not need to be const Done.
lgtm https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:669: pins.push_back(pin_infos[i].app_launcher_id); nit: the previous method of presizing the vector was slightly more optimal since it doesn't require additional allocations, was there a reason you changed that? https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:677: DCHECK(!launcher_id.empty()); We shouldn't convert app_launcher_id to a string just to maintain this DCHECK, it makes the rest of the code more confusing. Instead: 1) Remove the above two lines. 2) Use launcher_id.ToString() in the SetPinPosition call below. 3) In the AppLauncherId constructors, add DCHECK(!app_id.empty()), and add a comment in the header that |app_id| must not be empty. Then we can always guarantee that any AppLauncherId is valid. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:689: DCHECK(!launcher_id.empty()); DCHECK not needed, see above. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:691: DCHECK_NE(launcher_id, launcher_id_before); nit: Maybe name these app_launcher_id_str and app_launcher_id_before_str. "launcher_id" is too close to "launch_id". https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:63: explicit AppLauncherId(const std::string& app_id); Make this the second constructor and add a comment above it: // Creates an AppLauncherId with an empty |launch_id| (this is the default and more common case). https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:71: std::string ToString() const { return app_id_ + launch_id_; } Don't inline this. Only trivial accessors (e.g. the accessors below) should be inlined. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1223: const std::string launcher_id = pref_app_launcher_id.ToString(); nit: app_launcher_id_str
Thank you, Steven, for review. I updated the CL and added a comment regarding the app_id check if not empty. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.cc (right): https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:669: pins.push_back(pin_infos[i].app_launcher_id); On 2016/10/13 17:59:15, stevenjb wrote: > nit: the previous method of presizing the vector was slightly more optimal since > it doesn't require additional allocations, was there a reason you changed that? Thanks for this, I will revert to the previous method. This was changed before being able to copy and store AppLauncherId. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:677: DCHECK(!launcher_id.empty()); On 2016/10/13 17:59:15, stevenjb wrote: > We shouldn't convert app_launcher_id to a string just to maintain this DCHECK, > it makes the rest of the code more confusing. Instead: > 1) Remove the above two lines. > 2) Use launcher_id.ToString() in the SetPinPosition call below. > 3) In the AppLauncherId constructors, add DCHECK(!app_id.empty()), and add a > comment in the header that |app_id| must not be empty. Then we can always > guarantee that any AppLauncherId is valid. We could remove this check and add it to the constructor in the majority of cases, but there are some places where the app_launcher_id_str is not checked if empty e.g. app_launcher_id_before_str from SetPinPosition or in the pins vector init from GetPinnedAppsFromPrefs. Should we add an AppLauncherId empty constructor? https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.cc:691: DCHECK_NE(launcher_id, launcher_id_before); On 2016/10/13 17:59:15, stevenjb wrote: > nit: Maybe name these app_launcher_id_str and app_launcher_id_before_str. > "launcher_id" is too close to "launch_id". > Done. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:63: explicit AppLauncherId(const std::string& app_id); On 2016/10/13 17:59:15, stevenjb wrote: > Make this the second constructor and add a comment above it: > // Creates an AppLauncherId with an empty |launch_id| (this is the default and > more common case). > Done. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:71: std::string ToString() const { return app_id_ + launch_id_; } On 2016/10/13 17:59:15, stevenjb wrote: > Don't inline this. Only trivial accessors (e.g. the accessors below) should be > inlined. Done. https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc (right): https://codereview.chromium.org/2352353002/diff/200001/chrome/browser/ui/ash/... chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl.cc:1223: const std::string launcher_id = pref_app_launcher_id.ToString(); On 2016/10/13 17:59:15, stevenjb wrote: > nit: app_launcher_id_str Done.
@stevenjb Could you please take a look at the new patch set? Regarding the comment from the previous review about the app_id check if not empty: * I included the check in the constructors and removed it from the codebase where necessary. * I added an empty constructor for the AppLauncherId to include the cases where the app_launcher_id could be empty e.g. app_launcher_id_before_str in SetPinPosition from chrome_launcher_prefs.cc . Thank you.
Thanks, LGTM! https://codereview.chromium.org/2352353002/diff/280001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/280001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: // Empty constructor. nit: // Empty constructor for pre-allocating.
Thank you for review. https://codereview.chromium.org/2352353002/diff/280001/chrome/browser/ui/ash/... File chrome/browser/ui/ash/chrome_launcher_prefs.h (right): https://codereview.chromium.org/2352353002/diff/280001/chrome/browser/ui/ash/... chrome/browser/ui/ash/chrome_launcher_prefs.h:66: // Empty constructor. On 2016/10/18 21:07:28, stevenjb wrote: > nit: // Empty constructor for pre-allocating. Done.
The CQ bit was checked by andra.paraschiv@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from skuhne@chromium.org, jamescook@chromium.org, msw@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2352353002/#ps300001 (title: "Review v10")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest ========== to ========== Add AppLauncherId wrapper for items shown in shelf Add AppLauncherId wrapper in the chrome launcher prefs codebase. It includes a unique chrome launcher id for a shelf item. The "AppLauncherId(const std::string& app_id, const std::string& launch_id)" constructor is currently unused, but will be used in a follow-up CL. Based on https://codereview.chromium.org/2290603002 Co-Authored-By: Valentin Ilie <valentin.ilie@intel.com>; BUG=610299 TEST=LauncherPlatformAppBrowserTest Committed: https://crrev.com/f9a980f3718e5d262ee429032fb7083caa9ba343 Cr-Commit-Position: refs/heads/master@{#426145} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/f9a980f3718e5d262ee429032fb7083caa9ba343 Cr-Commit-Position: refs/heads/master@{#426145} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
