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

Autoplay refactoring #10178

Merged
merged 1 commit into from
Jul 31, 2017
Merged

Autoplay refactoring #10178

merged 1 commit into from
Jul 31, 2017

Conversation

darkdh
Copy link
Member

@darkdh darkdh commented Jul 27, 2017

fixes #9143 - Autoplay notification should be dismissed if user ignores notification and continues stream
fixes #9008 - Playing media on a site adds it to the autoplay site permission list (even when defaulted to "always allow")
fixes #9171 - Notification bar - buttons not responding

Auditors: @bsclifton, @bridiver, @cezaraugusto

Test Plan:

(see top of each issue for steps to test)

Automated test plan

Covered by automated tests (see travis-ci output)

Tests added by this PR:
npm run test -- --grep="autoplay"
npm run unittest -- --grep="autoplayReducer unit tests"

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes look great! ❤️ Really nice work on this one 😄 👍

When I run the unit tests, I am seeing one error (not sure if related):

1) menuUtil tests createRecentlyClosedTemplateItems returns an array of closedFrames preceded by a separator and "Recently Closed" items:

      AssertionError: '' == 'RECENTLYCLOSED'
      + expected - actual

      +RECENTLYCLOSED

Can you check this out?

@darkdh
Copy link
Member Author

darkdh commented Jul 31, 2017

looking at it.
It only happens when doing whole unit test
If I run the test separately, it will pass
screen shot 2017-07-31 at 11 25 44 am

fixes #9143
fixes #9008
fixes #9171

Auditors: @bsclifton, @bridiver, @cezaraugusto

Test Plan:
Covered by automatic test

Specific STR for #9171:
1. Turn on autoplay to `always ask` and make sure there is no any allow
autoplay permissions
2. Go to https://youtu.be/g_6yBZKj-eo and don't click on notification
bar
3. Open another tab and go to
https://www.w3schools.com/tags/tryit.asp?filename=tryhtml5_video_autoplay
and still don't click on notification bar
4. Go back the youtube tab you will find out the notification bar is non
clickable
5. Go to the w3schools tab and it will also be non clickable
@darkdh
Copy link
Member Author

darkdh commented Jul 31, 2017

missing removeSiteSettingSpy.restore() and mockery.deregisterAll() added

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++!

@bsclifton bsclifton merged commit 0ca210a into master Jul 31, 2017
@bsclifton bsclifton deleted the autoplay_refactor branch July 31, 2017 22:31
bsclifton added a commit that referenced this pull request Jul 31, 2017
@codecov-io
Copy link

Codecov Report

Merging #10178 into master will increase coverage by 0.15%.
The diff coverage is 95.55%.

@@            Coverage Diff             @@
##           master   #10178      +/-   ##
==========================================
+ Coverage   52.78%   52.93%   +0.15%     
==========================================
  Files         227      228       +1     
  Lines       20219    20295      +76     
  Branches     3237     3251      +14     
==========================================
+ Hits        10672    10744      +72     
- Misses       9547     9551       +4
Flag Coverage Δ
#unittest 52.93% <95.55%> (+0.15%) ⬆️
Impacted Files Coverage Δ
js/constants/appConstants.js 100% <ø> (ø) ⬆️
js/actions/appActions.js 13.33% <0%> (-0.1%) ⬇️
app/browser/reducers/autoplayReducer.js 97.29% <100%> (ø)

darkdh pushed a commit that referenced this pull request Jan 4, 2018
@darkdh
Copy link
Member Author

darkdh commented Jan 4, 2018

0.19.x 5a7a5da

@cezaraugusto
Copy link
Contributor

0.21.x dbf83b2
0.20.x-backup 8c686b4
0.20.x 7375c25
0.19.x 5a7a5da

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.