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

Edge: Improve handling of LocationListener#changed() #1451

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

sratz
Copy link
Member

@sratz sratz commented Sep 5, 2024

Previously, the changed() method was not called consistently.

Move the firing of 'changed' LocationEvents in Edge from handleSourceChanged() to handleNavigationCompleted().

Fixes #1464.

@sratz sratz self-assigned this Sep 5, 2024
@sratz sratz added the edge Edge Browser label Sep 5, 2024
@sratz sratz added this to the 4.34 M1 milestone Sep 5, 2024
Copy link
Contributor

github-actions bot commented Sep 5, 2024

Test Results

   486 files  ±0     486 suites  ±0   9m 6s ⏱️ - 1m 27s
 4 158 tests +1   4 150 ✅ +1   8 💤 ±0  0 ❌ ±0 
16 386 runs  +4  16 294 ✅ +4  92 💤 ±0  0 ❌ ±0 

Results for commit 932b4a2. ± Comparison against base commit e313cfa.

♻️ This comment has been updated with latest results.

Comment on lines 629 to 631
if (url.startsWith("data:text/html;")) {
url = "about:blank";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should check whether this conflicts with #1395

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it probably does, with #1395 we won't reach this piece of code anymore.

I have added a comment in #1395.

Copy link
Member Author

Choose a reason for hiding this comment

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

That part is now obsolete, as no more data:text/html URLs occur with #1395 merged.

Previously, the changed() method was not called consistently.

Move the firing of 'changed' LocationEvents in Edge from
handleSourceChanged() to handleNavigationCompleted().

Add a test case to Test_org_eclipse_swt_browser_Browser for this
scenario (setText() twice).
@sratz
Copy link
Member Author

sratz commented Sep 13, 2024

Also added a test case for this scenario.

If there are no objections I'd like to merge this Today, so that it's available in the I-builds on Monday.

@sratz sratz merged commit f1ed9f9 into master Sep 13, 2024
14 checks passed
@sratz sratz deleted the edge-locationlistener branch September 13, 2024 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
edge Edge Browser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edge: LocationListener#changed() is not called
2 participants