Summary: | Quirksmode (CSS1): Removing inline border styles is impossible | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adele Peterson <adele> | ||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Major | CC: | bugs-webkit, eric, ian, mitz | ||||||
Priority: | P2 | ||||||||
Version: | 420+ | ||||||||
Hardware: | All | ||||||||
OS: | OS X 10.4 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 9610 | ||||||||
Attachments: |
|
Description
Adele Peterson
2006-06-02 15:14:45 PDT
CHanging component. Created attachment 12285 [details]
Expand shorthands for removal
Please let me know if this is going in the right direction. Not sure about using a HashMap - perhaps a simple array indexed by property ID would be better.
Yeah, this is great. What about "border" though? I see expansions for border-top/left/right/bottom but not for border. I assume it will be recursive and will expand border into border-top/left/right/bottom and then from there into top-width/color/style, etc. (In reply to comment #5) > Yeah, this is great. What about "border" though? I see expansions for > border-top/left/right/bottom but not for border. Hiding here (I wanted to reuse the first 4 arrays): + shorthandMap->set(CSS_PROP_BORDER, new PropertyLonghand(borderTopProperties, (sizeof(borderTopProperties) + sizeof(borderRightProperties) + sizeof(borderBottomProperties) + sizeof(borderLeftProperties)) / sizeof(borderTopProperties[0]))); > I assume it will be recursive > and will expand border into border-top/left/right/bottom and then from there > into top-width/color/style, etc. Given that border is the only 2-level (and 2-way) "hierarchy", I figured it would be an overkill to implement it with recursion. > Comment on attachment 12285 [details]
Expand shorthands for removal
+struct PropertyLonghand
+{
We should put the brace on the line iwth the struct name.
+static HashMap<int, PropertyLonghand* >* shorthandMap;
I don't think the space before the > is needed here. I think the values should be PropertyLonghand, no need for pointers.
The shortHandMap could be a static inside the CSSMutableStyleDeclaration::removeProperty function -- I think I like that slightly better.
It would be nice if the function that turns a propertyID into a pointer to a list of property IDs and a length was put with the other shorthand handling rather than right here in CSSMutableStyleDeclaration -- CSSMutableStyleDeclaration could just call a function defined there. That might make it easier to keep the list of shorthands in sync with the other lists of shorthands.
Should I mark this review- now, or do you want other people to look at it?
Comment on attachment 12285 [details] Expand shorthands for removal > Hiding here (I wanted to reuse the first 4 arrays): > + shorthandMap->set(CSS_PROP_BORDER, new PropertyLonghand(borderTopProperties, (sizeof(borderTopProperties) + sizeof(borderRightProperties) + sizeof(borderBottomProperties) > + sizeof(borderLeftProperties)) / sizeof(borderTopProperties[0]))); That technique is not reliable. It relies on the four arrays being consecutive in global data, something that is not guaranteed by the C++ language. Created attachment 12303 [details] Expand shorthands for removal (In reply to comment #7) > +struct PropertyLonghand > +{ > > We should put the brace on the line iwth the struct name. Done. > +static HashMap<int, PropertyLonghand* >* shorthandMap; > > I don't think the space before the > is needed here. I think the values should > be PropertyLonghand, no need for pointers. Done. > The shortHandMap could be a static inside the > CSSMutableStyleDeclaration::removeProperty function -- I think I like that > slightly better. Done. > It would be nice if the function that turns a propertyID into a pointer to a > list of property IDs and a length was put with the other shorthand handling > rather than right here in CSSMutableStyleDeclaration -- > CSSMutableStyleDeclaration could just call a function defined there. That might > make it easier to keep the list of shorthands in sync with the other lists of > shorthands. Agreed, but I think that can (and should) be a part of bug 12159. > That technique is not reliable. It relies on the four arrays being consecutive > in global data, something that is not guaranteed by the C++ language. Fixed. I also updated the set of properties in this patch to be exactly those properties that are parsed as shorthands. Interestingly enough (for me) the list doesn't include 'font', '-webkit-border-radius' and 'overflow'. Added change log and a test. Comment on attachment 12303 [details]
Expand shorthands for removal
r=me
Landed in r18753. |