Bug 41465

Summary: [Qt] QtWebKit should have documentation clarifying its mobile features usage
Product: WebKit Reporter: Jesus Sanchez-Palencia <jesus>
Component: New BugsAssignee: Jesus Sanchez-Palencia <jesus>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, christian.webkit, david.boddie, hausmann, henry.haverinen, jesus, joone, kenneth, laszlo.gombos, menard, tonikitoo, webkit.review.bot, zeno
Priority: P3 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 55056    
Attachments:
Description Flags
Patch
none
Documentation
none
Documentation
none
Updated patch with some minor style and content changes.
none
Patch
none
new patch rebased and with a few fixes
none
new patch
none
possibly a final patch after review and fix from qt docs team none

Description Jesus Sanchez-Palencia 2010-07-01 08:43:17 PDT
[Qt] QtWebKit should have documentation clarifying its mobile features usage
Comment 1 Jesus Sanchez-Palencia 2010-07-01 08:46:34 PDT
Created attachment 60254 [details]
Patch
Comment 2 Jesus Sanchez-Palencia 2010-07-02 10:06:07 PDT
Simon, Kenneth and Henry,

could you guys please take a look at this documentation and see what could be improved?

Thanks in advance!
Comment 3 Antonio Gomes 2010-07-03 12:13:49 PDT
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" :)
Comment 4 Jesus Sanchez-Palencia 2010-07-05 11:45:06 PDT
Created attachment 60562 [details]
Documentation

Thanks for the review, Antonio.

I've made the required changes.
Comment 5 Henry Haverinen 2010-07-27 02:23:17 PDT

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?
Comment 6 Jesus Sanchez-Palencia 2010-07-27 16:20:20 PDT
Created attachment 62768 [details]
Documentation

Thanks for the review, Henry.
I've modified the previous patch with the needed corrections.
Comment 7 Kenneth Rohde Christiansen 2010-07-27 18:10:37 PDT
Simon, what do you think about this?
Comment 8 Jesus Sanchez-Palencia 2010-09-11 14:19:56 PDT
Any other review on this, guys?
Comment 9 Kenneth Rohde Christiansen 2010-09-12 10:37:57 PDT
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
Comment 10 Benjamin Poulain 2011-01-08 08:15:50 PST
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.
Comment 11 Kenneth Rohde Christiansen 2011-01-08 08:39:17 PST
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
Comment 12 Kenneth Rohde Christiansen 2011-01-08 08:41:13 PST
Don't we have any technical writers to work on this? It is a shame if we don't.
Comment 13 Alexis Menard (darktears) 2011-03-31 05:58:11 PDT
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.
Comment 14 David Boddie 2011-06-21 09:46:24 PDT
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 15 Kenneth Rohde Christiansen 2011-06-21 10:08:48 PDT
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

???
Comment 16 David Boddie 2011-06-21 10:45:23 PDT
(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?
Comment 17 Kenneth Rohde Christiansen 2011-06-21 11:09:25 PDT
(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 :-)
Comment 18 Jesus Sanchez-Palencia 2011-11-01 03:53:18 PDT
Created attachment 113155 [details]
Patch
Comment 19 Jesus Sanchez-Palencia 2011-11-01 03:56:57 PDT
(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.
Comment 20 Jesus Sanchez-Palencia 2011-11-01 04:02:44 PDT
(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 21 Kenneth Rohde Christiansen 2011-11-01 04:29:46 PDT
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
Comment 22 Jesus Sanchez-Palencia 2011-11-01 05:04:55 PDT
Created attachment 113158 [details]
new patch rebased and with a few fixes
Comment 23 Kenneth Rohde Christiansen 2011-11-02 05:40:20 PDT
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
Comment 24 Kenneth Rohde Christiansen 2011-11-02 05:40:52 PDT
Cant you get a qt doc guy to review this?
Comment 25 Jesus Sanchez-Palencia 2011-11-02 07:56:34 PDT
Created attachment 113318 [details]
new patch
Comment 26 Jesus Sanchez-Palencia 2011-11-02 07:57:13 PDT
(In reply to comment #24)
> Cant you get a qt doc guy to review this?

Not sure, who should I talk to?
Comment 27 Jesus Sanchez-Palencia 2011-11-03 07:53:05 PDT
Created attachment 113492 [details]
possibly a final patch after review and fix from qt docs team
Comment 28 Kenneth Rohde Christiansen 2011-11-03 08:18:45 PDT
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 29 Jesus Sanchez-Palencia 2011-11-03 08:26:59 PDT
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 30 Simon Hausmann 2011-11-03 08:31:49 PDT
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>
Comment 31 Simon Hausmann 2011-11-03 08:32:00 PDT
All reviewed patches have been landed.  Closing bug.