Refactor the two URLRequestContext subclasses in profile.cc into a new shared

Refactor the two URLRequestContext subclasses in profile.cc into a new shared
ChromeRequestContext class. This will allow us to put browser-specific request
context here rather than in URLRequestContext and eliminates a lot of
duplicate code.

I looked at having two different classes using either inheritance (as proposed
by the existing TODO) or composition, but it seems like there isn't enough
difference between these two classes to justify it. 

Removed is_off_the_record() because it wasn't being used anywhere and did a
few other minor code cleanup things.

[Aaron Boodman, 2008-12-22 09:01:56]

Feel free to reassign if there is someone better for this review.

[darin (slow to review), 2008-12-22 18:05:15]

good idea to put this URLRequestContext subclass in the browser/net directory.

some comments...

http://codereview.chromium.org/16408/diff/22/24
File chrome/browser/net/chrome_request_context.cc (right):

http://codereview.chromium.org/16408/diff/22/24#newcode1
Line 1: #include "chrome/browser/net/chrome_request_context.h"
this file needs a license header.

http://codereview.chromium.org/16408/diff/22/25
File chrome/browser/net/chrome_request_context.h (right):

http://codereview.chromium.org/16408/diff/22/25#newcode22
Line 22: class ChromeRequestContext : public URLRequestContext,
nit: maybe ChromeURLRequestContext would be a better name, even though it is
more to type.

http://codereview.chromium.org/16408/diff/22/25#newcode28
Line 28: static bool IsInstance(URLRequestContext* context);
I don't understand why this method exists.  The point of ChromeRequestContext is
to say that all URLRequestContext pointers used in the Chrome process are of
type ChromeRequestContext, precisely eliminating the need for any RTTI-style
casting.  Just cast to ChromeRequestContext and be happy.

http://codereview.chromium.org/16408/diff/22/25#newcode30
Line 30: // Constructs an instance. This is expected to be called on the UI
thread.
nit: please group all of the methods that need to be called on the UI thread and
all of the methods that need to be called on the IO thread.  see
render_widget_helper.h for example.  it helps people not make mistakes if they
can clearly see the groupings IMO.

http://codereview.chromium.org/16408/diff/22/25#newcode41
Line 41: virtual void Observe(NotificationType type,
does this need to be public?  i think it is only ever called via
NotificationObserver, so it can be private.

http://codereview.chromium.org/16408/diff/22/25#newcode50
Line 50: void OnAcceptLanguageChange(std::string accept_language);
it looks like this On* methods can be made private.

http://codereview.chromium.org/16408/diff/22/25#newcode56
Line 56: virtual ~ChromeRequestContext();
this should be made private since this class is reference counted.  you don't
want somebody mistakenly calling your destructor!

http://codereview.chromium.org/16408/diff/22/28
File net/url_request/url_request_context.h (left):

http://codereview.chromium.org/16408/diff/22/28#oldcode67
Line 67: bool is_off_the_record() { return is_off_the_record_; }
nice cleanup, thanks!

[Aaron Boodman, 2008-12-22 20:47:18]

http://codereview.chromium.org/16408/diff/22/24
File chrome/browser/net/chrome_request_context.cc (right):

http://codereview.chromium.org/16408/diff/22/24#newcode1
Line 1: #include "chrome/browser/net/chrome_request_context.h"
On 2008/12/22 18:05:15, darin wrote:
> this file needs a license header.

Blargh, I always forget those. Done.

http://codereview.chromium.org/16408/diff/22/25
File chrome/browser/net/chrome_request_context.h (right):

http://codereview.chromium.org/16408/diff/22/25#newcode22
Line 22: class ChromeRequestContext : public URLRequestContext,
On 2008/12/22 18:05:15, darin wrote:
> nit: maybe ChromeURLRequestContext would be a better name, even though it is
> more to type.

Done.

http://codereview.chromium.org/16408/diff/22/25#newcode28
Line 28: static bool IsInstance(URLRequestContext* context);
On 2008/12/22 18:05:15, darin wrote:
> I don't understand why this method exists.  The point of ChromeRequestContext
is
> to say that all URLRequestContext pointers used in the Chrome process are of
> type ChromeRequestContext, precisely eliminating the need for any RTTI-style
> casting.  Just cast to ChromeRequestContext and be happy.

URLRequestContext is already inherited directly in unit tests, and I could see
people wanting to do so in the browser too. It would be hard for them to know
that elsewhere in the code we downcast every single URLRequestContext to
ChromeURLRequestContext. This seemed like a relatively simple safety measure.

Also note that this is essentially the same thing that the existing Gears
support does. I hope that code could be refactored to take advantage of this,
but I don't understand it quite well enough yet to suggest how.

http://codereview.chromium.org/16408/diff/22/25#newcode30
Line 30: // Constructs an instance. This is expected to be called on the UI
thread.
On 2008/12/22 18:05:15, darin wrote:
> nit: please group all of the methods that need to be called on the UI thread
and
> all of the methods that need to be called on the IO thread.  see
> render_widget_helper.h for example.  it helps people not make mistakes if they
> can clearly see the groupings IMO.

Done.

http://codereview.chromium.org/16408/diff/22/25#newcode41
Line 41: virtual void Observe(NotificationType type,
On 2008/12/22 18:05:15, darin wrote:
> does this need to be public?  i think it is only ever called via
> NotificationObserver, so it can be private.

Done.

http://codereview.chromium.org/16408/diff/22/25#newcode50
Line 50: void OnAcceptLanguageChange(std::string accept_language);
On 2008/12/22 18:05:15, darin wrote:
> it looks like this On* methods can be made private.

Done.

http://codereview.chromium.org/16408/diff/22/25#newcode56
Line 56: virtual ~ChromeRequestContext();
On 2008/12/22 18:05:15, darin wrote:
> this should be made private since this class is reference counted.  you don't
> want somebody mistakenly calling your destructor!

Done.

[darin (slow to review), 2008-12-22 21:01:32]

http://codereview.chromium.org/16408/diff/22/25
File chrome/browser/net/chrome_request_context.h (right):

http://codereview.chromium.org/16408/diff/22/25#newcode28
Line 28: static bool IsInstance(URLRequestContext* context);
If anyone else in Chrome is using a different URLRequestContext, then it is a
bug.  Using a different URLRequestContext would mean not sharing the
authentication cache, for instance, which would cause the application to have to
prompt users more than once for the username/password to their proxy server. 
Things like that mean that we should never have raw URLRequestContext usage in
Chrome, so we don't need a way to test if the URLRequestContext is actually a
ChromeULRRequestContext.

Those unit tests should be fine.  Your URLRequestJob shouldn't be registered in
those unit tests.  Alternatively, those unit tests should be using
ChromeURLRequestContext instead since they are testing code designed to run in
Chrome.

I would prefer to avoid adding this code since it adds a synchronization point
with the UI thread, and once you get enough of those you start to hurt
performance.  Since we don't need this thread synchronization, I'd prefer to do
without it.

I don't understand why the Gears code needs this kind of code either.

[eroman, 2008-12-22 21:10:10]

Can you restore a shared ProxyService between off the record contexts and
regular?

(I have been trying to eliminate the places where they are duplicated to avoid
headaches.)

[Aaron Boodman, 2008-12-22 23:19:55]

Removed IsInstance(), made ProxyService instance shared between normal and OTR,
and changed Init() and InitOffTheRecord() into factory functions (which is what
they really wanted to be).

http://codereview.chromium.org/16408/diff/22/25
File chrome/browser/net/chrome_request_context.h (right):

http://codereview.chromium.org/16408/diff/22/25#newcode28
Line 28: static bool IsInstance(URLRequestContext* context);
On 2008/12/22 21:01:32, darin wrote:
> If anyone else in Chrome is using a different URLRequestContext, then it is a
> bug.  Using a different URLRequestContext would mean not sharing the
> authentication cache, for instance, which would cause the application to have
to
> prompt users more than once for the username/password to their proxy server. 
> Things like that mean that we should never have raw URLRequestContext usage in
> Chrome, so we don't need a way to test if the URLRequestContext is actually a
> ChromeULRRequestContext.
> 
> Those unit tests should be fine.  Your URLRequestJob shouldn't be registered
in
> those unit tests.  Alternatively, those unit tests should be using
> ChromeURLRequestContext instead since they are testing code designed to run in
> Chrome.
> 
> I would prefer to avoid adding this code since it adds a synchronization point
> with the UI thread, and once you get enough of those you start to hurt
> performance.  Since we don't need this thread synchronization, I'd prefer to
do
> without it.
> 
> I don't understand why the Gears code needs this kind of code either.

Ok, you're the boss. Removed over-engineered IsInstance() method and supporting
code.

[darin (slow to review), 2008-12-23 04:23:24]

LGreatTM!

[eroman, 2008-12-23 05:38:32]

LGTM here too!

thanks for making those changes.