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.

Sign up for GitHub

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

Obtains the Publisher/Content Creators portrait image to use for the BAT panel favicon #3464

Closed
wants to merge 2 commits into from
Closed

Obtains the Publisher/Content Creators portrait image to use for the BAT panel favicon #3464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
127a7f2
Obtains the Publisher/Content Creators portrait image to use for the …
masparrow Sep 18, 2019
e010255
Only update empty favicon url if none present already
masparrow Sep 26, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 47 additions & 27 deletions vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,22 @@ std::string Vimeo::GetIdFromVideoPage(const std::string& data) {
}

// static
std::string Vimeo::GenerateFaviconUrl(const std::string& id) {
if (id.empty()) {
std::string Vimeo::GenerateFaviconUrl(const std::string& data) {
if (data.empty()) {
return "";
}

std::string portraitUrlID = braveledger_media::ExtractData(data,
"src_2x\":\"https:\\/\\/i.vimeocdn.com\\/portrait\\/", "_");

if (portraitUrlID.empty()) {
return "";
}

return base::StringPrintf("https://i.vimeocdn.com/portrait/%s_300x300.webp",
id.c_str());
std::string favIconUrl = base::StringPrintf("https://i.vimeocdn.com/portrait/%s_300x300.webp",
portraitUrlID.c_str());

return favIconUrl;
}

// static
Expand Down Expand Up @@ -425,6 +434,8 @@ void Vimeo::OnEmbedResponse(
publisher_name = *data->FindStringKey("author_name");
}

const std::string publisher_favicon = GenerateFaviconUrl(response);

const std::string media_key = GetMediaKey(std::to_string(video_id),
"vimeo-vod");

Expand All @@ -433,6 +444,7 @@ void Vimeo::OnEmbedResponse(
media_key,
publisher_url,
publisher_name,
publisher_favicon,
visit_data,
window_id,
_1,
Expand All @@ -446,6 +458,7 @@ void Vimeo::OnPublisherPage(
const std::string& media_key,
const std::string& publisher_url,
const std::string& publisher_name,
const std::string& publisher_favicon,
const ledger::VisitData& visit_data,
const uint64_t window_id,
int response_status_code,
Expand All @@ -470,6 +483,7 @@ void Vimeo::OnPublisherPage(
publisher_url,
publisher_key,
publisher_name,
publisher_favicon,
user_id);
}

Expand All @@ -492,6 +506,7 @@ void Vimeo::OnUnknownPage(

std::string user_id = GetIdFromPublisherPage(response);
std::string publisher_name;
const std::string publisher_favicon = GenerateFaviconUrl(response);
std::string media_key;
if (!user_id.empty()) {
// we are on publisher page
Expand Down Expand Up @@ -521,6 +536,7 @@ void Vimeo::OnUnknownPage(
visit_data.url,
publisher_key,
publisher_name,
publisher_favicon,
user_id);
}

Expand All @@ -529,16 +545,19 @@ void Vimeo::OnPublisherPanleInfo(
uint64_t window_id,
const std::string& publisher_url,
const std::string& publisher_name,
const std::string& publisher_favicon,
const std::string& user_id,
ledger::Result result,
ledger::PublisherInfoPtr info) {
if (!info || result == ledger::Result::NOT_FOUND) {
SavePublisherInfo(media_key,
0,
user_id,
publisher_name,
publisher_url,
window_id);
0,
user_id,
publisher_name,
publisher_url,
window_id,
"",
publisher_favicon);
} else {
ledger_->OnPanelPublisherInfo(result, std::move(info), window_id);
}
Expand All @@ -550,6 +569,7 @@ void Vimeo::GetPublisherPanleInfo(
const std::string& publisher_url,
const std::string& publisher_key,
const std::string& publisher_name,
const std::string& publisher_favicon,
const std::string& user_id) {
auto filter = ledger_->CreateActivityFilter(
publisher_key,
Expand All @@ -565,6 +585,7 @@ void Vimeo::GetPublisherPanleInfo(
window_id,
publisher_url,
publisher_name,
publisher_favicon,
user_id,
_1,
_2));
Expand Down Expand Up @@ -607,13 +628,13 @@ void Vimeo::OnMediaPublisherInfo(
events[media_key] = event_info;

SavePublisherInfo("",
duration,
"",
publisher_info->name,
publisher_info->url,
0,
publisher_info->id,
publisher_info->favicon_url);
duration,
"",
publisher_info->name,
publisher_info->url,
0,
publisher_info->id,
publisher_info->favicon_url);
}

void Vimeo::OnPublisherVideoPage(
Expand Down Expand Up @@ -649,12 +670,16 @@ void Vimeo::OnPublisherVideoPage(
const uint64_t duration = GetDuration(old_event, event_info);
events[media_key] = event_info;

const std::string publisher_favicon = GenerateFaviconUrl(response);

SavePublisherInfo(media_key,
duration,
user_id,
GetNameFromVideoPage(response),
GetUrlFromVideoPage(response),
0);
duration,
user_id,
GetNameFromVideoPage(response),
GetUrlFromVideoPage(response),
0,
"",
publisher_favicon);
}

void Vimeo::OnSaveMediaVisit(
Expand Down Expand Up @@ -695,15 +720,10 @@ void Vimeo::SavePublisherInfo(
return;
}

std::string icon = publisher_favicon;
if (icon.empty()) {
icon = GenerateFaviconUrl(user_id);
}

ledger::VisitData visit_data;
visit_data.provider = VIMEO_MEDIA_TYPE;
visit_data.url = publisher_url;
visit_data.favicon_url = icon;
visit_data.favicon_url = publisher_favicon;
visit_data.name = publisher_name;

ledger_->SaveMediaVisit(key,
Expand Down
5 changes: 4 additions & 1 deletion vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ class Vimeo : public ledger::LedgerCallbackHandler {

static std::string GetIdFromVideoPage(const std::string& data);

static std::string GenerateFaviconUrl(const std::string& id);
static std::string GenerateFaviconUrl(const std::string& data);

static std::string GetNameFromVideoPage(const std::string& data);

Expand Down Expand Up @@ -81,6 +81,7 @@ class Vimeo : public ledger::LedgerCallbackHandler {
const std::string& media_key,
const std::string& publisher_url,
const std::string& publisher_name,
const std::string& publisher_favicon,
const ledger::VisitData& visit_data,
const uint64_t window_id,
int response_status_code,
Expand All @@ -99,6 +100,7 @@ class Vimeo : public ledger::LedgerCallbackHandler {
uint64_t window_id,
const std::string& publisher_url,
const std::string& publisher_name,
const std::string& publisher_favicon,
const std::string& user_id,
ledger::Result result,
ledger::PublisherInfoPtr info);
Expand All @@ -109,6 +111,7 @@ class Vimeo : public ledger::LedgerCallbackHandler {
const std::string& publisher_url,
const std::string& publisher_key,
const std::string& publisher_name,
const std::string& publisher_favicon,
const std::string& user_id);

void OnMediaPublisherInfo(
Expand Down
5 changes: 3 additions & 2 deletions vendor/bat-native-ledger/src/bat/ledger/internal/media/vimeo_unittest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,9 @@ TEST(VimeoTest, GenerateFaviconUrl) {
ASSERT_EQ(result, "");

// all good
result = Vimeo::GenerateFaviconUrl("234123423");
ASSERT_EQ(result, "https://i.vimeocdn.com/portrait/234123423_300x300.webp");
std::string responseData = "{\"src_2x\":\"https:\\/\\/i.vimeocdn.com\\/portrait\\/6040298_32x32.jpg\"}";
result = Vimeo::GenerateFaviconUrl(responseData);
ASSERT_EQ(result, "https://i.vimeocdn.com/portrait/6040298_300x300.webp");
}

TEST(VimeoTest, GetNameFromVideoPage) {
Expand Down
4 changes: 3 additions & 1 deletion vendor/bat-native-ledger/src/bat/ledger/internal/publisher/publisher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should check for !is_verified and always clear if that is the case (discussed with @NejcZdovc)

publisher_info->favicon_url = ledger::kClearFavicon;
}
Copy link
Contributor Author

@masparrow masparrow Sep 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

publisher_info->name = visit_data.name;
Expand Down