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

Jump to bottom

Search engine result pages are not being classified #3510

Closed
wants to merge 1 commit into from
Closed

Search engine result pages are not being classified #3510

wants to merge 1 commit into from

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Sep 23, 2019 •
edited
Loading

Fixes brave/brave-browser#6000

Submitter Checklist:

Test Plan:

  • Confirm SERP for the following are classified:

https://amazon.com
https://baidu.com
https://bing.com
https://duckduckgo.com
https://fireball.com
https://github.com
https://google.com
https://google.ca
https://google.co.uk
https://google.co.jp
https://stackoverflow.com
https://developer.mozilla.org
https://twitter.com
https://en.wikipedia.org
https://search.yahoo.com
https://search.yahoo.co.jp
https://youtube.com
https://startpage.com
https://infogalactic.com
https://wolframalpha.com
https://semanticscholar.org
https://qwant.com
https://yandex.com
https://ecosia.org
https://searx.me
https://findx.com

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

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@tmancey tmancey added this to the 0.72.x - Nightly milestone Sep 23, 2019
@tmancey tmancey self-assigned this Sep 23, 2019
@tmancey tmancey force-pushed the issues/6000 branch 2 times, most recently from c4b4f6f to 5d71b0f Compare September 23, 2019 18:52
@tmancey tmancey requested a review from emerick September 23, 2019 18:53
emerick
emerick previously approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@tmancey tmancey requested a review from masparrow September 23, 2019 19:02
masparrow
masparrow previously approved these changes Sep 23, 2019
View reviewed changes
Copy link
Contributor

@masparrow masparrow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM (untested - will test if I can get steps)

@tmancey tmancey changed the title Round robin ads delivery SERP pages are not being classified in the ads user model Sep 23, 2019
@tmancey tmancey requested review from emerick and masparrow and removed request for NejcZdovc, emerick and masparrow September 23, 2019 19:21
@tmancey tmancey closed this Sep 23, 2019
@tmancey tmancey reopened this Sep 23, 2019
@tmancey tmancey dismissed stale reviews from masparrow and emerick via 31b4b76 September 23, 2019 20:22
@tmancey tmancey changed the title SERP pages are not being classified in the ads user model [WIP] SERP pages are not being classified in the ads user model Mar 2, 2020
@tmancey tmancey force-pushed the issues/6000 branch 3 times, most recently from 85f1aeb to 5f21434 Compare March 4, 2020 00:35
@tmancey tmancey added the CI/skip Do not run CI builds (except noplatform) label Mar 23, 2020
@tmancey tmancey changed the title [WIP] SERP pages are not being classified in the ads user model Search engine result pages are not being classified Mar 23, 2020
@tmancey tmancey removed the CI/skip Do not run CI builds (except noplatform) label Apr 22, 2020
@NejcZdovc NejcZdovc marked this pull request as draft August 19, 2020 09:21
@tmancey tmancey removed the bug label Dec 6, 2020
@tmancey tmancey marked this pull request as ready for review December 6, 2020 13:24
@tmancey tmancey force-pushed the issues/6000 branch 4 times, most recently from a787fa5 to 594f87d Compare December 6, 2020 14:16
moritzhaller
moritzhaller reviewed Dec 7, 2020
View reviewed changes
vendor/bat-native-ads/src/bat/ads/internal/search_engine/search_providers.h
@@ -49,12 +49,22 @@ const std::vector<SearchProviderInfo> _search_providers = {
// TODO(https://github.com/brave/brave-browser/issues/8487): Brave Ads
// search providers definition doesn't match all patterns
"https://google.com",
"https://www.google.com/search?q={searchTerms}",
"https://www.google.com/search?{searchTerms}",
Copy link
Contributor

Choose a reason for hiding this comment

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

don't we need to include the q as key here here so GetValueForKeyInQuery knows which value to retrieve? Maybe we can change SearchProviderInfo to include a key property, then we don't need to do the regex bit on the pattern.

@tmancey
Copy link
Collaborator Author

tmancey commented Dec 21, 2021

Closing PR as stale, will open a new PR when work is prioritized in the future

@tmancey tmancey closed this Dec 21, 2021
@tmancey tmancey deleted the issues/6000 branch December 21, 2021 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moritzhaller moritzhaller moritzhaller left review comments

@masparrow masparrow masparrow approved these changes

@emerick emerick emerick left review comments

@yachtcaptain23 yachtcaptain23 Awaiting requested review from yachtcaptain23

Assignees

@tmancey tmancey

Labels
feature/ads
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Classify search engine results page content
5 participants
@tmancey @moritzhaller @emerick @masparrow @NejcZdovc