PSA: base::ObserverList FOR_EACH_OBSERVER macro and GetNext method are deprecated

[Alexey]

Hi!

Please, use the range-based for loop instead.

Example:

FOR_EACH_OBSERVER(CompositorObserver, observer_list_,

                    OnCompositingShuttingDown(this));

should be replaced with:

for (auto& observer : observer_list_)

  observer.OnCompositingShuttingDown(this);

Note that some places use GetNext method directly. So

base::ObserverListBase<RenderViewObserver>::Iterator it(&observers_);

RenderViewObserver* observer;

while ((observer = it.GetNext()) != NULL)

  if (observer->OnMessageReceived(message))

    return true;

should become:

for (auto& observer : observers_) {

  if (observer.OnMessageReceived(message))

    return true;

}

  

Alexey.

[Lei Zhang]

There are nearly 2K instances of FOR_EACH_OBSERVER. Do you have a plan
to convert them? Left on their own, the deprecated code will continue
to proliferate because people will oftentimes copy existing code and
paste them to create new code.

> --
> --
> Chromium Developers mailing list: chromi...@chromium.org
> View archives, change email options, or unsubscribe:
> http://groups.google.com/a/chromium.org/group/chromium-dev

[Daniel Cheng]

I see removal of GetNext() as the highest priority work item here, and it actually looks relatively easy. Codesearch sees fewer than 70 references to it as of now, and I doubt there are that many more in platform-specific code.

Once we do get rid of GetNext(), we can remove FOR_EACH_OBSERVER instances more gradually (sharding this work out would probably help expedite it; each individual instance is pretty easy to fix up, but fixing 2000 instances by yourself probably isn't so fun).

Daniel

[Elliott Sprehn]

Can we add a presubmit that prevents new usage of the macro?

[Vaibhav Pithadiya]

DO NOT MAIL ME AGAIN PLEASE .. 

.

DO NOT DISTURB

.

THANK YOU

You received this message because you are subscribed to the Google Groups "Chromium-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[PhistucK]

You are subscribed to a mailing list. To unsubscribe -

Try sending a blank e-mail to chromium-dev...@chromium.org (note that +unsubscribe in the address, it is important). If you already did exactly that, skip this.

If that does not work and you are still getting e-mails -

1. Go to https://groups.google.com/a/chromium.org/forum/m/#!forum/chromium-dev.

2. Click on the button that has an icon of a head and shoulders.

3. Click on "Leave group" and you are done. Or -

 a. Select in the selection box - "Don't send email updates".

 b. Click on "Save".

☆PhistucK

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

--
--
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 unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev+unsubscribe@chromium.org.

[Václav Brožek]

On Wed, 12 Oct 2016 at 07:13 Daniel Cheng <dch...@chromium.org> wrote:

I see removal of GetNext() as the highest priority work item here, and it actually looks relatively easy. Codesearch sees fewer than 70 references to it as of now, and I doubt there are that many more in platform-specific code.

Once we do get rid of GetNext(), we can remove FOR_EACH_OBSERVER instances more gradually (sharding this work out would probably help expedite it; each individual instance is pretty easy to fix up, but fixing 2000 instances by yourself probably isn't so fun).

For people who want to help out and convert code they own, is there a tracking bug to which to associate those CLs?

Cheers,

Vaclav

[Daniel Cheng]

I filed https://bugs.chromium.org/p/chromium/issues/detail?id=655021 earlier for this (and https://codereview.chromium.org/2417513002/ is the presubmit which is in review).

Daniel

[Anatoly Pilikov]

Hello guys!

The suggestion to replace old macro with plain for-loop seems weird to me because:

1. someone has to compact list after notification is finished (doesn't matter how many nested notifications we have) because there could be observers that have left the list as the result of a notification.

2. someone has to check if an observer is null in the list. It can be null because a notification can result in removing some of the observers out of the list while it is being looped.

Plain for-loop does none of these. Am I missing something?

Thank you.

среда, 12 октября 2016 г., 11:37:20 UTC+3 пользователь Daniel Cheng написал:

[Anatoly Pilikov]

Thank you Daniel.

 

I missed your recent refactoring. Now I looked into the current implementation and was satisfied. Good job, thank you.

 

12.10.2016, 19:24, "Daniel Cheng" <dch...@chromium.org>:

On Wed, Oct 12, 2016 at 9:19 AM Anatoly Pilikov <zve...@yandex-team.ru> wrote:

Hello guys!

 

The suggestion to replace old macro with plain for-loop seems weird to me because:

1. someone has to compact list after notification is finished (doesn't matter how many nested notifications we have) because there could be observers that have left the list as the result of a notification.

 

The compaction still happens during the destruction of the outermost ObserverList::Iter.

 

2. someone has to check if an observer is null in the list. It can be null because a notification can result in removing some of the observers out of the list while it is being looped.

 

ObserverList::Iter::operator++ knows to skip nulled out elements when advancing: it's no different from GetNext() in this respect.

 

Plain for-loop does none of these. Am I missing something?

 

Under the hood, a range-based for loop generates code that is equivalent to using ObserverList::Iter, so we get the correct behaviors.

 

Daniel

 

 

-- 
Anatoly Pilikov

Yandex

 

[Daniel Cheng]

On Wed, Oct 12, 2016 at 9:19 AM Anatoly Pilikov <zve...@yandex-team.ru> wrote:

Hello guys!

The suggestion to replace old macro with plain for-loop seems weird to me because:

1. someone has to compact list after notification is finished (doesn't matter how many nested notifications we have) because there could be observers that have left the list as the result of a notification.

The compaction still happens during the destruction of the outermost ObserverList::Iter.

 

2. someone has to check if an observer is null in the list. It can be null because a notification can result in removing some of the observers out of the list while it is being looped.

ObserverList::Iter::operator++ knows to skip nulled out elements when advancing: it's no different from GetNext() in this respect.

 

Plain for-loop does none of these. Am I missing something?

Under the hood, a range-based for loop generates code that is equivalent to using ObserverList::Iter, so we get the correct behaviors.

Daniel

 

[Alexey]

Sure that we after complete and utter eradication of that macro :)

We'll do that on a per-folder basis with a help of owners (no rush).

Please, post your intents to help in https://bugs.chromium.org/p/chromium/issues/detail?id=655021

to avoid collisions.

[Daniel Cheng]

I threw together a simple spreadsheet with instructions to help track this work: https://docs.google.com/spreadsheets/d/14u7dpq7NcBTtQj3hhEOrnA5Eq3Q9N_ON4w9pLCLC2cI/edit?usp=sharing

I tried to split up the work into reasonable sized chunks, but please consider the current split to just be a guideline: if a chunk of work seems too large, feel free to add a new row and split off a CL.

Daniel

[Alexey]

And... Done!

FOR_EACH_OBSERVER macro has been erased.

Big thanks to ericwilligers@, tfarina@, yusukes@, vabr@, dcheng@ and all the reviewers!

Alexey.

[Lei Zhang]

There are nearly 2K... what the? Where did they all go? ;) Congrats!

[Yuchen Liu]

Same question here. Is there a script to do this?

[Daniel Cheng]

Yes, the details are in the linked spreadsheet (https://docs.google.com/spreadsheets/d/14u7dpq7NcBTtQj3hhEOrnA5Eq3Q9N_ON4w9pLCLC2cI/edit?usp=sharing), but it's just a matter of using perl to expand the macro, followed by git cl format to fix the formatting.

Daniel