Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2024973002:
headless: Allow protocol handler customization (Closed)
|
Created:
4 years, 6 months ago by Sami Modified:
4 years, 6 months ago Reviewers:
altimin, Ryan Sleevi, Randy Smith (Not in Mondays), Eric Seckler, mmenke, alex clarke (OOO till 29th) CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionheadless: Allow protocol handler customization
Allow headless clients to provide custom protocol handlers. This lets
the clients override how URL requests are fulfilled for different
schemes types.
Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3fM/edit#bookmark=id.5bqp8w95liri
BUG=546953
Committed: https://crrev.com/a65d5e613d85dba0d97b45c651cad1e39ccc21bc
Cr-Commit-Position: refs/heads/master@{#397184}
Patch Set 1 #Patch Set 2 : Fix typo #
Total comments: 4
Patch Set 3 : Review comments #
Total comments: 2
Messages
Total messages: 35 (15 generated)
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. BUG=595353 ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 ==========
skyostil@chromium.org changed reviewers: + alexclarke@chromium.org, altimin@chromium.org
skyostil@chromium.org changed reviewers: + rsleevi@chromium.org
skyostil@chromium.org changed reviewers: + eseckler@chromium.org
altimin@/alexclarke@/eseckler@: PTAL Ryan, looks like you're out at the moment. We wanted to have at least a headless networking landed to unblock some further work. Let's revisit this once you're back.
On 2016/05/31 15:50:46, Sami wrote: > altimin@/alexclarke@/eseckler@: PTAL > > Ryan, looks like you're out at the moment. We wanted to have at least a headless > networking landed to unblock some further work. Let's revisit this once you're > back. *at least a prototype of*
lgtm I'm not up to speed with network stuff, but as far as headless goes this seems fine. https://codereview.chromium.org/2024973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_url_request_context_getter.cc (right): https://codereview.chromium.org/2024973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_url_request_context_getter.cc:190: if (protocol_handlers_.find(url::kDataScheme) == protocol_handlers_.end()) { Maybe add a comment explaining why we need to add these (and not other) default handlers. https://codereview.chromium.org/2024973002/diff/20001/headless/lib/headless_b... File headless/lib/headless_browser_browsertest.cc (right): https://codereview.chromium.org/2024973002/diff/20001/headless/lib/headless_b... headless/lib/headless_browser_browsertest.cc:59: for (int i = 0; i < buf_size; i++) { Nit: this class is small enough it probably doesn't matter, but this feels like it's reinventing the wheel :)
Thanks, addressed. https://codereview.chromium.org/2024973002/diff/20001/headless/lib/browser/he... File headless/lib/browser/headless_url_request_context_getter.cc (right): https://codereview.chromium.org/2024973002/diff/20001/headless/lib/browser/he... headless/lib/browser/headless_url_request_context_getter.cc:190: if (protocol_handlers_.find(url::kDataScheme) == protocol_handlers_.end()) { On 2016/05/31 16:55:50, alex clarke wrote: > Maybe add a comment explaining why we need to add these (and not other) default > handlers. Done. https://codereview.chromium.org/2024973002/diff/20001/headless/lib/headless_b... File headless/lib/headless_browser_browsertest.cc (right): https://codereview.chromium.org/2024973002/diff/20001/headless/lib/headless_b... headless/lib/headless_browser_browsertest.cc:59: for (int i = 0; i < buf_size; i++) { On 2016/05/31 16:55:50, alex clarke wrote: > Nit: this class is small enough it probably doesn't matter, but this feels like > it's reinventing the wheel :) Good point, replaced with some convenience classes from net/.
The CQ bit was checked by skyostil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexclarke@chromium.org Link to the patchset: https://codereview.chromium.org/2024973002/#ps40001 (title: "Review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024973002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2024973002/40001
On 2016/05/31 18:02:32, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/2024973002/40001 > View timeline at > https://chromium-cq-status.appspot.com/patch-timeline/2024973002/40001 Please don't commit this until you've had a //net reviewer look, if you're looking for //net feedback. I cannot offer reviews after the fact. Perhaps mmenke or rdsmith can help you.
Message was sent while issue was closed.
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 Committed: https://crrev.com/789f9abbbc69e5d485984a9d7b4e3d167054919f Cr-Commit-Position: refs/heads/master@{#396884} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/789f9abbbc69e5d485984a9d7b4e3d167054919f Cr-Commit-Position: refs/heads/master@{#396884}
Message was sent while issue was closed.
On 2016/05/31 18:42:01, Ryan Sleevi (OOO til 6-6) wrote: > Please don't commit this until you've had a //net reviewer look, if you're > looking for //net feedback. I cannot offer reviews after the fact. > > Perhaps mmenke or rdsmith can help you. Sorry, cq'd before I saw this. I'll revert and ask them to have a look.
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2026253002/ by skyostil@chromium.org. The reason for reverting is: Doing another review round with net/..
Message was sent while issue was closed.
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 Committed: https://crrev.com/789f9abbbc69e5d485984a9d7b4e3d167054919f Cr-Commit-Position: refs/heads/master@{#396884} ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 Committed: https://crrev.com/789f9abbbc69e5d485984a9d7b4e3d167054919f Cr-Commit-Position: refs/heads/master@{#396884} ==========
skyostil@chromium.org changed reviewers: + mmenke@chromium.org, rdsmith@chromium.org
mmenke@/rdsmith@, could one of you please have a look? See the linked (internal!) design doc for background & motivations.
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 Committed: https://crrev.com/789f9abbbc69e5d485984a9d7b4e3d167054919f Cr-Commit-Position: refs/heads/master@{#396884} ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 ==========
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=595353 ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=546953 ==========
Is the plan to use a custom protocol handler for HTTP/HTTPS? If so, worth noting that eventually there will be a default handler for those two protocols (CL's actually out for review, though it's been stalled for a while, not sure why - https://codereview.chromium.org/1888963004/) https://codereview.chromium.org/2024973002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_url_request_context_getter.cc (right): https://codereview.chromium.org/2024973002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_url_request_context_getter.cc:114: if (!url_request_context_) { I'd recommend you switch to using URLRequestContextBuilder - it's updated as the magic incantation to set up a URLRequestContext changes. It also avoids duplicating a lot of code. If it doesn't support setting something you need to set on the URLREquestContext, it's fine to add it. Beyond the scope of this CL, of course.
On 2016/06/01 15:32:19, mmenke (OOO May 30-31) wrote: > Is the plan to use a custom protocol handler for HTTP/HTTPS? If so, worth > noting that eventually there will be a default handler for those two protocols > (CL's actually out for review, though it's been stalled for a while, not sure > why - https://codereview.chromium.org/1888963004/) Yes, the idea is to have custom handlers for HTTP and HTTPS. Looks like even after that CL we'd still be able to create a URLRequestJobFactoryImpl without those handlers and using our custom ones should still work, right? https://codereview.chromium.org/2024973002/diff/40001/headless/lib/browser/he... File headless/lib/browser/headless_url_request_context_getter.cc (right): https://codereview.chromium.org/2024973002/diff/40001/headless/lib/browser/he... headless/lib/browser/headless_url_request_context_getter.cc:114: if (!url_request_context_) { On 2016/06/01 15:32:19, mmenke (OOO May 30-31) wrote: > I'd recommend you switch to using URLRequestContextBuilder - it's updated as the > magic incantation to set up a URLRequestContext changes. It also avoids > duplicating a lot of code. If it doesn't support setting something you need to > set on the URLREquestContext, it's fine to add it. Beyond the scope of this CL, > of course. Thanks for the tip; filed https://bugs.chromium.org/p/chromium/issues/detail?id=616475.
On 2016/06/01 15:44:52, Sami wrote: > On 2016/06/01 15:32:19, mmenke (OOO May 30-31) wrote: > > Is the plan to use a custom protocol handler for HTTP/HTTPS? If so, worth > > noting that eventually there will be a default handler for those two protocols > > (CL's actually out for review, though it's been stalled for a while, not sure > > why - https://codereview.chromium.org/1888963004/) > > Yes, the idea is to have custom handlers for HTTP and HTTPS. Looks like even > after that CL we'd still be able to create a URLRequestJobFactoryImpl without > those handlers and using our custom ones should still work, right? Correct, assuming you don't want your custom handlers to be able to return NULL to indicate that we should fall back to the built-in HTTP factories. That's the one behavior that will be different.
On 2016/06/01 15:51:15, mmenke (OOO May 30-31) wrote: > Correct, assuming you don't want your custom handlers to be able to return NULL > to indicate that we should fall back to the built-in HTTP factories. That's the > one behavior that will be different. Cool, that sounds fine. I don't think we'd need dynamic switching like that.
On 2016/06/01 15:52:51, Sami wrote: > On 2016/06/01 15:51:15, mmenke (OOO May 30-31) wrote: > > Correct, assuming you don't want your custom handlers to be able to return > NULL > > to indicate that we should fall back to the built-in HTTP factories. That's > the > > one behavior that will be different. > > Cool, that sounds fine. I don't think we'd need dynamic switching like that. Great, this approach seems fine, then, as long as you're fine not getting the cookies, disk cache, and the like (Which was already mentioned on the doc, so I assume you can live with it).
On 2016/06/01 17:05:05, mmenke wrote: > Great, this approach seems fine, then, as long as you're fine not getting the > cookies, disk cache, and the like (Which was already mentioned on the doc, so I > assume you can live with it). Thanks for checking. Yes, we'll want to customize a bit how caching, cookies, etc. work here in any case. (We're still working out the details for these and other things from the storage partition.)
The CQ bit was checked by skyostil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2024973002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2024973002/40001
Message was sent while issue was closed.
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=546953 ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=546953 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=546953 ========== to ========== headless: Allow protocol handler customization Allow headless clients to provide custom protocol handlers. This lets the clients override how URL requests are fulfilled for different schemes types. Design doc: https://docs.google.com/document/d/1m4CCZGJWOJdHY5MVwQqY3sT-5kBoz7NqmGvIb6-l3... BUG=546953 Committed: https://crrev.com/a65d5e613d85dba0d97b45c651cad1e39ccc21bc Cr-Commit-Position: refs/heads/master@{#397184} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a65d5e613d85dba0d97b45c651cad1e39ccc21bc Cr-Commit-Position: refs/heads/master@{#397184} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
