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

Improves twitch logging time #13326

Merged
merged 1 commit into from
Mar 15, 2018
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Feb 27, 2018

Resolves #13257

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.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

Goal of this PR is to improve time logging when you play VOD (recorded videos).

  1. Clean profile
  2. Start browser with LEDGER_LOGGING=true npm run start (this will help you check out events)
  3. Enable payments
  4. Go to twitch and watch videos
  5. Try play, pause, seek in all possible combinations and variations
  6. Make sure that time is logged correctly (there can be +-1s)

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

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

@NejcZdovc NejcZdovc added this to the 0.21.x (Beta Channel) milestone Feb 27, 2018
@NejcZdovc NejcZdovc self-assigned this Feb 27, 2018
@codecov-io
Copy link

codecov-io commented Feb 27, 2018

Codecov Report

Merging #13326 into master will increase coverage by <.01%.
The diff coverage is 92%.

@@            Coverage Diff             @@
##           master   #13326      +/-   ##
==========================================
+ Coverage    56.5%    56.5%   +<.01%     
==========================================
  Files         284      284              
  Lines       28681    28710      +29     
  Branches     4737     4748      +11     
==========================================
+ Hits        16206    16223      +17     
- Misses      12475    12487      +12
Flag Coverage Δ
#unittest 56.5% <92%> (ø) ⬆️
Impacted Files Coverage Δ
app/common/constants/twitchEvents.js 100% <100%> (ø) ⬆️
app/browser/api/ledger.js 59.65% <62.5%> (+0.01%) ⬆️
app/common/lib/ledgerUtil.js 94.19% <97.56%> (+0.09%) ⬆️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
app/renderer/components/reduxComponent.js 57.75% <0%> (-3.45%) ⬇️
js/stores/windowStore.js 28.48% <0%> (-0.3%) ⬇️

@NejcZdovc NejcZdovc modified the milestones: 0.21.x (Beta Channel), 0.22.x (Developer Channel) Feb 27, 2018
@NejcZdovc NejcZdovc force-pushed the hotfix/#13257-logging branch 3 times, most recently from 35a4428 to da716d0 Compare February 28, 2018 14:28
mrose17
mrose17 previously approved these changes Mar 14, 2018
Copy link
Member

@mrose17 mrose17 left a comment

Choose a reason for hiding this comment

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

thanks! this is a great improvement.

Resolves brave#13257

Auditors:

Test Plan:
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.

Tried skipping back and forth, mixing in with live stream.
All scenarios I tried worked great! 😄 Awesome job

@bsclifton bsclifton merged commit 98ce6da into brave:master Mar 15, 2018
bsclifton added a commit that referenced this pull request Mar 15, 2018
bsclifton added a commit that referenced this pull request Mar 15, 2018
@bsclifton
Copy link
Member

master 98ce6da
0.23.x 780e7d5
0.22.x d7d339f

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

Successfully merging this pull request may close these issues.

4 participants