-
Notifications
You must be signed in to change notification settings - Fork 896
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
[ads] Followup to "Send HTTP response status for landed events as part of the confirmation token redemption" #36752 #25523
Conversation
7d1fd2b
to
605946c
Compare
Privacy review discussion https://github.com/brave/reviews/issues/1553, thanks. cc @fmarier |
eaf85f0
to
8bba718
Compare
8bba718
to
44a8a08
Compare
components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data_unittest.cc
Show resolved
Hide resolved
844c115
to
c260255
Compare
634cf31
to
c036f23
Compare
components/brave_ads/core/internal/account/user_data/fixed/page_land_user_data_unittest.cc
Outdated
Show resolved
Hide resolved
bool IsErrorPage(content::NavigationHandle* navigation_handle); | ||
// NOTE: DO NOT use this method before the navigation commit as it will return | ||
// null. It is safe to use from `WebContentsObserver::DidFinishNavigation()`. | ||
std::optional<int> HttpStatusCode( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is used only in DidFinishNavigation if I'm right. I'd prefer to remove such a context dependent method in favor of the lambda function in DidFinishNavigation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. We have a comment in the header:
// NOTE: DO NOT use this method before the navigation commit as it will return
// null. It is safe to use from `WebContentsObserver::DidFinishNavigation()`.
std::optional<int> HttpStatusCode(
a6096fc
to
8aa6b3b
Compare
27c5e3c
to
e7dfcf3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
privacy lgtm
57190b4
to
669fa1c
Compare
Deprecate kHttpUpgradeRequired and use net::HTTP_UPGRADE_REQUIRED
Co-authored-by: Francois Marier <[email protected]>
…t of the confirmation token redemption" #36752 Co-authored-by: Francois Marier <[email protected]>
run_loop_.Run(); | ||
} | ||
|
||
/////////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Is this needed?
/////////////////////////////////////////////////////////////////////////////// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fmarier this is expected because I break up public/private sections using this divider. Thanks
Released in v1.73.14 |
Resolves brave/brave-browser#40991
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Verify that the HTTP response status codes
400
,401
,403
,404
,407
,408
,429
,451
,500
,502
,503
, and504
are included in the confirmation token redemption payload as"httpResponseStatus": "###"
for Rewards users (e.g.,"httpResponseStatus": "503"
). For any unlisted status codes, the response should only reflect the status code class (e.g.,1xx
,2xx
,3xx
,4xx
,5xx
). The "httpResponseStatus" key should NOT be included for non-Rewards users.