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

[ads] Fixes error page should not trigger NotifyTab[Text/Html]ContentDidChange #23631

Merged
merged 1 commit into from
May 28, 2024

Conversation

aseren
Copy link
Collaborator

@aseren aseren commented May 14, 2024

Resolves brave/brave-browser#38271

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  • Fresh install
  • Join Brave Rewards

Test Case 1

  • Open a webpage with 500 error
    EXPECTATION: NotifyTab[Text/Html]ContentDidChange is not triggered
    There shouldn't be following logs:
    • Tab id 204824608 HTML content changed
    • Tab id 204824608 text content changed

Test Case 2

  • Open a webpage with 404 error
    EXPECTATION: NotifyTab[Text/Html]ContentDidChange is not triggered
    There shouldn't be following logs:
    • Tab id 204824608 HTML content changed
    • Tab id 204824608 text content changed

Test Case 3

  • Open a webpage with non existent DNS name

EXPECTATION: NotifyTab[Text/Html]ContentDidChange is not triggered
There shouldn't be following logs:

  • Tab id 204824608 HTML content changed
  • Tab id 204824608 text content changed

@aseren aseren requested a review from tmancey May 14, 2024 02:49
@aseren aseren requested a review from a team as a code owner May 14, 2024 02:49
Copy link
Collaborator

@tmancey tmancey left a comment

Choose a reason for hiding this comment

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

LGTM

{
tab.shouldNotifyAdsServiceTabDidChange = false
let isErrorPage = InternalURL(responseURL)?.isErrorPage == true || response.statusCode >= 400
let isSessionRestorePage = InternalURL(responseURL)?.isSessionRestore == true
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: 'isSessionRestore'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines 688 to 689
let isErrorPage = InternalURL(responseURL)?.isErrorPage == true || response.statusCode >= 400
let isSessionRestore = InternalURL(responseURL)?.isSessionRestore == true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Rather than calling InternalUrl(responseURL)? twice, maybe assign to url and then in the condition call url.isErrorPage and url.isSessionRestore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@aseren aseren force-pushed the issues/38271 branch 2 times, most recently from 2aad935 to 7725af9 Compare May 20, 2024 02:17
@aseren aseren merged commit 66a670a into master May 28, 2024
19 checks passed
@aseren aseren deleted the issues/38271 branch May 28, 2024 15:59
@github-actions github-actions bot added this to the 1.68.x - Nightly milestone May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants