Bug 93863 – [css3-text] Add CSS3 Text decoration compile flag

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

[Bruno Abinader (history only)]

A series of CSS3 text decoration properties implementations are currently on the works. These adds new properties which specifications are not finished yet, thus variable to changes. A new feature flag is required, with its boilerplate implementation handled in this bug and further bugs related to these implementations shall take advantage of this flag.

[Bruno Abinader (history only)]

Created attachment 158181 [details]
Patch

Proposed patch.

[Gustavo Noronha (kov)]

Comment on attachment 158181 [details]
Patch

Attachment 158181 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13492343

[Gyuyoung Kim]

Comment on attachment 158181 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:24
> +    WEBKIT_OPTION_DEFINE(ENABLE_CSS3_FLEXBOX "Toggle CSS3 Text Decoration support" OFF)

Don't you need to add ENABLE_CSS3_TEXT_DECORATION instead of ENABLE_CSS3_FLEXBOX ?

[Bruno Abinader (history only)]

Created attachment 158187 [details]
Patch

Fixed typo in Source/cmake/WebKitFeatures.cmake.

[Bruno Abinader (history only)]

(In reply to comment #3)
> (From update of attachment 158181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158181&action=review
> 
> > Source/cmake/WebKitFeatures.cmake:24
> > +    WEBKIT_OPTION_DEFINE(ENABLE_CSS3_FLEXBOX "Toggle CSS3 Text Decoration support" OFF)
> 
> Don't you need to add ENABLE_CSS3_TEXT_DECORATION instead of ENABLE_CSS3_FLEXBOX ?

Indeed, my fault :/ Fixed now with a new patch.

[Gustavo Noronha (kov)]

Comment on attachment 158187 [details]
Patch

Attachment 158187 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13482985

[Julien Chaffraix]

Comment on attachment 158187 [details]
Patch

For the record, I would recommend a runtime feature flag, which is maybe a bit easier to implement. The idea being that it would avoid having you pepper the code with #ifdef's and also enable you to run your tests vs having to disable them on a compile flag.

Look at the new CSS properties (region, grid layout or shape inside) for some guidance (basically you disable CSS parsing and the rest should be fine)

[Bruno Abinader (history only)]

Created attachment 158192 [details]
Patch

Fixed another typo in Source/WebCore/GNUmakefile.am.

[Bruno Abinader (history only)]

(In reply to comment #7)
> (From update of attachment 158187 [details])
> For the record, I would recommend a runtime feature flag, which is maybe a bit easier to implement. The idea being that it would avoid having you pepper the code with #ifdef's and also enable you to run your tests vs having to disable them on a compile flag.
> 
> Look at the new CSS properties (region, grid layout or shape inside) for some guidance (basically you disable CSS parsing and the rest should be fine)

Thanks for the info, Julien! I have studied their implementations, and concluded that #ifdef's are still needed on all casses, so the compile time is still needed. The difference is that when using runtime flag in combination with compile flag, the compile flag comes enabled by default, and runtime flag comes disabled (which disables CSS Parsing, as you mentioned earlier). So all in all, the implementation passes through the compilation process, but is not yet exposed to web if the web application does not explicity enables it. I am finishing the new patch with this implementation atm, just asking if the assumption I described now is valid?

[Bruno Abinader (history only)]

Created attachment 158336 [details]
Patch

Compile flag now comes enabled by default. Runtime flag is handled on bug 93966 and will come disabled by default (thus not exposing the feature to the web y yet).

[Bruno Abinader (history only)]

Comment on attachment 158336 [details]
Patch

As discussed with Julien on IRC, it is preferred to have only runtime flag. Said this, this bug is now stalled until further notice (mark it as invalid, perhaps?).

[Bruno Abinader (history only)]

Created attachment 158451 [details]
Patch

As heavily discussed on webkit-dev (see http://lists.webkit.org/pipermail/webkit-dev/2012-August/021870.html ), using compile flag was selected as preferably option. This makes the flag disabled by default (as explained in ChangeLog). Changes from bug 90959 related to layout tests were also reverted due to this.

[Julien Chaffraix]

Comment on attachment 158451 [details]
Patch

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

The change is fine.

But the test landed in bug 90959 (fast/css/text-decoration-line.html) is not failing as it should. I think this is because you provide both properties (non-prefixed and prefixed) in the test rendering the test basically useless as it passes regardless of whether we render the prefixed version properly. This should be fixed and tested before this gets into the tree.

> Source/WebCore/ChangeLog:32
> +        (WebCore::StyleResolver::collectMatchingRulesForList):

Please fill in the details in the ChangeLog, especially since you are retroactively protecting some of your implementation.

[WebKit Review Bot]

Comment on attachment 158451 [details]
Patch

Attachment 158451 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13494802

New failing tests:
fast/css/text-decoration-line.html

[WebKit Review Bot]

Created attachment 158474 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Bruno Abinader (history only)]

Created attachment 158479 [details]
Patch

Added info about changes to the code previously added by r125205, skipping fast/css3-text-decoration directory until further notice.

[Julien Chaffraix]

Comment on attachment 158479 [details]
Patch

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_FRAME);$(ENABLE_3D_RENDERING);$(ENABLE_ACCELERATED_2D_CANVAS);$(ENABLE_BLOB);$(ENABLE_CHANNEL_MESSAGING);$(ENABLE_CSS3*_FLEXBOX);$(ENABLE_CSS3_TEXT_DECORATION);$(ENABLE_CSS_BOX_DECORATION_BREAK);$(ENABLE_CSS_FILTERS);$(ENABLE_CSS_GRID_LAYOUT);$(ENABLE_CSS_SHADERS);$(ENABLE_CSS_COMPOSITING);$(ENABLE_CSS_REGIONS);$(ENABLE_CSS_EXCLUSIONS);$(ENABLE_CUSTOM_SCHEME_HANDLER);$(ENABLE_SQL_DATABASE);$(ENABLE_DATAGRID);$(ENABLE_DATALIST_ELEMENT);$(ENABLE_DATA_TRANSFER_ITEMS);$(ENABLE_DETAILS_ELEMENT);$(ENABLE_DEVICE_ORIENTATION);$(ENABLE_DIRECTORY_UPLOAD);$(ENABLE_FILTERS);$(ENABLE_FILE_SYSTEM);$(ENABLE_FULLSCREEN_API);$(ENABLE_GAMEPAD);$(ENABLE_GEOLOCATION);$(ENABLE_HIGH_DPI_CANVAS);$(ENABLE_ICONDATABASE);$(ENABLE_INDEXED_DATABASE);$(ENABLE_INPUT_TYPE_COLOR);$(ENABLE_INPUT_SPEECH);$(ENABLE_INPUT_TYPE_DATE);$(ENABLE_INPUT_TYPE_DATETIME);$(ENABLE_INPUT_TYPE_DATETIMELOCAL);$(ENABLE_INPUT_TYPE_MONTH);$(ENABLE_INPUT_TYPE_TIME);$(ENABLE_INPUT_TYPE_WEEK);$(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_OBSERVERS);$(ENABLE_NOTIFICATIONS);$(ENABLE_PAGE_VISIBILITY_API);$(ENABLE_PROGRESS_ELEMENT);$(ENABLE_QUOTA);$(ENABLE_REGISTER_PROTOCOL_HANDLER);$(ENABLE_SCRIPTED_SPEECH);$(ENABLE_SHADOW_DOM);$(ENABLE_SHARED_WORKERS);$(ENABLE_STYLE_SCOPED);$(ENABLE_SVG);$(ENABLE_SVG_DOM_OBJC_BINDINGS);$(ENABLE_SVG_FONTS);$(ENABLE_TEXT_AUTOSIZING);$(ENABLE_UNDO_MANAGER);$(ENABLE_VIDEO);$(ENABLE_MEDIA_SOURCE);$(ENABLE_MEDIA_STATISTICS);$(ENABLE_WEB_SOCKETS);$(ENABLE_WEB_TIMING);$(ENABLE_WORKERS);$(ENABLE_XSLT)"

You mistakenly added a '*' in ENABLE_CSS3_FLEXBOX.

[Bruno Abinader (history only)]

Created attachment 158627 [details]
Patch

Cleaned up ChangeLogs and fixed typo in FeatureDefinesCairo.vsprops as reviewed by Julien.

[Bruno Abinader (history only)]

Comment on attachment 158627 [details]
Patch

Re-requesting review flag as I mistakenly erased Julien's r+ by misusing webkit-patch tool :/

[WebKit Review Bot]

Comment on attachment 158627 [details]
Patch

Clearing flags on attachment: 158627

Committed r125716: <http://trac.webkit.org/changeset/125716>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

Please update http://trac.webkit.org/wiki/FeatureFlags

[Bruno Abinader (history only)]

(In reply to comment #22)
> Please update http://trac.webkit.org/wiki/FeatureFlags

Done in version 49: https://trac.webkit.org/wiki/FeatureFlags?version=49
Thanks for the reminder!