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
feat: webContents.loadURL returns a promise #15855
Conversation
775952e
to
cfdca61
Compare
navigationStarted = true | ||
} | ||
} | ||
const stopLoadingListener = () => { |
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.
We could instead use content::ChildProcessSecurityPolicy::CanRequestURL
to determine if a navigation will happen and abort with did-fail-load
on our end for these types of cases.
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 figured this would be a reasonable "catch-all", since I think did-stop-loading
will be emitted regardless of what happens?
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.
but CanRequestURL
might be a good extra check to give a more informative error
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.
since I think did-stop-loading will be emitted regardless of what happens?
yup that is correct, its emitted whenever a document stops its resource loading be it failure or success. Having this check can simplify existing url validity check https://github.com/electron/electron/blob/master/atom/browser/api/atom_api_web_contents.cc#L1157
cfdca61
to
886aefc
Compare
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.
Looks good from here
Release Notes Persisted
|
Description of Change
Many tests use the following pattern to load a page in a WebContents and wait for the load to finish:
This is an aesthetic improvement over the callback method of
webContents.on('did-finish-load', ...)
, but is unfortunately flaky: it is sometimes possible that the'did-finish-load'
event is emitted before the listener is registered (see #15853 for example, which fixes the flaky test described in #15095). This is to do with the fact that the tests use theBrowserWindow
module via theremote
API, so there is a short window of time after loading the URL but before the event listener is registered. If the event is fired during that window, it will be lost, and the test times out.It's possible to eliminate the additional
emittedOnce
helper call, clean up the code, and remove the possibility of flakiness by havingloadURL
return a promise. The above could would be written as follows with this patch:Checklist
npm test
passesRelease Notes
Notes: webContents.loadURL and loadFile now return a promise