search

ltilve / chromium

Chromium.org open source browser project, git cloned from http://git.chromium.org/chromium/src.git
BSD 3-Clause "New" or "Revised" License
0 stars 0 forks source link

Remove SidebarManager from browser process #17

Closed ltilve closed 9 years ago

ltilve commented 9 years ago

From Devlin review: https://codereview.chromium.org/1152613003/diff/20001/chrome/browser/browser_process.h#newcode124

We almost certainly don't want to have a SidebarManager as part of the browser process (sidebars cannot, for instance, cross profiles). This should be tied more closely to a profile, and should probably either be a BrowserContextKeyedService (or one of its subclasses) or live on the ExtensionSystem. My first inclination is probably that the latter makes more sense.

ltilve commented 9 years ago

We are working on some changes to avoid having sidebarManager living at the process browser, but at the extensionSystem instead. To make this, it's also neccesary to avoid the way that its getInstance() was called, to get the extensionSystem from the browserContext instead, and solve some scoping problems. However this approach is still not working. The changes at the temporary branch https://github.com/ltilve/chromium/commits/lkgr-igalia-sidebar-CL

ltilve commented 9 years ago

We managed to get the code to work after moving sidebarManager to extensions namespace, squashed all the changes https://github.com/ltilve/chromium/commit/ff93fc0273f513be8ae8244c10819597db7d8880 and updated codereview issue https://codereview.chromium.org/1152613003/#msg13