The effect in all the cases is that PageLoadState::isLoading() is false right after requesting a load, and the activeURL can be NULL or a previous URL.
Created attachment 238300 [details] Patch I've added a new unit test, but only to GTK and EFL builds, since I don't have a mac to add it to the xcode file too.
Making this depend on #136913, since the new unit test will fail if #136913 is not fixed before.
loadFile() has already been fixed in r174029
Created attachment 238763 [details] Rebased patch after r174029
Sam, Anders, could one of you review?
Created attachment 239519 [details] Rebased patch
loadData was already fixed in r176408
Created attachment 243972 [details] Rebased patch
Created attachment 243974 [details] Now also including the xcode file changes To add the new api test to the build also for mac
(In reply to comment #5) > Sam, Anders, could one of you review?
Owners?
Comment on attachment 243974 [details] Now also including the xcode file changes View in context: https://bugs.webkit.org/attachment.cgi?id=243974&action=review I’m saying review+ but I’d still like to understand if Anders agrees about this use of "about:blank". > Source/WebKit2/UIProcess/WebPageProxy.cpp:860 > + auto transaction = m_pageLoadState.transaction(); > + m_pageLoadState.setPendingAPIRequestURL(transaction, ASCIILiteral("about:blank")); I think it’s a little peculiar to actually set the URL to about::blank here. Do we agree that explicitly giving it this particular URL makes sense? Does WebKit already treat this as if it came from that URL in other ways or is that a new concept we are introducing? If WebKit does already use this URL then why are we having to inject it specifically here? I think this would read better without the local variable even though the other function does have it in a local variable since it uses it twice. I think blankURL().string() is better than ASCIILiteral("about:blank"). m_pageLoadState.setPendingAPIRequestURL(m_pageLoadState.transaction(), blankURL().string()); > Source/WebKit2/UIProcess/WebPageProxy.cpp:875 > + auto transaction = m_pageLoadState.transaction(); > + m_pageLoadState.setPendingAPIRequestURL(transaction, ASCIILiteral("about:blank")); Same comments as above.
Comment on attachment 243974 [details] Now also including the xcode file changes View in context: https://bugs.webkit.org/attachment.cgi?id=243974&action=review >> Source/WebKit2/UIProcess/WebPageProxy.cpp:860 >> + m_pageLoadState.setPendingAPIRequestURL(transaction, ASCIILiteral("about:blank")); > > I think it’s a little peculiar to actually set the URL to about::blank here. Do we agree that explicitly giving it this particular URL makes sense? Does WebKit already treat this as if it came from that URL in other ways or is that a new concept we are introducing? If WebKit does already use this URL then why are we having to inject it specifically here? > > I think this would read better without the local variable even though the other function does have it in a local variable since it uses it twice. I think blankURL().string() is better than ASCIILiteral("about:blank"). > > m_pageLoadState.setPendingAPIRequestURL(m_pageLoadState.transaction(), blankURL().string()); I found comments in FrameLoader::reload that say, "If a window is created by javascript, its main frame can have an empty but non-nil URL." Maybe that’s how the plain text string should work. Have an empty URL, not "about:blank".
Comment on attachment 243974 [details] Now also including the xcode file changes View in context: https://bugs.webkit.org/attachment.cgi?id=243974&action=review >>> Source/WebKit2/UIProcess/WebPageProxy.cpp:860 >>> + m_pageLoadState.setPendingAPIRequestURL(transaction, ASCIILiteral("about:blank")); >> >> I think it’s a little peculiar to actually set the URL to about::blank here. Do we agree that explicitly giving it this particular URL makes sense? Does WebKit already treat this as if it came from that URL in other ways or is that a new concept we are introducing? If WebKit does already use this URL then why are we having to inject it specifically here? >> >> I think this would read better without the local variable even though the other function does have it in a local variable since it uses it twice. I think blankURL().string() is better than ASCIILiteral("about:blank"). >> >> m_pageLoadState.setPendingAPIRequestURL(m_pageLoadState.transaction(), blankURL().string()); > > I found comments in FrameLoader::reload that say, "If a window is created by javascript, its main frame can have an empty but non-nil URL." > > Maybe that’s how the plain text string should work. Have an empty URL, not "about:blank". This is what we are doing in other load methods, see WebPageProxy::loadHTMLString() and WebPageProxy::loadData(). You can see in the unit test I'm adding that it also checks existing cases like WKPageLoadData that indeed returns about:blank. So, I don't know if it's the best to do it, but it's consistent with what we do already.
(In reply to comment #12) > Comment on attachment 243974 [details] > Now also including the xcode file changes > > View in context: > https://bugs.webkit.org/attachment.cgi?id=243974&action=review > > I’m saying review+ but I’d still like to understand if Anders agrees about > this use of "about:blank". > > > Source/WebKit2/UIProcess/WebPageProxy.cpp:860 > > + auto transaction = m_pageLoadState.transaction(); > > + m_pageLoadState.setPendingAPIRequestURL(transaction, ASCIILiteral("about:blank")); > > I think it’s a little peculiar to actually set the URL to about::blank here. > Do we agree that explicitly giving it this particular URL makes sense? Does > WebKit already treat this as if it came from that URL in other ways or is > that a new concept we are introducing? If WebKit does already use this URL > then why are we having to inject it specifically here? > > I think this would read better without the local variable even though the > other function does have it in a local variable since it uses it twice. I > think blankURL().string() is better than ASCIILiteral("about:blank"). > > m_pageLoadState.setPendingAPIRequestURL(m_pageLoadState.transaction(), > blankURL().string()); The local variable is not because it's reused, but because we want the transaction to be finished when the function goes out of scope, the transaction destructor is the one committing the changes to the page load state.
Created attachment 270483 [details] Patch for landing
Anders?
This patch has almost two years, and has been r+'ed for three months, just pending the final approval from Anders. And the same situation happens in bug #139342. If nobody objects I'll apply the inverse logic and land these patches next Monday, so Anders can roll them out if he disagrees. I don't see any other option to unblock these bugs.
OK.
Committed r199660: <http://trac.webkit.org/changeset/199660>
Re-opened since this is blocked by bug 156691
Created attachment 276628 [details] Try to fix iOS build I think the only problem was a missing #if WK_HAVE_C_SPI in the test
Committed r199663: <http://trac.webkit.org/changeset/199663>