Skip to content
This repository has been archived by the owner on Jul 3, 2019. It is now read-only.

Implement LocalConnection #2225

Closed
wants to merge 6 commits into from

Conversation

tschneidereit
Copy link
Contributor

Not finished, contains only the content-facing parts. Those should be pretty much done, though, modulo bug fixes.

Review on Reviewable

@tschneidereit tschneidereit force-pushed the localconnection branch 2 times, most recently from fc1b134 to 54328b9 Compare May 6, 2015 15:59
@@ -273,6 +291,98 @@ var ShumwayCom = {
return;
}
onSystemResourceCallback = callback;
},
createLocalConnection: function(connectionName, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this code into separate jsm-files (similar to RtmpUtils and SpecialInflateUtils). You can create e.g. localConnections object that will encapsulate the logic, the _getLocalConnection and localConnectionsRegistry can be moved there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had planned on doing that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, place it under the preference for now.

And this is why all changes should go through review.
The various backends are stubbed out and do nothing whatsoever right now.
This fully works for the non-cross-domain case, and not in the extension. It'll change a bit to move error handling out of the message handler - across iframes we won't be able to create exceptions in the sender's `AXSecurityDomain`.
This fully works, including allowDomain. It needs very careful checking and probably a bugfix or two, though.
By setting it for the base URL and stripping it when loading files.
Contains a shell trace test. While the test only excerts the player-internal implementation of the message sending machinery, it does test all the content-facing parts and all error handling. I verified that the implementation works as expected across browser tabs (within the same browser instance, of course) with Manual testing.
@tschneidereit
Copy link
Contributor Author

r? @yurydelendik after actually getting things working.

@yurydelendik
Copy link
Contributor

Review status: 11 of 36 files reviewed, 6 unresolved discussions, some commit checks failed.
Reviewed files:

  • examples/inspector/js/inspectorPlayer.js @ r3
  • extension/firefox/content/web/viewerPlayer.js @ r2
  • src/avm2/errors.ts @ r2
  • src/flash/events/AsyncErrorEvent.as @ r2
  • src/flash/events/AsyncErrorEvent.ts @ r2
  • src/flash/events/StatusEvent.as @ r2
  • src/flash/events/StatusEvent.ts @ r2
  • src/flash/link.ts @ r2
  • src/flash/module.ts @ r2
  • src/flash/net/LocalConnection.as @ r2
  • src/flash/references.ts @ r2

extension/firefox/chrome/ShumwayCom.jsm, line 120 [r3] (raw file):
Unneeded change


src/base/external.ts, line 54 [r3] (raw file):
replace all LocalConnection members with getLocalConnectionManager which will return an object (as example see createSpecialStorage above) with these methods.


src/flash/display/LoaderInfo.ts, line 129 [r3] (raw file):
We shall do it before constructing LoaderInfo.


src/flash/references.ts, line 64 [r3] (raw file):
nit: remove extra space before '///'


src/flash/references.ts, line 91 [r3] (raw file):
nit: remove extra space before '///'


src/player/player.ts, line 61 [r3] (raw file):
This entire class move is unrelated to LocalConnection, so skipping review for that. Please submit separate PR if review of that part is needed.


Comments from the review on Reviewable.io

@yurydelendik
Copy link
Contributor

Okay, reviewable.io does not allow fully review this PR

screen shot 2015-05-29 at 1 06 15 pm

Could you submit a PR that will just move Player class? It's not related to this PR. If this PR depends on this, let's block it by the Player class refactoring.

(Also, please notice that Travis is failing)

@tschneidereit
Copy link
Contributor Author

Closing because reviewable.io really doesn't like this PR. Probably caused by first renaming player.ts and then doing the renaming in another PR, rebasing this one on top of that. Ideally, reviewable should just ignore the entire renaming operation, but it doesn't. Will open a fresh PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants