Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846383002:
Improve JumpList icon folders' naming style (Closed)
|
Created:
3 years, 7 months ago by chengx Modified:
3 years, 7 months ago Reviewers:
grt (UTC plus 2) CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImprove JumpList icon folders' naming style
Currently, the JumpList class has a data member icon_dir_ to hold
/profile/kJumpListIconDirname, which indicates that there's only
one directory to hold jumplist icons. However, there are actually
more than one icon directory. When generating these directory
names, we append various kFoos to icon_dir_.
It's better to append (chrome::kJumpListIconDirname + kFoo) as a
whole to the profile directory as needed at the various points of
use, rather than the current adding various kFoos to icon_dir_.
This avoids the confusion.
BUG=718314
Review-Url: https://codereview.chromium.org/2846383002
Cr-Commit-Position: refs/heads/master@{#469431}
Committed: https://chromium.googlesource.com/chromium/src/+/6dbff02c181d5689f8c509b0890733c5c7b9a413
Patch Set 1 #
Total comments: 4
Patch Set 2 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into updatejumplisticonsfol… #Patch Set 3 : Address comments #
Total comments: 14
Patch Set 4 : Update GenerateJumplistIconDirName method and more #
Total comments: 6
Patch Set 5 : Address nits. #Patch Set 6 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into updatejumplisticonsfol… #Messages
Total messages: 50 (41 generated)
Description was changed from ========== Update JumpListIcons* folder names BUG= ========== to ========== Update JumpListIcons* folder names Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoo to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG= ==========
Description was changed from ========== Update JumpListIcons* folder names Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoo to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG= ========== to ========== Update JumpListIcons* folder names Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoo to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG= ==========
Description was changed from ========== Update JumpListIcons* folder names Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoo to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG= ========== to ========== Update JumpListIcons* folder names Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Update JumpListIcons* folder names Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ========== to ========== Update JumpListIcons* folders' naming mechanism Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Update JumpListIcons* folders' naming mechanism Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ========== to ========== Update JumpListIcons* folders' naming style Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Update JumpListIcons* folders' naming style Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpListIcons* folders' naming style Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Improve JumpListIcons* folders' naming style Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
PTAL~ I understand we prefer char[] over string when defining global const variable, but using string here makes the code simpler IMHO. If using char[], I need to construct a string using this char[] and add kFoo to the constructed string. Thoughts please?
https://codereview.chromium.org/2846383002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:246: base::FilePath icon_dir_most_visited = profile->GetPath().Append( a Profile* may only be used on the UI thread. you must pass the path over to this thread rather than the Profile* (for reference, see browser_context.h, which defines Profile's base class). https://codereview.chromium.org/2846383002/diff/1/chrome/common/chrome_consta... File chrome/common/chrome_constants.h (right): https://codereview.chromium.org/2846383002/diff/1/chrome/common/chrome_consta... chrome/common/chrome_constants.h:75: extern const base::FilePath::StringType kJumpListIconDirname; this must remain a CharType array since introducing static ctors and dtors for globals is banned.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #3 (id:60001) has been deleted
PTAL~ https://codereview.chromium.org/2846383002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:246: base::FilePath icon_dir_most_visited = profile->GetPath().Append( On 2017/05/01 09:22:56, grt (UTC plus 2) wrote: > a Profile* may only be used on the UI thread. you must pass the path over to > this thread rather than the Profile* (for reference, see browser_context.h, > which defines Profile's base class). Done. I've updated to pass the profile path. https://codereview.chromium.org/2846383002/diff/1/chrome/common/chrome_consta... File chrome/common/chrome_constants.h (right): https://codereview.chromium.org/2846383002/diff/1/chrome/common/chrome_consta... chrome/common/chrome_constants.h:75: extern const base::FilePath::StringType kJumpListIconDirname; On 2017/05/01 09:22:56, grt (UTC plus 2) wrote: > this must remain a CharType array since introducing static ctors and dtors for > globals is banned. Done. I've reverted the changes in chrome_constants.*
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. Therefore, icon_dir_ in no longer needed. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ==========
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ==========
https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:202: const base::FilePath::StringType& dir_name_append, take this as a base::FilePath::StringPieceType to avoid making an extra copy https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:202: const base::FilePath::StringType& dir_name_append, nit: dir_name_append -> suffix https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:203: base::FilePath* out_dir) { we treat FilePath as a value type -- return this directly rather than take it as an out param https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:204: base::FilePath::StringType jumplisticon_base_dir( how about: base::FilePath::StringType dir_name(chrome::kJumpListIconDirname); suffix.AppendToString(&dir_name); return profile_dir.Append(dir_name); https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:455: if (profile_) { should this also stop the timer? https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:693: base::FilePath profile_dir = profile_->GetPath(); null check on profile_ as above? i suppose that's there because of the clearing in Terminate.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I've addressed all the feedback comments in the new patch set. PTAL, thanks! https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:202: const base::FilePath::StringType& dir_name_append, On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > nit: dir_name_append -> suffix Done. https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:202: const base::FilePath::StringType& dir_name_append, On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > take this as a base::FilePath::StringPieceType to avoid making an extra copy Done. https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:203: base::FilePath* out_dir) { On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > we treat FilePath as a value type -- return this directly rather than take it as > an out param Done. I thought we should avoid copying strings. This doesn't apply to FilePath? https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:204: base::FilePath::StringType jumplisticon_base_dir( On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > how about: > base::FilePath::StringType dir_name(chrome::kJumpListIconDirname); > suffix.AppendToString(&dir_name); > return profile_dir.Append(dir_name); Accepted. Thanks! https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:455: if (profile_) { On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > should this also stop the timer? It should. I'll fix this in the "delay and coalesce" CL. Thanks! https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:693: base::FilePath profile_dir = profile_->GetPath(); On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > null check on profile_ as above? i suppose that's there because of the clearing > in Terminate. Yes. A null check is needed here.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=40407, 179576 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=718314 ==========
lgtm w/ nits https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:203: base::FilePath* out_dir) { On 2017/05/03 18:36:30, chengx wrote: > On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > > we treat FilePath as a value type -- return this directly rather than take it > as > > an out param > > Done. I thought we should avoid copying strings. This doesn't apply to FilePath? No strings are copied here. Between RVO (return value optimization) and copy elision, the FilePath returned by Append is passed straight back to the caller. This is more efficient that what you had before, since there's no longer an extra empty string created by the caller and then replaced with the one returned by Append. C++: the land of most surprise! https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:200: base::FilePath GenerateJumplistIconDirName( // Returns the full path of the JumpListIcons[|suffix|] directory in |profile_dir|. https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:692: profile_ ? IncognitoModePrefs::GetAvailability(profile_->GetPrefs()) this null check is no longer needed due to the early return above. https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:706: profile_dir.Append(chrome::kJumpListIconDirname), nit: i'd be inclined to use GenerateJumplistIconDirName here for the sake of uniformity. it's up to you.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
All the feedback comments have been addressed in the new patch set. Thanks! https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:203: base::FilePath* out_dir) { On 2017/05/04 11:41:55, grt (UTC plus 2) wrote: > On 2017/05/03 18:36:30, chengx wrote: > > On 2017/05/03 10:10:41, grt (UTC plus 2) wrote: > > > we treat FilePath as a value type -- return this directly rather than take > it > > as > > > an out param > > > > Done. I thought we should avoid copying strings. This doesn't apply to > FilePath? > > No strings are copied here. Between RVO (return value optimization) and copy > elision, the FilePath returned by Append is passed straight back to the caller. > This is more efficient that what you had before, since there's no longer an > extra empty string created by the caller and then replaced with the one returned > by Append. C++: the land of most surprise! Thanks for the explanation! https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:200: base::FilePath GenerateJumplistIconDirName( On 2017/05/04 11:41:55, grt (UTC plus 2) wrote: > // Returns the full path of the JumpListIcons[|suffix|] directory in > |profile_dir|. Done. https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:692: profile_ ? IncognitoModePrefs::GetAvailability(profile_->GetPrefs()) On 2017/05/04 11:41:55, grt (UTC plus 2) wrote: > this null check is no longer needed due to the early return above. Done. https://codereview.chromium.org/2846383002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:706: profile_dir.Append(chrome::kJumpListIconDirname), On 2017/05/04 11:41:55, grt (UTC plus 2) wrote: > nit: i'd be inclined to use GenerateJumplistIconDirName here for the sake of > uniformity. it's up to you. I've changed to use GenerateJumplistIconDirName.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2846383002/#ps140001 (title: "Merge branch 'master' of https://chromium.googlesource.com/chromium/src into updatejumplisticonsfol…")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493926992684930,
"parent_rev": "b4f862467b0dce4e75b337f28fab0b7a5019f6ba", "commit_rev":
"6dbff02c181d5689f8c509b0890733c5c7b9a413"}
Message was sent while issue was closed.
Description was changed from ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=718314 ========== to ========== Improve JumpList icon folders' naming style Currently, the JumpList class has a data member icon_dir_ to hold /profile/kJumpListIconDirname, which indicates that there's only one directory to hold jumplist icons. However, there are actually more than one icon directory. When generating these directory names, we append various kFoos to icon_dir_. It's better to append (chrome::kJumpListIconDirname + kFoo) as a whole to the profile directory as needed at the various points of use, rather than the current adding various kFoos to icon_dir_. This avoids the confusion. BUG=718314 Review-Url: https://codereview.chromium.org/2846383002 Cr-Commit-Position: refs/heads/master@{#469431} Committed: https://chromium.googlesource.com/chromium/src/+/6dbff02c181d5689f8c509b08907... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001) as https://chromium.googlesource.com/chromium/src/+/6dbff02c181d5689f8c509b08907... |
