Bug 198741

<rdar://problem/31164316>

Created attachment 371820 [details] Patch

Comment on attachment 371820 [details] Patch I'm not 100% sure what exactly the difference is between `m_committedState` and `m_uncommittedState`, but it makes sense when I think about it, and it worked when testing locally. Another thing I thought of was to have `WebAutomationProxy` become a subclass of `PageLoadState::Observer` and more "eagerly" fire `waitForNavigationToComplete` callbacks `didChangeIsLoading` is called and `isLoading` is false at that time. I figured I'd wait to get some feedback to see if this is even the right direction before trying that.

Comment on attachment 371820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=371820&action=review Makes sense to me. r=me > Source/WebKit/ChangeLog:12 > + If the timing of the communications to the `WebPageProxy` about the fact that it's actually > + `PageLoadingState::isLoading` is too "slow" or happens after > + `WebAutomationSession::waitForNavigationToCompleteOnPage` is called, the automation session > + will continue running commands, which can lead to a JavaScript error ("Callback was not This seems a bit complex. It just sounds like there is a race between waitForNavigationToCompleteOnPage and PageLoadingState isLoading in provisional loads, and this better addresses it. > Source/WebKit/UIProcess/PageLoadState.h:128 > + bool isLoadingUncommitted() const; I wonder if a better name might be `isPerfomingProvisionalLoad()` or just a general `isPerformingLoad` which checks committed / uncommitted. Maybe the networking guys have a better suggestion given they work in this area more often.

Created attachment 372102 [details] Patch

Comment on attachment 372102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372102&action=review > Source/WebKit/UIProcess/PageLoadState.cpp:175 > +bool PageLoadState::hasPendingLoad() const I'd have expected a name like this to check both committed and uncommitted. If the name was `hasPendingProvisionalLoad` I'd expect it to check uncommitted.

Comment on attachment 372102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372102&action=review >> Source/WebKit/UIProcess/PageLoadState.cpp:175 >> +bool PageLoadState::hasPendingLoad() const > > I'd have expected a name like this to check both committed and uncommitted. > If the name was `hasPendingProvisionalLoad` I'd expect it to check uncommitted. I'd rather not use "provisional", as I think that has a different meaning in this context. Both `m_committedState` and `m_uncommittedState` have a `provisionalURL` member. Looking back at the commit history (<https://trac.webkit.org/r160405>), it's been referred to as the "uncommitted" state ("pending" was the closest word I could think of at the time). Including a check for `isLoading` (which checks `m_comittedState`) in this function would be weird with the current name, as `isLoading(m_committedState)` isn't really "pending" (it's been committed). If we keep the name as is, I'd prefer to not include `m_committedState`, but if we change it to something more "inclusive" then I have no objection.

Created attachment 372169 [details] Patch

Comment on attachment 372169 [details] Patch Clearing flags on attachment: 372169 Committed r246456: <https://trac.webkit.org/changeset/246456>

All reviewed patches have been landed. Closing bug.