Bug 102446

Summary: Make StyleResolver::applyProperty use isInherit in CSSPropertyWebkitMarquee instead of calculating equivalent in-place.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: New BugsAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, cmarcelo, menard, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Luke Macpherson
Reported 2012-11-15 16:36:07 PST
Make StyleResolver::applyProperty use isInherit in CSSPropertyWebkitMarquee instead of calculating equivalent in-place.
Attachments
Patch (1.60 KB, patch)
2012-11-15 16:38 PST, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Luke Macpherson
Comment 1 2012-11-15 16:38:08 PST
Created attachment 174555 [details] Patch
Alexey Proskuryakov
Comment 2 2012-11-15 23:02:47 PST
Comment on attachment 174555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174555&action=review > Source/WebCore/ChangeLog:8 > + !m_parentNode || !value->isInheritedValue() is equivalent to !isInherit (by De Morgan's law). Would it make sense to ASSERT it?
Luke Macpherson
Comment 3 2012-11-18 15:09:28 PST
Comment on attachment 174555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174555&action=review >> Source/WebCore/ChangeLog:8 >> + !m_parentNode || !value->isInheritedValue() is equivalent to !isInherit (by De Morgan's law). > > Would it make sense to ASSERT it? Not really. It's a mathematical certainty. This is likely a case where the original author was simply unaware of isInherit.
Allan Sandfeld Jensen
Comment 4 2012-11-20 13:43:01 PST
Comment on attachment 174555 [details] Patch Looks good to me. Not everyday you see mathematical proven patches.
Tony Chang
Comment 5 2012-11-26 13:34:32 PST
Comment on attachment 174555 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174555&action=review >>> Source/WebCore/ChangeLog:8 >>> + !m_parentNode || !value->isInheritedValue() is equivalent to !isInherit (by De Morgan's law). >> >> Would it make sense to ASSERT it? > > Not really. It's a mathematical certainty. This is likely a case where the original author was simply unaware of isInherit. FWIW, refactoring could cause this to get out of sync, so I don't think it would hurt. That said, we don't do that for other cases in the switch, so it's probably not a big deal.
WebKit Review Bot
Comment 6 2012-11-26 13:44:57 PST
Comment on attachment 174555 [details] Patch Clearing flags on attachment: 174555 Committed r135760: <http://trac.webkit.org/changeset/135760>
WebKit Review Bot
Comment 7 2012-11-26 13:45:00 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.