[webkit-reviews] review granted: [Bug 93863] [css3-text] Add CSS3 Text decoration compile flag : [Attachment 158479] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Bruno Abinader
<bruno.abinader at basyskom.com>'s request for review:
Bug 93863: [css3-text] Add CSS3 Text decoration compile flag
https://bugs.webkit.org/show_bug.cgi?id=93863

Attachment 158479: Patch
https://bugs.webkit.org/attachment.cgi?id=158479&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=158479&action=review


> Source/WebCore/ChangeLog:23
> +	   feature is sound and ready to be exposed to web.

Usually descriptions should be contextualized, not copied and pasted among
ChangeLogs. This paragraph only makes sense for WebCore and LayoutTests, all
the other ChangeLogs don't care about that.

Also you don't *have* to put something in lieu of the placeholder text when it
doesn't make any sense to put it. You can always look at other ChangeLog
entries for guidance.

> Source/WebCore/ChangeLog:41
> +	   * css/StyleResolver.cpp: Added missing #ifdef's.
> +	   (WebCore::StyleResolver::collectMatchingRulesForList):

Nit: You can always move the text under a block, to avoid repeating the same
idea for several files. On top of that, it's merely a 'what' comment, usually
'why' comments are better (which is what you put in the global description but
it's hidden by the rest of comments).

> WebKitLibraries/win/tools/vsprops/FeatureDefinesCairo.vsprops:12
> +	       
PreprocessorDefinitions="$(ENABLE_IFRAME_SEAMLESS);$(ENABLE_REQUEST_ANIMATION_F
RAME);$(ENABLE_3D_RENDERING);$(ENABLE_ACCELERATED_2D_CANVAS);$(ENABLE_BLOB);$(E
NABLE_CHANNEL_MESSAGING);$(ENABLE_CSS3*_FLEXBOX);$(ENABLE_CSS3_TEXT_DECORATION)
;$(ENABLE_CSS_BOX_DECORATION_BREAK);$(ENABLE_CSS_FILTERS);$(ENABLE_CSS_GRID_LAY
OUT);$(ENABLE_CSS_SHADERS);$(ENABLE_CSS_COMPOSITING);$(ENABLE_CSS_REGIONS);$(EN
ABLE_CSS_EXCLUSIONS);$(ENABLE_CUSTOM_SCHEME_HANDLER);$(ENABLE_SQL_DATABASE);$(E
NABLE_DATAGRID);$(ENABLE_DATALIST_ELEMENT);$(ENABLE_DATA_TRANSFER_ITEMS);$(ENAB
LE_DETAILS_ELEMENT);$(ENABLE_DEVICE_ORIENTATION);$(ENABLE_DIRECTORY_UPLOAD);$(E
NABLE_FILTERS);$(ENABLE_FILE_SYSTEM);$(ENABLE_FULLSCREEN_API);$(ENABLE_GAMEPAD)
;$(ENABLE_GEOLOCATION);$(ENABLE_HIGH_DPI_CANVAS);$(ENABLE_ICONDATABASE);$(ENABL
E_INDEXED_DATABASE);$(ENABLE_INPUT_TYPE_COLOR);$(ENABLE_INPUT_SPEECH);$(ENABLE_
INPUT_TYPE_DATE);$(ENABLE_INPUT_TYPE_DATETIME);$(ENABLE_INPUT_TYPE_DATETIMELOCA
L);$(ENABLE_INPUT_TYPE_MONTH);$(ENABLE_INPUT_TYPE_TIME);$(ENABLE_INPUT_TYPE_WEE
K);$(ENABLE_JAVASCRIPT_DEBUGGER);$(ENABLE_LEGACY_CSS_VENDOR_PREFIXES);$(ENABLE_
LEGACY_NOTIFICATIONS);$(ENABLE_LINK_PREFETCH);$(ENABLE_LINK_PRERENDER);$(ENABLE
_MATHML);$(ENABLE_METER_ELEMENT);$(ENABLE_MICRODATA);$(ENABLE_MUTATION_OBSERVER
S);$(ENABLE_NOTIFICATIONS);$(ENABLE_PAGE_VISIBILITY_API);$(ENABLE_PROGRESS_ELEM
ENT);$(ENABLE_QUOTA);$(ENABLE_REGISTER_PROTOCOL_HANDLER);$(ENABLE_SCRIPTED_SPEE
CH);$(ENABLE_SHADOW_DOM);$(ENABLE_SHARED_WORKERS);$(ENABLE_STYLE_SCOPED);$(ENAB
LE_SVG);$(ENABLE_SVG_DOM_OBJC_BINDINGS);$(ENABLE_SVG_FONTS);$(ENABLE_TEXT_AUTOS
IZING);$(ENABLE_UNDO_MANAGER);$(ENABLE_VIDEO);$(ENABLE_MEDIA_SOURCE);$(ENABLE_M
EDIA_STATISTICS);$(ENABLE_WEB_SOCKETS);$(ENABLE_WEB_TIMING);$(ENABLE_WORKERS);$
(ENABLE_XSLT)"

You mistakenly added a '*' in ENABLE_CSS3_FLEXBOX.