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.

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

Panel, tipping, and favicon should now be working correctly for twitch publishers #888

Merged
merged 4 commits into from
Jan 22, 2019
Merged

Panel, tipping, and favicon should now be working correctly for twitch publishers #888

merged 4 commits into from
Jan 22, 2019

Conversation

jasonrsadler
Copy link
Contributor

@jasonrsadler jasonrsadler commented Nov 11, 2018

Fixes brave/brave-browser#1814
Fixes brave/brave-browser#2089
Fixes brave/brave-browser#1521

Rebased off of brave-intl/bat-native-ledger#177

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).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • 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.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

  1. Start Brave and navigate to twitch.tv
  2. navigate to a publisher's live stream
  3. Click BAT panel and ensure that tipping functions correctly
  4. navigate to a publisher's live stream.
  5. Select Videos from the channel headings and select a video-on-demand
  6. Click BAT panel and ensure that tipping functions correctly
  7. navigate to a publisher's live stream.
  8. Select Videos from the channel headings and select a clip to play
  9. Click BAT panel and ensure that tipping functions correctly
  10. Repeat steps 1-9 for verified publishers and ensure that correct favicon displays

EDIT: Favicon should only show for verified publishers

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

@jasonrsadler jasonrsadler changed the title WIP - Twitch favicon and panel fixes Panel, tipping, and favicon should now be working correctly for twitch publishers Dec 2, 2018
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

  1. if you go to https://www.twitch.tv/directory/game/Fortnite you get
    image

It should be regular twitch publisher info. Same happens for https://app.twitch.tv/download, https://www.twitch.tv/p/partners, https://help.twitch.tv/customer/en/portal/articles/658863-creating-an-account-with-twitch, etc

  1. seeing a lot of empty logs like this
    [87144:775:1205/103540.829246:VERBOSE1:bat_get_media.cc(62)] Media Id:

  2. please open security review for it

  3. I get this errors when I close browser

[87144:775:1205/104748.100851:INFO:CONSOLE(0)] "Error handling response: TypeError: Cannot read property '0' of undefined
    at chrome-extension://jidkidbbcafjabdphckchenhfomhnfma/out/brave_rewards_panel_background.bundle.js:426:35", source: chrome-extension://jidkidbbcafjabdphckchenhfomhnfma/_generated_background_page.html (0)
[87144:775:1205/104748.172842:INFO:CONSOLE(0)] "Unchecked runtime.lastError: Cannot access contents of url "https://app.twitch.tv/download". Extension manifest must request permission to access this host.", source: chrome-extension://jidkidbbcafjabdphckchenhfomhnfma/_generated_background_page.html (0)

'<img class=\"tw-avatar__img tw-image\" alt=\"'
let itr = 0
const limit = 20
let interval = setInterval(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

does this effect responsiveness of twitch in anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

who clears interval is you close panel?

Copy link
Contributor Author

@jasonrsadler jasonrsadler Dec 5, 2018

Choose a reason for hiding this comment

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

interval is cleared at limit still if panel is closed. Although it still clears it can be restarted by opening the panel again. I would think we could set the limit down to 4 or 5 (vice 20). wdyt?

Even under a development run I do not notice any considerable difference in performance when interacting with the site. This is a small line of eval code that returns the outerHtml that, when generated with the parts we need, equates to roughly 220Kb

Copy link
Contributor

Choose a reason for hiding this comment

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

can we clear interval as soon as you close panel?

Copy link
Contributor Author

@jasonrsadler jasonrsadler Dec 5, 2018

Choose a reason for hiding this comment

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

There may be a way but it would involve setting a local storage flag whenever panel is opened and then resetting it when it closes.

example:

chrome.runtime.onConnect.addListener(function (externalPort) {
  chrome.storage.local.set({
    'rewards_panel_open': true
  })
  externalPort.onDisconnect.addListener(function () {
    chrome.storage.local.set({
      'rewards_panel_open': false <-- we would read from local storage in the interval function after this is set and clear it
    })
  })
})

I'm in the process of testing this now but do you see any immediate problems that could lead to inconsistencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition, we seem to get the information we need right away, so I would also want to shorten the limit as a safeguard fallback

Copy link
Contributor Author

@jasonrsadler jasonrsadler Dec 5, 2018

Choose a reason for hiding this comment

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

So using the above, we are able to clear the timeout when the panel is closed. Test is successful. Let me know if your tests are satisfactory and I'll remove it.

@jasonrsadler
Copy link
Contributor Author

jasonrsadler commented Dec 5, 2018

For a directory path, we can specifically include that in the paths to ignore under getActivityFromUrl or we can start a list of paths that should ignore media handling (For the even we discover other directories that are site specific rather than publisher specific). Which would you prefer?

Fixed. Added a separate "getPublisherData" call that is specific to twitch pages. We hand off to GetActivityFromUrl within here so no functionality is missed.

@jasonrsadler
Copy link
Contributor Author

jasonrsadler commented Dec 5, 2018

seeing a lot of empty logs like this [87144:775:1205/103540.829246:VERBOSE1:bat_get_media.cc(62)] Media Id:
This is currently happening on master. The log statement should be after
if (mediaId.empty()) { return; }

I moved it below to prevent log spamming. (native-ledger)

@jasonrsadler
Copy link
Contributor Author

"I get this errors when I close browser"

clearing the interval when closing the panel should minimize the first error.

I've modified the scripting to only include 'www' twitch urls. We don't need info from other subdomains.

Copy link
Member

@diracdeltas diracdeltas left a comment

Choose a reason for hiding this comment

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

see comments above. also needs tests.

@jasonrsadler
Copy link
Contributor Author

Still one small intermittent issue but I don't think we will see it on a release build. If panel is open when a new page is about to load or loading, It could populate the publisher and then switch back to twitch. Closing the panel and reopening will mitigate.

Fixes brave/brave-browser#2089
Fixes brave/brave-browser#1521

Updates for twitch panel

Add conditional for verified favicon

Testing vod vs live favicon pull

Adding conditional and fixing var name

Cleaning up debug code

Added pointer check and verified becoming non-verified check
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

There are two follow up's that we need to do, otherwise looks good

brave/brave-browser#3043
brave/brave-browser#3044

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

Successfully merging this pull request may close these issues.

No Tipping Option on YouTube (or Twitch) Cannot tip directly to Twitch channels. Favicon is not shown
5 participants