Forward declared types in base::ScopedObservation<>

[Andrew Rayskiy]

Hello :)

base::ScopedObservation<> is a very handy helper that is unfortunately designed in such a way that Add/Remove observer functions are set as part of the template. Sometimes it comes in handy (when function names are different), but most of the time it just blocks the ability to use it with forward-declared types and forces the developer to add unnecessary includes.

It's relatively easy to allow forward-declared sources with standard function names (i.e. when they define AddObserver / RemoveObserver) -- just move these out of the template into Observe / Reset accordingly. Are there any cons to this? (And is it even possible to create this specialization and preserve the original class name, i.e. keep `ScopedObservation<Source, Observer, func, func>` and `ScopedObservation<Source, Observation>` separately?)

Thanks,

Andrew

[Alan Cutter]

I don't think it would be possible to move Observe/Reset into a separate CC file as their machine code implementation depends on the template parameter.

[Daniel Cheng]

It's possible to delay it so it can be forward declared in the header with something like https://godbolt.org/z/zz7Wfxz8M. If you uncomment the call to RegisterObserver() line, it will stop compiling—but usually that will be in the .cc file.

There are probably other, potentially better, ways; I just threw something together quickly as an example.

Daniel

--
You received this message because you are subscribed to the Google Groups "cxx" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cxx+uns...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/cxx/3c08133f-be5a-402d-95ed-6bdb30ff4e4en%40chromium.org.

[Andrew Rayskiy]

Yes, I had something similar in my mind (modulo template specializations to retain the existing behaviour for cases with deviating function names) -- crrev/c/3990749 :) I'd appreciate any feedback on this!

-- Andrew

[Jeroen Dhollander]

Sorry for the late feedback, but looking at the fix CL (https://crrev.com/c/4002909) I'm not sure I like the solution, so I'd like to propose an alternative solution.

The currently proposed solution changes this

```

namespace my_namespace {

class MyParentClass {

    private:

          base::ScopedObservation<LinuxUi,

                          CursorThemeManagerObserver,
                          &LinuxUi::AddCursorThemeObserver,
                          &LinuxUi::RemoveCursorThemeObserver>
              cursor_theme_observation_{this};

};

} // namespace my_namespace 

```

into the much more unwieldy and harder to write

```

// Must be in namespace base!

namespace base {

template <>
struct ScopedObservationTraits<ui::LinuxUi, ui::CursorThemeManagerObserver> {
  static void AddObserver(ui::LinuxUi* source,
                          ui::CursorThemeManagerObserver* observer) {
    source->AddCursorThemeObserver(observer);
  }
  static void RemoveObserver(ui::LinuxUi* source,
                             ui::CursorThemeManagerObserver* observer) {
    source->RemoveCursorThemeObserver(observer);
  }
};

}  // namespace base

namespace my_namespace {

class MyParentClass {

    private:

          base::ScopedObservation<LinuxUi, CursorThemeManagerObserver,>

              cursor_theme_observation_{this};

};

} // namespace my_namespace 

```

Which looks like a serious regression to me.

So what if we simply create 2 different templated classes:

   * `ScopedObservation`, which can be used for any class that has `AddObserver()`/`RemoveObserver()` methods, and which allows the observer class to be forward declared, and

   * `CustomScopedObservation`, which allows custom method names but doesn't allow the observer class to be forward declared (which makes this exactly the same as the old `ScopedObservation` class).

This changes the above example back to

```

namespace my_namespace {

class MyParentClass {

    private:

          base::CustomScopedObservation<LinuxUi,

                          CursorThemeManagerObserver,
                          &LinuxUi::AddCursorThemeObserver,
                          &LinuxUi::RemoveCursorThemeObserver>
              cursor_theme_observation_{this};

};

} // namespace my_namespace 

```

at the small(ish) cost of some one time duplication (the `CustomScopedObservation` class, which costs about 22 lines of code (44 if you add in all the comments and closing `}`).

WDYT?

With regards,

Jeroen

[Alan Cutter]

Having two distinct names SGTM. It would be good to see the ScopedObservationTraits variadic template removed.

[Daniel Cheng]

The traits version scales better if there are multiple observers that need the custom methods, rather than requiring all the observers to specify the custom callback functions.

I don't think it's strictly about minimizing LoC.

Daniel

[Jeroen Dhollander]

I don't think the traits version necessarily scales better, as a common approach seemed to have been to add a `using` statement in a header. You can see plenty of examples of this in https://crrev.com/c/4002909:

```

// in header:

using ScopedAssistantClientObserver = base::ScopedObservation<
    ServiceController,
    AssistantClientObserver,
    &ServiceController::AddAndFireAssistantClientObserver,
    &ServiceController::RemoveAssistantClientObserver>;

```

This again makes implementing multiple observers of the same type a breeze.

And I agree it's not about minimizing LoC but about readability, ease of use and ease of implementation, and I feel that the traits version scores worse in at least 2 of these 3 categories (header becomes less readable and implementation requires a 'weird' traits struct which you can't even put in your normal namespaces).

I'm really worried this change is going to push people away from using a ScopedObservation and back to manually adding/removing their observers.

Jeroen

[Daniel Cheng]

It's certainly true that writing the traits is more obtuse. However, it's a one-time cost and it's also not hard to avoid the traits: just don't use custom observer methods.

Introducing a type alias requires people to know about it rather than just consistently using ScopedObservation<Source, Observer>.

Daniel

[Andrew Rayskiy]

From my empirical research non-compliant function names are quite rare (in comparison with AddObserver/RemoveObserver) and therefore shouldn’t be too heavy of a burden. As for using declarations, those were commonly found in source headers as standalone declarations, but in actual code people preferred to write them down in full (mostly because they weren’t aware of those things). 

How do we feel about macros, on a side note? This specialization could be conveniently packed into something like SPECIALIZE_SCOPED_OBSERVATION_TRAITS(Siurce, Observer, AddFoo, RemoveFoo) if LoC is a concern.

[Peter Kasting]

Looking at the linked CL, I have to say this feels like a loss. There's a lot more boilerplate overall as most observer classes are only used in a small number of places, so the traits class outweighs the saved extra template args; and the extra boilerplate is harder to read. I think it is also harder to figure out how to use such a class if another class isn't already using the one you want. You need to go specialize a traits class not in your own code but in the header for the observer you're trying to use.

I wish we had not moved forward with this. Looking at this, Jeroen's proposal, and the original code, I prefer the original code. It is also possible to imagine other solutions, such as passing optional constructor args that are pointers-to-members to the add/remove functions, but that seems slightly worse than the original solution too.

Can we revert?

PK

[Daniel Cheng]

I think most classes should just not use non-default observer methods. Using fewer custom observer methods would be a happy outcome to me and would be less complex overall.

There are nearly 2000 files with references to ScopedObservation, and ~150 files were changed due to the traits change, with 30 traits implementations added. There's ~200 references  to ScopedObservation in the side-by-side diff, so let's say there's ~100 ScopedObservation instances that require traits. That implies there are probably a fair number of instances where the Traits is only used once, but then there are also instances where the Traits are reused quite a few times.

To be perfectly honest, I really dislike the focus on forward declaring all the things and would prefer we just include the full header; then I suppose all of this discussion would be moot. :)

Daniel