Bug 25867 – [QT] Implement original_url getter method to the API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Antonio Gomes]

In some cases (e.g. sites that redirects) the application may be interested in the original URL entered by the user, not only in the resolved one.

We should consider including support for that in our API, if convenient.

[Kenneth Rohde Christiansen]

Please name it originalUrl() and let it return a QUrl. What to do it they user gives an invalid URL? what will originalUrl return? a nulled QUrl?

[Antonio Gomes]

(In reply to comment #1)
> What to do it they user gives an invalid URL? what will originalUrl return? a nulled QUrl?

it will return an empty qurl, not null.

[Antonio Gomes]

TESTCASE:

In practical terms, I am calling "original url" here the url fixed but not yet resolved
loaded by the current main frame, e.g. :

1) if user runs: $ ./QtLauncher google.com
2) it gets "fixed" to "http://google.com/"
3) it gets "resolved" to "http://www.google.com"
4) sometimes it also gets "redirected" to other pages , e.g.
"http://www.google.com.br"

--> originalUrl() would return 2).
--> url() would return either 3) or 4).

SCENARIO:

The motivation for this simple add comes from a feature we needed to implement a while back while working on a frontend for our webkit-efl port [1], and we thought that it could also be useful for other ports.

[1] http://code.staikos.net/cgi-bin/gitweb.cgi?p=webkit;a=shortlog;h=shared/webkit-efl

In our case we had the following scenario:

1) we have a history management implemented in ui side.
2) during the mainFrame's loading process, it firstly calls |WebCoreSupport::updateGlobalHistory()| passing the URL being loaded(see quoted code below). It is the url not resolved or redirected yet.

(...)
void FrameLoaderClientQt::updateGlobalHistory()
{
   QWebHistoryInterface *history = QWebHistoryInterface::defaultInterface();
   if (history)
       history->addHistoryEntry(m_frame->loader()->documentLoader()->urlForHistory().prettyURL());
}
(...)

We stored the url passed in in our history database as a primary key.

3) yet in the loading process, server responses to the loading process and we get urlChanged and titleChanged signals emited. In our case, we want to update the history database entry of the just obtained |title| for the web page. At that time (when titleChanged signal gets emited) we get the original url by calling mainFrame->originalUrl(), find the history entry in the database
associated to it and update it w/ the new title value.

[Antonio Gomes]

Created attachment 32309 [details]
patch 0.1 - implement originalUrl support

ps: I have not figure out a good way to add tests to this. any input welcome.

[Antonio Gomes]

Created attachment 32310 [details]
patch 0.2 - implement originalUrl support

updated to ToT

[Simon Hausmann]

Comment on attachment 32310 [details]
patch 0.2 - implement originalUrl support

Clearing from the review queue as per comment from IRC:
<tonikitoo> manyoso, tronical i am finishing originalURl patch ... adding a better test (as suggested by fawek) and better docs, as per simon's suggestion ...

[Antonio Gomes]

Created attachment 32888 [details]
patch 0.2 - implement originalUrl support

Test added: it creates a proper FakeNetworkAccessManager (:QNetworkAccessManager) and FakeReply (:QNetworkReply) in order to a redirect.

[Antonio Gomes]

Comment on attachment 32888 [details]
patch 0.2 - implement originalUrl support

see patch 0.3 also in

http://code.staikos.net/cgi-bin/gitweb.cgi?p=webkit;a=commitdiff;h=7aa68b24bcf949b3f78c25008e48a5de44d074b6

[Antonio Gomes]

Comment on attachment 32888 [details]
patch 0.2 - implement originalUrl support

cleanring review flag ... improvements to come !

[Antonio Gomes]

Created attachment 32958 [details]
patch 0.3 - implement originalUrl support

WIP: cleaned up

[Antonio Gomes]

Created attachment 33085 [details]
patch 0.4 - implement originalUrl support

[Antonio Gomes]

patch is ready for review, but can not land before patch in bug 27444.

setting it as a dep.

[Antonio Gomes]

Created attachment 33361 [details]
patch 0.5 - implement originalUrl support

ready for review.

[Antonio Gomes]

Comment on attachment 33361 [details]
patch 0.5 - implement originalUrl support

clearing review again. got comments from itc

[Antonio Gomes]

Created attachment 33432 [details]
patch 0.6 - implement originalUrl support

1) ran style code script
2) left name as originalURL to match w/ QWebHistory::originalURL (as w/ gtk API )
3) updated to ToT
4) adde class doc overview

[Antonio Gomes]

branch where to find the neswest patch (easier cherry pick)

http://code.staikos.net/cgi-bin/gitweb.cgi?p=webkit;a=shortlog;h=antonio/original-url

[Simon Hausmann]

Comment on attachment 33432 [details]
patch 0.6 - implement originalUrl support

r=me

Please submit a separate patch for the function renaming, as discussed on IRC :)

[Antonio Gomes]

fixed

thanks simon, adam and kenneth. 

(In reply to comment #17)
> (From update of attachment 33432 [details])
> r=me

landed in r46364

> Please submit a separate patch for the function renaming, as discussed on IRC
> :)

landed in r46367 (r=Adam Treat on irc)

[Robert Hogan]

Why the check for d->frameLoaderClient->m_loadSucceeded when deciding which url to return?

This check happens to break my use-case of this feature, which is to handle SSL failures that occur as the result of redirects (e.g. https://hotmail.com). When the failure occurs m_loadSucceeded is not yet true, and won't be until the ssl failure is accepted by the user.

[Antonio Gomes]

(In reply to comment #19)
> Why the check for d->frameLoaderClient->m_loadSucceeded when deciding which url
> to return?

mwenge, when the load is not successful (e.g. user loads an non-existent url) the d->loader()->activeDocumentLoader stays referring the latest successful load (not to the latest failing one). so we could not query the originalUrl off of it.

please see details in bug 27444 .

given that, I used this m_loadSucceeded flag to identify if the load was successful or not. If it is not, we do a special handler for it.

> This check happens to break my use-case of this feature, which is to handle SSL
> failures that occur as the result of redirects (e.g. https://hotmail.com). When
> the failure occurs m_loadSucceeded is not yet true, and won't be until the ssl
> failure is accepted by the user.

so it is not TRUE yet BUT load has not finished ... hum ... right I will think of something for it. we could have another bug for it, maybe

[Antonio Gomes]

> > This check happens to break my use-case of this feature, which is to handle SSL
> > failures that occur as the result of redirects (e.g. https://hotmail.com). When
> > the failure occurs m_loadSucceeded is not yet true, and won't be until the ssl
> > failure is accepted by the user.
> 
> so it is not TRUE yet BUT load has not finished ... hum ... right I will think
> of something for it. we could have another bug for it, maybe

filed bug 27804