Skip to content

Commit

Permalink
Retire ScopedObserver in *.md.
Browse files Browse the repository at this point in the history
ScopedObserver is being deprecated in favor of two new classes:
- base::ScopedObservation for observers that only ever observe
  a single source.
- base::ScopedMultiSourceObservation for observers that do or may
  observe more than a single source.

Bug: 1145565
Change-Id: I025c150c0970d4258b8f25b7ec0c2d959866e8a6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2911213
Auto-Submit: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#885198}
  • Loading branch information
sigurasg authored and Chromium LUCI CQ committed May 20, 2021
1 parent ac1913b commit fb9a9f7
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 36 deletions.
4 changes: 2 additions & 2 deletions docs/patterns/inversion-of-control.md
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ They also have disadvantages:
Some key classes in `//base`:
* [ScopedObserver]
* [base::ScopedObservation]
* [ObserverList] and [CheckedObserver]
* [Subscription] and [CallbackList]
Expand Down Expand Up @@ -394,7 +394,7 @@ should be a property on the framework object instead of a delegate method.
[Callback]: ../../base/callback.h
[CheckedObserver]: ../../base/observer_list_types.h
[ObserverList]: ../../base/observer_list.h
[ScopedObserver]: ../../base/scoped_observer.h
[base::ScopedObservation]: ../../base/scoped_observation.h
[Subscription]: ../../base/callback_list.h
[URLRequestJobFactory::ProtocolHandler]: ../../net/url_request/url_request_job_factory.h
[Unretained]: ../../base/bind.h
Expand Down
12 changes: 6 additions & 6 deletions docs/patterns/synchronous-runloop.md
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ class GizmoReadyWaiter : public GizmoObserver {
void WaitForGizmoReady() {
if (!gizmo_->ready()) {
gizmo_observer_.Add(gizmo_);
gizmo_observation_.Observe(gizmo_);
run_loop_.Run();
}
}
Expand All @@ -162,7 +162,7 @@ class GizmoReadyWaiter : public GizmoObserver {
private:
RunLoop run_loop_;
Gizmo* gizmo_;
ScopedObserver<Gizmo, GizmoObserver> gizmo_observer_{this};
base::ScopedObservation<Gizmo, GizmoObserver> gizmo_observation_{this};
};
```

Expand Down Expand Up @@ -311,21 +311,21 @@ class GizmoReadyWaiter : public GizmoObserver {
ASSERT_TRUE(gizmo_)
<< "Trying to call Wait() after the Gizmo was destroyed!";
if (!gizmo_->ready()) {
gizmo_observer_.Add(gizmo_);
gizmo_observation_.Observe(gizmo_);
run_loop_.Run();
}
}

// GizmoObserver:
void OnGizmoReady(Gizmo* observed_gizmo) {
gizmo_observer_.Remove(observed_gizmo);
gizmo_observation_.Reset();
run_loop_.Quit();
}
void OnGizmoDestroying(Gizmo* observed_gizmo) {
DCHECK_EQ(gizmo_, observed_gizmo);
gizmo_ = nullptr;
// Remove the observer now, to avoid a UAF in the destructor.
gizmo_observer_.Remove(observed_gizmo);
gizmo_observation_.Reset();
// Bail out so we don't time out in the test waiting for a ready state
// that will never come.
run_loop_.Quit();
Expand All @@ -336,7 +336,7 @@ class GizmoReadyWaiter : public GizmoObserver {
private:
RunLoop run_loop_;
Gizmo* gizmo_;
ScopedObserver<Gizmo, GizmoObserver> gizmo_observer_{this};
base::ScopedObservation<Gizmo, GizmoObserver> gizmo_observation_{this};
};
```
Expand Down
46 changes: 23 additions & 23 deletions docs/ui/learn/bestpractices/ownership.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ Accordingly, the best practices for ownership and lifetime management in Views
code are similar to those in the rest of Chromium, but include some details
specific to the Views APIs.

* **[Avoid bare new.](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++-dos-and-donts.md#use-and-instead-of-bare)**
* **[Avoid bare new.](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++-dos-and-donts.md#use-and-instead-of-bare)**
Use `std::make_unique<>()` when creating objects, and distinguish owning and
non-owning uses by [using smart and raw pointer types](https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md#object-ownership-and-calling-conventions).
* **Use [the unique\_ptr<> version of View::AddChildView<>()](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=404;drc=7cba41605a8489bace83f380760486638a2a8a4a).**
non-owning uses by [using smart and raw pointer types](https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/c++.md#object-ownership-and-calling-conventions).
* **Use [the unique\_ptr<> version of View::AddChildView<>()](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=404;drc=7cba41605a8489bace83f380760486638a2a8a4a).**
This clearly conveys ownership transfer from the caller to the parent `View`.
It also `DCHECK()`s that `set_owned_by_client()` has not been called,
preventing double-ownership.
Expand All @@ -28,7 +28,7 @@ specific to the Views APIs.

**Avoid**

Typical code from [`dice_bubble_sync_promo_view.cc`](https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/sync/dice_bubble_sync_promo_view.cc;l=49;drc=459f2d16a6592b1290a1c2197231d46f48f49af4)
Typical code from [`dice_bubble_sync_promo_view.cc`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/sync/dice_bubble_sync_promo_view.cc;l=49;drc=459f2d16a6592b1290a1c2197231d46f48f49af4)
using bare `new`:

#####
Expand Down Expand Up @@ -90,7 +90,7 @@ signin_button_view_ =

## Avoid View::set_owned_by_client()

The [`View::set_owned_by_client()`](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=390;drc=7cba41605a8489bace83f380760486638a2a8a4a)
The [`View::set_owned_by_client()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=390;drc=7cba41605a8489bace83f380760486638a2a8a4a)
flag means that a `View` is owned by something other than its parent `View`.
This **method is deprecated** (and will eventually be removed) since it results
in APIs that are easy to misuse, code where ownership is unclear, and a higher
Expand All @@ -104,7 +104,7 @@ would allow a long-lived "model" or "controller" with more-transient "view"s.

**Avoid**

Code in [`time_view.{h,cc}`](https://source.chromium.org/chromium/chromium/src/+/master:ash/system/time/time_view.h;l=35;drc=7d8bc7f807a433e6a127806e991fe780aa27ce77)
Code in [`time_view.{h,cc}`](https://source.chromium.org/chromium/chromium/src/+/main:ash/system/time/time_view.h;l=35;drc=7d8bc7f807a433e6a127806e991fe780aa27ce77)
that uses `set_owned_by_client()` to have non-parented `View`s, so it can swap
layouts without recreating the children:

Expand Down Expand Up @@ -278,7 +278,7 @@ Old code in `cast_dialog_no_sinks_view.{h,cc}` that used weak pointers to

**Best practice**

[Current version](https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/media_router/cast_dialog_no_sinks_view.h;l=20;drc=2a147d67e51e67144257d3a405e3aafec3827513)
[Current version](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/media_router/cast_dialog_no_sinks_view.h;l=20;drc=2a147d67e51e67144257d3a405e3aafec3827513)
eliminates lifetime concerns by using a `OneShotTimer`, which is canceled when
destroyed:

Expand Down Expand Up @@ -336,7 +336,7 @@ CastDialogNoSinksView::CastDialogNoSinksView(

## Use View::RemoveChildViewT<>()

[`View::RemoveChildViewT<>()`](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=443;drc=7cba41605a8489bace83f380760486638a2a8a4a)
[`View::RemoveChildViewT<>()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=443;drc=7cba41605a8489bace83f380760486638a2a8a4a)
**clearly conveys ownership transfer** from the parent `View` to the caller.
Callers who wish to delete a `View` can simply ignore the return argument. This
is preferable to calling `RemoveChildView()` and deleting the raw pointer
Expand All @@ -351,7 +351,7 @@ future).

**Avoid**

Typical code in [`network_list_view.cc`](https://source.chromium.org/chromium/chromium/src/+/master:ash/system/network/network_list_view.cc;l=296;drc=4867a6ce87c51024259259baad08b0ba8ae030a5)
Typical code in [`network_list_view.cc`](https://source.chromium.org/chromium/chromium/src/+/main:ash/system/network/network_list_view.cc;l=296;drc=4867a6ce87c51024259259baad08b0ba8ae030a5)
which manually deletes a child after removing it:

#####
Expand Down Expand Up @@ -395,9 +395,9 @@ Rewriting using `RemoveChildViewT<>()` is shorter and safer:
## Prefer scoping objects

**Prefer scoping objects to paired Add/Remove-type calls**. For example, use
a [`ScopedObserver<>`](https://source.chromium.org/chromium/chromium/src/+/master:base/scoped_observer.h;l=43;drc=6bd38d45dbd4144235318b607216e51a7f9dc60e)
instead of directly calling [`View::AddObserver()`](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1346;drc=7cba41605a8489bace83f380760486638a2a8a4a)
and [`RemoveObserver()`](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1347;drc=7cba41605a8489bace83f380760486638a2a8a4a).
a [`base::ScopedObservation<>`](https://source.chromium.org/chromium/chromium/src/+/main:base/scoped_observation.h;l=49;drc=12993351a415619a445a836ca8a94a6310569dbd)
instead of directly calling [`View::AddObserver()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=1346;drc=7cba41605a8489bace83f380760486638a2a8a4a)
and [`RemoveObserver()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=1347;drc=7cba41605a8489bace83f380760486638a2a8a4a).
Such objects reduce the likelihood of use-after-free.

|||---|||
Expand All @@ -406,14 +406,14 @@ Such objects reduce the likelihood of use-after-free.

**Avoid**

Typical code in [`avatar_toolbar_button.cc`](https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/profiles/avatar_toolbar_button.cc;l=50;drc=f81f0d5d17c037c5866b8808322931f313b796e1)
Typical code in [`avatar_toolbar_button.cc`](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/views/profiles/avatar_toolbar_button.cc;l=50;drc=f81f0d5d17c037c5866b8808322931f313b796e1)
that uses paired add/remove calls:

#####

**Best practice**

Rewriting using `ScopedObserver<>` eliminates the destructor body entirely:
Rewriting using `base::ScopedObservation<>` eliminates the destructor body entirely:

|||---|||

Expand Down Expand Up @@ -454,17 +454,17 @@ class AvatarToolbarButton
public ToolbarIconContainerView::Observer {
...
private:
ScopedObserver<AvatarToolbarButton,
base::ScopedObservation<AvatarToolbarButton,
ToolbarIconContainerView::Observer>
observer_{this};
observation_{this};
};
AvatarToolbarButton::AvatarToolbarButton(
Browser* browser, ToolbarIconContainerView* parent)
: browser_(browser), parent_(parent) {
...
if (parent_)
observer_.Add(parent_);
observation_.Observe(parent_);
}
AvatarToolbarButton::~AvatarToolbarButton() = default;
Expand Down Expand Up @@ -495,7 +495,7 @@ creation:

**Best practice**

[Current version](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/widget/widget_interactive_uitest.cc;l=492;drc=cbe5dca6cb1e1511c82776370a964d99cbf5205d)
[Current version](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/widget/widget_interactive_uitest.cc;l=492;drc=cbe5dca6cb1e1511c82776370a964d99cbf5205d)
uses scoping objects that are simpler and safer:

|||---|||
Expand Down Expand Up @@ -537,19 +537,19 @@ TEST_F(WidgetTestInteractive,
## Only use Views objects on the UI thread

**Always use `View`s on the main (UI) thread.** Like most Chromium code, the
Views toolkit is [not thread-safe](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=170;drc=7cba41605a8489bace83f380760486638a2a8a4a).
Views toolkit is [not thread-safe](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=170;drc=7cba41605a8489bace83f380760486638a2a8a4a).

## Add child Views in a View's constructor

In most cases, **add child `View`s in a `View`'s constructor.** From an
ownership perspective, doing so is safe even though the `View` is not yet in a
`Widget`; if the `View` is destroyed before ever being placed in a `Widget`, it
will still destroy its child `View`s. Child `View`s may need to be added at
other times (e.g. in [`AddedToWidget()`](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1439;drc=7cba41605a8489bace83f380760486638a2a8a4a)
or [`OnThemeChanged()`](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1545;drc=7cba41605a8489bace83f380760486638a2a8a4a),
other times (e.g. in [`AddedToWidget()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=1439;drc=7cba41605a8489bace83f380760486638a2a8a4a)
or [`OnThemeChanged()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=1545;drc=7cba41605a8489bace83f380760486638a2a8a4a),
if constructing the `View` requires a color; or lazily, if creation is expensive
or a `View` is not always needed); however, do not copy any existing code that
adds child `View`s in a [`ViewHierarchyChanged()`](https://source.chromium.org/chromium/chromium/src/+/master:ui/views/view.h;l=1422;drc=7cba41605a8489bace83f380760486638a2a8a4a)
adds child `View`s in a [`ViewHierarchyChanged()`](https://source.chromium.org/chromium/chromium/src/+/main:ui/views/view.h;l=1422;drc=7cba41605a8489bace83f380760486638a2a8a4a)
override, as such code is usually an artifact of misunderstanding the Views
ownership model.

Expand All @@ -559,7 +559,7 @@ ownership model.

**Avoid**

Typical code in [`native_app_window_views.cc`](https://source.chromium.org/chromium/chromium/src/+/master:extensions/components/native_app_window/native_app_window_views.cc;l=285;drc=d66100ebd950c39b80cfd1c72cec618b33eb3491)
Typical code in [`native_app_window_views.cc`](https://source.chromium.org/chromium/chromium/src/+/main:extensions/components/native_app_window/native_app_window_views.cc;l=285;drc=d66100ebd950c39b80cfd1c72cec618b33eb3491)
that sets up a child `View` in `ViewHierarchyChanged()`:

#####
Expand Down
9 changes: 4 additions & 5 deletions docs/webui_explainer.md
Original file line number Diff line number Diff line change
Expand Up @@ -596,14 +596,13 @@ A safer way to set up communication is:
```c++
class MyHandler : public content::WebUIMessageHandler {
public:
MyHandler() : observer_(this) {}
void OnJavascriptAllowed() override {
observer_.Add(GetGlobalService()); // <-- DO THIS.
observation_.Observe(GetGlobalService()); // <-- DO THIS.
}
void OnJavascriptDisallowed() override {
observer_.RemoveAll(); // <-- AND THIS.
observation_.Reset(); // <-- AND THIS.
}
ScopedObserver<MyHandler, GlobalService> observer_; // <-- ALSO HANDY.
base::ScopedObservation<MyHandler, GlobalService> observation_{this}; // <-- ALSO HANDY.
```
when a renderer has been created and the
document has loaded enough to signal to the C++ that it's ready to respond to
Expand Down Expand Up @@ -635,7 +634,7 @@ Often, it makes sense to disconnect from observers in

```c++
void OvenHandler::OnJavascriptDisallowed() {
scoped_oven_observer_.RemoveAll()
scoped_oven_observation_.Reset()
}
```

Expand Down

0 comments on commit fb9a9f7

Please sign in to comment.