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

fix(MonetizationLinkManager): simplify search, get first link from iframe #922

Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

fix(MonetizationLinkManager): simplify search, get first link from iframe #922

wants to merge 21 commits into from

Conversation

DarianM
Copy link
Member

@DarianM DarianM commented Feb 13, 2025 •
edited
Loading

Closes #920 .

Changes proposed in this pull request

  • add mutation observer to first level frames
  • removed onAddedNode/onRemovedNode
  • use onWholeDocumentObserved to search every time a new node gets removed or added

Current known limitation for when the first monetization link is removed, the second should be monetized, to be addressed in #921

@DarianM DarianM linked an issue Feb 13, 2025 that may be closed by this pull request
Simplify monetization link detection and handling #920
Open
@github-actions github-actions bot added the area: content Improvements or additions to extension content script label Feb 13, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 13, 2025 •
edited
Loading

Extension builds preview

Name Link
Latest commit b9c27fd
Latest job logs Run #13650186134
BadgeDownload
BadgeDownload

DarianM
DarianM commented Feb 13, 2025
View reviewed changes
Copy link
Member Author

@DarianM DarianM left a comment

Choose a reason for hiding this comment

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

test-e2e

@sidvishnoi sidvishnoi changed the title fix(content): monetize dynamic added links in iframe fix(MonetizationLinkManager): simplify link tag search, monetize all links Feb 14, 2025
@sidvishnoi sidvishnoi self-requested a review February 17, 2025 10:59
sidvishnoi
sidvishnoi reviewed Feb 17, 2025
View reviewed changes
Copy link
Member

@sidvishnoi sidvishnoi left a comment

Choose a reason for hiding this comment

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

To fix: If first link tag in iframe is disabled, second isn't marked for monetization, even though first is sent for STOP_MONETIZATION.

@sidvishnoi sidvishnoi changed the title fix(MonetizationLinkManager): simplify link tag search, monetize all links fix(MonetizationLinkManager): simplify search, get first link from iframe Feb 17, 2025
@DarianM
Copy link
Member Author

DarianM commented Feb 17, 2025

To fix: If first link tag in iframe is disabled, second isn't marked for monetization, even though first is sent for STOP_MONETIZATION.

Let's treat this in the followup issue and let it be as a limitation for now.
Let's decide in the next issue how to mark the next monetizable link; i.e. keeping a payment session ready for the next links and fire them only when needed

@github-actions github-actions bot added area: background Improvements or additions to extension background script area: popup Improvements or additions to extension popup area: tests Improvements or additions to tests area: i18n area: pages Changes to any of extension's pages labels Feb 28, 2025
@github-actions github-actions bot removed area: background Improvements or additions to extension background script area: popup Improvements or additions to extension popup labels Feb 28, 2025
@github-actions github-actions bot removed area: tests Improvements or additions to tests area: i18n area: pages Changes to any of extension's pages labels Feb 28, 2025
@github-actions github-actions bot added the area: tests Improvements or additions to tests label Feb 28, 2025
@DarianM
Copy link
Member Author

DarianM commented Mar 1, 2025 •
edited
Loading

@sidvishnoi this should be reviewed and merged first
it has dynamic add tags in iframe tests enabled, but not the promotion of the 2nd tag ones.

sidvishnoi
sidvishnoi reviewed Mar 3, 2025
View reviewed changes
src/content/services/monetizationLinkManager.ts
Comment on lines -137 to -139
for (const link of monetizationLinks) {
this.observeLinkAttrs(link);
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm. This is now observing only valid link tags?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sidvishnoi sidvishnoi sidvishnoi left review comments

@raducristianpopa raducristianpopa Awaiting requested review from raducristianpopa

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area: content Improvements or additions to extension content script area: tests Improvements or additions to tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Simplify monetization link detection and handling
2 participants
@DarianM @sidvishnoi