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] General code health #24397

Merged
merged 1 commit into from
Jun 28, 2024
Merged

[ads] General code health #24397

merged 1 commit into from
Jun 28, 2024

Conversation

tmancey
Copy link
Collaborator

@tmancey tmancey commented Jun 26, 2024

Resolves brave/brave-browser#39403

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:

Regression test for search result ads.

@tmancey tmancey self-assigned this Jun 26, 2024
@tmancey tmancey force-pushed the issues/39403 branch 11 times, most recently from a1e1a1f to 66c34b3 Compare June 26, 2024 21:30
@tmancey tmancey marked this pull request as ready for review June 26, 2024 21:33
@tmancey tmancey requested review from a team as code owners June 26, 2024 21:33
@tmancey tmancey force-pushed the issues/39403 branch 3 times, most recently from 0c873f6 to 973108b Compare June 26, 2024 22:22
@diracdeltas
Copy link
Member

ok to ignore the security action findings as long as these are all refactoring changes, not adding any new code

@tmancey tmancey requested a review from a team as a code owner June 28, 2024 02:51
@tmancey tmancey force-pushed the issues/39403 branch 3 times, most recently from b857a15 to 84d392d Compare June 28, 2024 03:26
@tmancey tmancey enabled auto-merge June 28, 2024 03:29
@tmancey tmancey force-pushed the issues/39403 branch 3 times, most recently from 06bf662 to fcc35e0 Compare June 28, 2024 03:45
@tmancey tmancey disabled auto-merge June 28, 2024 03:50
Copy link
Contributor

[puLL-Merge] - brave/brave-core@24397

Description

This PR refactors the handling of search result ads in Brave, renaming components and restructuring the code to improve organization and clarity. The main changes involve renaming "SearchResultAd" to "CreativeSearchResultAd" and moving files to a new "creatives" directory structure. The PR also includes updates to related tests and changes to the mojom interface.

Changes

Changes

  1. browser/brave_ads/BUILD.gn:

    • Updated file paths and names to reflect the new structure
  2. browser/brave_ads/creatives/search_result_ad/:

    • Added new files for handling creative search result ads
    • Implemented CreativeSearchResultAdTabHelper to replace SearchResultAdTabHelper
    • Added new test files and utilities for creative search result ads
  3. browser/brave_ads/ad_units/search_result_ad/:

    • Removed old files related to search result ads
  4. browser/brave_tab_helpers.cc:

    • Updated to use CreativeSearchResultAdTabHelper instead of SearchResultAdTabHelper
  5. components/brave_ads/content/browser/BUILD.gn:

    • Updated file paths and names to reflect the new structure
  6. components/brave_ads/content/browser/creatives/search_result_ad/:

    • Added new files for handling creative search result ads
    • Implemented CreativeSearchResultAdHandler to replace SearchResultAdHandler
    • Added new test files and utilities for creative search result ads
  7. components/brave_ads/core/mojom/brave_ads.mojom:

    • Renamed ConversionInfo to CreativeSetConversionInfo
    • Updated CreativeSearchResultAdInfo to use CreativeSetConversionInfo
  8. ios/brave-ios/Sources/.../BraveSearchResultAdScriptHandler.swift:

    • Updated to use CreativeSetConversionInfo instead of ConversionInfo
  9. test/data/brave_ads/:

    • Renamed and updated test HTML files

Possible Issues

  • The extensive renaming and restructuring may require updates to any external code or documentation that references the old naming conventions or file structures.
  • There's a potential for introducing bugs during such a large refactoring, especially in areas where the logic has been significantly changed or moved.

Security Hotspots

No significant security hotspots were identified in this change. The refactoring primarily focuses on reorganization and renaming, which should not introduce new security vulnerabilities if implemented correctly.

@tmancey tmancey force-pushed the issues/39403 branch 3 times, most recently from 25c27a0 to d259290 Compare June 28, 2024 14:10
@tmancey tmancey force-pushed the issues/39403 branch 4 times, most recently from a7b940f to 058542e Compare June 28, 2024 15:49
@tmancey tmancey enabled auto-merge June 28, 2024 15:50
@tmancey tmancey disabled auto-merge June 28, 2024 16:05
@tmancey tmancey enabled auto-merge June 28, 2024 16:19
@tmancey tmancey merged commit 18f68bd into master Jun 28, 2024
19 checks passed
@tmancey tmancey deleted the issues/39403 branch June 28, 2024 17:12
@github-actions github-actions bot added this to the 1.69.x - Nightly milestone Jun 28, 2024
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.

[ads] General code health
9 participants