Project Carnitas - Code Cleanup

[Ben Goodger (Google)]

As previously mentioned, it's past time that we did something as a team about the hygiene of our codebase.

To that end, I am coordinating a collection of initiatives over at least the next quarter to address specific areas of concern, under the umbrella of general codebase and API cleanup. This work is known collectively as Carnitas.

The high level goal is to make the code clearer, simpler, more automatically and self documenting. Some of the high impact work collected under this umbrella so far are:

- A generalized TabContents API. This includes simplifying the API for TabContents and extracting it and relevant mparch pieces from src/chrome as much as possible into a standalone dep. This will allow a variety of products based on Chrome to reliably consume this core component.

- Much cleanup in browser-land. It is kind of spaghetti soup in src/chrome/browser right now. I would like to break it down into more clearly isolated pieces. We are also seeking to minimize dependencies between components and make sure the dependencies flow in the right direction. This will allow us in time to reduce the size of browser.lib and be smarter about our build. We are also seeking to define a clearer API between Chrome and ChromeOS in the FE so that Chrome changes impose less headache for the ChromeOS folk.

- Encourage C++ best practices and style guide adherence through automatic means (clang). This will allow us to maintain readability standards as we grow the team rapidly.

Are there other major areas of cleanup or refactoring that should be covered by Carnitas? If so, please let me know. I plan to send out a weekly update of progress to this list so people can see what we're up to and how we're tracking towards our objectives.

Also, if you're interested in helping with cleanup and have cycles spare, let me know.

Regards,

-Ben

(Why "Carnitas"? Because it's fun to say!)

[Paweł Hajdan, Jr.]

On Thu, Jan 13, 2011 at 16:51, Ben Goodger (Google) <b...@chromium.org> wrote:

- Much cleanup in browser-land. It is kind of spaghetti soup in src/chrome/browser right now. I would like to break it down into more clearly isolated pieces. We are also seeking to minimize dependencies between components and make sure the dependencies flow in the right direction.

Does that include DEPS files for chrome/browser subdirectories? That would be interesting.

[Ben Goodger (Google)]

Yes! I need to create a wiki page that describes all aspects of the browser dir reorg. I will do that this afternoon.

-Ben

[Ben Goodger (Google)]

I have created a Wiki page here:

http://code.google.com/p/chromium/wiki/CarnitasProjects

... feel free to add other major work you plan on doing this quarter. I will ping people on this page weekly for status updates.

-Ben

[John Abd-El-Malek]

One more area, related to cleaning up chrome/browser.  Historically new features have been implemented by adding code to RenderViewHost/TabContents/ResourceMessageFilter on the browser side, RenderView on the renderer side, render_messages_internal.h for IPCs, and WebView/WebViewClient on the WebKit API side.  Sometimes the feature code was in separate classes, but RenderView/RenderViewHost/ResourceMessageFilter still did the IPC message dispatching and called out to these classes.  It turns out that it's not sustainable to have these files grow at the same pace that we add features.

I've started to refactor code to a model where features can be implemented in self-contained classes in the browser/renderer/WebKit, and not have the above classes know about them.

Filtering messages on the IO thread (i.e. what has always been done in ResourceMessageFilter) is done.  New features that require more than a few IPCs to be filtered on the IO thread should be defining a new filter which inherits from BrowserMessageFilter.  If they have a bunch of IPCs, these should ideally also be declared in a separate X_messages.h.  Declaring IPC files is now simpler than before, i.e. only X_messages.cc and .h are needed, and no more X_messages_internal.h.  There are plenty of examples.

Separating the code on the browser UI thread and renderer main thread is the next step.  I'm almost done doing this for AutoFill code as a guinea pig.  Once it's done, I will add a wiki that shows the recommended way, and move over the rest of the code.  Please follow this for new features as well, and as a reviewer look for this.

On Thu, Jan 13, 2011 at 7:51 AM, Ben Goodger (Google) <b...@chromium.org> wrote:

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[John Abd-El-Malek]

On Thu, Jan 13, 2011 at 10:18 AM, John Abd-El-Malek <j...@chromium.org> wrote:

One more area, related to cleaning up chrome/browser.  Historically new features have been implemented by adding code to RenderViewHost/TabContents/ResourceMessageFilter on the browser side, RenderView on the renderer side, render_messages_internal.h for IPCs, and WebView/WebViewClient on the WebKit API side.  Sometimes the feature code was in separate classes, but RenderView/RenderViewHost/ResourceMessageFilter still did the IPC message dispatching and called out to these classes.  It turns out that it's not sustainable to have these files grow at the same pace that we add features.

I've started to refactor code to a model where features can be implemented in self-contained classes in the browser/renderer/WebKit, and not have the above classes know about them.

Filtering messages on the IO thread (i.e. what has always been done in ResourceMessageFilter) is done.  New features that require more than a few IPCs to be filtered on the IO thread should be defining a new filter which inherits from BrowserMessageFilter.  If they have a bunch of IPCs, these should ideally also be declared in a separate X_messages.h.  Declaring IPC files is now simpler than before, i.e. only X_messages.cc and .h are needed, and no more X_messages_internal.h.  There are plenty of examples.

Separating the code on the browser UI thread and renderer main thread is the next step.  I'm almost done doing this for AutoFill code as a guinea pig.  Once it's done, I will add a wiki that shows the recommended way, and move over the rest of the code.  Please follow this for new features as well, and as a reviewer look for this.

This is now done.  On the renderer side, we have RenderViewObserver.  On the browser UI thread, there's WebNavigationObserver.  The details are available at https://sites.google.com/a/chromium.org/dev/developers/design-documents/multi-process-architecture/how-to-add-new-features.  If you're adding a new feature that spans processes and works with IPCs, please make sure to read this first.