Edge: Send out LocationEvents for #fragment navigation #1584
Conversation
Test Results 483 files ±0 483 suites ±0 8m 45s ⏱️ +23s For more details on these failures, see this check. Results for commit 5086276. ± Comparison against base commit d9d9bf0. ♻️ This comment has been updated with latest results. |
|
Any objection to merge this for RC1? It fixes a regression introduced with #1451. The fix was validated in the associated issue. @HeikoKlare could you have a look at the code changes? @akurtakov @merks Pinging you for PMC approval. |
|
It's quite a bit of code that I will not have time review right now. If you and @HeikoKlare agree it's a good thing then I'm happy to defer to your good judgement: PMC +1 |
There was a problem hiding this comment.
The change looks sound and since it has been validated in the issue thread and only affects Edge (which is an opt-in feature), I consider this safe to merge for RC1.
I only see a potential code duplication that we might clean up before merging. But I leave it up to you whether to improve that or not. You have my approval to merge this in either case.
| browser.getDisplay().asyncExec(() -> { | ||
| if (browser.isDisposed()) return; | ||
| LocationEvent event = new LocationEvent(browser); | ||
| event.display = browser.getDisplay(); | ||
| event.widget = browser; | ||
| event.location = location; | ||
| event.top = true; | ||
| for (LocationListener listener : locationListeners) { | ||
| listener.changed(event); | ||
| if (browser.isDisposed()) return; | ||
| } | ||
| }); |
There was a problem hiding this comment.
This seems to be mostly duplicate code to what happens in handleDocumentTitleChanged(). Can that be extracted into a separate, parameterized method?
There was a problem hiding this comment.
Yes, indeed. There's several places in Edge class that could be simplified.
I think we should have a round of refactoring and deduplication beginning of next dev cycle.
I'll merge this as-is for now.
|
Test failures on Linux are unrelated. |
3 participants
Resolves #1581.