Bug 122201 - Add *CSS* prefix to FontValue to generate toCSSFontValue()
Summary: Add *CSS* prefix to FontValue to generate toCSSFontValue()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-01 18:59 PDT by Gyuyoung Kim
Modified: 2013-10-05 10:04 PDT (History)
9 users (show)

See Also:


Attachments
Patch for ews (26.32 KB, patch)
2013-10-01 19:04 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for ews (30.00 KB, patch)
2013-10-01 20:33 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (30.48 KB, patch)
2013-10-03 09:10 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for landing (30.43 KB, patch)
2013-10-05 07:01 PDT, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2013-10-01 18:59:16 PDT
In order to generate toCSSFontValue(), we need to add *CSS* prefix to FontValue.
Comment 1 Gyuyoung Kim 2013-10-01 19:04:36 PDT
Created attachment 213142 [details]
Patch for ews
Comment 2 Build Bot 2013-10-01 19:45:56 PDT
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
Comment 3 Gyuyoung Kim 2013-10-01 20:33:34 PDT
Created attachment 213145 [details]
Patch for ews
Comment 4 Darin Adler 2013-10-02 09:13:48 PDT
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.
Comment 5 Gyuyoung Kim 2013-10-03 09:07:30 PDT
(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.
Comment 6 Gyuyoung Kim 2013-10-03 09:10:18 PDT
Created attachment 213263 [details]
Patch
Comment 7 Gyuyoung Kim 2013-10-03 22:57:02 PDT
Darin, could you take a look this again ?
Comment 8 Darin Adler 2013-10-04 17:09:21 PDT
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?
Comment 9 Gyuyoung Kim 2013-10-05 07:01:05 PDT
Created attachment 213450 [details]
Patch for landing
Comment 10 Gyuyoung Kim 2013-10-05 07:04:33 PDT
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.
Comment 11 WebKit Commit Bot 2013-10-05 10:04:49 PDT
Comment on attachment 213450 [details]
Patch for landing

Clearing flags on attachment: 213450

Committed r156961: <http://trac.webkit.org/changeset/156961>
Comment 12 WebKit Commit Bot 2013-10-05 10:04:53 PDT
All reviewed patches have been landed.  Closing bug.