Bug 29639 - [Qt] Update the stypeProperty API of QWebElement.
Summary: [Qt] Update the stypeProperty API of QWebElement.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2009-09-22 05:51 PDT by Jocelyn Turcotte
Modified: 2009-09-25 01:44 PDT (History)
3 users (show)

See Also:


Attachments
Patch (28.29 KB, patch)
2009-09-22 05:54 PDT, Jocelyn Turcotte
eric: review-
Details | Formatted Diff | Diff
Split patch (16.08 KB, patch)
2009-09-24 04:29 PDT, Jocelyn Turcotte
no flags Details | Formatted Diff | Diff
Split patch v0.2 (16.18 KB, patch)
2009-09-24 05:20 PDT, Jocelyn Turcotte
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jocelyn Turcotte 2009-09-22 05:51:52 PDT
Change to the QWebElement as discussed in meetings+mails.

The ResolveRule vs. StyleResolveStrategy enum name was still under discussion, please add any comment regarding this if needed.

The patch also update the documentation.
Comment 1 Jocelyn Turcotte 2009-09-22 05:54:48 PDT
Created attachment 39916 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2009-09-22 05:57:52 PDT
I like the new StyleResolveStrategy, but I think we should use the old name for it, the ResolveRule, as it is more a rule than a strategy, as what we are doing is very well defined. You could call it StyleResolveRule or maybe even CssResolveRule?
Comment 3 Antonio Gomes 2009-09-22 06:22:00 PDT
patch does much more than what is described in bug title: "Update QWebElement API to remove QtScript related methods."

could we split it up ? maybe even in two bugs ?
Comment 4 Eric Seidel (no email) 2009-09-23 17:36:33 PDT
Comment on attachment 39916 [details]
Patch

r- based on the above request to have this split.  28k is pretty big for a patch, making it difficult to review. :(
Comment 5 Jocelyn Turcotte 2009-09-24 04:29:40 PDT
Created attachment 40058 [details]
Split patch

Yes this makes sense, I've split the patch in 3 including these two other bugs:
https://bugs.webkit.org/show_bug.cgi?id=29708
https://bugs.webkit.org/show_bug.cgi?id=29709

This patch now only contain the modifications about QWebElement::styleProperty:

(QWebElement::styleProperty):
- Merge the stypeProperty and the computedStyleProperty methods
- Remove the default value for the style resolving enum
- Rename ResolveRule to StyleResolveStrategy
(QWebElement::setStyleProperty):
- Remove the priority argument since it is possible to control the
  behaviour by adding !important or removing in the value.
Comment 6 Jocelyn Turcotte 2009-09-24 05:20:14 PDT
Created attachment 40060 [details]
Split patch v0.2

- Removes the extra computedStyle method from qwebelement.h
- Specify in setStyleProperty's doc that this modifies the inline style.
Comment 7 Antonio Gomes 2009-09-24 05:28:02 PDT
jocelyn, changes look nice to me, as they make api simpler.

one simple question: should we keep the "\since 4.6" ?

(..)
-    \enum QWebElement::ResolveRule
-    \since 4.6
+    \enum QWebElement::StyleResolveStrategy
(..)
Comment 8 Jocelyn Turcotte 2009-09-24 05:48:13 PDT
In fact QWebElement in whole is new to 4.6 so it is not needed.
I took the opportunity of this patch to change this, I should have noted this extra change earlier in the bug or changelog.

(In reply to comment #7)
> jocelyn, changes look nice to me, as they make api simpler.
> 
> one simple question: should we keep the "\since 4.6" ?
> 
> (..)
> -    \enum QWebElement::ResolveRule
> -    \since 4.6
> +    \enum QWebElement::StyleResolveStrategy
> (..)
Comment 9 Simon Hausmann 2009-09-25 01:13:29 PDT
Comment on attachment 40060 [details]
Split patch v0.2

r=me. Let's discuss Strategy vs. Rule with Kenneth though, we can still change it
Comment 10 Simon Hausmann 2009-09-25 01:44:00 PDT
Committed r48749: <http://trac.webkit.org/changeset/48749>