Bug 38119 – [Qt] QWebPage viewMode property

• Status: CLOSED
• Resolution: FIXED
• Component: Platform

[Luiz Agostini]

Replacing method qt_wrt_setViewMode by wrt_viewMode property.

[Luiz Agostini]

Created attachment 54287 [details]
patch 1

[Laszlo Gombos]

Would be useful to explain why the change of hart about the interface name ? I do support getting rid of qt_ prefix, but I'm not sure if wrt make sense in the name as this might not be just for wrt really.

[Luiz Agostini]

(In reply to comment #2)
> Would be useful to explain why the change of hart about the interface name ? I
> do support getting rid of qt_ prefix, but I'm not sure if wrt make sense in the
> name as this might not be just for wrt really.

I agree.

[Luiz Agostini]

Created attachment 54300 [details]
patch 2

[Kenneth Rohde Christiansen]

Comment on attachment 54300 [details]
patch 2

So this means that this is now public API. Do we really want that yet?

Simon? Spec is Last Call Working Draft btw.

[Simon Hausmann]

(In reply to comment #5)
> (From update of attachment 54300 [details])
> So this means that this is now public API. Do we really want that yet?
> 
> Simon? Spec is Last Call Working Draft btw.

I would prefer not to add public API anymore at this point unless it's really necessary.

I'm certainly in favour of using dynamic properties for this instead, as changing these private functions always creates a big mess for updating the def files on Symbian.

[Luiz Agostini]

Created attachment 54350 [details]
patch 3

[Luiz Agostini]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 54300 [details] [details])
> > So this means that this is now public API. Do we really want that yet?
> > 
> > Simon? Spec is Last Call Working Draft btw.
> 
> I would prefer not to add public API anymore at this point unless it's really
> necessary.
> 
> I'm certainly in favour of using dynamic properties for this instead, as
> changing these private functions always creates a big mess for updating the def
> files on Symbian.

Simon, s this what you mean?

[Kenneth Rohde Christiansen]

Comment on attachment 54350 [details]
patch 3

I guess it would be nice with an autotest for this feature as well.

[Luiz Agostini]

Created attachment 54525 [details]
patch 4

[Kenneth Rohde Christiansen]

Comment on attachment 54525 [details]
patch 4

Great Luiz! When this is cherry picked into QtWebKit 2.0, please inform Edisson et al.

[WebKit Commit Bot]

Comment on attachment 54525 [details]
patch 4

Clearing flags on attachment: 54525

Committed r58405: <http://trac.webkit.org/changeset/58405>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

I think the name of the property should not include wrt prefix as this is not specific to wrt (see also comment #3 and patch #2). Patch will follow for the rename.

[Laszlo Gombos]

Created attachment 54668 [details]
drop wrt prefix from the property name

[Simon Hausmann]

Comment on attachment 54668 [details]
drop wrt prefix from the property name

Hmm, I admit I prefer a prefixed or somewhat obfuscated name for these dynamic and private properties, to avoid clashes with a real "viewMode" property (Q_PROPERTY) if we introduce it in the future. Q_PROPERTY and dynamic properties share the same namespace.

In Qt we use _qt_ as prefix for private dynamic properties.

[Laszlo Gombos]

Having an extra prefix (like qt_) is fine with me. My only point was that the viewMode property is not specific to wrt.

[Kenneth Rohde Christiansen]

So should it be _qt_viewMode or qt_viewMode?

[Laszlo Gombos]

Hm.. I also so _q_ (without the "t") naming patter in Qt (see qwidget.cpp). 

Simon, maybe best if you make a call on this.

[Laszlo Gombos]

Created attachment 54995 [details]
Use _q_ prefix for the property name

According to http://doc.trolltech.com/4.6/qobject.html#setProperty _q_ seems to be the correct prefix.

[WebKit Commit Bot]

Comment on attachment 54995 [details]
Use _q_ prefix for the property name

Clearing flags on attachment: 54995

Committed r58764: <http://trac.webkit.org/changeset/58764>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Laszlo, applying these changesets to the release branch is going to remove the qt_wrt_setViewMode symbol. Is that okay with you?

[Laszlo Gombos]

(In reply to comment #23)
> Laszlo, applying these changesets to the release branch is going to remove the
> qt_wrt_setViewMode symbol. Is that okay with you?

Yes that is OK with me. I think the conclusion was that it is too early for a public API and I do not see a big difference between the private exported symbol and the dynamic property.

[Simon Hausmann]

Revision r58405 cherry-picked into qtwebkit-2.0 with commit 81f2bccdfe405c730b1a19baf17861d205e801d8
Revision r58764 cherry-picked into qtwebkit-2.0 with commit 5aaee8fcd1db133d76b56c989477ab1a92759f9f