Design Principles for desktop code in //chrome/browser

[Erik Chen]

Hi chromium-dev,

//chrome/OWNERS have put together design principles to make it easier to write correct code, easier to maintain existing code, and harder to write incorrect code. This is a living document, feel free to reach out with questions, feedback or concerns

Calling out two guidelines here, see doc for more many more:

(1) 

Avoid using Browser*. Conceptually, Browser* is a container for hundreds of other features. Passing Browser* to a class creates circular dependencies, makes it impossible to perform dependency injection (and therefore to write unit tests), and creates unclear API surfaces, often with re-entrancy.

Do not do this:

FooFeature(Browser* browser) : browser_(browser) {}

FooFeature::DoStuff() { DoStuffWith(browser_->profile()->GetPrefs()); }

Do this:

FooFeature(PrefService* prefs) : prefs_(prefs) {}

FooFeature::DoStuff() { DoStuffWith(prefs_); }

We realize lots of existing code is using Browser. We do not expect such code to immediately be rewritten.

(2)

A feature often has tab-scoped, browser-window-scoped, and profile-scoped state. Tab-scoped state should live in a controller owned by TabFeatures, browser-window-scoped state should live in a controller owned by BrowserWindowFeatures, and profile-scoped state should live in a ProfileKeyedService. TabInterface and BrowserWindowInterface are used to bridge.

Properly scoped state avoids categories of bugs and subtle issues. For example: one common mistake is to create a content::WebContentsUserData and to store window-scoped state on this class. This results in subtle bugs (or crashes) when the WebContents (often associated with a tab) is dragged into a new window. 

Do not do this (drawn from real examples):

FooTabFeature : public content::WebContentsUserData {  // Instantiated by tab_helpers.cc

  // As per (1) above, we shouldn't be passing or storing Browser* anyways.

  // Another common anti-pattern is to dynamically look up Browser* via browser_finder methods such as chrome::FindLastActive(). These often result in the wrong Browser*

  Browser* browser_;  

  // This will point to the wrong BrowserView if the tab is dragged into a new window.

  // Furthermore, BrowserView* itself is a container of hundreds of other views. The tab-scoped feature typically wants something much more specific.

  BrowserView* browser_view_;  

  // Extensions are profile-scoped, and thus any state/logic should be in a ProfileKeyedService. 

  // If the user has multiple tabs (possibly across multiple windows) simultaneously interacting with FooTabFeature, then it's likely that this will become inconsistent.

  void InstallExtension();

  void UninstallExtension();

};

Instead do this:

FooTabFeature {  // Instantiated by TabFeatures

  // Guaranteed to remain valid for the lifetime of this class. This can be used to dynamically access relevant window state via tab_->GetBrowserWindowInterface()->GetFeatures().GetFooWindowFeature()

  TabInterface* tab_;

};

FooService {

  void InstallExtension();

  void UninstallExtension();

};