Bug 41465

Created attachment 60254 [details] Patch

Simon, Kenneth and Henry, could you guys please take a look at this documentation and see what could be improved? Thanks in advance!

Although I generally like the writing as a blog post, I am not sure the language used works as good for a documentation. Anyways, good work. Please,find below some questions and comments: + Among a tons "a tons" you are using a singular article and the a plural substantive. "Among tons ..." maybe? + First we should make it clear that \l{QGraphicsWebView} is the way forward, + so if you want to target mobile devices, don't even consider \l{QWebView}. Why is + that? Well, the \l{QWebView} is based on the \l{QWidget} system, thus it cannot + easily support rotation, overlays, hardware accelerated compositing and tiling. + If you need a \l{QWidget} anyway, you can always construct a \l{QGraphicsView} + (which is a \l{QWidget}) with a \l{QGraphicsWebView} inside. Although I agree with what is said above, it sounds a bit too personal and intrusive than it should be IMO. + So let's start with the most simple \l{QGraphicsWebView} based "browser" + ever: Ditto. Are you sure it needs to mention it is the most simple browser ever? Is it really it? + Loading, layouting maybe the it sounds better as "laying out" or "rendering". + webview.load(QUrl("http://www.wouwlabs.com/blogs/jeez")); maybe some more generic url here, too. Like "docs.qt.nokia" :)

Created attachment 60562 [details] Documentation Thanks for the review, Antonio. I've made the required changes.

Good stuff, thanks for writing this document, Jesus! A couple of small mainly editorial comments below. + that I'm disabling the scrollbars on the graphics view because QtWebKit handles I suggest saying "we're disabling" for a bit more neutral tone. + have to do and makes it possible to make nicely kinetic scrolling a + possibility. Please reword, for example "enables nice kinetic scrolling" + Loading, rendering, laying out, etc are blocking operations. Though barely noticeable on + a Desktop machines, these operations can block for a long time on a mobile "on a desktop machine" or "on desktop machines" + One way to over come this issue, is to do all loading, layouting and + painting (basically all non-UI related work) in another thread or process, and + just blit the result from the web process/thread to the UI. We should probably mention that this is a possibility that we are researching for a future version of QtWebKit, as this is not available out of the box in the current version. Instead, freezing the backing store can be used when performing a zooming operation, for instance. You could refer to the part later in this document that discusses this. About tiling, we already have brief reference documentation, which we could point to in this document: http://doc.qt.nokia.com/4.7-snapshot/qwebsettings.html#WebAttribute-enum (see the entry for TiledBackingStoreEnabled) + In QtWebKit trunk we already have support for this with a nice API. You Most developers will see this documentation only once it is included in a QtWebKit release. Most are not aware of the QtWebKit trunk. Maybe we can just omit this sentence?

Created attachment 62768 [details] Documentation Thanks for the review, Henry. I've modified the previous patch with the needed corrections.

Simon, what do you think about this?

Any other review on this, guys?

Comment on attachment 62768 [details] Documentation View in context: https://bugs.webkit.org/attachment.cgi?id=62768&action=prettypatch > WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:12 > + Among tons of bug fixes and good performance improvements there are also > + lots of new features being developed, mainly geared toward mobile deployment. Maybe these lines are not relevant? > WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:74 > + More information about Tiling can be found here: \l{http://doc.qt.nokia.com/4.7-snapshot/qwebsettings.html#WebAttribute-enum} (see the entry for TiledBackingStoreEnabled) I think this should be done differently to link inside the documentation > WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:100 > + Qt 4.7 docs also says: \e{"This property should be used in conjunction with > + the QWebPage::preferredContentsSize property. If not explicitly set, the > + preferredContentsSize is automatically set to a reasonable value."} we probably should refer to Qt 4.7 > WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:172 > + You must connect the signal \c{QWebPage::viewportChangeRequested(const > + QWebPage::ViewportHints& hints)} to a slot of your mobile web view and use what > + is provided by \l{QWebPage::ViewportHints} to update your viewport size, scale > + range, and so on. We are about to change the above API, please check the relevant bug

Jesus, any chance you could update this if it is not too much work at the moment? We regularly get questions about mobile features, it would be nice to be able to point at the documentation.

This documentation needs info about setActualVisibleContentsRect. You might want to incorporate some of my comments on implementing pinch zoom here: https://lists.webkit.org/pipermail/webkit-qt/2011-January/001103.html

Don't we have any technical writers to work on this? It is a shame if we don't.

Set as a P3, it's a nice to have thing. I sent an email to the doc team of Qt to see if they can look at it.

Created attachment 98000 [details] Updated patch with some minor style and content changes. I've made some changes directly to the patch rather than create a new one, trying very hard not to break it. Let me know if I need to regenerate it.

Comment on attachment 98000 [details] Updated patch with some minor style and content changes. View in context: https://bugs.webkit.org/attachment.cgi?id=98000&action=review > WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:31 > * handles scrolling and scroll bars automatically. This is to allow scrolling ???

(In reply to comment #15) > (From update of attachment 98000 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=98000&action=review > > > WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:31 > > * handles scrolling and scroll bars automatically. This is to allow scrolling > > ??? Maybe it should refer to QWebView there. Another issue was that the new documentation mentions the QWebPage::viewportChangeRequested() signal, but this does not appear to exist in QtWebKit supplied with Qt 4.7. Should I be looking for an equivalent signal elsewhere?

(In reply to comment #16) > (In reply to comment #15) > > (From update of attachment 98000 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=98000&action=review > > > > > WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:31 > > > * handles scrolling and scroll bars automatically. This is to allow scrolling > > > > ??? > > Maybe it should refer to QWebView there. > > Another issue was that the new documentation mentions the QWebPage::viewportChangeRequested() signal, but this does not appear to exist in QtWebKit supplied with Qt 4.7. Should I be looking for an equivalent signal elsewhere? I was wondering why the * was inserted there :-)

Created attachment 113155 [details] Patch

(In reply to comment #18) > Created an attachment (id=113155) [details] > Patch I rebased the patch and removed the weird * from line 31. I just checked qwebpage.h and the signal viewportChangeRequested() is there and it's documented on qwebpage.cpp, but I have no clue why it's not on Qt 4.7 docs.

(In reply to comment #19) > (In reply to comment #18) > > Created an attachment (id=113155) [details] [details] > > Patch > > I rebased the patch and removed the weird * from line 31. > I just checked qwebpage.h and the signal viewportChangeRequested() is there and it's documented on qwebpage.cpp, but I have no clue why it's not on Qt 4.7 docs. The signal is available on Qt 4.8.

Comment on attachment 113155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113155&action=review > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:8 > + A lot of effort has been put into QtWebKit to make it attractive on for remove on > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:12 > + Among a number of bug fixes and performance improvements there are also > + new features being developed, mainly geared toward mobile deployment. noone cares aboutperformance and bug fixes in documentation > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:13 > + The goal with this tutorial is to help you understand some of these new remove new > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:29 > + \l{QGraphicsWebView} to the scene. It might seem a bit useless as you can only > + navigate through one website, but it serves well as a simple example. Notice no reason to say that it seems useless. > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:30 > + that we're disabling the scroll bars on the graphics view because QtWebKit QGraphicsView ? > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:44 > + a grid of tiles, so that when you update some area, instead of just updating > + the area you actually update the whole tile. This offers a few advantages for Not the whole truth. We update sub regions of times > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:59 > + just blit the result from the web process/thread to the UI. There is research > + in progress to enable this for a future version of QtWebKit, but for now heh! Talk about WebKit2? > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:153 > + \section1 The 'viewport' Meta-Tag Very simplistic... what about reusing from the N9 documentation

Created attachment 113158 [details] new patch rebased and with a few fixes

Comment on attachment 113158 [details] new patch rebased and with a few fixes View in context: https://bugs.webkit.org/attachment.cgi?id=113158&action=review > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:9 > + use in mobile devices. on* > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:11 > + There are new features being developed, mainly geared toward mobile deployment. remove this line > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:12 > + The goal with this tutorial is to help you understand some of these the mobile features > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:26 > + Here we just set up a \l{QGraphicsView} application and add a remove just > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:28 > + that we're disabling the scroll bars on the QGraphicsView because QtWebKit scrollbars? > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:52 > + their finger to scroll, giving a bad user experience. leading to a bad

Cant you get a qt doc guy to review this?

Created attachment 113318 [details] new patch

(In reply to comment #24) > Cant you get a qt doc guy to review this? Not sure, who should I talk to?

Created attachment 113492 [details] possibly a final patch after review and fix from qt docs team

Comment on attachment 113492 [details] possibly a final patch after review and fix from qt docs team View in context: https://bugs.webkit.org/attachment.cgi?id=113492&action=review > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:151 > + As some sites do not work with 960 pixels width or want to have control of isn't it 980 ? just wondering > Source/WebKit/qt/docs/qtwebkit-goes-mobile.qdoc:153 > + how the page is laid out, QtWebKit, Android, Firefox Mobile and > + the iPhone Safari browser support a meta-tag called \c viewport. This makes The blackberry as well

Comment on attachment 113492 [details] possibly a final patch after review and fix from qt docs team CQ+ it now because it needs to get be back ported to QtWebKit 2.2 (and Qt 4.8) today. Anything else I will fix later. Thanks for the reviews, everyone!

Comment on attachment 113492 [details] possibly a final patch after review and fix from qt docs team Clearing flags on attachment: 113492 Committed r99196: <http://trac.webkit.org/changeset/99196>

All reviewed patches have been landed. Closing bug.