|
|
DescriptionChange 4 funcs to JumpList member funcs, retire wstring for jumplist*
This CL seems to have a lot of code changes, but in fact it does
nothing but to change 4 anonymous functions to JumpList class member
functions. Originally these 4 functions were put in an anonymous
namespace, and they are now moved to the end of jumplist.cc. Their
declarations are added in jumplist.h. The body of these 4 functions
have no changes at all. However, one PostTask function call is updated
accordingly due to this change.
Making these 4 functions be member functions of JumpList class allows
them to get access the member variables (which I am going to add) of
the JumpList class. This lays out the foundation for a number of
JumpList improvements I am going to do in parallel. Otherwise, there
will be a lot of duplicated code changes and heavy merge conflicts
ahead. Moreover, this also makes it easier to review the coming CLs on
top of this.
This CL also changes all std::wstring to base::string16 in jumplist.*
and jumplist_updater.* to get rid of presubmit warnings.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2870853002
Cr-Commit-Position: refs/heads/master@{#470455}
Committed: https://chromium.googlesource.com/chromium/src/+/e69eb0e891909eacc2ad5b22787df30517428ea4
Patch Set 1 #
Total comments: 12
Patch Set 2 : Change std::wstring to base::string16 #
Total comments: 2
Patch Set 3 : Change base::unretained(this) to this #
Messages
Total messages: 42 (29 generated)
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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Change to JumpList class member functions BUG= ========== to ========== Change 4 anonymous functions to be member functions of JumpList class This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there'll be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + gab@chromium.org, grt@chromium.org
Hi Greg/Gabriel, can one of you take a look at this CL please? This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. These 4 functions get re-located but the body of the functions have no change at all. So it won't take long to review it. For the motivation, please refer to the CL description.
Good idea to make a move-only CL first, please resolve the string16 and other comments in precursor CLs as required to keep this one move-only :) https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:481: base::Bind(&JumpList::RunUpdateJumpList, base::Unretained(this), Looks like jumplist is owned by BrowserView [1]. As such base::Unretained is inappropriate as it will crash if the BrowserView is destroyed while this task is in the queue. What do you the outcome to be when the BrowserView is destroyed? Cancel the pending tasks or keep Jumplist alive until its tasks complete? JumpList is RefCounted but there doesn't appear to be a good reason for that (it has a single owner). If cancellation is appropriate then let's change it to not be RefCounted (i.e. be owned via a std::unique_ptr) and add a WeakPtrFactory member which can be used to weakly bind tasks (will be cancelled if Jumplist is destroyed early). Note however that the current behavior (per the anonymous tasks) is to keep running after the BrowserView goes away. [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:614: JumpListUpdater jumplist_updater(app_id); Looks like this is taking a string& (so taking it by char* above was resulting in an unnecessary string copy) Consider updating JumpListUpdater to use string16 as well for consistency. https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:196: const std::wstring& app_id, Use base::string16 instead of std::wstring (I'm surprised this didn't hit the presubmit against wstring [1]?) And #include "base/strings/string16.h" [1] https://cs.chromium.org/chromium/src/PRESUBMIT.py?rcl=f41443c03b03d8e2d83314a... https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:224: std::wstring app_id_; base::string16 (for this and all other wstring in these two files for consistency)
Description was changed from ========== Change 4 anonymous functions to be member functions of JumpList class This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there'll be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. BUG=40407, 179576 ========== to ========== Change 4 anonymous functions to be member functions of JumpList class This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16. 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 ========== Change 4 anonymous functions to be member functions of JumpList class This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16. BUG=40407, 179576 ========== to ========== Change 4 anonymous functions to be member functions of JumpList class This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16 in jumplist.* and jumplist_updater.* to get rid of presubmit warnings. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: - grt@chromium.org
Hi Gabriel, I've addressed all the review comments in the new patch set. PTAL~ https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:481: base::Bind(&JumpList::RunUpdateJumpList, base::Unretained(this), On 2017/05/09 14:54:28, gab wrote: > Looks like jumplist is owned by BrowserView [1]. As such base::Unretained is > inappropriate as it will crash if the BrowserView is destroyed while this task > is in the queue. > > What do you the outcome to be when the BrowserView is destroyed? Cancel the > pending tasks or keep Jumplist alive until its tasks complete? > > JumpList is RefCounted but there doesn't appear to be a good reason for that (it > has a single owner). If cancellation is appropriate then let's change it to not > be RefCounted (i.e. be owned via a std::unique_ptr) and add a WeakPtrFactory > member which can be used to weakly bind tasks (will be cancelled if Jumplist is > destroyed early). > > Note however that the current behavior (per the anonymous tasks) is to keep > running after the BrowserView goes away. > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... Thanks for pointing me to this, and I'm happy to fix it. How about this? I've filed crbug.com/720028 for this, and I'll use another CL for this purpose. It's out of the scope of this CL anyway IMHO. https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:614: JumpListUpdater jumplist_updater(app_id); On 2017/05/09 14:54:28, gab wrote: > Looks like this is taking a string& (so taking it by char* above was resulting > in an unnecessary string copy) > > Consider updating JumpListUpdater to use string16 as well for consistency. I've updated JumpList::UpdateJumpList to use string16& from wchar_t*. I've also updated JumpListUpdater to use base::string16. https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.h (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:196: const std::wstring& app_id, On 2017/05/09 14:54:28, gab wrote: > Use base::string16 instead of std::wstring (I'm surprised this didn't hit the > presubmit against wstring [1]?) > > And #include "base/strings/string16.h" > > [1] > https://cs.chromium.org/chromium/src/PRESUBMIT.py?rcl=f41443c03b03d8e2d83314a... Done. It did hit the presubmit against wstring, but I thought it was okay for Windows platform. I was not aware we have base::string16. My bad. https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.h:224: std::wstring app_id_; On 2017/05/09 14:54:28, gab wrote: > base::string16 (for this and all other wstring in these two files for > consistency) Done.
Description was changed from ========== Change 4 anonymous functions to be member functions of JumpList class This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16 in jumplist.* and jumplist_updater.* to get rid of presubmit warnings. BUG=40407, 179576 ========== to ========== Change some funcs to JumpList class member funcs, retire wstring for jumplist* files This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16 in jumplist.* and jumplist_updater.* to get rid of presubmit warnings. BUG=40407, 179576 ==========
Description was changed from ========== Change some funcs to JumpList class member funcs, retire wstring for jumplist* files This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16 in jumplist.* and jumplist_updater.* to get rid of presubmit warnings. BUG=40407, 179576 ========== to ========== Change 4 funcs to JumpList member funcs, retire wstring for jumplist* This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16 in jumplist.* and jumplist_updater.* to get rid of presubmit warnings. BUG=40407, 179576 ==========
https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:481: base::Bind(&JumpList::RunUpdateJumpList, base::Unretained(this), On 2017/05/09 18:04:14, chengx wrote: > On 2017/05/09 14:54:28, gab wrote: > > Looks like jumplist is owned by BrowserView [1]. As such base::Unretained is > > inappropriate as it will crash if the BrowserView is destroyed while this task > > is in the queue. > > > > What do you the outcome to be when the BrowserView is destroyed? Cancel the > > pending tasks or keep Jumplist alive until its tasks complete? > > > > JumpList is RefCounted but there doesn't appear to be a good reason for that > (it > > has a single owner). If cancellation is appropriate then let's change it to > not > > be RefCounted (i.e. be owned via a std::unique_ptr) and add a WeakPtrFactory > > member which can be used to weakly bind tasks (will be cancelled if Jumplist > is > > destroyed early). > > > > Note however that the current behavior (per the anonymous tasks) is to keep > > running after the BrowserView goes away. > > > > [1] > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... > > Thanks for pointing me to this, and I'm happy to fix it. How about this? I've > filed crbug.com/720028 for this, and I'll use another CL for this purpose. It's > out of the scope of this CL anyway IMHO. Well you can't leave Unretained(this) in this CL though, it will crash if BrowserView is destroyed while the task is in the queue. You need to just pass |this| and force an extra ref to be bound to keep |this| alive until the callback runs. Looks like this is already done in at least one place [1] in the codebase, which might be why it's currently refcounted -- but that callsite could perhaps also use WeakPtr eventually? [1] https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?type=cs&q... https://codereview.chromium.org/2870853002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:294: base::string16 url_string_wide = base::UTF8ToWide(url_string); s/UTF8ToWide/UTF8ToUTF16/ for consistency (here and one more in this file)
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...
PTAL, thanks! https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:481: base::Bind(&JumpList::RunUpdateJumpList, base::Unretained(this), On 2017/05/09 18:37:36, gab wrote: > On 2017/05/09 18:04:14, chengx wrote: > > On 2017/05/09 14:54:28, gab wrote: > > > Looks like jumplist is owned by BrowserView [1]. As such base::Unretained is > > > inappropriate as it will crash if the BrowserView is destroyed while this > task > > > is in the queue. > > > > > > What do you the outcome to be when the BrowserView is destroyed? Cancel the > > > pending tasks or keep Jumplist alive until its tasks complete? > > > > > > JumpList is RefCounted but there doesn't appear to be a good reason for that > > (it > > > has a single owner). If cancellation is appropriate then let's change it to > > not > > > be RefCounted (i.e. be owned via a std::unique_ptr) and add a WeakPtrFactory > > > member which can be used to weakly bind tasks (will be cancelled if Jumplist > > is > > > destroyed early). > > > > > > Note however that the current behavior (per the anonymous tasks) is to keep > > > running after the BrowserView goes away. > > > > > > [1] > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi... > > > > Thanks for pointing me to this, and I'm happy to fix it. How about this? I've > > filed crbug.com/720028 for this, and I'll use another CL for this purpose. > It's > > out of the scope of this CL anyway IMHO. > > Well you can't leave Unretained(this) in this CL though, it will crash if > BrowserView is destroyed while the task is in the queue. > > You need to just pass |this| and force an extra ref to be bound to keep |this| > alive until the callback runs. > Looks like this is already done in at least one place [1] in the codebase, which > might be why it's currently refcounted -- but that callsite could perhaps also > use WeakPtr eventually? > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?type=cs&q... Oh, I see. I've changed base::Unretained(this) to |this|. I am not sure what it means by "force an extra ref to be bound to keep |this| alive ... " though. Do I do need to make more changes? https://codereview.chromium.org/2870853002/diff/40001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/40001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:294: base::string16 url_string_wide = base::UTF8ToWide(url_string); On 2017/05/09 18:37:37, gab wrote: > s/UTF8ToWide/UTF8ToUTF16/ for consistency (here and one more in this file) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:614: JumpListUpdater jumplist_updater(app_id); On 2017/05/09 18:04:14, chengx wrote: > On 2017/05/09 14:54:28, gab wrote: > > Looks like this is taking a string& (so taking it by char* above was resulting > > in an unnecessary string copy) > > > > Consider updating JumpListUpdater to use string16 as well for consistency. > > I've updated JumpList::UpdateJumpList to use string16& from wchar_t*. I've also > updated JumpListUpdater to use base::string16. No, refcounting is the default for Bind. Since Jumplist inherits from base::RefCounted passing |this| makes Bind take a reference (it simply wouldn't compile if it wasn't RefCounted).
https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2870853002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:614: JumpListUpdater jumplist_updater(app_id); On 2017/05/09 20:07:11, gab wrote: > On 2017/05/09 18:04:14, chengx wrote: > > On 2017/05/09 14:54:28, gab wrote: > > > Looks like this is taking a string& (so taking it by char* above was > resulting > > > in an unnecessary string copy) > > > > > > Consider updating JumpListUpdater to use string16 as well for consistency. > > > > I've updated JumpList::UpdateJumpList to use string16& from wchar_t*. I've > also > > updated JumpListUpdater to use base::string16. > > No, refcounting is the default for Bind. Since Jumplist inherits from > base::RefCounted passing |this| makes Bind take a reference (it simply wouldn't > compile if it wasn't RefCounted). Cool. Thanks for the explanation!
The CQ bit was checked by chengx@chromium.org
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 chengx@chromium.org
The CQ bit was checked by chengx@chromium.org
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 chengx@chromium.org
The CQ bit was checked by chengx@chromium.org
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by chengx@chromium.org
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": 60001, "attempt_start_ts": 1494389944459740, "parent_rev": "28b78a893c03f9da7d84fbee4e1a652211b6b905", "commit_rev": "e69eb0e891909eacc2ad5b22787df30517428ea4"}
Message was sent while issue was closed.
Description was changed from ========== Change 4 funcs to JumpList member funcs, retire wstring for jumplist* This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16 in jumplist.* and jumplist_updater.* to get rid of presubmit warnings. BUG=40407, 179576 ========== to ========== Change 4 funcs to JumpList member funcs, retire wstring for jumplist* This CL seems to have a lot of code changes, but in fact it does nothing but to change 4 anonymous functions to JumpList class member functions. Originally these 4 functions were put in an anonymous namespace, and they are now moved to the end of jumplist.cc. Their declarations are added in jumplist.h. The body of these 4 functions have no changes at all. However, one PostTask function call is updated accordingly due to this change. Making these 4 functions be member functions of JumpList class allows them to get access the member variables (which I am going to add) of the JumpList class. This lays out the foundation for a number of JumpList improvements I am going to do in parallel. Otherwise, there will be a lot of duplicated code changes and heavy merge conflicts ahead. Moreover, this also makes it easier to review the coming CLs on top of this. This CL also changes all std::wstring to base::string16 in jumplist.* and jumplist_updater.* to get rid of presubmit warnings. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2870853002 Cr-Commit-Position: refs/heads/master@{#470455} Committed: https://chromium.googlesource.com/chromium/src/+/e69eb0e891909eacc2ad5b22787d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/e69eb0e891909eacc2ad5b22787d... |