ownership.md

Best practice: ownership

In the common case, a View instance lives within a hierarchy of Views, up to a RootView object that is owned by a Widget. Calling AddChildView<>() typically passes ownership of the child to the parent, while RemoveChildView[T<>]() gives ownership back to the caller. While other ownership patterns exist, newly-added code should use this one.

[TOC]

Lifetime basics

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. Use std::make_unique<>() when creating objects, and distinguish owning and non-owning uses by using smart and raw pointer types.
• Use the unique_ptr<> version of View::AddChildView<>(). 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.

|||---|||

Avoid

Typical code from dice_bubble_sync_promo_view.cc using bare new:

Best practice

Rewriting using the unique_ptr<> version of AddChildView<>() is shorter and safer:

|||---|||

|||---|||

...
views::Label* title =
    new views::Label(
        title_text, views::style::CONTEXT_DIALOG_BODY_TEXT,
        text_style);

title->SetHorizontalAlignment(
    gfx::HorizontalAlignment::ALIGN_LEFT);
title->SetMultiLine(true);
AddChildView(title);
...
signin_button_view_ =
    new DiceSigninButtonView(
        account, account_icon, this,
        /*use_account_name_as_title=*/true);
AddChildView(signin_button_view_);
...

...
auto* title =
    AddChildView(
        std::make_unique<views::Label>(
            title_text, views::style::CONTEXT_DIALOG_BODY_TEXT,
            text_style));
title->SetHorizontalAlignment(
    gfx::HorizontalAlignment::ALIGN_LEFT);
title->SetMultiLine(true);

...
signin_button_view_ =
    AddChildView(
        std::make_unique<DiceSigninButtonView>(
            account, account_icon, this,
            /*use_account_name_as_title=*/true));
...

|||---|||

Avoid View::set_owned_by_client()

The View::set_owned_by_client() 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 likelihood of bugs. Needing this flag may be a signal that the View subclass in question is too heavyweight, and refactoring using MVC or a similar paradigm would allow a long-lived "model" or "controller" with more-transient "view"s.

|||---|||

Avoid

Code in time_view.{h,cc} that uses set_owned_by_client() to have non-parented Views, so it can swap layouts without recreating the children:

Best practice

Rewriting using subclasses to encapsulate layout allows the parent to merely adjust visibility:

|||---|||

|||---|||

class ASH_EXPORT TimeView : public ActionableView,
                            public ClockObserver {
  ...
 private:



  std::unique_ptr<views::Label> horizontal_label_;
  ...
  std::unique_ptr<views::Label> vertical_label_hours_;
  std::unique_ptr<views::Label> vertical_label_minutes_;
  ...
};

void TimeView::SetupLabels() {
  horizontal_label_.reset(new views::Label());
  SetupLabel(horizontal_label_.get());
  vertical_label_hours_.reset(new views::Label());
  SetupLabel(vertical_label_hours_.get());
  vertical_label_minutes_.reset(new views::Label());
  SetupLabel(vertical_label_minutes_.get());
  ...
}

void TimeView::SetupLabel(views::Label* label) {
  label->set_owned_by_client();
  ...
}











void TimeView::UpdateClockLayout(
    ClockLayout clock_layout) {
  ...
  if (clock_layout == ClockLayout::HORIZONTAL_CLOCK) {
    RemoveChildView(vertical_label_hours_.get());
    RemoveChildView(vertical_label_minutes_.get());
    SetLayoutManager(
        std::make_unique<views::FillLayout>());
    AddChildView(horizontal_label_.get());
  } else {
    RemoveChildView(horizontal_label_.get());
    // Remove the current layout manager since it could
    // be the FillLayout which only allows one child.
    SetLayoutManager(nullptr);
    // Pre-add the children since ownership is being
    // retained by this.
    AddChildView(vertical_label_hours_.get());
    AddChildView(vertical_label_minutes_.get());
    views::GridLayout* layout =
        SetLayoutManager(
            std::make_unique<views::GridLayout>());
    ...
  }
  Layout();
}

class ASH_EXPORT TimeView : public ActionableView,
                            public ClockObserver {
  ...
 private:
  class HorizontalLabelView;
  class VerticalLabelView;
  ...
  HorizontalLabelView* horizontal_label_;
  VerticalLabelView* vertical_label_;
  ...
};



TimeView::HorizontalLabelView::HorizontalLabelView() {
  SetLayoutManager(
      std::make_unique<views::FillLayout>());
  ...
}

TimeView::VerticalLabelView::VerticalLabelView() {
  views::GridLayout* layout =
        SetLayoutManager(
            std::make_unique<views::GridLayout>());
  ...
}

void TimeView::TimeView(ClockLayout clock_layout,
                        ClockModel* model) {
  ...
  horizontal_label_ =
     AddChildView(
        std::make_unique<HorizontalLabelView>());
  vertical_label_ =
     AddChildView(
        std::make_unique<VerticalLabelView());
  ...
}

void TimeView::UpdateClockLayout(
    ClockLayout clock_layout) {
  ...
  const bool is_horizontal =
      clock_layout == ClockLayout::HORIZONTAL_CLOCK;
  horizontal_label_->SetVisible(is_horizontal);
  vertical_label_->SetVisible(!is_horizontal);
  Layout();
}

|||---|||

Avoid refcounting and WeakPtrs

Refcounting and WeakPtrs may also be indicators that a View is doing more than merely displaying UI. Views objects should only handle UI. Refcounting and WeakPtr needs should generally be handled by helper objects.

|||---|||

Avoid

Old code in cast_dialog_no_sinks_view.{h,cc} that used weak pointers to PostDelayedTask() to itself:

Best practice

Current version eliminates lifetime concerns by using a OneShotTimer, which is canceled when destroyed:

|||---|||

|||---|||

class CastDialogNoSinksView ... {
  ...
 private:
  base::WeakPtrFactory<CastDialogNoSinksView>
      weak_factory_{this};
  ...
};

CastDialogNoSinksView::CastDialogNoSinksView(
    Profile* profile) : profile_(profile) {
  ...
  base::PostDelayedTask(
      FROM_HERE, {content::BrowserThread::UI},
      base::BindOnce(
          &CastDialogNoSinksView::ShowHelpIconView,
          weak_factory_.GetWeakPtr()),
      kSearchWaitTime);
}

class CastDialogNoSinksView ... {
  ...
 private:
  base::OneShotTimer timer_;
  ...
};


CastDialogNoSinksView::CastDialogNoSinksView(
    Profile* profile) : profile_(profile) {
  ...
  timer_.Start(
      FROM_HERE, kSearchWaitTime,
      base::BindOnce(
          &CastDialogNoSinksView::SetHelpIconView,
          base::Unretained(this)));
}

|||---|||

Use View::RemoveChildViewT<>()

View::RemoveChildViewT<>() 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 (cumbersome and error-prone), calling RemoveChildView() without ever deleting the pointer (leaks the View), or simply deleting a pointer to a still-parented View (will work today, but is semantically incorrect and may be removed in the future).

|||---|||

Avoid

Typical code in network_list_view.cc which manually deletes a child after removing it:

Best practice

Rewriting using RemoveChildViewT<>() is shorter and safer:

|||---|||

|||---|||

  ... if (mobile_header_view_) {
    scroll_content()->RemoveChildView(
        mobile_header_view_);
    delete mobile_header_view_;
    mobile_header_view_ = nullptr;
    needs_relayout_ = true;
  }
  ...

  ... if (mobile_header_view_) {
    scroll_content()->RemoveChildViewT(
        mobile_header_view_);

    mobile_header_view_ = nullptr;
    needs_relayout_ = true;
  }
  ...

|||---|||

Prefer scoping objects

Prefer scoping objects to paired Add/Remove-type calls. For example, use a base::ScopedObservation<> instead of directly calling View::AddObserver() and RemoveObserver(). Such objects reduce the likelihood of use-after-free.

|||---|||

Avoid

Typical code in avatar_toolbar_button.cc that uses paired add/remove calls:

Best practice

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

|||---|||

|||---|||

AvatarToolbarButton::AvatarToolbarButton(
    Browser* browser, ToolbarIconContainerView* parent)
    : browser_(browser), parent_(parent) {
  ...
  if (parent_)
    parent_->AddObserver(this);
}

AvatarToolbarButton::~AvatarToolbarButton() {
  if (parent_)
    parent_->RemoveObserver(this);
}

class AvatarToolbarButton
    : public ToolbarButton,
      public ToolbarIconContainerView::Observer {
  ...
 private:
  base::ScopedObservation<AvatarToolbarButton,
                 ToolbarIconContainerView::Observer>
      observation_{this};
};

AvatarToolbarButton::AvatarToolbarButton(
    Browser* browser, ToolbarIconContainerView* parent)
    : browser_(browser), parent_(parent) {
  ...
  if (parent_)
    observation_.Observe(parent_);
}

AvatarToolbarButton::~AvatarToolbarButton() = default;

|||---|||

Keep lifetimes fully nested

For objects you own, destroy in the reverse order you created, so lifetimes are nested rather than partially-overlapping. This can also reduce the likelihood of use-after-free, usually by enabling code to make simplifying assumptions (e.g. that an observed object always outlives this).

|||---|||

Avoid

Old code in widget_interactive_uitest.cc that destroys in the same order as creation:

Best practice

Current version uses scoping objects that are simpler and safer:

|||---|||

|||---|||

TEST_F(WidgetTestInteractive,
       ViewFocusOnWidgetActivationChanges) {
  Widget* widget1 = CreateTopLevelPlatformWidget();
  ...

  Widget* widget2 = CreateTopLevelPlatformWidget();
  ...
  widget1->CloseNow();
  widget2->CloseNow();
}

TEST_F(WidgetTestInteractive,
       ViewFocusOnWidgetActivationChanges) {
  WidgetAutoclosePtr widget1(
      CreateTopLevelPlatformWidget());
  ...
  WidgetAutoclosePtr widget2(
      CreateTopLevelPlatformWidget());
  ...
}

|||---|||

Only use Views objects on the UI thread

Always use Views on the main (UI) thread. Like most Chromium code, the Views toolkit is not thread-safe.

Add child Views in a View's constructor

In most cases, add child Views 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 Views. Child Views may need to be added at other times (e.g. in AddedToWidget() or OnThemeChanged(), 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 Views in a ViewHierarchyChanged() override, as such code is usually an artifact of misunderstanding the Views ownership model.

|||---|||

Avoid

Typical code in native_app_window_views.cc that sets up a child View in ViewHierarchyChanged():

Best practice

Rewriting to do this at construction/Init() makes |web_view_|'s lifetime easier to reason about:

|||---|||

|||---|||

void NativeAppWindowViews::ViewHierarchyChanged(
    const views::ViewHierarchyChangedDetails& details) {
  if (details.is_add && details.child == this) {
    DCHECK(!web_view_);
    web_view_ =
        AddChildView(
            std::make_unique<views::WebView>(nullptr));







    web_view_->SetWebContents(
        app_window_->web_contents());
  }
}

NativeAppWindowViews::NativeAppWindowViews() {
  ...
  web_view_ =
      AddChildView(
          std::make_unique<views::WebView>(nullptr));
}

void NativeAppWindowViews::Init(
    extensions::AppWindow* app_window,
    const extensions::AppWindow::CreateParams&
        create_params) {
  app_window_ = app_window;
  web_view_->SetWebContents(
      app_window_->web_contents());
  ...
}

|||---|||