Bug 33875 – [Qt] QWebPage::userAgentForUrl is terrible API

• Component: WebKit Qt
• Status: CLOSED FIXED

[Holger Freyther]

To call this method from the FrameLoaderClientQt we will have to convert KURL to QUrl for every network request we are making. The only good usecase is to set a new user agent or have a site specific user agent. Both of these usecases can be done with a setUserAgent call...

[Holger Freyther]

Created attachment 46962 [details]
Add a QWebPage::setUserAgent which will allow us to not call userAgentForUrl and avoid the KURL -> QUrl conversion

Proposal for a new API avoiding the KURl -> QUrl conversion in all cases when a user agent is set.

[Simon Hausmann]

Comment on attachment 46962 [details]
Add a QWebPage::setUserAgent which will allow us to not call userAgentForUrl and avoid the KURL -> QUrl conversion


Very nice catch!

> +    static QString defaultUserAgent(const QLocale& locale);

Shouldn't the locale parameter have a default constructed QLocale() as default value?

[Simon Hausmann]

Comment on attachment 46962 [details]
Add a QWebPage::setUserAgent which will allow us to not call userAgentForUrl and avoid the KURL -> QUrl conversion

Apart from the default argument this patch looks good to me. A nice speed-up and API convenience :)

[Eric Seidel (no email)]

Attachment 46962 [details] was posted by a committer and has review+, assigning to Holger Freyther for commit.

[Holger Freyther]

Created attachment 48217 [details]
Second attempt with a unit test.

Same patch with a unit test for the new API. The unit tests tests:
     1.) That the old behaviour is default
     2.) Setting a user agent is effective
     3.) Setting an empty user agent goes back to the old behavior.

[Eric Seidel (no email)]

Comment on attachment 46962 [details]
Add a QWebPage::setUserAgent which will allow us to not call userAgentForUrl and avoid the KURL -> QUrl conversion

Cleared Simon Hausmann's review+ from obsolete attachment 46962 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Adam Barth]

Comment on attachment 48217 [details]
Second attempt with a unit test.

I'm not qualified to review Qt API changes, but the API change was already r+ed previously.  This patch just adds unit tests.  Why has this patch been sitting up for review for over a month?

[Benjamin Poulain]

Created attachment 50491 [details]
Just cache the result of QWebPage::userAgentForUrl()

I am not in favor of the API change. From the numbers I have, the conversion KUrl->QUrl needed for QWebPage::userAgentForUrl().

From the benchs I have, the time spent in QWebPage::userAgentForUrl() is mostly taken by playing with the QString. I have attached a patch to make QWebPage::userAgentForUrl() disappear under 0.1% of the time consumption.

[Holger Freyther]

(In reply to comment #8)
> Created an attachment (id=50491) [details]
> Just cache the result of QWebPage::userAgentForUrl()

> From the benchs I have, the time spent in QWebPage::userAgentForUrl() is mostly
> taken by playing with the QString. I have attached a patch to make
> QWebPage::userAgentForUrl() disappear under 0.1% of the time consumption.

This assumes caching is an acceptable change. Currently when I change the QLocale of the QWebView, or use QSettings to set another application name.. starting from the next request I do have the right locale and app name set. If we decide that it does not matter for us this change is great and we should get it landed.

Did you measure this on a N900? I assume using the loading test? Could you do me a favor, pick any website from your test set, run the test for this site, and upload the result of opreport -c. I hope/assume you have frame pointers in your binary by now. It would be really awesome if you could do that.

Otherwise I will upload the profile from speedprof (memprof frontend) from my macbook, but having a result from the N900 would be better.

[Benjamin Poulain]

(In reply to comment #9)
> (In reply to comment #8)
> This assumes caching is an acceptable change. Currently when I change the
> QLocale of the QWebView, or use QSettings to set another application name..
> starting from the next request I do have the right locale and app name set. If
> we decide that it does not matter for us this change is great and we should get
> it landed.

For the locale modification, we can just reset the cache when receiving QEvent::LocaleChange.

The application name is a problem. We will have to update the documentation if we go that way.

> Did you measure this on a N900? I assume using the loading test? Could you do
> me a favor, pick any website from your test set, run the test for this site,
> and upload the result of opreport -c. I hope/assume you have frame pointers in
> your binary by now. It would be really awesome if you could do that.
> 
> Otherwise I will upload the profile from speedprof (memprof frontend) from my
> macbook, but having a result from the N900 would be better.

You can upload the profile from your macbook. If the improvement is important on a laptop, it will definitely improves the performance of the N900.

[Holger Freyther]

(In reply to comment #10)
> (In reply to comment #9)

> For the locale modification, we can just reset the cache when receiving
> QEvent::LocaleChange.
> 
> The application name is a problem. We will have to update the documentation if
> we go that way.

I would like to see your patch modified in the following way. The current userAgentForUrl method is using the view() pointer and different QWidgets can have different QLocales (probably an uncommon usecase). I would like to cache the userAgent in the private data of the QWebPage and add an invalidateCaches() method to QWebPagePrivate that will throw away whatever we have cached, currently this would be the ua string. Does this sound sensible?


> > Otherwise I will upload the profile from speedprof (memprof frontend) from my
> > macbook, but having a result from the N900 would be better.
> 
> You can upload the profile from your macbook. If the improvement is important
> on a laptop, it will definitely improves the performance of the N900.

Okay, I will do.

[Benjamin Poulain]

(In reply to comment #11)
> I would like to see your patch modified in the following way. The current
> userAgentForUrl method is using the view() pointer and different QWidgets can
> have different QLocales (probably an uncommon usecase). I would like to cache
> the userAgent in the private data of the QWebPage and add an invalidateCaches()
> method to QWebPagePrivate that will throw away whatever we have cached,
> currently this would be the ua string. Does this sound sensible?

Yep, sounds like a plan :)

Since the time is mostly spent getting the locale, and creating the string with QString::arg(), it makes sense to cache the locale (+ QWebPagePrivete::invalidateCaches() called on LocaleChange), and use QString builder instead of QString::arg().

[Holger Freyther]

Comment on attachment 48217 [details]
Second attempt with a unit test.

I'm withdrawing the patch. I still believe there is a underlying issue but I lack the time to work on it.

[Benjamin Poulain]

Created attachment 54809 [details]
Better timing on the userAgentForUrl()

This version avoid some allocations/reallocation of memory, most of the string being static now.

With this patch, the time of QUrl::setEncodedUrl() is the bottleneck.
Together QUrl::setEncodedUrl() and QWebPage::userAgentForUrl() take 0.02% of the time.

[Kenneth Rohde Christiansen]

Comment on attachment 54809 [details]
Better timing on the userAgentForUrl()


WebKit/qt/Api/qwebpage.cpp:3059
 +      // spliting the string in three and user QStringBuilder is better than using QString::arg()
splitting

WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:1795
 +      const QString markerString = QString::fromLatin1(" nn-NO)");
why two newlines here?

[Benjamin Poulain]

Created attachment 54811 [details]
Better timing on the userAgentForUrl()

Addressed the comments of Kenneth.

[Benjamin Poulain]

It should be safe to put this patch in the release.

Medium risk is a wrong string being produced.
Worst case WebKit not compiling on an exotic platform.

[WebKit Commit Bot]

Comment on attachment 54811 [details]
Better timing on the userAgentForUrl()

Clearing flags on attachment: 54811

Committed r58648: <http://trac.webkit.org/changeset/58648>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r58648 cherry-picked into qtwebkit-2.0 with commit bbc4af4deb39c80ed9f5cbbe13dba96b44a1124d

[Laszlo Gombos]

The committed patch does indeed changes the user Agent string (and breaks some business assumptions), which should have been part of a separate patch. 

For example this patch adds "SymbianOS" twice to the User Agent string. Fixing it as part of bug 38389.

[Benjamin Poulain]

(In reply to comment #21)
> The committed patch does indeed changes the user Agent string (and breaks some business assumptions), which should have been part of a separate patch. 
> For example this patch adds "SymbianOS" twice to the User Agent string. Fixing it as part of bug 38389.

My apologies, I messed the platform and OS for symbian when rewriting the function.
Thanks for fixing it.