Adds twitch support #13142

NejcZdovc · 2018-02-14T17:51:56Z

Resolves #13139

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:

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

codecov-io · 2018-02-14T18:19:19Z

Codecov Report

Merging #13142 into master will increase coverage by 0.14%.
The diff coverage is 86.77%.

@@            Coverage Diff             @@
##           master   #13142      +/-   ##
==========================================
+ Coverage   55.93%   56.07%   +0.14%     
==========================================
  Files         281      282       +1     
  Lines       27856    28017     +161     
  Branches     4571     4602      +31     
==========================================
+ Hits        15580    15711     +131     
- Misses      12276    12306      +30

Flag	Coverage Δ
#unittest	`56.07% <86.77%> (+0.14%)`	⬆️

Impacted Files	Coverage Δ
app/filtering.js	`18% <0%> (+0.08%)`	⬆️
app/common/constants/twitchEvents.js	`100% <100%> (ø)`
app/common/constants/ledgerMediaProviders.js	`100% <100%> (ø)`	⬆️
app/browser/api/ledger.js	`58.68% <66.66%> (-0.11%)`	⬇️
...erer/components/preferences/payment/ledgerTable.js	`86.05% <84.61%> (-0.45%)`	⬇️
app/common/cache/ledgerVideoCache.js	`95.45% <86.66%> (-4.55%)`	⬇️
app/common/lib/ledgerUtil.js	`94.1% <91.97%> (-2.18%)`	⬇️
js/stores/appStoreRenderer.js	`91.66% <0%> (-8.34%)`	⬇️
app/renderer/components/reduxComponent.js	`57.75% <0%> (-3.45%)`	⬇️
js/stores/windowStore.js	`27.65% <0%> (-0.3%)`	⬇️
... and 4 more

NejcZdovc · 2018-02-15T15:14:47Z

blocked on brave-intl/bat-publisher#12

bsclifton · 2018-02-22T17:24:47Z

app/common/lib/ledgerUtil.js

+ // Twitch
+ if (
+ (
+ (firstPartyUrl && firstPartyUrl.startsWith('https://www.twitch.tv')) ||


food for thought- with more integrations being added, it might make sense to move the URLs into ledgerMediaProviders.js. That module could then export:

type (ex: YOUTUBE, TWITCH)

providerUrls (indexed by type)

this url's are used only here, so I wouldn't add it into the providers

bsclifton · 2018-02-22T17:40:32Z

app/common/lib/ledgerUtil.js

+ {
+ if (
+ (
+ data.event === 'minute-watched' ||


these events would be great to put into a module (so it could work like an enum); ex: twitchEvents.MINUTE_WATCHED

another way you could do this:
if (['minute-watched', 'video-play', 'player_click_playpause', 'vod_seek'].includes(data.event))

added new syntax

bsclifton · 2018-02-22T17:45:30Z

app/common/lib/ledgerUtil.js

+ const previousData = ledgerVideoCache.getDataByVideoId(state, mediaKey)
+
+ if (previousData.isEmpty() && data.event === 'video-play') {
+ return milliseconds.second * 10


this (and the magic number 10 below) might be better expressed as a constant; like twitchMinimumSeconds (or something like this). For readability and also so it's easy to change both (if needed)

bsclifton · 2018-02-22T17:46:19Z

app/common/lib/ledgerUtil.js

+ return milliseconds.second * 10
+ }
+
+ if (!data.properties) {


should this be moved up to the initial check? ex:
if (data == null || mediaKey == null || !data.properties) {

we can, done

bsclifton · 2018-02-22T17:47:26Z

app/common/lib/ledgerUtil.js

+ }
+
+ // we get seconds back, so we need to convert it into ms
+ time = time * 1000


for readability, you can use milliseconds.second instead of 1000 😄

bsclifton · 2018-02-22T17:51:36Z

app/common/lib/ledgerUtil.js

+ })
+}
+
+const getMediaFavicon = (providerName) => {


would this be better named as getDefaultMediaFavicon, since it would only be used when one is not present (ex: when watching live stream on Twitch)

bsclifton

Comment left 😄 Overall- awesome job! Works extremely well

Initial findings:

~~seeking doesn't record time watched before heartbeat~~ you fixed this issue 😄
playing again (after pause) and then pausing AGAIN doesn't capture time. I captured Resume after pause on twitch media does not resume timer #13257 for this

NOT TWITCH SPECIFIC (existing bug)
When criteria is met, publisher button (to right of URL bar) doesn't become clickable to include. You mentioned this would be fixed though w/ next version

diracdeltas · 2018-02-22T18:05:45Z

so do you still need the Twitch scraper in https://github.com/brave-intl/bat-publisher/blob/master/getMedia.js#L182 ? if not let's revert it.

NejcZdovc · 2018-02-22T18:30:55Z

@diracdeltas we are not using scraper for Twitch. We skip it here https://github.com/brave-intl/bat-publisher/blob/master/getMedia.js#L151

diracdeltas · 2018-02-22T22:08:31Z

app/common/lib/ledgerUtil.js

+ // Twitch
+ if (
+ (
+ (firstPartyUrl && firstPartyUrl.startsWith('https://www.twitch.tv')) ||


this origin check can be spoofed, e.g. by a request to 'https://www.twitch.tv.evil.com'. you probably want to add a trailing slash or do a proper URL parse.

diracdeltas · 2018-02-22T22:08:49Z

app/common/lib/ledgerUtil.js

+ if (
+ (
+ (firstPartyUrl && firstPartyUrl.startsWith('https://www.twitch.tv')) ||
+ (referrer && referrer.startsWith('https://player.twitch.tv'))


same issue as https://github.com/brave/browser-laptop/pull/13142/files#r170109044

diracdeltas · 2018-02-22T22:08:57Z

app/common/lib/ledgerUtil.js

+ (firstPartyUrl && firstPartyUrl.startsWith('https://www.twitch.tv')) ||
+ (referrer && referrer.startsWith('https://player.twitch.tv'))
+ ) &&
+ url.startsWith('https://api.mixpanel.com')


same issue as https://github.com/brave/browser-laptop/pull/13142/files#r170109044

diracdeltas · 2018-02-22T22:15:05Z

app/filtering.js

 // this can happen if the tab is closed and the webContents is no longer available
 if (!firstPartyUrl) {
 muonCb({ cancel: true })
 return
 }

+ if (module.exports.isResourceEnabled('ledger') && module.exports.isResourceEnabled('ledgerMedia')) {


curious why this was moved

also this should check !isPrivate. i know it checks for a private tab state later in the ledger reducer, but can't hurt to do it here

added private check

this block was moved, because we need to get data from mixpanel which is blocked on our side and we need to get data first and then block it

diracdeltas

https://github.com/brave/browser-laptop/pull/13142/files#r170109044 and following comments

bsclifton

Tested again after updates per code review. Works great! Made sure to try Twitch in a private tab and confirmed nothing is recorded. Nice job 😄

Resolves brave#13139 Auditors: Test Plan:

diracdeltas · 2018-02-23T06:32:04Z

docs/state.md

@@ -93,6 +93,7 @@ AppStore
 ledgerVideos: {
 [mediaKey]: {
 publisher: string // publisher key
+ beatData: object // data that we get from a heartbeat


where is this set?

comments addressed

bsclifton · 2018-02-23T07:03:20Z

master e1738ad

@NejcZdovc can you help me cherry-pick into 0.22.x and 0.21.x?

Adds twitch support

NejcZdovc · 2018-02-23T08:32:28Z

0.22 33c7601
0.21 0f77c26

Adds twitch support

NejcZdovc · 2018-02-24T12:31:43Z

0.21 twitch d39cb3c

NejcZdovc added feature/rewards PR/work-in-progress ⚒ labels Feb 14, 2018

NejcZdovc self-assigned this Feb 14, 2018

NejcZdovc force-pushed the feature/#13139-twitch branch from b8b0d8e to 09f2bba Compare February 15, 2018 15:05

NejcZdovc added this to the 0.21.x (Beta Channel) milestone Feb 15, 2018

NejcZdovc added the PR/blocked label Feb 15, 2018

NejcZdovc force-pushed the feature/#13139-twitch branch 4 times, most recently from 05ac8c1 to 9b98357 Compare February 20, 2018 18:48

NejcZdovc removed the PR/blocked label Feb 21, 2018

NejcZdovc force-pushed the feature/#13139-twitch branch 5 times, most recently from a1bbea0 to 96073c3 Compare February 22, 2018 06:26

NejcZdovc removed the PR/work-in-progress ⚒ label Feb 22, 2018

NejcZdovc requested a review from bsclifton February 22, 2018 06:28

NejcZdovc force-pushed the feature/#13139-twitch branch from 96073c3 to 3fbf258 Compare February 22, 2018 09:57

bsclifton force-pushed the feature/#13139-twitch branch from 3fbf258 to c54a1e3 Compare February 22, 2018 16:12

bsclifton reviewed Feb 22, 2018

View reviewed changes

NejcZdovc force-pushed the feature/#13139-twitch branch from c54a1e3 to c1186ae Compare February 22, 2018 17:40

bsclifton reviewed Feb 22, 2018

View reviewed changes

bsclifton suggested changes Feb 22, 2018

View reviewed changes

bsclifton mentioned this pull request Feb 22, 2018

Resume after pause on twitch media does not resume timer #13257

Closed

diracdeltas mentioned this pull request Feb 22, 2018

add twitch#channel URI brave-intl/bat-publisher#11

Merged

NejcZdovc requested a review from bsclifton February 22, 2018 19:26

diracdeltas reviewed Feb 22, 2018

View reviewed changes

diracdeltas previously requested changes Feb 22, 2018

View reviewed changes

NejcZdovc requested a review from diracdeltas February 23, 2018 05:03

bsclifton previously approved these changes Feb 23, 2018

View reviewed changes

Adds twitch support

b0738dc

Resolves brave#13139 Auditors: Test Plan:

NejcZdovc dismissed bsclifton’s stale review via b0738dc February 23, 2018 06:15

NejcZdovc force-pushed the feature/#13139-twitch branch from edc1e12 to b0738dc Compare February 23, 2018 06:15

bsclifton approved these changes Feb 23, 2018

View reviewed changes

diracdeltas reviewed Feb 23, 2018

View reviewed changes

bsclifton merged commit e1738ad into brave:master Feb 23, 2018

NejcZdovc pushed a commit that referenced this pull request Feb 23, 2018

Merge pull request #13142 from NejcZdovc/feature/#13139-twitch

33c7601

Adds twitch support

NejcZdovc pushed a commit that referenced this pull request Feb 23, 2018

Merge pull request #13142 from NejcZdovc/feature/#13139-twitch

0f77c26

Adds twitch support

NejcZdovc modified the milestones: 0.21.x (Beta Channel), 0.21.x (Twitch) Feb 24, 2018

NejcZdovc pushed a commit that referenced this pull request Feb 24, 2018

Merge pull request #13142 from NejcZdovc/feature/#13139-twitch

d39cb3c

Adds twitch support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds twitch support #13142

Adds twitch support #13142

NejcZdovc commented Feb 14, 2018 •

edited

Loading

codecov-io commented Feb 14, 2018 •

edited

Loading

NejcZdovc commented Feb 15, 2018

bsclifton Feb 22, 2018

NejcZdovc Feb 22, 2018

bsclifton Feb 22, 2018

bsclifton Feb 22, 2018 •

edited

Loading

NejcZdovc Feb 22, 2018

bsclifton Feb 22, 2018

NejcZdovc Feb 22, 2018

bsclifton Feb 22, 2018

NejcZdovc Feb 22, 2018

bsclifton Feb 22, 2018

NejcZdovc Feb 22, 2018

bsclifton Feb 22, 2018

NejcZdovc Feb 22, 2018

bsclifton left a comment •

edited

Loading

diracdeltas commented Feb 22, 2018

NejcZdovc commented Feb 22, 2018

diracdeltas Feb 22, 2018

NejcZdovc Feb 23, 2018

diracdeltas Feb 22, 2018

NejcZdovc Feb 23, 2018

diracdeltas Feb 22, 2018

NejcZdovc Feb 23, 2018

diracdeltas Feb 22, 2018

diracdeltas Feb 23, 2018 •

edited

Loading

NejcZdovc Feb 23, 2018

NejcZdovc Feb 23, 2018

diracdeltas left a comment

bsclifton left a comment

diracdeltas Feb 23, 2018

bsclifton commented Feb 23, 2018 •

edited

Loading

NejcZdovc commented Feb 23, 2018

NejcZdovc commented Feb 24, 2018

Adds twitch support #13142

Adds twitch support #13142

Conversation

NejcZdovc commented Feb 14, 2018 • edited Loading

Submitter Checklist:

Test Plan:

Reviewer Checklist:

codecov-io commented Feb 14, 2018 • edited Loading

Codecov Report

NejcZdovc commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

diracdeltas commented Feb 22, 2018

NejcZdovc commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas left a comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Feb 23, 2018 • edited Loading

NejcZdovc commented Feb 23, 2018

NejcZdovc commented Feb 24, 2018

NejcZdovc commented Feb 14, 2018 •

edited

Loading

codecov-io commented Feb 14, 2018 •

edited

Loading

bsclifton Feb 22, 2018 •

edited

Loading

bsclifton left a comment •

edited

Loading

diracdeltas Feb 23, 2018 •

edited

Loading

bsclifton commented Feb 23, 2018 •

edited

Loading