Obtains the Publisher/Content Creators portrait image to use for the BAT panel favicon #3464
Conversation
NejcZdovc
changed the title
vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.cc
Outdated
Show resolved
Hide resolved
vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.cc
Outdated
Show resolved
Hide resolved
masparrow
force-pushed
the
issues/5720
branch
2 times, most recently
from
b219e49 to
879a529
Compare
There was a problem hiding this comment.
There is one regression compares to master:
Steps:
- start browser with production flag
- enable rewards
- go to https://vimeo.com/190607335
- click on BAT logo
master: Displays publisher with wrong img
this PR: Doesn't display publisher (or display it with a big delay). If it's displayed logo is not displayed
|
|
@masparrow switch to production and it will be verified |
masparrow
force-pushed
the
issues/5720
branch
from
be9862e to
879a529
Compare
| publisher_info->favicon_url = ledger::kClearFavicon; | ||
| if (publisher_info->favicon_url.empty()) { | ||
| publisher_info->favicon_url = ledger::kClearFavicon; | ||
| } |
There was a problem hiding this comment.
During a page load, we save the pub info a lot. At one point during the page load/parse, we have a valid favicon url, but at a later point, it is empty as it cannot be parsed (at that point). If an empty URL is passed in and we already have a favicon, this change prevents us clearing the existing favicon url. Can anyone see a problem with this, as other paths than Vimeo will use the same logic flow...
There was a problem hiding this comment.
I think we would have a problem with this code as if we save some wrong url that is not stored in chromium favicon storage then we will never clear it
There was a problem hiding this comment.
Bear in mind if we do parse a URL, it will always be saved. Also - if we implement the favicon cache expiration we mentioned, it will eventually self-repair even if the above did happen. WDYT? (I've yet to test this against production BTW - the change is here for discussion only ATM)
There was a problem hiding this comment.
Further - this doesn't seem to work anyway 🤔 which is a bit odd. The challenge ATM is that during the page scrape, the content doesn't have anything containing the Users portrait ID, so it ends up removing any prior Favicon.
masparrow
force-pushed
the
issues/5720
branch
from
083c8bc to
e010255
Compare
| @@ -259,7 +259,9 @@ void Publisher::SaveVisitInternal( | |||
| publisher_info->favicon_url = fav_icon; | |||
| } | |||
| } else { | |||
| publisher_info->favicon_url = ledger::kClearFavicon; | |||
| if (publisher_info->favicon_url.empty()) { | |||
There was a problem hiding this comment.
This should check for !is_verified and always clear if that is the case (discussed with @NejcZdovc)
|
Closing as stale |
Resolves brave/brave-browser#5720
Submitter Checklist:
npm run lint)git rebase master(if needed).git rebase -ito squash commits (if needed).Test Plan:
(With a clean install, or an existing install and using a previously unvisited Vimeo publisher)
Nb. with existing (real world) installs, previously visited publishers will still have the wrong favicon, as it is cached. A separate ticket will be raised to resolve this.
Reviewer Checklist:
After-merge Checklist:
changes has landed on.