Summary: | [Qt] Update the stypeProperty API of QWebElement. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jocelyn Turcotte <jturcotte> | ||||||||
Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Enhancement | CC: | hausmann, kenneth, tonikitoo | ||||||||
Priority: | P2 | Keywords: | Qt | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Jocelyn Turcotte
2009-09-22 05:51:52 PDT
Created attachment 39916 [details]
Patch
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? 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 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. :(
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. 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.
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 (..) 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 on attachment 40060 [details]
Split patch v0.2
r=me. Let's discuss Strategy vs. Rule with Kenneth though, we can still change it
Committed r48749: <http://trac.webkit.org/changeset/48749> |