Remove base::MessageLoop::{Quit,QuitClosure} functions

Remove base::MessageLoop::{Quit,QuitClosure} functions

This patch removes aliases for base::MessageLoop::QuitWhenIdle & base::MessageLoop::QuitWhenIdleClosure, and includes minor formatting changes made by using git cl-format.

BUG=131220
TEST=
R=danakj@chromium.org,brettw@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/94337f42ead062a7c7da18d611789ea5c4455b5f
Cr-Commit-Position: refs/heads/master@{#355763}

[ki.stfu, 2015-10-05 13:08:08]

Hi!

I checked that it works on Linux. Could you dry-run it
for checking on other platforms?

[achuithb, 2015-10-05 20:15:15]

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

[commit-bot: I haz the power, 2015-10-05 20:18:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390513002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390513002/1

[commit-bot: I haz the power, 2015-10-05 20:42:44]

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

[commit-bot: I haz the power, 2015-10-05 20:42:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[ki.stfu, 2015-10-06 11:15:29]

The CQ bit was checked by ki.stfu@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-06 11:16:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390513002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390513002/60001

[commit-bot: I haz the power, 2015-10-06 11:16:37]

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

[commit-bot: I haz the power, 2015-10-06 11:16:39]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[vabr (Chromium), 2015-10-06 13:04:01]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-10-06 13:04:02]

FWIW, files with "password" in the path LGTM.

Cheers,
vaclav

[ki.stfu, 2015-10-06 17:29:41]

The CQ bit was checked by ki.stfu@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-06 17:31:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390513002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390513002/60001

[commit-bot: I haz the power, 2015-10-06 17:43:44]

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

[commit-bot: I haz the power, 2015-10-06 17:43:46]

Dry run: This issue passed the CQ dry run.

[ki.stfu, 2015-10-07 08:18:08]

The CQ bit was checked by ki.stfu@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-07 08:19:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390513002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390513002/80001

[commit-bot: I haz the power, 2015-10-07 09:58:38]

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

[commit-bot: I haz the power, 2015-10-07 09:58:39]

Dry run: This issue passed the CQ dry run.

[ki.stfu, 2015-10-09 07:22:21]

ping

[vabr (Chromium), 2015-10-09 07:38:57]

On 2015/10/09 07:22:21, ki.stfu wrote:
> ping

It might be helpful to specify who you are pinging and what for, exactly.
Also note that 2 of your reviewers are likely still out of office.

Cheers,
Vaclav

[ki.stfu, 2015-10-09 07:43:15]

ki.stfu@gmail.com changed reviewers:
+ mark@chromium.org, thestig@chromium.org

[ki.stfu, 2015-10-09 07:45:50]

On 2015/10/09 07:38:57, vabr (Chromium) wrote:
> On 2015/10/09 07:22:21, ki.stfu wrote:
> > ping
> 
> It might be helpful to specify who you are pinging and what for, exactly.
> Also note that 2 of your reviewers are likely still out of office.
> 
> Cheers,
> Vaclav

mark@/thakis@/danakj@/thestig@: review please changes in base/message_loop/

[Lei Zhang, 2015-10-09 08:40:43]

Nobody really likes to review CLs with 300 files in them. Things also get hairy
when it takes many OWNERS to get all the approvals you need.

Since there's no dependencies here aside from the changes in base/, consider the
strategy of splitting this into smaller CLs. Reviewers will be more receptive to
smaller CLs, but don't go overboard in that direction and send a CL for each
single file. Balance is good.

- For instance, if you split chrome/ off into its own CL, you can send that to
me. (That's 150 files right there. I will grumble a little and then review it.)

- For each top level directory that has more than 10 changed files, split that
off into its own CL, and find a _single_ person from that directory's OWNERS
file as reviewer. There's usually no need for the shotgun approach of adding all
the OWNERS for a directory like you just did for base/.

- For top level directories that has 4 - 9 changed files, combine 2-3 top level
directories of that size into a CL, and find reviewers from those directories's
OWNERS files.

- For the few files left over, and base/ which has to be last, ask a base/ + top
level OWNER.

[ki.stfu, 2015-10-09 10:51:36]

Actually I planned to get an approval only from the owners of base/message_loop.

FMPOV, only base/message_loop.h contains meaningful changes and therefore I'm
interested in the approval from base/message_loop's owners. But for other files
- it's just a renaming Quit -> QuitWhenIdle and QuitClosure ->
QuitWhenIdleClosure. I don't think anyone except base/message_loop's owners can
say something reasonable about these changes (in particular, about renaming),
and I don't want to do >10 CLs for that. It's a waste of time and effort.

The Chromium guide says, that if I want to do something that will affect a large
number of files, I can tell about it to chromium-dev@ and, if there's no
objections, do a single CL for all changes. How about this?

[danakj, 2015-10-09 16:02:09]

base/ and cc/ LGTM anyhow.

[danakj, 2015-10-09 16:06:23]

On Fri, Oct 9, 2015 at 6:51 AM, ki.stfu@gmail.com via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> Actually I planned to get an approval only from the owners of
> base/message_loop.
>
> FMPOV, only base/message_loop.h contains meaningful changes and therefore
> I'm
> interested in the approval from base/message_loop's owners. But for other
> files
> - it's just a renaming Quit -> QuitWhenIdle and QuitClosure ->
> QuitWhenIdleClosure. I don't think anyone except base/message_loop's
> owners can
> say something reasonable about these changes (in particular, about
> renaming),
> and I don't want to do >10 CLs for that. It's a waste of time and effort.
>
> The Chromium guide says, that if I want to do something that will affect a
> large
> number of files, I can tell about it to chromium-dev@ and, if there's no
> objections, do a single CL for all changes. How about this?
>

I still think you should split it up. 300 files is too much to even load in
chrome with my rietveld extensions, it's just huge to even read. You can do
smaller patches elsewhere and cite the change in base, once you have
approval from a base owner for a small CL making that change. That's how I
would do it.


>
> https://codereview.chromium.org/1390513002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ki.stfu, 2015-10-09 17:02:11]

On 2015/10/09 16:06:23, danakj_OOO_til_10-12 wrote:
> On Fri, Oct 9, 2015 at 6:51 AM, mailto:ki.stfu@gmail.com via
> http://codereview.chromium.org
<mailto:reply@chromiumcodereview-hr.appspotmail.com> wrote:
> 
> > Actually I planned to get an approval only from the owners of
> > base/message_loop.
> >
> > FMPOV, only base/message_loop.h contains meaningful changes and therefore
> > I'm
> > interested in the approval from base/message_loop's owners. But for other
> > files
> > - it's just a renaming Quit -> QuitWhenIdle and QuitClosure ->
> > QuitWhenIdleClosure. I don't think anyone except base/message_loop's
> > owners can
> > say something reasonable about these changes (in particular, about
> > renaming),
> > and I don't want to do >10 CLs for that. It's a waste of time and effort.
> >
> > The Chromium guide says, that if I want to do something that will affect a
> > large
> > number of files, I can tell about it to chromium-dev@ and, if there's no
> > objections, do a single CL for all changes. How about this?
> >
> 
> I still think you should split it up. 300 files is too much to even load in
> chrome with my rietveld extensions, it's just huge to even read. You can do
> smaller patches elsewhere and cite the change in base, once you have
> approval from a base owner for a small CL making that change. That's how I
> would do it.
It will be 25 CLs in case of 1 CL per a top-level directory. That's a lot of
work :(

[Lei Zhang, 2015-10-09 17:22:57]

On 2015/10/09 17:02:11, ki.stfu wrote:
> It will be 25 CLs in case of 1 CL per a top-level directory. That's a lot of
> work :(

If you roughly follow my suggestion, but just double the numbers of files, this
comes out to less than 10 CLs.

It's not that hard to split this up. You have the power of git:

git diff master chrome > /tmp/chrome.patch
git checkout master
git checkout -b chrome_quit
patch -p1 < /tmp/chrome.patch
git commit ...
git cl upload

# repeat for (extensions + ipc), repeat for (remoting + ui) ...

> The Chromium guide says, that if I want to do something that will affect a
large
> number of files, I can tell about it to chromium-dev@ and, if there's no
> objections, do a single CL for all changes. How about this?

Really, you'll have a quicker time doing this in smaller chunks. It's like
eating 3 meals a day vs 1 big meal a day. You get the same number of calories,
but with less indigestion. Here, you are just causing reviewers indigestion and
we are telling you to avoid doing that.

[groby-ooo-7-16, 2015-10-09 18:14:50]

> It will be 25 CLs in case of 1 CL per a top-level directory. That's a lot of
> work :(

Yes. But if you don't break it up, you're simply offloading the work to your
reviewers. Please don't do that. 

Not only because it's hard to review something this large without eyes glazing
over, but because review time for changes increases quicker than the size of the
changes. You'll get things landed quicker if they're broken up to a reasonable
size.

And really, git makes it easy. Either Lei's way, or

# Put the entire patch in a stash so you can re-apply locally without messign up
issue numbers
git checkout -b quit_main master
git cl patch 1390513002
git stash save "The entire thing"

# Build a branch for a subdir, apply only that subdir
git checkout -b c_b_quit master
git add -- chrome/browser
git commit
git checkout -- .

# Repeat for other branches. 

You should be able to create your CLs in 15-30 minutes and be done with it. Your
reviewers will get them done quickly because they're small. (Bonus points if you
made the change with a script and share the script in the CL message )

[Lei Zhang, 2015-10-09 19:56:19]

For the benefit of others reviewers: You can download the patch, and locally
modify the patch with:

%s/QuitWhenIdleClosure/QuitClosure/g
%s/QuitWhenIdle/Quit/g

Apply the resulting patch, and look at the git diff output.

[ki.stfu, 2015-10-09 20:21:20]

On 2015/10/09 19:56:19, Lei Zhang wrote:
> For the benefit of others reviewers: You can download the patch, and locally
> modify the patch with:
> 
> %s/QuitWhenIdleClosure/QuitClosure/g
> %s/QuitWhenIdle/Quit/g
> 
> Apply the resulting patch, and look at the git diff output.

FWIW, unfortunately you will not see empty diff in this case, because sometimes
it contains  formatting changes.

[ki.stfu, 2015-10-09 20:27:04]

Btw, thank you all, guys!

[Lei Zhang, 2015-10-09 20:45:26]

On 2015/10/09 20:21:20, ki.stfu wrote:
> FWIW, unfortunately you will not see empty diff in this case, because
sometimes
> it contains  formatting changes.

Correct, and that's fine.

[ki.stfu, 2015-10-10 08:15:29]

ki.stfu@gmail.com changed reviewers:
+ ben@chromium.org
- achuith@chromium.org, jbates@chromium.org, mark@chromium.org,
thakis@chromium.org, thestig@chromium.org, vabr@chromium.org

[ki.stfu, 2015-10-10 08:49:44]

I have split PS6 on small pieces:
ash/, chromeos/, device/: https://codereview.chromium.org/1396083003/
extensions/, ipc/: https://codereview.chromium.org/1389163008/
chrome/: https://codereview.chromium.org/1395103003/
components/: https://codereview.chromium.org/1402553002/
content/: https://codereview.chromium.org/1398153002/
media/: https://codereview.chromium.org/1396113004/
mojo/: https://codereview.chromium.org/1402533003/
net/: https://codereview.chromium.org/1398973003/
remoting/: https://codereview.chromium.org/1394803004/
ui/: https://codereview.chromium.org/1402543002/

The other changes were left in current CL: see PS7.

danakj@: review changes in base/
ben@: please review this CL entirely

We will be ready to merge the this CL when all listed CLs are merged.

[danakj, 2015-10-12 21:05:50]

On 2015/10/10 08:49:44, ki.stfu wrote:
> I have split PS6 on small pieces:
> ash/, chromeos/, device/: https://codereview.chromium.org/1396083003/
> extensions/, ipc/: https://codereview.chromium.org/1389163008/
> chrome/: https://codereview.chromium.org/1395103003/
> components/: https://codereview.chromium.org/1402553002/
> content/: https://codereview.chromium.org/1398153002/
> media/: https://codereview.chromium.org/1396113004/
> mojo/: https://codereview.chromium.org/1402533003/
> net/: https://codereview.chromium.org/1398973003/
> remoting/: https://codereview.chromium.org/1394803004/
> ui/: https://codereview.chromium.org/1402543002/
> 
> The other changes were left in current CL: see PS7.
> 
> danakj@: review changes in base/

It still LGTM

> ben@: please review this CL entirely
> 
> We will be ready to merge the this CL when all listed CLs are merged.

[ki.stfu, 2015-10-13 13:06:23]

On 2015/10/12 21:05:50, danakj wrote:
> On 2015/10/10 08:49:44, ki.stfu wrote:
> > I have split PS6 on small pieces:
> > ash/, chromeos/, device/: https://codereview.chromium.org/1396083003/
> > extensions/, ipc/: https://codereview.chromium.org/1389163008/
> > chrome/: https://codereview.chromium.org/1395103003/
> > components/: https://codereview.chromium.org/1402553002/
> > content/: https://codereview.chromium.org/1398153002/
> > media/: https://codereview.chromium.org/1396113004/
> > mojo/: https://codereview.chromium.org/1402533003/
> > net/: https://codereview.chromium.org/1398973003/
> > remoting/: https://codereview.chromium.org/1394803004/
> > ui/: https://codereview.chromium.org/1402543002/
> > 
> > The other changes were left in current CL: see PS7.
> > 
> > danakj@: review changes in base/
> 
> It still LGTM
> 
> > ben@: please review this CL entirely
> > 
> > We will be ready to merge the this CL when all listed CLs are merged.

ben@: the listed CLs were landed. Could you review this CL?

[ki.stfu, 2015-10-22 19:15:02]

ki.stfu@gmail.com changed reviewers:
+ brettw@chromium.org

[ki.stfu, 2015-10-22 19:15:03]

brettw@: could review this CL please?

[brettw, 2015-10-23 02:15:20]

lgtm

[ki.stfu, 2015-10-23 03:54:14]

Description was changed from

==========
Remove base::MessageLoop::{Quit,QuitClosure} functions

This patch removes aliases for base::MessageLoop::QuitWhenIdle &
base::MessageLoop::QuitWhenIdleClosure, and includes minor formatting changes
made by using git cl-format. 

BUG=131220
TEST=
R=danakj@chromium.org,ben@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Remove base::MessageLoop::{Quit,QuitClosure} functions

This patch removes aliases for base::MessageLoop::QuitWhenIdle &
base::MessageLoop::QuitWhenIdleClosure, and includes minor formatting changes
made by using git cl-format. 

BUG=131220
TEST=
R=danakj@chromium.org,brettw@chromium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ki.stfu, 2015-10-23 03:54:14]

ki.stfu@gmail.com changed reviewers:
- ben@chromium.org

[ki.stfu, 2015-10-23 03:54:30]

The CQ bit was checked by ki.stfu@gmail.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-23 03:54:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390513002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390513002/120001

[commit-bot: I haz the power, 2015-10-23 07:06:52]

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

[commit-bot: I haz the power, 2015-10-23 07:06:53]

Dry run: This issue passed the CQ dry run.

[ki.stfu, 2015-10-23 11:17:58]

The CQ bit was checked by ki.stfu@gmail.com

[ki.stfu, 2015-10-23 11:17:59]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1390513002/#ps120001
(title: "Split into small CLs")

[commit-bot: I haz the power, 2015-10-23 11:18:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1390513002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1390513002/120001

[commit-bot: I haz the power, 2015-10-23 11:24:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-23 11:25:23]

Patchset 7 (id:??) landed as
https://crrev.com/94337f42ead062a7c7da18d611789ea5c4455b5f
Cr-Commit-Position: refs/heads/master@{#355763}