| Summary: | [Qt] QWebPage viewMode property | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Luiz Agostini <luiz> | ||||||||||||||
| Component: | Platform | Assignee: | Simon Hausmann <hausmann> | ||||||||||||||
| Status: | CLOSED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | commit-queue, hausmann, kenneth, laszlo.gombos | ||||||||||||||
| Priority: | P3 | Keywords: | Qt | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Bug Depends on: | |||||||||||||||||
| Bug Blocks: | 35784 | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Luiz Agostini
2010-04-26 05:55:25 PDT
Created attachment 54287 [details]
patch 1
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. (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. Created attachment 54300 [details]
patch 2
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.
(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. Created attachment 54350 [details]
patch 3
(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? Comment on attachment 54350 [details]
patch 3
I guess it would be nice with an autotest for this feature as well.
Created attachment 54525 [details]
patch 4
Comment on attachment 54525 [details]
patch 4
Great Luiz! When this is cherry picked into QtWebKit 2.0, please inform Edisson et al.
Comment on attachment 54525 [details] patch 4 Clearing flags on attachment: 54525 Committed r58405: <http://trac.webkit.org/changeset/58405> All reviewed patches have been landed. Closing bug. 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. Created attachment 54668 [details]
drop wrt prefix from the property name
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.
Having an extra prefix (like qt_) is fine with me. My only point was that the viewMode property is not specific to wrt. So should it be _qt_viewMode or qt_viewMode? 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. 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. 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> All reviewed patches have been landed. Closing bug. Laszlo, applying these changesets to the release branch is going to remove the qt_wrt_setViewMode symbol. Is that okay with you? (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. |