Change 4 funcs to JumpList member funcs, retire wstring for jumplist*

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/+/e69eb0e891909eacc2ad5b22787df30517428ea4

[chengx, 2017-05-09 05:30:46]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 05:31:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-05-09 05:59:58]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 06:00:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-05-09 06:05:02]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-09 06:41:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 06:41:01]

Dry run: This issue passed the CQ dry run.

[chengx, 2017-05-09 07:07:31]

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, 2017-05-09 07:12:16]

chengx@chromium.org changed reviewers:
+ gab@chromium.org, grt@chromium.org

[chengx, 2017-05-09 07:12:16]

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.

[gab, 2017-05-09 14:54:28]

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)

[chengx, 2017-05-09 17:58:40]

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
==========

[chengx, 2017-05-09 17:58:57]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 17:59:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-05-09 18:01:19]

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, 2017-05-09 18:01:19]

chengx@chromium.org changed reviewers:
- grt@chromium.org

[chengx, 2017-05-09 18:04:14]

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.

[chengx, 2017-05-09 18:20:59]

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
==========

[chengx, 2017-05-09 18:21:47]

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
==========

[gab, 2017-05-09 18:37:37]

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)

[chengx, 2017-05-09 18:51:06]

The CQ bit was checked by chengx@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 18:52:42]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-05-09 18:56:04]

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.

[commit-bot: I haz the power, 2017-05-09 19:51:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 19:51:05]

Dry run: This issue passed the CQ dry run.

[gab, 2017-05-09 20:07:11]

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).

[chengx, 2017-05-09 20:11:32]

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!

[chengx, 2017-05-09 20:11:50]

The CQ bit was checked by chengx@chromium.org

[commit-bot: I haz the power, 2017-05-09 20:13:05]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-05-09 21:36:55]

The CQ bit was unchecked by chengx@chromium.org

[chengx, 2017-05-09 21:36:57]

The CQ bit was checked by chengx@chromium.org

[commit-bot: I haz the power, 2017-05-09 21:37:16]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chengx, 2017-05-09 23:13:52]

The CQ bit was unchecked by chengx@chromium.org

[chengx, 2017-05-09 23:13:57]

The CQ bit was checked by chengx@chromium.org

[commit-bot: I haz the power, 2017-05-09 23:14:21]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 02:07:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 02:07:54]

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?))

[chengx, 2017-05-10 04:19:04]

The CQ bit was checked by chengx@chromium.org

[commit-bot: I haz the power, 2017-05-10 04:19:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 04:44:40]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1494389944459740,
"parent_rev": "28b78a893c03f9da7d84fbee4e1a652211b6b905", "commit_rev":
"e69eb0e891909eacc2ad5b22787df30517428ea4"}

[commit-bot: I haz the power, 2017-05-10 04:44:53]

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...
==========

[commit-bot: I haz the power, 2017-05-10 04:44:55]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/e69eb0e891909eacc2ad5b22787d...