Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1217563005:
Create unique HttpNetworkSession for storage partition. (Closed)
DescriptionCreate unique HttpNetworkSession for storage partition.
This CL provides an isolated HttpNetWork session for apps/webview. It creates a separate network session, along with isolated versions of the necessary ancillary objects.
BUG=291417
Patch Set 1 #Patch Set 2 : Fix compile on Windows/Android. #Patch Set 3 : Add test; need to resolve bug. #Patch Set 4 : Network session check should run on IO thread. #Patch Set 5 : Add isolated HostResolver. #Patch Set 6 : Wired up remaining state for HttpNetworkSession. #
Total comments: 1
Depends on Patchset: Messages
Total messages: 23 (5 generated)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1217563005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Not looking for a review for this (yet), but just wanted to share it as an experiment. This creates what is (I hope) an isolated HttpNetworkSession that will be attached to any storage partition with a non-empty partition_domain. This includes the webview-based signin page. It passes all the try-bot tests, though that may just point to a lack of tests for apps with isolated storage partitions. I've manually done some testing to show that it does work for plain-vanilla signin (i.e. no client certs involved). Obviously we need to have a fix ready for the client certs first, before we can truly isolate the HttpNetworkSession, and this may look radically different down the road (e.g. if we choose to avoid giving isolated storage partitions their own request contexts as Matt has suggested). But, it at least appears to work (more or less).
wjmaclean@chromium.org changed reviewers: + davidben@chromium.org, mmenke@chromium.org
davidben@ mmenke@ - please take a look and let me know if this looks like its moving in the right direction. In particular, are we creating all the ancillary objects required to properly isolate the HttpNetWork session? Cheers, James
This is just seems very fragile and ugly (No offense - the existing code to share stuff between contexts is similarly ugly). Wonder if now is the time to switch to URLRequestContextBuilder, to get better isolation, without having quite so much ugliness. That also means sharing stuff is opt-in, rather than opt-out, which seems less regression prone. The URLRequestContextBuilder would certainly need to be modified quite a bit, and I think we'd just want to share the NetLog (And maybe proxy stuff and the NetworkDelegate, though I think a separate the NetworkDelegate, at least, makes sense, though that would have implications for Flywheel).
On 2015/07/09 15:42:48, mmenke wrote: > This is just seems very fragile and ugly (No offense - the existing code to > share stuff between contexts is similarly ugly). > > Wonder if now is the time to switch to URLRequestContextBuilder, to get better > isolation, without having quite so much ugliness. That also means sharing stuff > is opt-in, rather than opt-out, which seems less regression prone. > > The URLRequestContextBuilder would certainly need to be modified quite a bit, > and I think we'd just want to share the NetLog (And maybe proxy stuff and the > NetworkDelegate, though I think a separate the NetworkDelegate, at least, makes > sense, though that would have implications for Flywheel). Another, less ambitious, option would be to share the code we use to set up the main context. If the main context is currently sharing things with the system context that we don't want app contexts to share with the main context, then the main context probably shouldn't be sharing those things with the system context, either.
On 2015/07/09 16:14:38, mmenke wrote: > On 2015/07/09 15:42:48, mmenke wrote: > > This is just seems very fragile and ugly (No offense - the existing code to > > share stuff between contexts is similarly ugly). He he ... no offense taken ... it *is* a bit of a mess at present. > > Wonder if now is the time to switch to URLRequestContextBuilder, to get better > > isolation, without having quite so much ugliness. That also means sharing > stuff > > is opt-in, rather than opt-out, which seems less regression prone. > > > > The URLRequestContextBuilder would certainly need to be modified quite a bit, > > and I think we'd just want to share the NetLog (And maybe proxy stuff and the > > NetworkDelegate, though I think a separate the NetworkDelegate, at least, > makes > > sense, though that would have implications for Flywheel). > > Another, less ambitious, option would be to share the code we use to set up the > main context. If the main context is currently sharing things with the system > context that we don't want app contexts to share with the main context, then the > main context probably shouldn't be sharing those things with the system context, > either. I'm ok with either option ... could sharing the code that sets up the main context be a stepping stone to get to the URLRequestContextBuilder solution (I'm assuming that long-term this is a better solution?)? Or are they divergent paths? (I haven't had time yet to read through the code and think about the potential ramifications.)
On 2015/07/09 16:19:57, wjmaclean wrote: > On 2015/07/09 16:14:38, mmenke wrote: > > On 2015/07/09 15:42:48, mmenke wrote: > > > This is just seems very fragile and ugly (No offense - the existing code to > > > share stuff between contexts is similarly ugly). > > He he ... no offense taken ... it *is* a bit of a mess at present. > > > > Wonder if now is the time to switch to URLRequestContextBuilder, to get > better > > > isolation, without having quite so much ugliness. That also means sharing > > stuff > > > is opt-in, rather than opt-out, which seems less regression prone. > > > > > > The URLRequestContextBuilder would certainly need to be modified quite a > bit, > > > and I think we'd just want to share the NetLog (And maybe proxy stuff and > the > > > NetworkDelegate, though I think a separate the NetworkDelegate, at least, > > makes > > > sense, though that would have implications for Flywheel). > > > > Another, less ambitious, option would be to share the code we use to set up > the > > main context. If the main context is currently sharing things with the system > > context that we don't want app contexts to share with the main context, then > the > > main context probably shouldn't be sharing those things with the system > context, > > either. > > I'm ok with either option ... could sharing the code that sets up the main > context be a stepping stone to get to the URLRequestContextBuilder solution (I'm > assuming that long-term this is a better solution?)? Or are they divergent > paths? (I haven't had time yet to read through the code and think about the > potential ramifications.) Sorry, wrote this an hour or two ago, but forgot to actually send it. If we were doing both, we'd probably want to merge the code, and then switch to the builder (And no, they aren't divergent). Doing them in the other order wouldn't really make much sense, so let's just try and merge the context creation logic - think that alone would be a big step forward. Note that I haven't thought a whole lot about how involved merging context construction will be, but I think it would be a big step forward in cleaning up these pretty hideous classes. https://codereview.chromium.org/1217563005/diff/120001/chrome/browser/profile... File chrome/browser/profiles/profile_impl_io_data.cc (right): https://codereview.chromium.org/1217563005/diff/120001/chrome/browser/profile... chrome/browser/profiles/profile_impl_io_data.cc:653: context->CopyFrom(main_context); And just to help motivate my comment about sharing stuff by default being a problem - we're sharing the SdchManager here, which has its own memory cache, and makes requests that go through the main context (And its disk cache).
On 2015/07/09 18:36:54, mmenke wrote: > Note that I haven't thought a whole lot about how involved merging context > construction will be, but I think it would be a big step forward in cleaning up > these pretty hideous classes. Not wanting to discourage cleanup work (you know I block CLs for it), but I did just want to echo that, as a result of Chrome signin switching to <webview>, this has broken Chrome signin for enterprises using client certs + SAML IDPs. I'm just wanting to encourage a careful evaluation to see if there's a reasonable way to break the work into digestable concrete tasks that we can live with along each step of the way, since I do get a little wary when you're scared of the complexity :)
On 2015/07/09 19:46:51, Ryan Sleevi (slow through 7-15 wrote: > On 2015/07/09 18:36:54, mmenke wrote: > > Note that I haven't thought a whole lot about how involved merging context > > construction will be, but I think it would be a big step forward in cleaning > up > > these pretty hideous classes. > > Not wanting to discourage cleanup work (you know I block CLs for it), but I did > just want to echo that, as a result of Chrome signin switching to <webview>, > this has broken Chrome signin for enterprises using client certs + SAML IDPs. > > I'm just wanting to encourage a careful evaluation to see if there's a > reasonable way to break the work into digestable concrete tasks that we can live > with along each step of the way, since I do get a little wary when you're scared > of the complexity :) I don't think we're going to get this into M45, regardless, due to complexity, particularly since this isn't an M45 regression. And I don't *think* the refactor will be sufficiently bad that we'll have trouble targeting M46.
On 2015/07/09 19:50:22, mmenke wrote: > On 2015/07/09 19:46:51, Ryan Sleevi (slow through 7-15 wrote: > > On 2015/07/09 18:36:54, mmenke wrote: > > > Note that I haven't thought a whole lot about how involved merging context > > > construction will be, but I think it would be a big step forward in cleaning > > up > > > these pretty hideous classes. > > > > Not wanting to discourage cleanup work (you know I block CLs for it), but I > did > > just want to echo that, as a result of Chrome signin switching to <webview>, > > this has broken Chrome signin for enterprises using client certs + SAML IDPs. > > > > I'm just wanting to encourage a careful evaluation to see if there's a > > reasonable way to break the work into digestable concrete tasks that we can > live > > with along each step of the way, since I do get a little wary when you're > scared > > of the complexity :) > > I don't think we're going to get this into M45, regardless, due to complexity, > particularly since this isn't an M45 regression. And I don't *think* the > refactor will be sufficiently bad that we'll have trouble targeting M46. Having now had a chance to read through the code that creates the main context, I notice it is somewhat distributed (across several methods/classes). Is it reasonable to assume a good first step would be to try and re-factor it to get it in one place? If so, it seems like a good approach might be to virtualize Build() in URLRequestContextBuilder, and putting the construction code in a new sub-class. Then somewhere down the road we could, relatively painlessly, switch over to the new pathway. So I'm guessing, based on comments in the current code for the main context, that part of the challenge will be re-entrancy (in the form of attempts by other code at accessing the partially built context). Is that a reasonable assessment?
On Wed, Jul 15, 2015 at 3:36 PM, <wjmaclean@chromium.org> wrote: > On 2015/07/09 19:50:22, mmenke wrote: > >> On 2015/07/09 19:46:51, Ryan Sleevi (slow through 7-15 wrote: >> > On 2015/07/09 18:36:54, mmenke wrote: >> > > Note that I haven't thought a whole lot about how involved merging >> context >> > > construction will be, but I think it would be a big step forward in >> > cleaning > >> > up >> > > these pretty hideous classes. >> > >> > Not wanting to discourage cleanup work (you know I block CLs for it), >> but I >> did >> > just want to echo that, as a result of Chrome signin switching to >> <webview>, >> > this has broken Chrome signin for enterprises using client certs + SAML >> > IDPs. > >> > >> > I'm just wanting to encourage a careful evaluation to see if there's a >> > reasonable way to break the work into digestable concrete tasks that we >> can >> live >> > with along each step of the way, since I do get a little wary when >> you're >> scared >> > of the complexity :) >> > > I don't think we're going to get this into M45, regardless, due to >> complexity, >> particularly since this isn't an M45 regression. And I don't *think* the >> refactor will be sufficiently bad that we'll have trouble targeting M46. >> > > Having now had a chance to read through the code that creates the main > context, > I notice it is somewhat distributed (across several methods/classes). Is it > reasonable to assume a good first step would be to try and re-factor it to > get > it in one place? > > If so, it seems like a good approach might be to virtualize Build() in > URLRequestContextBuilder, and putting the construction code in a new > sub-class. > Then somewhere down the road we could, relatively painlessly, switch over > to the > new pathway. > What do we get out of making Build in URLRequestContextBuilder virtual? I really don't want to encourage people to start rolling their own URLRequestContextBuilder subclasses. That having been said, I'm fine with adding features to URLRequestContextBuilder, if we could make it work for ProfileIOData's needs. > So I'm guessing, based on comments in the current code for the main > context, > that part of the challenge will be re-entrancy (in the form of attempts by > other > code at accessing the partially built context). Is that a reasonable > assessment? > > https://codereview.chromium.org/1217563005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/07/15 19:41:34, mmenke wrote: > > What do we get out of making Build in URLRequestContextBuilder virtual? I see this as a temporary change, mainly motivated by not wanting to disturb the current Build() function (which seems to have at least one user) until we're ready to roll out the new one. It might not be necessary. > I really don't want to encourage people to start rolling their own > URLRequestContextBuilder subclasses. Agreed. > That having been said, I'm fine with adding features to > URLRequestContextBuilder, if we could make it work for ProfileIOData's > needs.
On Wed, Jul 15, 2015 at 3:48 PM, <wjmaclean@chromium.org> wrote: > On 2015/07/15 19:41:34, mmenke wrote: > > What do we get out of making Build in URLRequestContextBuilder virtual? >> > > I see this as a temporary change, mainly motivated by not wanting to > disturb the > current Build() function (which seems to have at least one user) until > we're > ready to roll out the new one. It might not be necessary. There are currently a fair number of consumers of the class, none of which need all the flexibility ProfileIOData currently uses. The builder was created more with their needs in mind, than ProfileIOData's - before, everyone was rolling their own code to create a URLRequestContext, and their code tended to either get something wrong, and/or wasn't updated to support new features when they were added to URLRequestContext. One huge difference between current consumers of the Builder and ProfileIOData: The URLRequestBuilder currently takes ownership of all URLRequestContext components passed in to it. ProfileIOData, on the other hand, expects to share stuff between components (A pattern which has, admittedly, led to problems, though mostly in other URLRequestContexts sharing stuff with ProfileIOData's contexts). This may be an irreconcilable difference between the two sets of expectations. Adding an option to have it take ownership of something or not, for almost every single parameter, seems pretty ugly. > > > I really don't want to encourage people to start rolling their own >> URLRequestContextBuilder subclasses. >> > > Agreed. > > That having been said, I'm fine with adding features to >> URLRequestContextBuilder, if we could make it work for ProfileIOData's >> needs. >> > > > https://codereview.chromium.org/1217563005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Sorry, lost context here... So the way I thinking we could do this is make one URLRequestContextBuilder for the main URLRequestContext, which owns everything, and another media one which shares everything but the cache (That cache would have to be owned by the media context). Then just make sure we delete the media contexts first. To make the builder work, you'd need to carefully compare how things are done for the profile, and how the build works, and add the necessary options to the builder. Could refactor the profile code in a couple phases to do everything in one spot, so it looks more like the builder, before switching over, though given the split between incognito and non-incognito app contexts, that intermediary step may be pretty complicated. An alternative approach would be to switch over incognito first, which would probably need a lot fewer modifications to the builder, since nothing needs to be stored on disk, and then switch over non-incognito (Possibly sharing code with the incognito patch, just setting a few more things, like cache path). On Wed, Jul 15, 2015 at 4:32 PM, Matt Menke <mmenke@chromium.org> wrote: > On Wed, Jul 15, 2015 at 3:48 PM, <wjmaclean@chromium.org> wrote: > >> On 2015/07/15 19:41:34, mmenke wrote: >> >> What do we get out of making Build in URLRequestContextBuilder virtual? >>> >> >> I see this as a temporary change, mainly motivated by not wanting to >> disturb the >> current Build() function (which seems to have at least one user) until >> we're >> ready to roll out the new one. It might not be necessary. > > > There are currently a fair number of consumers of the class, none of which > need all the flexibility ProfileIOData currently uses. The builder was > created more with their needs in mind, than ProfileIOData's - before, > everyone was rolling their own code to create a URLRequestContext, and > their code tended to either get something wrong, and/or wasn't updated to > support new features when they were added to URLRequestContext. > > One huge difference between current consumers of the Builder and > ProfileIOData: The URLRequestBuilder currently takes ownership of all > URLRequestContext components passed in to it. ProfileIOData, on the other > hand, expects to share stuff between components (A pattern which has, > admittedly, led to problems, though mostly in other URLRequestContexts > sharing stuff with ProfileIOData's contexts). This may be an > irreconcilable difference between the two sets of expectations. Adding an > option to have it take ownership of something or not, for almost every > single parameter, seems pretty ugly. > > >> >> >> I really don't want to encourage people to start rolling their own >>> URLRequestContextBuilder subclasses. >>> >> >> Agreed. >> >> That having been said, I'm fine with adding features to >>> URLRequestContextBuilder, if we could make it work for ProfileIOData's >>> needs. >>> >> >> >> https://codereview.chromium.org/1217563005/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/07/15 21:03:58, mmenke wrote: > Sorry, lost context here... > > So the way I thinking we could do this is make one URLRequestContextBuilder > for the main URLRequestContext, which owns everything, and another media > one which shares everything but the cache (That cache would have to be > owned by the media context). Then just make sure we delete the media > contexts first. > > To make the builder work, you'd need to carefully compare how things are > done for the profile, and how the build works, and add the necessary > options to the builder. Could refactor the profile code in a couple phases > to do everything in one spot, so it looks more like the builder, before > switching over, though given the split between incognito and non-incognito > app contexts, that intermediary step may be pretty complicated. > > An alternative approach would be to switch over incognito first, which > would probably need a lot fewer modifications to the builder, since nothing > needs to be stored on disk, and then switch over non-incognito (Possibly > sharing code with the incognito patch, just setting a few more things, like > cache path). > > On Wed, Jul 15, 2015 at 4:32 PM, Matt Menke <mailto:mmenke@chromium.org> wrote: > > > On Wed, Jul 15, 2015 at 3:48 PM, <mailto:wjmaclean@chromium.org> wrote: > > > >> On 2015/07/15 19:41:34, mmenke wrote: > >> > >> What do we get out of making Build in URLRequestContextBuilder virtual? > >>> > >> > >> I see this as a temporary change, mainly motivated by not wanting to > >> disturb the > >> current Build() function (which seems to have at least one user) until > >> we're > >> ready to roll out the new one. It might not be necessary. > > > > > > There are currently a fair number of consumers of the class, none of which > > need all the flexibility ProfileIOData currently uses. The builder was > > created more with their needs in mind, than ProfileIOData's - before, > > everyone was rolling their own code to create a URLRequestContext, and > > their code tended to either get something wrong, and/or wasn't updated to > > support new features when they were added to URLRequestContext. > > > > One huge difference between current consumers of the Builder and > > ProfileIOData: The URLRequestBuilder currently takes ownership of all > > URLRequestContext components passed in to it. ProfileIOData, on the other > > hand, expects to share stuff between components (A pattern which has, > > admittedly, led to problems, though mostly in other URLRequestContexts > > sharing stuff with ProfileIOData's contexts). This may be an > > irreconcilable difference between the two sets of expectations. Adding an > > option to have it take ownership of something or not, for almost every > > single parameter, seems pretty ugly. > > > > > >> > >> > >> I really don't want to encourage people to start rolling their own > >>> URLRequestContextBuilder subclasses. > >>> > >> > >> Agreed. > >> > >> That having been said, I'm fine with adding features to > >>> URLRequestContextBuilder, if we could make it work for ProfileIOData's > >>> needs. > >>> > >> > >> > >> https://codereview.chromium.org/1217563005/ > >> > > > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. After reading through the code, I see what you mean about the different ownership issues. I'll start putting together a proof-of-concept of a builder for the main/otr profiles ... with luck that shouldn't be too awful, but the only way to find the pitfalls is to try it ...
On 2015/07/16 20:11:54, wjmaclean wrote: > On 2015/07/15 21:03:58, mmenke wrote: > > Sorry, lost context here... > > > > So the way I thinking we could do this is make one URLRequestContextBuilder > > for the main URLRequestContext, which owns everything, and another media > > one which shares everything but the cache (That cache would have to be > > owned by the media context). Then just make sure we delete the media > > contexts first. > > > > To make the builder work, you'd need to carefully compare how things are > > done for the profile, and how the build works, and add the necessary > > options to the builder. Could refactor the profile code in a couple phases > > to do everything in one spot, so it looks more like the builder, before > > switching over, though given the split between incognito and non-incognito > > app contexts, that intermediary step may be pretty complicated. > > > > An alternative approach would be to switch over incognito first, which > > would probably need a lot fewer modifications to the builder, since nothing > > needs to be stored on disk, and then switch over non-incognito (Possibly > > sharing code with the incognito patch, just setting a few more things, like > > cache path). > > > > On Wed, Jul 15, 2015 at 4:32 PM, Matt Menke <mailto:mmenke@chromium.org> > wrote: > > > > > On Wed, Jul 15, 2015 at 3:48 PM, <mailto:wjmaclean@chromium.org> wrote: > > > > > >> On 2015/07/15 19:41:34, mmenke wrote: > > >> > > >> What do we get out of making Build in URLRequestContextBuilder virtual? > > >>> > > >> > > >> I see this as a temporary change, mainly motivated by not wanting to > > >> disturb the > > >> current Build() function (which seems to have at least one user) until > > >> we're > > >> ready to roll out the new one. It might not be necessary. > > > > > > > > > There are currently a fair number of consumers of the class, none of which > > > need all the flexibility ProfileIOData currently uses. The builder was > > > created more with their needs in mind, than ProfileIOData's - before, > > > everyone was rolling their own code to create a URLRequestContext, and > > > their code tended to either get something wrong, and/or wasn't updated to > > > support new features when they were added to URLRequestContext. > > > > > > One huge difference between current consumers of the Builder and > > > ProfileIOData: The URLRequestBuilder currently takes ownership of all > > > URLRequestContext components passed in to it. ProfileIOData, on the other > > > hand, expects to share stuff between components (A pattern which has, > > > admittedly, led to problems, though mostly in other URLRequestContexts > > > sharing stuff with ProfileIOData's contexts). This may be an > > > irreconcilable difference between the two sets of expectations. Adding an > > > option to have it take ownership of something or not, for almost every > > > single parameter, seems pretty ugly. > > > > > > > > >> > > >> > > >> I really don't want to encourage people to start rolling their own > > >>> URLRequestContextBuilder subclasses. > > >>> > > >> > > >> Agreed. > > >> > > >> That having been said, I'm fine with adding features to > > >>> URLRequestContextBuilder, if we could make it work for ProfileIOData's > > >>> needs. > > >>> > > >> > > >> > > >> https://codereview.chromium.org/1217563005/ > > >> > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > After reading through the code, I see what you mean about the different > ownership issues. > > I'll start putting together a proof-of-concept of a builder for the main/otr > profiles ... with luck that shouldn't be too awful, but the only way to find the > pitfalls is to try it ... Just FYI: I'm removing myself as a reviewer from your two older reviews. Just trimming down my pending review list.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org
Message was sent while issue was closed.
On 2015/09/11 17:05:31, mmenke wrote: > On 2015/07/16 20:11:54, wjmaclean wrote: > > On 2015/07/15 21:03:58, mmenke wrote: > > > Sorry, lost context here... > > > > > > So the way I thinking we could do this is make one URLRequestContextBuilder > > > for the main URLRequestContext, which owns everything, and another media > > > one which shares everything but the cache (That cache would have to be > > > owned by the media context). Then just make sure we delete the media > > > contexts first. > > > > > > To make the builder work, you'd need to carefully compare how things are > > > done for the profile, and how the build works, and add the necessary > > > options to the builder. Could refactor the profile code in a couple phases > > > to do everything in one spot, so it looks more like the builder, before > > > switching over, though given the split between incognito and non-incognito > > > app contexts, that intermediary step may be pretty complicated. > > > > > > An alternative approach would be to switch over incognito first, which > > > would probably need a lot fewer modifications to the builder, since nothing > > > needs to be stored on disk, and then switch over non-incognito (Possibly > > > sharing code with the incognito patch, just setting a few more things, like > > > cache path). > > > > > > On Wed, Jul 15, 2015 at 4:32 PM, Matt Menke <mailto:mmenke@chromium.org> > > wrote: > > > > > > > On Wed, Jul 15, 2015 at 3:48 PM, <mailto:wjmaclean@chromium.org> wrote: > > > > > > > >> On 2015/07/15 19:41:34, mmenke wrote: > > > >> > > > >> What do we get out of making Build in URLRequestContextBuilder virtual? > > > >>> > > > >> > > > >> I see this as a temporary change, mainly motivated by not wanting to > > > >> disturb the > > > >> current Build() function (which seems to have at least one user) until > > > >> we're > > > >> ready to roll out the new one. It might not be necessary. > > > > > > > > > > > > There are currently a fair number of consumers of the class, none of which > > > > need all the flexibility ProfileIOData currently uses. The builder was > > > > created more with their needs in mind, than ProfileIOData's - before, > > > > everyone was rolling their own code to create a URLRequestContext, and > > > > their code tended to either get something wrong, and/or wasn't updated to > > > > support new features when they were added to URLRequestContext. > > > > > > > > One huge difference between current consumers of the Builder and > > > > ProfileIOData: The URLRequestBuilder currently takes ownership of all > > > > URLRequestContext components passed in to it. ProfileIOData, on the other > > > > hand, expects to share stuff between components (A pattern which has, > > > > admittedly, led to problems, though mostly in other URLRequestContexts > > > > sharing stuff with ProfileIOData's contexts). This may be an > > > > irreconcilable difference between the two sets of expectations. Adding an > > > > option to have it take ownership of something or not, for almost every > > > > single parameter, seems pretty ugly. > > > > > > > > > > > >> > > > >> > > > >> I really don't want to encourage people to start rolling their own > > > >>> URLRequestContextBuilder subclasses. > > > >>> > > > >> > > > >> Agreed. > > > >> > > > >> That having been said, I'm fine with adding features to > > > >>> URLRequestContextBuilder, if we could make it work for ProfileIOData's > > > >>> needs. > > > >>> > > > >> > > > >> > > > >> https://codereview.chromium.org/1217563005/ > > > >> > > > > > > > > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > > email > > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > After reading through the code, I see what you mean about the different > > ownership issues. > > > > I'll start putting together a proof-of-concept of a builder for the main/otr > > profiles ... with luck that shouldn't be too awful, but the only way to find > the > > pitfalls is to try it ... > > Just FYI: I'm removing myself as a reviewer from your two older reviews. Just > trimming down my pending review list. Archived. Useful learning exercise, but not landable in this form ... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
