Bug 38119

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:

Keywords:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

35784

Attachments:

Description	Flags
patch 1	none
patch 2	none
patch 3	none
patch 4	none
drop wrt prefix from the property name	none
Use _q_ prefix for the property name	none

Luiz Agostini

Reported 2010-04-26 05:55:25 PDT

Replacing method qt_wrt_setViewMode by wrt_viewMode property.

Attachments
patch 1 (5.11 KB, patch) 2010-04-26 06:24 PDT, Luiz Agostini	no flags	Details Formatted Diff Diff
patch 2 (5.08 KB, patch) 2010-04-26 08:00 PDT, Luiz Agostini	no flags	Details Formatted Diff Diff
patch 3 (5.27 KB, patch) 2010-04-26 16:59 PDT, Luiz Agostini	no flags	Details Formatted Diff Diff
patch 4 (6.91 KB, patch) 2010-04-27 22:56 PDT, Luiz Agostini	no flags	Details Formatted Diff Diff
drop wrt prefix from the property name (3.61 KB, patch) 2010-04-28 21:07 PDT, Laszlo Gombos	no flags	Details Formatted Diff Diff
Use _q_ prefix for the property name (3.61 KB, patch) 2010-05-03 22:04 PDT, Laszlo Gombos	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

Luiz Agostini

Comment 1 2010-04-26 06:24:21 PDT

Created attachment 54287 [details] patch 1

Laszlo Gombos

Comment 2 2010-04-26 07:24:53 PDT

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

Comment 3 2010-04-26 07:42:51 PDT

(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

Comment 4 2010-04-26 08:00:37 PDT

Created attachment 54300 [details] patch 2

Kenneth Rohde Christiansen

Comment 5 2010-04-26 08:13:03 PDT

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

Comment 6 2010-04-26 09:00:38 PDT

(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

Comment 7 2010-04-26 16:59:22 PDT

Created attachment 54350 [details] patch 3

Luiz Agostini

Comment 8 2010-04-26 17:01:40 PDT

(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 9 2010-04-26 18:53:18 PDT

Comment on attachment 54350 [details] patch 3 I guess it would be nice with an autotest for this feature as well.

Luiz Agostini

Comment 10 2010-04-27 22:56:26 PDT

Created attachment 54525 [details] patch 4

Kenneth Rohde Christiansen

Comment 11 2010-04-28 05:48:03 PDT

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 12 2010-04-28 05:58:27 PDT

Comment on attachment 54525 [details] patch 4 Clearing flags on attachment: 54525 Committed r58405: <http://trac.webkit.org/changeset/58405>

WebKit Commit Bot

Comment 13 2010-04-28 05:58:39 PDT

All reviewed patches have been landed. Closing bug.

Laszlo Gombos

Comment 14 2010-04-28 21:06:57 PDT

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

Comment 15 2010-04-28 21:07:53 PDT

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

Simon Hausmann

Comment 16 2010-04-29 05:23:45 PDT

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

Comment 17 2010-04-29 05:26:47 PDT

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

Comment 18 2010-04-29 05:30:32 PDT

So should it be _qt_viewMode or qt_viewMode?

Laszlo Gombos

Comment 19 2010-04-29 08:48:38 PDT

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

Comment 20 2010-05-03 22:04:27 PDT

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 21 2010-05-04 09:31:35 PDT

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

Comment 22 2010-05-04 09:31:42 PDT

All reviewed patches have been landed. Closing bug.

Simon Hausmann

Comment 23 2010-05-07 00:49:32 PDT

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

Laszlo Gombos

Comment 24 2010-05-07 10:39:08 PDT

(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

Comment 25 2010-05-07 16:01:00 PDT

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

Note You need to log in before you can comment on or make changes to this bug.