Skip to content

Add gatheringTimeoutAfterHost option #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Rob--W
Copy link

@Rob--W Rob--W commented Sep 12, 2015

Add "gatheringTimeoutAfterHost" option. The existing gatheringTimeout option serves a similar purpose, with a subtle distinction:

  • gatheringTimeout forces ICE gathering to stop after a fixed amount of time, even if no candidates have been found.
  • gatheringTimeoutAfterHost waits until at least one host candidate has been found before starting the timer.

The timer logic was spread all over the place, I also deduplicated the logic and put the relevant logic in two helper functions.

Add "gatheringTimeoutAfterHost" option. The existing gatheringTimeout
option serves a similar purpose, with a subtle distinction:
- gatheringTimeout forces ICE gathering to stop after a fixed amount of
  time, even if no candidates have been found.
- gatheringTimeoutAfterHost waits until at least one host candidate has
  been found before starting the timer.

The timer logic was spread all over the place, I also deduplicated the
logic and put the relevant logic in two helper functions.
@Rob--W Rob--W force-pushed the ice-gathering-timers branch from 10ab440 to 1491023 Compare September 12, 2015 17:23
@Rob--W
Copy link
Author

Rob--W commented Nov 14, 2015

@ibc Could you review and merge this PR?

@@ -16,6 +16,7 @@ var merge = require('merge'),
REGEXP_NORMALIZED_CANDIDATE: new RegExp(/^candidate:/i),
REGEXP_FIX_CANDIDATE: new RegExp(/(^a=|\r|\n)/gi),
REGEXP_RELAY_CANDIDATE: new RegExp(/ relay /i),
REGEXP_HOST_CANDIDATE: / typ host/i,
Copy link
Contributor

Choose a reason for hiding this comment

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

May you follow the same syntax here?

Copy link
Author

Choose a reason for hiding this comment

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

I can do that.

Why do you use the RegExp constructor in the first place?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would accept the faster solution regardless which one it is. I expected that creating a RegExp instance is the same as creating a literal regexp.

Copy link
Author

Choose a reason for hiding this comment

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

The literal regexp already returns a RegExp instance. So what you're doing is basically taking a RegExp and constructing a new instance of it - 2 instances in total.

I'll update the PR with a commit that removes all unnecessary RegExp constructions.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's fine, thanks.

@ibc
Copy link
Contributor

ibc commented Mar 18, 2016

Sorry for the late response.

Honestly, I think we should avoid custom API. Both gatheringTimeout and gatheringTimeoutAfterRealy provide a temporal workaround for non trickle-ICE users, but I don't feel comfortable having them.

`/pattern/flags` already results in a RegExp instance,
there is no point in using `new RegExp(/pattern/flags)`.
@Rob--W
Copy link
Author

Rob--W commented Mar 30, 2016

@ibc Done, thanks for the review!

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

Successfully merging this pull request may close these issues.

2 participants