Bug 122201 – Add *CSS* prefix to FontValue to generate toCSSFontValue()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Gyuyoung Kim]

In order to generate toCSSFontValue(), we need to add *CSS* prefix to FontValue.

[Gyuyoung Kim]

Created attachment 213142 [details]
Patch for ews

[Build Bot]

Comment on attachment 213142 [details]
Patch for ews

Attachment 213142 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/2903202

[Gyuyoung Kim]

Created attachment 213145 [details]
Patch for ews

[Darin Adler]

Comment on attachment 213145 [details]
Patch for ews

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

Looks, OK, but I’d like to see a patch that was made to properly reflect a file move.

This patch does not seem to have been prepared with a "svn mv" or "git mv" command. The patch does not show diffs between the old contents of FontValue.cpp and the new contents of CSSFontValue.cpp. Ideally we want to do that properly so that our Subversion history includes the history of the file, and doesn't just treat it as a new file.

> Source/WebCore/css/CSSValue.cpp:54
>  #if ENABLE(CSS_VARIABLES)
>  #include "CSSVariableValue.h"
>  #endif

This should be broken out into a separate paragraph, not just left in the middle of the list.

> Source/WebCore/css/CSSValue.cpp:55
> +#include "CSSFontValue.h"

This should be sorted alphabetically, not just left where it was.

[Gyuyoung Kim]

(In reply to comment #4)
> (From update of attachment 213145 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213145&action=review
> 
> Looks, OK, but I’d like to see a patch that was made to properly reflect a file move.

I used "git mv" command to rename FontValue.cpp|h. If you fetch "213145" attachment, you can see the FontValue.cpp|h are renamed.

#renamed:Source/WebCore/css/FontValue.cpp->Source/WebCore/css/CSSFontValue.cpp
#renamed:Source/WebCore/css/FontValue.h->Source/WebCore/css/CSSFontValue.h


> > Source/WebCore/css/CSSValue.cpp:54
> >  #if ENABLE(CSS_VARIABLES)
> >  #include "CSSVariableValue.h"
> >  #endif
> 
> This should be broken out into a separate paragraph, not just left in the middle of the list.
> 
> > Source/WebCore/css/CSSValue.cpp:55
> > +#include "CSSFontValue.h"
> 
> This should be sorted alphabetically, not just left where it was.

Ok, Fixed.

[Gyuyoung Kim]

Created attachment 213263 [details]
Patch

[Gyuyoung Kim]

Darin, could you take a look this again ?

[Darin Adler]

Comment on attachment 213263 [details]
Patch

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

> Source/WebCore/css/CSSValue.cpp:182
> +            return compareCSSValues<CSSFontValue>(*this, other);

Why aren’t we using the checked cast here?

> Source/WebCore/css/CSSValue.cpp:284
> +        return static_cast<const CSSFontValue*>(this)->customCSSText();

Why aren’t we using the checked cast here?

> Source/WebCore/css/CSSValue.cpp:399
> +        delete static_cast<CSSFontValue*>(this);

Why aren’t we using the checked cast here?

[Gyuyoung Kim]

Created attachment 213450 [details]
Patch for landing

[Gyuyoung Kim]

Comment on attachment 213263 [details]
Patch

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

>> Source/WebCore/css/CSSValue.cpp:182
>> +            return compareCSSValues<CSSFontValue>(*this, other);
> 
> Why aren’t we using the checked cast here?

*compareCSSValues* is not static_cast<>

>> Source/WebCore/css/CSSValue.cpp:284
>> +        return static_cast<const CSSFontValue*>(this)->customCSSText();
> 
> Why aren’t we using the checked cast here?

I'd like to change <const CSSFooValue> in a new bug at once. It looks I need to check if there is no layout test regression.

>> Source/WebCore/css/CSSValue.cpp:399
>> +        delete static_cast<CSSFontValue*>(this);
> 
> Why aren’t we using the checked cast here?

Fixed.

[WebKit Commit Bot]

Comment on attachment 213450 [details]
Patch for landing

Clearing flags on attachment: 213450

Committed r156961: <http://trac.webkit.org/changeset/156961>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.