Join GitHub today
GitHub is home to over 36 million developers working together to host and review code, manage projects, and build software together.
Sign upfeat: migrate protocol module to NetworkService (Part 4) #18084
+449
−165
Conversation
electron-cation
bot
added
the
new-pr 🌱
label
May 1, 2019
deepak1556
approved these changes
May 1, 2019
| return; | ||
| } | ||
|
|
||
| new NodeStreamLoader(std::move(head), std::move(client), data.isolate(), |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
zcbenz
May 2, 2019
Author
Member
This seems to be the only choice, I couldn't find a way to have Chromium manage its lifetime. content::FileURLLoader is also managing its own lifetime:
https://cs.chromium.org/chromium/src/content/browser/file_url_loader_factory.cc?type=cs&g=0&l=341
This comment has been minimized.
This comment has been minimized.
deepak1556
May 2, 2019
Member
Oh cool, in that case should we check if producer_ and client_ handles are bound before accessing the methods when we receive data from JS land ?
This comment has been minimized.
This comment has been minimized.
zcbenz
May 2, 2019
Author
Member
content::FileURLLoader does not check bound for every call but it deletes itself when connection error happens, I have added the same for NodeStreamLoader.
electron-cation
bot
removed
the
new-pr 🌱
label
May 2, 2019
zcbenz
added some commits
Apr 30, 2019
zcbenz
force-pushed the
ns-protocol-stream
branch
from
3066e28
to
3fbc68e
May 2, 2019
zcbenz
merged commit 0a6eb8a
into
master
May 3, 2019
6 checks passed
Semantic Pull Request
ready to be squashed
Details
appveyor: win-ia32-testing
AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr
AppVeyor build succeeded
Details
appveyor: win-x64-testing
AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr
AppVeyor build succeeded
Details
release-notes
Release notes found
This comment has been minimized.
This comment has been minimized.
release-clerk
bot
commented
May 3, 2019
|
No Release Notes |
zcbenz
deleted the
ns-protocol-stream
branch
May 3, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
zcbenz commentedMay 1, 2019
Description of Change
Refs #15791.
This is part of the changes to reimplement the protocol module with NetworkService API, the new implementation lives in parallel with current implementation and only gets used when
--enable-features=NetworkServiceis passed.This PR implements the
protocol.registerStreamProtocolwith NetworkService APIs, and also refactors the code ofAtomURLLoaderFactory.Due to the migration of NetworkService APIs, it is now possible to return different types of responses in one protocol handler, I have added an experimental
protocol.registerProtocolAPI to demonstrate the possibility:Checklist
npm testpassesRelease Notes
Notes: no-notes