Bug 33875 - [Qt] QWebPage::userAgentForUrl is terrible API
Summary: [Qt] QWebPage::userAgentForUrl is terrible API
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Holger Freyther
URL:
Keywords: Performance, Qt
Depends on:
Blocks: 34208 35784
  Show dependency treegraph
 
Reported: 2010-01-19 16:46 PST by Holger Freyther
Modified: 2010-05-20 02:04 PDT (History)
7 users (show)

See Also:


Attachments
Add a QWebPage::setUserAgent which will allow us to not call userAgentForUrl and avoid the KURL -> QUrl conversion (8.59 KB, patch)
2010-01-19 16:47 PST, Holger Freyther
no flags Details | Formatted Diff | Diff
Second attempt with a unit test. (10.59 KB, patch)
2010-02-05 02:57 PST, Holger Freyther
zecke: commit-queue-
Details | Formatted Diff | Diff
Just cache the result of QWebPage::userAgentForUrl() (710 bytes, patch)
2010-03-11 05:41 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Better timing on the userAgentForUrl() (16.23 KB, patch)
2010-04-30 09:46 PDT, Benjamin Poulain
kenneth: review+
kenneth: commit-queue-
Details | Formatted Diff | Diff
Better timing on the userAgentForUrl() (16.44 KB, patch)
2010-04-30 10:15 PDT, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Holger Freyther 2010-01-19 16:46:18 PST
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...
Comment 1 Holger Freyther 2010-01-19 16:47:58 PST
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.
Comment 2 Simon Hausmann 2010-01-22 13:52:12 PST
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?
Comment 3 Simon Hausmann 2010-01-27 08:32:08 PST
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 :)
Comment 4 Eric Seidel (no email) 2010-02-01 16:12:29 PST
Attachment 46962 [details] was posted by a committer and has review+, assigning to Holger Freyther for commit.
Comment 5 Holger Freyther 2010-02-05 02:57:34 PST
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.
Comment 6 Eric Seidel (no email) 2010-02-08 15:11:53 PST
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.
Comment 7 Adam Barth 2010-03-09 07:45:46 PST
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?
Comment 8 Benjamin Poulain 2010-03-11 05:41:38 PST
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.
Comment 9 Holger Freyther 2010-03-11 05:56:25 PST
(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.
Comment 10 Benjamin Poulain 2010-03-11 07:19:09 PST
(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.
Comment 11 Holger Freyther 2010-03-11 16:34:23 PST
(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.
Comment 12 Benjamin Poulain 2010-03-12 00:09:38 PST
(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().
Comment 13 Holger Freyther 2010-03-26 07:16:04 PDT
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.
Comment 14 Benjamin Poulain 2010-04-30 09:46:13 PDT
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.
Comment 15 Kenneth Rohde Christiansen 2010-04-30 09:50:53 PDT
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?
Comment 16 Benjamin Poulain 2010-04-30 10:15:59 PDT
Created attachment 54811 [details]
Better timing on the userAgentForUrl()

Addressed the comments of Kenneth.
Comment 17 Benjamin Poulain 2010-04-30 10:29:36 PDT
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.
Comment 18 WebKit Commit Bot 2010-05-02 01:55:13 PDT
Comment on attachment 54811 [details]
Better timing on the userAgentForUrl()

Clearing flags on attachment: 54811

Committed r58648: <http://trac.webkit.org/changeset/58648>
Comment 19 WebKit Commit Bot 2010-05-02 01:55:21 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Simon Hausmann 2010-05-02 09:21:42 PDT
Revision r58648 cherry-picked into qtwebkit-2.0 with commit bbc4af4deb39c80ed9f5cbbe13dba96b44a1124d
Comment 21 Laszlo Gombos 2010-05-19 18:19:10 PDT
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.
Comment 22 Benjamin Poulain 2010-05-20 02:04:56 PDT
(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.