Bug 58964 - Allow casting between CSSPrimitiveValue and EBorderCollapse to remove special-case logic from CSSStyleSelector.
Summary: Allow casting between CSSPrimitiveValue and EBorderCollapse to remove special...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-19 22:31 PDT by Luke Macpherson
Modified: 2011-04-20 09:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.39 KB, patch)
2011-04-19 22:36 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (6.39 KB, patch)
2011-04-19 23:01 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-04-19 22:31:48 PDT
Allow casting between CSSPrimitiveValue and EBorderCollapse to remove special-case logic from CSSStyleSelector.
Comment 1 Luke Macpherson 2011-04-19 22:36:33 PDT
Created attachment 90308 [details]
Patch
Comment 2 WebKit Review Bot 2011-04-19 22:38:46 PDT
Attachment 90308 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:196:  _border_collapse is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Luke Macpherson 2011-04-19 22:44:06 PDT
Style fail is due to legacy code.
Let me know what you think. Seems that there are a lot of these cases that when fixed will greatly simplify CSSStyleSelector/CSSStyleApplyProperty.
Comment 4 Eric Seidel (no email) 2011-04-19 22:55:48 PDT
Comment on attachment 90308 [details]
Patch

Sounds good to me.  style->borderColapse() isn't called anywhere?
Comment 5 Simon Fraser (smfr) 2011-04-19 22:56:14 PDT
Comment on attachment 90308 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=90308&action=review

> Source/WebCore/ChangeLog:8
> +        No new tests as no new functinoality added.

Typo
Comment 6 Luke Macpherson 2011-04-19 23:01:24 PDT
Created attachment 90309 [details]
Patch
Comment 7 WebKit Review Bot 2011-04-19 23:03:49 PDT
Attachment 90309 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/rendering/style/RenderStyle.h:196:  _border_collapse is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Luke Macpherson 2011-04-19 23:04:41 PDT
(In reply to comment #4)
> (From update of attachment 90308 [details])
> Sounds good to me.  style->borderColapse() isn't called anywhere?

The enum maps to 0,1 in the same way as the boolean.
Also the place it gets called in CSSStyleSelector (in the macro) is of the form set(get()).
Comment 9 Luke Macpherson 2011-04-19 23:04:58 PDT
(In reply to comment #5)
> (From update of attachment 90308 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=90308&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests as no new functinoality added.
> 
> Typo

Fixed.
Comment 10 WebKit Commit Bot 2011-04-20 09:14:52 PDT
Comment on attachment 90309 [details]
Patch

Clearing flags on attachment: 90309

Committed r84380: <http://trac.webkit.org/changeset/84380>
Comment 11 WebKit Commit Bot 2011-04-20 09:14:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2011-04-20 09:28:22 PDT
http://trac.webkit.org/changeset/84380 might have broken Qt Linux Release minimal
Comment 13 Dimitri Glazkov (Google) 2011-04-20 09:47:09 PDT
(In reply to comment #12)
> http://trac.webkit.org/changeset/84380 might have broken Qt Linux Release minimal

Fix landed as http://trac.webkit.org/changeset/84385.