Bug 136916 – Pending API request URL no set when loading Alternate HTML or plain text

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Making this depend on #136913, since the new unit test will fail if #136913 is not fixed before.

[Carlos Garcia Campos]

loadFile() has already been fixed in r174029

[Carlos Garcia Campos]

Created attachment 238763 [details]
Rebased patch after r174029

[Darin Adler]

Sam, Anders, could one of you review?

[Carlos Garcia Campos]

Created attachment 239519 [details]
Rebased patch

[Carlos Garcia Campos]

loadData was already fixed in r176408

[Carlos Garcia Campos]

Created attachment 243972 [details]
Rebased patch

[Carlos Garcia Campos]

Created attachment 243974 [details]
Now also including the xcode file changes

To add the new api test to the build also for mac

[Michael Catanzaro]

(In reply to comment #5)
> Sam, Anders, could one of you review?

[Michael Catanzaro]

Owners?

[Darin Adler]

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.

[Darin Adler]

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".

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 270483 [details]
Patch for landing

[Carlos Garcia Campos]

Anders?

[Carlos Garcia Campos]

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.

[Anders Carlsson]

OK.

[Carlos Garcia Campos]

Committed r199660: <http://trac.webkit.org/changeset/199660>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 156691

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r199663: <http://trac.webkit.org/changeset/199663>