-
Notifications
You must be signed in to change notification settings - Fork 139
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
Store Edge instance in Environemnt record sync-ly #1595
Store Edge instance in Environemnt record sync-ly #1595
Conversation
Test Results 483 files ±0 483 suites ±0 8m 42s ⏱️ -13s For more details on these failures, see this check. Results for commit 33188fe. ± Comparison against base commit 0e7f799. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did test this with my actual product and it works now like before.
Thank you very much for the quick fix @amartya4256
d9087c5
to
7987494
Compare
Since we are adding it early, we also need to remove the instance if the initialization fails. I have amended the commit which includes the removal on failure as well. |
This commit stores the edge instance on creation in the webenvironment record synchronously for the sake of the consistency of the record. contributes to eclipse-platform#1592
7987494
to
33188fe
Compare
Great, thank you for this fast fix! As a short summary: this PR addresses a minor regression (#1592) introduced in this development cycle via #1378. The fix is simple and only affects Edge (an opt-in component), so there is low to no risk in this fix. The reporter of the regression @N1k145 confirmed that the proposed fix addresses the issue. In consequence (and in my opinion), this should go in RC1, which is why I kindly ask for PMC approval to merge this change @akurtakov @merks. |
As this change is in "opt-in" functionality, fixes regression and reporter verified it : PMC +1 |
Thank you for the immediate approval! The failing tests are documented: #1564 |
This PR contributes to storing the edge instance on creation in the webenvironment record synchronously for the sake of the consistency of the record. Since the record can be read by the cookie manager for several purposes on client calls, we need to make sure the record is in a consistent state, i.e. Linking the Edge instances (completely initialized or not) to their webview environment in the create method (synchronously).
In the bug #1592 the error occurs because we wait for the browser to initialize before it can be added to the record. It is safe to add it synchronously in the record since, all the further API calls of Edge browser is guarded by waitForFutureToFinish method.
contributes to #1592