Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Closes #6027 Add support for video autoplay permissions #5953

Merged
merged 2 commits into from
Feb 24, 2020
Merged

Closes #6027 Add support for video autoplay permissions #5953

merged 2 commits into from
Feb 24, 2020

Conversation

severinrudie
Copy link
Contributor

For Fenix#8411: fix crash in SitePermissionsFeature#updateSitePermissionsStatus

@Amejia481 I've spent a few hours trying to reproduce this crash without any luck. Hopefully this fixes the crash (and enables mozilla-mobile/fenix#5636), but at the very least it looks unlikely to break anything.

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
    I don't think this one needs a changelog entry? Happy to add one if I'm wrong.
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@severinrudie severinrudie added the 🕵️‍♀️ needs review PRs that need to be reviewed label Feb 14, 2020
@codecov
Copy link

codecov bot commented Feb 15, 2020 •
edited
Loading

Codecov Report

Merging #5953 into master will decrease coverage by 0.83%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #5953      +/-   ##
============================================
- Coverage     79.08%   78.25%   -0.84%     
+ Complexity     5299      368    -4931     
============================================
  Files           578       57     -521     
  Lines         26671     2249   -24422     
  Branches       3888      292    -3596     
============================================
- Hits          21094     1760   -19334     
+ Misses         3977      397    -3580     
+ Partials       1600       92    -1508
Impacted Files Coverage Δ Complexity Δ
...r/awesomebar/layout/DefaultSuggestionViewHolder.kt 90.00% <0.00%> (ø) 0.00% <0.00%> (ø%) ⬆️
...va/mozilla/components/service/sync/logins/Types.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%) ⬆️
...omponents/browser/awesomebar/SuggestionsAdapter.kt 90.00% <0.00%> (ø) 15.00% <0.00%> (ø%) ⬆️
...ozilla/components/support/android/test/LiveData.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%) ⬆️
...ts/support/android/test/leaks/LeakDetectionRule.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%) ⬆️
...illa/components/support/test/libstate/ext/Store.kt 80.00% <0.00%> (-14.45%) 0.00% <0.00%> (ø%)
...port/android/test/espresso/matcher/ViewMatchers.kt 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%) ⬆️
...mponents/service/sync/logins/AsyncLoginsStorage.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%) ⬆️
...vice/sync/logins/DefaultLoginValidationDelegate.kt 0.00% <0.00%> (ø) 0.00% <0.00%> (ø%) ⬆️
...ozilla/components/support/android/test/Matchers.kt 100.00% <0.00%> (+26.19%) 3.00% <0.00%> (-21.00%) ⬆️
... and 550 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3655ffe...0828215. Read the comment docs.

Amejia481
Amejia481 reviewed Feb 15, 2020
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great job it looks good! 👍
I believe we also have to add dialogs for them , if we don't do it, I think the exception could still happen and the feature won't be enable on Fenix mozilla-mobile/fenix#5636 .

@Amejia481 Amejia481 self-assigned this Feb 15, 2020
@Amejia481
Copy link
Contributor

I almost forget to mention, to automatically allow or deny auto play we need to add it to the SitePermissionsRules class

@Amejia481
Copy link
Contributor

Related mozilla-mobile/fenix#7507 (comment)

@Amejia481
Copy link
Contributor

As we don't want to show prompts for autoplay let's not implement #5953 (review) as in the future we will provide other ways to notify Fenix when a site is requesting autoplay permission.

We can improve the commit message as the main goal is to add support for autoplay site permissions :)

@severinrudie severinrudie force-pushed the f8411-crash-invalid-param branch 2 times, most recently from b0a4c8c to 16d900a Compare February 20, 2020 02:01
@severinrudie severinrudie force-pushed the f8411-crash-invalid-param branch 2 times, most recently from ed7b939 to e374937 Compare February 20, 2020 19:44
@severinrudie severinrudie mentioned this pull request Feb 21, 2020
Merged
4 tasks
Amejia481
Amejia481 reviewed Feb 21, 2020
View reviewed changes
...rmissions/src/main/java/mozilla/components/feature/sitepermissions/SitePermissionsFeature.kt
Comment on lines +275 to +279
return if (permissionRequest.isForAutoplay() && sitePermissionsRules == null) {
permissionRequest.reject()
session.contentPermissionRequest.consume { true }
null
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

After we add support for adding autoplay settings per site, we can delete this part :)

@Amejia481 Amejia481 mentioned this pull request Feb 21, 2020
Closed
@severinrudie severinrudie mentioned this pull request Feb 22, 2020
Closed
@liuche
Copy link
Contributor

liuche commented Feb 24, 2020

@baron-severin @Amejia481 so is this something that will land today? I'd like to get it landed at least some time early this week to pick up into a later Beta build, for migration, but let me know how that looks.

@severinrudie
Copy link
Contributor Author

severinrudie commented Feb 24, 2020 •
edited
Loading

@baron-severin @Amejia481 so is this something that will land today? I'd like to get it landed at least some time early this week to pick up into a later Beta build, for migration, but let me know how that looks.

@liuche we plan to land it this morning.

@Amejia481 Amejia481 self-requested a review February 24, 2020 21:26
Amejia481
Amejia481 approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@Amejia481 Amejia481 left a comment

Choose a reason for hiding this comment

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

Lets 🚢 it!

@Amejia481 Amejia481 changed the title For Fenix#8411: fix crash in SitePermissionsFeature#updateSitePermiss… Closes #6027 Add support for video autoplay permissions Feb 24, 2020
@severinrudie
Copy link
Contributor Author

bors r=Amejia481

@Amejia481
Copy link
Contributor

bors r+

@bors
Copy link

bors bot commented Feb 24, 2020

Already running a review

@bors
Copy link

bors bot commented Feb 24, 2020

Build succeeded

@bors bors bot merged commit b055ac3 into mozilla-mobile:master Feb 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Amejia481 Amejia481 Amejia481 approved these changes

Assignees

@Amejia481 Amejia481

Labels
🕵️‍♀️ needs review PRs that need to be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@severinrudie @Amejia481 @liuche