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

Classify web pages under cache-control for Brave Ads #5157

Merged
merged 1 commit into from
Apr 7, 2020
Merged

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Apr 4, 2020

Resolves brave/brave-browser#9021

Submitter Checklist:

Test Plan:

Confirm pages with cache-control are classified (that have enough content)

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
Copy link
Collaborator Author

tmancey commented Apr 4, 2020

@kylehickinson Raised brave/brave-ios#2444 for iOS

@@ -94,8 +94,7 @@ NS_SWIFT_NAME(BraveRewards)
/// Report that a page has loaded in the current browser tab, and the HTML is available for analysis
///
/// @note Send nil for `adsInnerText` if the load happened due to tabs restoring
/// after app launch or if response header for the page load contains
/// "cache-control: no-store"
/// after app launch
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kylehickinson I raised an issue for brave-ios so remove the cache control business logic from the iOS app (comment above updated to reflect the needed change)

@kjozwiak
Copy link
Member

kjozwiak commented Apr 15, 2020

@tmancey do you have any specific examples of websites that are using cache-control that didn't work before this fix landed? Is there a specific directive that we should be checking? cache-control: private or cache-control: public?

@tmancey
Copy link
Collaborator Author

tmancey commented Apr 15, 2020

@kjozwiak microsoft.com did not work before the change, thanks

@kjozwiak
Copy link
Member

kjozwiak commented Apr 15, 2020

@kjozwiak microsoft.com did not work before the change, thanks

Awesome, thanks @tmancey 👍

Reproduced the original issue on macOS 10.15.3 x64 Catalina using the following build:

Brave | 1.7.92 Chromium: 80.0.3987.163 (Official Build) (64-bit)
--- | ---
Revision | e7fbe071abe9328cdce4ffedac9822435fbd3656-refs/branch-heads/3987@{#1037}
OS | macOS Version 10.15.3 (Build 19D76)

Ensured that https://www.microsoft.com/en-ca/ was using no-store directive for cache-control. Example:

[29456:775:0415/120101.483927:INFO:ads_impl.cc(476)] OnTabUpdated.IsFocused for tab id: 2 and url: https://www.microsoft.com/en-ca/
[29456:775:0415/120101.502170:INFO:client.cc(668)] Successfully saved client state
[29456:775:0415/120101.838409:INFO:ads_impl.cc(492)] OnTabUpdated.IsBlurred for tab id: 2 and url: https://www.microsoft.com/en-ca/
[29456:775:0415/120101.863137:INFO:client.cc(668)] Successfully saved client state

Screen Shot 2020-04-15 at 11 56 01 AM

Verification PASSED on macOS 10.15.3 x64 Catalina using the following build:

Brave | 1.9.23 Chromium: 81.0.4044.92 (Official Build) nightly (64-bit)
--- | ---
Revision | 32921c79b6f01a0fb2deef0e1d45b42f96581051-refs/branch-heads/4044@{#883}
OS | macOS Version 10.15.3 (Build 19D76)

Ensured that visiting https://www.microsoft.com/en-ca/ classified the website correctly:

[29567:775:0415/120701.349426:INFO:ads_impl.cc(459)] OnTabUpdated.IsFocused for tab id: 7
[29567:775:0415/120707.704980:INFO:ad_conversions.cc(67)] Checking ad conversions
[29567:775:0415/120707.713464:INFO:ads_impl.cc(762)] Successfully classified page as technology & computing-software. Winning category over time is technology & computing-software
[29567:775:0415/120707.723147:INFO:client.cc(593)] Successfully saved client state

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.

Classify web pages under cache-control for Brave Ads
3 participants