Proposal: discourage use of base::Passed

[maxm...@chromium.org]

It seems to me like there's no good reason to use Passed now that we have OnceCallback. If using BindOnce, we can use std::move instead (and this is probably more intuitive/readable for people new to Chromium). If using BindRepeating with Passed, the callback can only be called once, so BindOnce should be used instead. There may be some old APIs which take RepeatingCallbacks but promise to only call them once, and in that case using Passed is at the discretion of the CL author and reviewers, but refactoring the API to work with OnceCallback should be preferred. A previous similar discussion seems to arrive at the same conclusions: https://groups.google.com/a/chromium.org/forum/#!topic/cxx/KXWZtoP5HJ4.

I put a CL updating callback.md up for discussion at https://chromium-review.googlesource.com/c/chromium/src/+/919066.

[Daniel Cheng]

Thanks for putting up a CL. I've already been asking people to do this in reviews (prefer std::move with base::OnceCallback), so updating the guidance here makes sense to me.

Daniel

On Wed, Feb 14, 2018 at 6:46 AM <maxm...@chromium.org> wrote:

It seems to me like there's no good reason to use Passed now that we have OnceCallback. If using BindOnce, we can use std::move instead (and this is probably more intuitive/readable for people new to Chromium). If using BindRepeating with Passed, the callback can only be called once, so BindOnce should be used instead. There may be some old APIs which take RepeatingCallbacks but promise to only call them once, and in that case using Passed is at the discretion of the CL author and reviewers, but refactoring the API to work with OnceCallback should be preferred. A previous similar discussion seems to arrive at the same conclusions: https://groups.google.com/a/chromium.org/forum/#!topic/cxx/KXWZtoP5HJ4.

I put a CL updating callback.md up for discussion at https://chromium-review.googlesource.com/c/chromium/src/+/919066.

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/abdff810-4359-41a5-8aa9-cd596c784bf5%40chromium.org.

[Gabriel Charette]

+1, thanks!

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/CAF3XrKoYTyCw-k6r51bTvjikmSJDk%3D3bETGP%2BMttgDWKaDzZ%3Dw%40mail.gmail.com.

[Claudio deSouza]

I have done some broader refactoring based on this suggestion... Waiting for someone to take it upon themselves to have a look on my changes.

[Avi Drissman]

So that you know, in general, people don't look at changes unsolicited. If you have made changes that you want to be reviewed, you need to send them out for review, and then they will be looked at.

Start with the people listed in the OWNERS file for the relevant directories.

(See http://dev.chromium.org/developers/contributing-code#TOC-Code-review .)

Avi

On Fri, Feb 16, 2018 at 8:53 AM, Claudio deSouza <claudi...@gmail.com> wrote:

I have done some broader refactoring based on this suggestion... Waiting for someone to take it upon themselves to have a look on my changes.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/4d454c44-8b1c-4f56-a067-36e8f48ce549%40chromium.org.

[Claudio deSouza]

Hello Avi,

This was sitting in crbug.com on the available category, so that's why I started working on it. I also put a comment on the ticket when I started working. Finally, I did add the owner of the request as the reviewer as soon as I submitted the code. Is there something I'm missing about what I'm supposed to do?

[Claudio deSouza]

Sorry, I misunderstood what you said, but yea, I did follow that process.

[Avi Drissman]

When you say you added the owner as a reviewer, did you click the "start review" button? If you only added them as a reviewer, they might say to themselves, "oh, I'm being added as a reviewer so that when the coder is done I'll be asked to do the review". It's important to explicitly send them the change for review, to make it clear that you're finished making the change and that you want a review.

Usually changes get reviewed in a few days. If it's been longer, you might not have actually sent it for review.

If you are sure you sent the change for review it is not impolite to follow up with the reviewer after a few days to make sure they are working on it.

Avi

On Fri, Feb 16, 2018 at 9:26 AM, Claudio deSouza <claudi...@gmail.com> wrote:

Hello Avi,

This was sitting in crbug.com on the available category, so that's why I started working on it. I also put a comment on the ticket when I started working. Finally, I did add the owner of the request as the reviewer as soon as I submitted the code. Is there something I'm missing about what I'm supposed to do?

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/29eccd40-058a-4d1b-a31d-e4b51b8924e1%40chromium.org.

[Avi Drissman]

Then follow up. You shouldn't be in the state of "oh, I sent the change to them but I'm waiting" for more than a few days.

On Fri, Feb 16, 2018 at 9:59 AM, Claudio deSouza <claudi...@gmail.com> wrote:

Sorry, I misunderstood what you said, but yea, I did follow that process.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-dev/0dd775a8-c05f-4cb2-9037-f896a20b9c03%40chromium.org.