Create java AwBrowserContext

Create java AwBrowserContext

This will allow all the singleton object (geolocation, cookie manager
etc) instances to be collected together by their logical association.

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184788

[Kristian Monsen, 2013-02-22 03:44:03]

Landing this one for joth

[benm (inactive), 2013-02-22 12:05:44]

thanks kristian!

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java
(right):

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:7:
import android.content.Context;
nit: not needed?

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:11:
import org.chromium.base.ThreadUtils;
these two not needed also?

Maybe check all these imports :)

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
(right):

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:44:
start(c, null);
see comment below, maybe we can just call c.getsharedpreferences instead of
passing null, make the start() that takes the sharedprefs private and not
deprecate this one?

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:56:
final SharedPreferences defaultContextPreferences) {
do we need to pass these sharedprefs in if we have the context available? Could
we just call context.getSharedPreferences(..) in the call to AwBrowserContext
ctor below?

[joth, 2013-02-22 18:16:40]

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
(right):

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:56:
final SharedPreferences defaultContextPreferences) {
On 2013/02/22 12:05:44, benm wrote:
> do we need to pass these sharedprefs in if we have the context available?
Could
> we just call context.getSharedPreferences(..) in the call to AwBrowserContext
> ctor below?

interesting idea...
I was wondering about just passing a "name" string through to the
AwBrowserContext constructor that we'd use for both shared prefs and to
partition the databases and cache files for that specific profile. We'd need to
pass |context| into there too then, but this maybe cleaner.
So then the embedding glue layer would call AwBrowserProcess.start(context,
DEFAULT_[PROFILE|BROWSING_CONTEXT]_NAME);

WDYT?

[Kristian Monsen, 2013-02-22 20:50:18]

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java
(right):

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:7:
import android.content.Context;
On 2013/02/22 12:05:44, benm wrote:
> nit: not needed?

Done.

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java:11:
import org.chromium.base.ThreadUtils;
On 2013/02/22 12:05:44, benm wrote:
> these two not needed also?
> 
> Maybe check all these imports :)

I think only shared prefs are needed.

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
(right):

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:44:
start(c, null);
On 2013/02/22 12:05:44, benm wrote:
> see comment below, maybe we can just call c.getsharedpreferences instead of
> passing null, make the start() that takes the sharedprefs private and not
> deprecate this one?

I like this. It means that the shared pref is "owned" by the AwBrowserContext,
which looks reasonable to me.

I would actually prefer to pass the context to the AwBrowserContext ctor and let
it create the sharedPref. Later on we can always pass in a string or an
identifier if we want to create more than one type of context. What do you
think?

https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/...
android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:56:
final SharedPreferences defaultContextPreferences) {
On 2013/02/22 18:16:40, joth wrote:
> On 2013/02/22 12:05:44, benm wrote:
> > do we need to pass these sharedprefs in if we have the context available?
> Could
> > we just call context.getSharedPreferences(..) in the call to
AwBrowserContext
> > ctor below?
> 
> interesting idea...
> I was wondering about just passing a "name" string through to the
> AwBrowserContext constructor that we'd use for both shared prefs and to
> partition the databases and cache files for that specific profile. We'd need
to
> pass |context| into there too then, but this maybe cleaner.
> So then the embedding glue layer would call AwBrowserProcess.start(context,
> DEFAULT_[PROFILE|BROWSING_CONTEXT]_NAME);
> 
> WDYT?

Would it currently be called anywhere with anything else than the default? If
not I prefer to keep it as local as possible, and to delay the plumbing to when
it would actually be used.

[joth, 2013-02-22 21:25:03]

On 22 February 2013 12:50, <kristianm@chromium.org> wrote:

>
> https://codereview.chromium.**org/12313055/diff/1/android_**
>
webview/java/src/org/chromium/**android_webview/**AwBrowserContext.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java>
> File
> android_webview/java/src/org/**chromium/android_webview/**
> AwBrowserContext.java
> (right):
>
> https://codereview.chromium.**org/12313055/diff/1/android_**
> webview/java/src/org/chromium/**android_webview/**
>
AwBrowserContext.java#newcode7<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode7>
> android_webview/java/src/org/**chromium/android_webview/**
> AwBrowserContext.java:7:
> import android.content.Context;
> On 2013/02/22 12:05:44, benm wrote:
>
>> nit: not needed?
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12313055/diff/1/android_**
> webview/java/src/org/chromium/**android_webview/**AwBrowserContext.java#**
>
newcode11<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode11>
> android_webview/java/src/org/**chromium/android_webview/**
> AwBrowserContext.java:11:
> import org.chromium.base.ThreadUtils;
> On 2013/02/22 12:05:44, benm wrote:
>
>> these two not needed also?
>>
>
>  Maybe check all these imports :)
>>
>
> I think only shared prefs are needed.
>
>
> https://codereview.chromium.**org/12313055/diff/1/android_**
>
webview/java/src/org/chromium/**android_webview/**AwBrowserProcess.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java>
> File
> android_webview/java/src/org/**chromium/android_webview/**
> AwBrowserProcess.java
> (right):
>
> https://codereview.chromium.**org/12313055/diff/1/android_**
> webview/java/src/org/chromium/**android_webview/**AwBrowserProcess.java#**
>
newcode44<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode44>
> android_webview/java/src/org/**chromium/android_webview/**
> AwBrowserProcess.java:44:
> start(c, null);
> On 2013/02/22 12:05:44, benm wrote:
>
>> see comment below, maybe we can just call c.getsharedpreferences
>>
> instead of
>
>> passing null, make the start() that takes the sharedprefs private and
>>
> not
>
>> deprecate this one?
>>
>
> I like this. It means that the shared pref is "owned" by the
> AwBrowserContext, which looks reasonable to me.
>
> I would actually prefer to pass the context to the AwBrowserContext ctor
> and let it create the sharedPref. Later on we can always pass in a
> string or an identifier if we want to create more than one type of
> context. What do you think?
>
>
> https://codereview.chromium.**org/12313055/diff/1/android_**
> webview/java/src/org/chromium/**android_webview/**AwBrowserProcess.java#**
>
newcode56<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode56>
> android_webview/java/src/org/**chromium/android_webview/**
> AwBrowserProcess.java:56:
> final SharedPreferences defaultContextPreferences) {
> On 2013/02/22 18:16:40, joth wrote:
>
>> On 2013/02/22 12:05:44, benm wrote:
>> > do we need to pass these sharedprefs in if we have the context
>>
> available?
>
>> Could
>> > we just call context.getSharedPreferences(.**.) in the call to
>>
> AwBrowserContext
>
>> > ctor below?
>>
>
>  interesting idea...
>> I was wondering about just passing a "name" string through to the
>> AwBrowserContext constructor that we'd use for both shared prefs and
>>
> to
>
>> partition the databases and cache files for that specific profile.
>>
> We'd need to
>
>> pass |context| into there too then, but this maybe cleaner.
>> So then the embedding glue layer would call
>>
> AwBrowserProcess.start(**context,
>
>> DEFAULT_[PROFILE|BROWSING_**CONTEXT]_NAME);
>>
>
>  WDYT?
>>
>
> Would it currently be called anywhere with anything else than the
> default? If not I prefer to keep it as local as possible, and to delay
> the plumbing to when it would actually be used.
>
>
Yep only one will exist so could embed it in, but the only reason to do it
now is to put the definition of the string into the glue where it belongs.
(logically, configuration choices like this should be there rather than
inside the chromium/ code)

[Kristian Monsen, 2013-02-22 21:46:31]

On 2013/02/22 21:25:03, joth wrote:
> On 22 February 2013 12:50, <mailto:kristianm@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.**org/12313055/diff/1/android_**
> >
>
webview/java/src/org/chromium/**android_webview/**AwBrowserContext.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java>
> > File
> > android_webview/java/src/org/**chromium/android_webview/**
> > AwBrowserContext.java
> > (right):
> >
> > https://codereview.chromium.**org/12313055/diff/1/android_**
> > webview/java/src/org/chromium/**android_webview/**
> >
>
AwBrowserContext.java#newcode7<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode7>
> > android_webview/java/src/org/**chromium/android_webview/**
> > AwBrowserContext.java:7:
> > import android.content.Context;
> > On 2013/02/22 12:05:44, benm wrote:
> >
> >> nit: not needed?
> >>
> >
> > Done.
> >
> >
> > https://codereview.chromium.**org/12313055/diff/1/android_**
> > webview/java/src/org/chromium/**android_webview/**AwBrowserContext.java#**
> >
>
newcode11<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode11>
> > android_webview/java/src/org/**chromium/android_webview/**
> > AwBrowserContext.java:11:
> > import org.chromium.base.ThreadUtils;
> > On 2013/02/22 12:05:44, benm wrote:
> >
> >> these two not needed also?
> >>
> >
> >  Maybe check all these imports :)
> >>
> >
> > I think only shared prefs are needed.
> >
> >
> > https://codereview.chromium.**org/12313055/diff/1/android_**
> >
>
webview/java/src/org/chromium/**android_webview/**AwBrowserProcess.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java>
> > File
> > android_webview/java/src/org/**chromium/android_webview/**
> > AwBrowserProcess.java
> > (right):
> >
> > https://codereview.chromium.**org/12313055/diff/1/android_**
> > webview/java/src/org/chromium/**android_webview/**AwBrowserProcess.java#**
> >
>
newcode44<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode44>
> > android_webview/java/src/org/**chromium/android_webview/**
> > AwBrowserProcess.java:44:
> > start(c, null);
> > On 2013/02/22 12:05:44, benm wrote:
> >
> >> see comment below, maybe we can just call c.getsharedpreferences
> >>
> > instead of
> >
> >> passing null, make the start() that takes the sharedprefs private and
> >>
> > not
> >
> >> deprecate this one?
> >>
> >
> > I like this. It means that the shared pref is "owned" by the
> > AwBrowserContext, which looks reasonable to me.
> >
> > I would actually prefer to pass the context to the AwBrowserContext ctor
> > and let it create the sharedPref. Later on we can always pass in a
> > string or an identifier if we want to create more than one type of
> > context. What do you think?
> >
> >
> > https://codereview.chromium.**org/12313055/diff/1/android_**
> > webview/java/src/org/chromium/**android_webview/**AwBrowserProcess.java#**
> >
>
newcode56<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode56>
> > android_webview/java/src/org/**chromium/android_webview/**
> > AwBrowserProcess.java:56:
> > final SharedPreferences defaultContextPreferences) {
> > On 2013/02/22 18:16:40, joth wrote:
> >
> >> On 2013/02/22 12:05:44, benm wrote:
> >> > do we need to pass these sharedprefs in if we have the context
> >>
> > available?
> >
> >> Could
> >> > we just call context.getSharedPreferences(.**.) in the call to
> >>
> > AwBrowserContext
> >
> >> > ctor below?
> >>
> >
> >  interesting idea...
> >> I was wondering about just passing a "name" string through to the
> >> AwBrowserContext constructor that we'd use for both shared prefs and
> >>
> > to
> >
> >> partition the databases and cache files for that specific profile.
> >>
> > We'd need to
> >
> >> pass |context| into there too then, but this maybe cleaner.
> >> So then the embedding glue layer would call
> >>
> > AwBrowserProcess.start(**context,
> >
> >> DEFAULT_[PROFILE|BROWSING_**CONTEXT]_NAME);
> >>
> >
> >  WDYT?
> >>
> >
> > Would it currently be called anywhere with anything else than the
> > default? If not I prefer to keep it as local as possible, and to delay
> > the plumbing to when it would actually be used.
> >
> >
> Yep only one will exist so could embed it in, but the only reason to do it
> now is to put the definition of the string into the glue where it belongs.
> (logically, configuration choices like this should be there rather than
> inside the chromium/ code)

I agree that any configuration should be in the glue layer, but think it is fine
to embed the string as long as there is only one here. I don't like to
complicate with code paths that are not really used.

It is just a preference though so will leave the final call to you.

[joth, 2013-02-23 01:35:09]

On 22 February 2013 13:46, <kristianm@chromium.org> wrote:

> On 2013/02/22 21:25:03, joth wrote:
>
>> On 22 February 2013 12:50, <mailto:kristianm@chromium.org**> wrote:
>>
>
>  >
>> > https://codereview.chromium.****org/12313055/diff/1/android_**
>> >
>>
>
> webview/java/src/org/chromium/****android_webview/****
> AwBrowserContext.java<https://**codereview.chromium.org/**
> 12313055/diff/1/android_**webview/java/src/org/chromium/**android_webview/
>
**AwBrowserContext.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java>
> >
>
>> > File
>> > android_webview/java/src/org/****chromium/android_webview/**
>> > AwBrowserContext.java
>> > (right):
>> >
>> > https://codereview.chromium.****org/12313055/diff/1/android_**
>> > webview/java/src/org/chromium/****android_webview/**
>> >
>>
>
> AwBrowserContext.java#**newcode7<https://codereview.**
> chromium.org/12313055/diff/1/**android_webview/java/src/org/**
>
chromium/android_webview/**AwBrowserContext.java#newcode7<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode7>
> **>
>
>> > android_webview/java/src/org/****chromium/android_webview/**
>>
>> > AwBrowserContext.java:7:
>> > import android.content.Context;
>> > On 2013/02/22 12:05:44, benm wrote:
>> >
>> >> nit: not needed?
>> >>
>> >
>> > Done.
>> >
>> >
>> > https://codereview.chromium.****org/12313055/diff/1/android_**
>> > webview/java/src/org/chromium/****android_webview/****
>> AwBrowserContext.java#**
>> >
>>
>
> newcode11<https://codereview.**chromium.org/12313055/diff/1/**
> android_webview/java/src/org/**chromium/android_webview/**
>
AwBrowserContext.java#**newcode11<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode11>
> >
>
>> > android_webview/java/src/org/****chromium/android_webview/**
>>
>> > AwBrowserContext.java:11:
>> > import org.chromium.base.ThreadUtils;
>> > On 2013/02/22 12:05:44, benm wrote:
>> >
>> >> these two not needed also?
>> >>
>> >
>> >  Maybe check all these imports :)
>> >>
>> >
>> > I think only shared prefs are needed.
>> >
>> >
>> > https://codereview.chromium.****org/12313055/diff/1/android_**
>> >
>>
>
> webview/java/src/org/chromium/****android_webview/****
> AwBrowserProcess.java<https://**codereview.chromium.org/**
> 12313055/diff/1/android_**webview/java/src/org/chromium/**android_webview/
>
**AwBrowserProcess.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java>
> >
>
>> > File
>> > android_webview/java/src/org/****chromium/android_webview/**
>> > AwBrowserProcess.java
>> > (right):
>> >
>> > https://codereview.chromium.****org/12313055/diff/1/android_**
>> > webview/java/src/org/chromium/****android_webview/****
>> AwBrowserProcess.java#**
>> >
>>
>
> newcode44<https://codereview.**chromium.org/12313055/diff/1/**
> android_webview/java/src/org/**chromium/android_webview/**
>
AwBrowserProcess.java#**newcode44<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode44>
> >
>
>> > android_webview/java/src/org/****chromium/android_webview/**
>>
>> > AwBrowserProcess.java:44:
>> > start(c, null);
>> > On 2013/02/22 12:05:44, benm wrote:
>> >
>> >> see comment below, maybe we can just call c.getsharedpreferences
>> >>
>> > instead of
>> >
>> >> passing null, make the start() that takes the sharedprefs private and
>> >>
>> > not
>> >
>> >> deprecate this one?
>> >>
>> >
>> > I like this. It means that the shared pref is "owned" by the
>> > AwBrowserContext, which looks reasonable to me.
>> >
>> > I would actually prefer to pass the context to the AwBrowserContext ctor
>> > and let it create the sharedPref. Later on we can always pass in a
>> > string or an identifier if we want to create more than one type of
>> > context. What do you think?
>> >
>> >
>> > https://codereview.chromium.****org/12313055/diff/1/android_**
>> > webview/java/src/org/chromium/****android_webview/****
>> AwBrowserProcess.java#**
>> >
>>
>
> newcode56<https://codereview.**chromium.org/12313055/diff/1/**
> android_webview/java/src/org/**chromium/android_webview/**
>
AwBrowserProcess.java#**newcode56<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode56>
> >
>
>> > android_webview/java/src/org/****chromium/android_webview/**
>>
>> > AwBrowserProcess.java:56:
>> > final SharedPreferences defaultContextPreferences) {
>> > On 2013/02/22 18:16:40, joth wrote:
>> >
>> >> On 2013/02/22 12:05:44, benm wrote:
>> >> > do we need to pass these sharedprefs in if we have the context
>> >>
>> > available?
>> >
>> >> Could
>> >> > we just call context.getSharedPreferences(.****.) in the call to
>>
>> >>
>> > AwBrowserContext
>> >
>> >> > ctor below?
>> >>
>> >
>> >  interesting idea...
>> >> I was wondering about just passing a "name" string through to the
>> >> AwBrowserContext constructor that we'd use for both shared prefs and
>> >>
>> > to
>> >
>> >> partition the databases and cache files for that specific profile.
>> >>
>> > We'd need to
>> >
>> >> pass |context| into there too then, but this maybe cleaner.
>> >> So then the embedding glue layer would call
>> >>
>> > AwBrowserProcess.start(****context,
>> >
>> >> DEFAULT_[PROFILE|BROWSING_****CONTEXT]_NAME);
>>
>> >>
>> >
>> >  WDYT?
>> >>
>> >
>> > Would it currently be called anywhere with anything else than the
>> > default? If not I prefer to keep it as local as possible, and to delay
>> > the plumbing to when it would actually be used.
>> >
>> >
>> Yep only one will exist so could embed it in, but the only reason to do it
>> now is to put the definition of the string into the glue where it belongs.
>> (logically, configuration choices like this should be there rather than
>> inside the chromium/ code)
>>
>
> I agree that any configuration should be in the glue layer, but think it
> is fine
> to embed the string as long as there is only one here. I don't like to
> complicate with code paths that are not really used.
>
> It is just a preference though so will leave the final call to you.
>
>
Ha, I remembered why I went for passing SharedPrefs now -- that class is
fairly test friendly and it's easy to imagine a future test wanting to pass
in a mock version.
That said, it's easy for us to split another seem to inject that (or a
whole mock browser context) in, so this argument doesn't really hold...

That said, we already have the string in the embedder today, so seems
really backwards for us to move it to the chromium code base with the
knowledge we logically should move it back.

So I'm going to stick with my previous answer: "pass the string in as a
param"





>
https://codereview.chromium.**org/12313055/<https://codereview.chromium.org/1...
>

[benm (inactive), 2013-02-25 12:26:25]

On 2013/02/23 01:35:09, joth wrote:
> On 22 February 2013 13:46, <mailto:kristianm@chromium.org> wrote:
> 
> > On 2013/02/22 21:25:03, joth wrote:
> >
> >> On 22 February 2013 12:50, <mailto:kristianm@chromium.org**> wrote:
> >>
> >
> >  >
> >> > https://codereview.chromium.****org/12313055/diff/1/android_**
> >> >
> >>
> >
> > webview/java/src/org/chromium/****android_webview/****
> > AwBrowserContext.java<https://**codereview.chromium.org/**
> > 12313055/diff/1/android_**webview/java/src/org/chromium/**android_webview/
> >
>
**AwBrowserContext.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java>
> > >
> >
> >> > File
> >> > android_webview/java/src/org/****chromium/android_webview/**
> >> > AwBrowserContext.java
> >> > (right):
> >> >
> >> > https://codereview.chromium.****org/12313055/diff/1/android_**
> >> > webview/java/src/org/chromium/****android_webview/**
> >> >
> >>
> >
> > AwBrowserContext.java#**newcode7<https://codereview.**
> > chromium.org/12313055/diff/1/**android_webview/java/src/org/**
> >
>
chromium/android_webview/**AwBrowserContext.java#newcode7<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode7>
> > **>
> >
> >> > android_webview/java/src/org/****chromium/android_webview/**
> >>
> >> > AwBrowserContext.java:7:
> >> > import android.content.Context;
> >> > On 2013/02/22 12:05:44, benm wrote:
> >> >
> >> >> nit: not needed?
> >> >>
> >> >
> >> > Done.
> >> >
> >> >
> >> > https://codereview.chromium.****org/12313055/diff/1/android_**
> >> > webview/java/src/org/chromium/****android_webview/****
> >> AwBrowserContext.java#**
> >> >
> >>
> >
> > newcode11<https://codereview.**chromium.org/12313055/diff/1/**
> > android_webview/java/src/org/**chromium/android_webview/**
> >
>
AwBrowserContext.java#**newcode11<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserContext.java#newcode11>
> > >
> >
> >> > android_webview/java/src/org/****chromium/android_webview/**
> >>
> >> > AwBrowserContext.java:11:
> >> > import org.chromium.base.ThreadUtils;
> >> > On 2013/02/22 12:05:44, benm wrote:
> >> >
> >> >> these two not needed also?
> >> >>
> >> >
> >> >  Maybe check all these imports :)
> >> >>
> >> >
> >> > I think only shared prefs are needed.
> >> >
> >> >
> >> > https://codereview.chromium.****org/12313055/diff/1/android_**
> >> >
> >>
> >
> > webview/java/src/org/chromium/****android_webview/****
> > AwBrowserProcess.java<https://**codereview.chromium.org/**
> > 12313055/diff/1/android_**webview/java/src/org/chromium/**android_webview/
> >
>
**AwBrowserProcess.java<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java>
> > >
> >
> >> > File
> >> > android_webview/java/src/org/****chromium/android_webview/**
> >> > AwBrowserProcess.java
> >> > (right):
> >> >
> >> > https://codereview.chromium.****org/12313055/diff/1/android_**
> >> > webview/java/src/org/chromium/****android_webview/****
> >> AwBrowserProcess.java#**
> >> >
> >>
> >
> > newcode44<https://codereview.**chromium.org/12313055/diff/1/**
> > android_webview/java/src/org/**chromium/android_webview/**
> >
>
AwBrowserProcess.java#**newcode44<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode44>
> > >
> >
> >> > android_webview/java/src/org/****chromium/android_webview/**
> >>
> >> > AwBrowserProcess.java:44:
> >> > start(c, null);
> >> > On 2013/02/22 12:05:44, benm wrote:
> >> >
> >> >> see comment below, maybe we can just call c.getsharedpreferences
> >> >>
> >> > instead of
> >> >
> >> >> passing null, make the start() that takes the sharedprefs private and
> >> >>
> >> > not
> >> >
> >> >> deprecate this one?
> >> >>
> >> >
> >> > I like this. It means that the shared pref is "owned" by the
> >> > AwBrowserContext, which looks reasonable to me.
> >> >
> >> > I would actually prefer to pass the context to the AwBrowserContext ctor
> >> > and let it create the sharedPref. Later on we can always pass in a
> >> > string or an identifier if we want to create more than one type of
> >> > context. What do you think?
> >> >
> >> >
> >> > https://codereview.chromium.****org/12313055/diff/1/android_**
> >> > webview/java/src/org/chromium/****android_webview/****
> >> AwBrowserProcess.java#**
> >> >
> >>
> >
> > newcode56<https://codereview.**chromium.org/12313055/diff/1/**
> > android_webview/java/src/org/**chromium/android_webview/**
> >
>
AwBrowserProcess.java#**newcode56<https://codereview.chromium.org/12313055/diff/1/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode56>
> > >
> >
> >> > android_webview/java/src/org/****chromium/android_webview/**
> >>
> >> > AwBrowserProcess.java:56:
> >> > final SharedPreferences defaultContextPreferences) {
> >> > On 2013/02/22 18:16:40, joth wrote:
> >> >
> >> >> On 2013/02/22 12:05:44, benm wrote:
> >> >> > do we need to pass these sharedprefs in if we have the context
> >> >>
> >> > available?
> >> >
> >> >> Could
> >> >> > we just call context.getSharedPreferences(.****.) in the call to
> >>
> >> >>
> >> > AwBrowserContext
> >> >
> >> >> > ctor below?
> >> >>
> >> >
> >> >  interesting idea...
> >> >> I was wondering about just passing a "name" string through to the
> >> >> AwBrowserContext constructor that we'd use for both shared prefs and
> >> >>
> >> > to
> >> >
> >> >> partition the databases and cache files for that specific profile.
> >> >>
> >> > We'd need to
> >> >
> >> >> pass |context| into there too then, but this maybe cleaner.
> >> >> So then the embedding glue layer would call
> >> >>
> >> > AwBrowserProcess.start(****context,
> >> >
> >> >> DEFAULT_[PROFILE|BROWSING_****CONTEXT]_NAME);
> >>
> >> >>
> >> >
> >> >  WDYT?
> >> >>
> >> >
> >> > Would it currently be called anywhere with anything else than the
> >> > default? If not I prefer to keep it as local as possible, and to delay
> >> > the plumbing to when it would actually be used.
> >> >
> >> >
> >> Yep only one will exist so could embed it in, but the only reason to do it
> >> now is to put the definition of the string into the glue where it belongs.
> >> (logically, configuration choices like this should be there rather than
> >> inside the chromium/ code)
> >>
> >
> > I agree that any configuration should be in the glue layer, but think it
> > is fine
> > to embed the string as long as there is only one here. I don't like to
> > complicate with code paths that are not really used.
> >
> > It is just a preference though so will leave the final call to you.
> >
> >
> Ha, I remembered why I went for passing SharedPrefs now -- that class is
> fairly test friendly and it's easy to imagine a future test wanting to pass
> in a mock version.
> That said, it's easy for us to split another seem to inject that (or a
> whole mock browser context) in, so this argument doesn't really hold...
> 
> That said, we already have the string in the embedder today, so seems
> really backwards for us to move it to the chromium code base with the
> knowledge we logically should move it back.
> 
> So I'm going to stick with my previous answer: "pass the string in as a
> param"
> 
> 
> 
> 
> 
> >
>
https://codereview.chromium.**org/12313055/%3Chttps://codereview.chromium.org...>
> >

SGTN

[Kristian Monsen, 2013-02-26 21:04:55]

In that case I suggest we keep it as is, and pass in SharedPrefs?

PTAL.

[joth, 2013-02-26 21:13:47]

lgtm

[commit-bot: I haz the power, 2013-02-26 22:09:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kristianm@chromium.org/12313055/5001

[commit-bot: I haz the power, 2013-02-27 00:25:23]

Change committed as 184788