Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use Network Information API to react to network changes #2663

Merged
merged 9 commits into from
Jul 9, 2020
Merged

Use Network Information API to react to network changes #2663

merged 9 commits into from
Jul 9, 2020

Conversation

avelad
Copy link
Member

@avelad avelad commented Jun 18, 2020

Resolves #1067

joeyparrish
joeyparrish requested changes Jun 29, 2020
View reviewed changes
Copy link
Member

@joeyparrish joeyparrish left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this!

lib/abr/simple_abr_manager.js Outdated Show resolved Hide resolved
lib/abr/simple_abr_manager.js Outdated Show resolved Hide resolved
avelad
avelad commented Jun 30, 2020
View reviewed changes
lib/abr/simple_abr_manager.js Outdated Show resolved Hide resolved
@avelad avelad requested a review from joeyparrish June 30, 2020 06:58
@avelad
Copy link
Member Author

avelad commented Jun 30, 2020

@TheModMaker I change it, but the new error is:

[WARNING] No changes detected, skipping. Use --force to override.
[INFO] Generating Closure dependencies...
[INFO] Linting JavaScript...
[INFO] Linting CSS...
[WARNING] No changes detected, skipping. Use --force to override.
[INFO] Linting HTML...
[WARNING] No changes detected, skipping. Use --force to override.
[INFO] Checking that the build files are complete...
[INFO] Checking for common misspellings...
[INFO] Checking correct usage of eslint-disable...
[INFO] Checking the tests for type errors...
[WARNING] No changes detected, skipping. Use --force to override.
/Users/alvaro.velad/workspace/pullrequest-shaka-player/externs/network_information.js:32: ERROR - [JSC_VAR_MULTIPLY_DECLARED_ERROR] Variable NetworkInformation declared more than once. First occurrence: externs.zip//w3c_netinfo.js
const NetworkInformation = {};
      ^^^^^^^^^^^^^^^^^^^^^^^

1 error(s), 0 warning(s)
[ERROR] Build failed

Can you guide me in the right direction?

@TheModMaker
Copy link
Contributor

The NetworkInformation is defined in the compiler's externs, so you can't re-define it. You could just add the methods like there is a new property in that file.

@avelad
Copy link
Member Author

avelad commented Jun 30, 2020 •
edited
Loading

OK, I added addEventListener and now it works. Thanks!

@avelad
Copy link
Member Author

avelad commented Jun 30, 2020

Ready to review again!

@avelad avelad requested a review from TheModMaker July 8, 2020 06:11
@avelad
Copy link
Member Author

avelad commented Jul 9, 2020

Friendly ping!

@joeyparrish joeyparrish merged commit 943bdb2 into shaka-project:master Jul 9, 2020
@avelad avelad deleted the network-changes branch July 10, 2020 11:37
shaka-bot pushed a commit that referenced this pull request Jul 22, 2020
@joeyparrish
fix(Tizen): Fix initial bandwidth estimate on Tizen
ef7767f 
Tizen 3 has the NetworkInformation API, but not the downlink
attribute.  This led to NaN as a bandwidth estimate.  Now we screen
for the presence of this attribute as well as the API overall.

Futhermore, we now disable this API in unit tests to better control
the fake estimates used during the tests.

Found while investigating #2620.  Introduced in PR #2663, and has not
affected any release versions.

Change-Id: Iaa1486545825ceee536ecbe5ea617f92de4fbc2d
@joeyparrish joeyparrish mentioned this pull request May 4, 2021
Closed
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

@michellezhuogg michellezhuogg michellezhuogg approved these changes

@TheModMaker TheModMaker Awaiting requested review from TheModMaker

Assignees
No one assigned
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use Network Information API to react to network changes
4 participants
@avelad @TheModMaker @joeyparrish @michellezhuogg