In order to generate toCSSFontValue(), we need to add *CSS* prefix to FontValue.
Created attachment 213142 [details] Patch for ews
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
Created attachment 213145 [details] Patch for ews
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.
(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.
Created attachment 213263 [details] Patch
Darin, could you take a look this again ?
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?
Created attachment 213450 [details] Patch for landing
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 on attachment 213450 [details] Patch for landing Clearing flags on attachment: 213450 Committed r156961: <http://trac.webkit.org/changeset/156961>
All reviewed patches have been landed. Closing bug.