Bug 93863

Summary: [css3-text] Add CSS3 Text decoration compile flag
Product: WebKit Reporter: Bruno Abinader (history only) <bruno.abinader>
Component: CSSAssignee: Bruno Abinader (history only) <bruno.abinader>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, gns, gyuyoung.kim, jchaffraix, macpherson, menard, rakuco, vestbo, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 58491, 90958, 90959, 91638, 92000, 92659, 92801, 92868, 93507, 93509, 93966, 94093, 94094, 94108, 94110, 94111, 94112, 94114    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from gce-cr-linux-05
none
Patch
none
Patch none

Description Bruno Abinader (history only) 2012-08-13 10:19:58 PDT
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.
Comment 1 Bruno Abinader (history only) 2012-08-13 18:33:56 PDT
Created attachment 158181 [details]
Patch

Proposed patch.
Comment 2 Gustavo Noronha (kov) 2012-08-13 18:54:01 PDT
Comment on attachment 158181 [details]
Patch

Attachment 158181 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13492343
Comment 3 Gyuyoung Kim 2012-08-13 19:02:17 PDT
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 ?
Comment 4 Bruno Abinader (history only) 2012-08-13 19:06:09 PDT
Created attachment 158187 [details]
Patch

Fixed typo in Source/cmake/WebKitFeatures.cmake.
Comment 5 Bruno Abinader (history only) 2012-08-13 19:07:02 PDT
(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.
Comment 6 Gustavo Noronha (kov) 2012-08-13 19:12:49 PDT
Comment on attachment 158187 [details]
Patch

Attachment 158187 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13482985
Comment 7 Julien Chaffraix 2012-08-13 19:19:50 PDT
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)
Comment 8 Bruno Abinader (history only) 2012-08-13 19:23:57 PDT
Created attachment 158192 [details]
Patch

Fixed another typo in Source/WebCore/GNUmakefile.am.
Comment 9 Bruno Abinader (history only) 2012-08-14 07:14:37 PDT
(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?
Comment 10 Bruno Abinader (history only) 2012-08-14 08:11:48 PDT
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).
Comment 11 Bruno Abinader (history only) 2012-08-14 09:30:26 PDT
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?).
Comment 12 Bruno Abinader (history only) 2012-08-14 17:08:59 PDT
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.
Comment 13 Julien Chaffraix 2012-08-14 17:45:33 PDT
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.
Comment 14 WebKit Review Bot 2012-08-14 18:48:21 PDT
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
Comment 15 WebKit Review Bot 2012-08-14 18:48:27 PDT
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
Comment 16 Bruno Abinader (history only) 2012-08-14 19:15:32 PDT
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.
Comment 17 Julien Chaffraix 2012-08-15 12:02:04 PDT
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.
Comment 18 Bruno Abinader (history only) 2012-08-15 13:38:44 PDT
Created attachment 158627 [details]
Patch

Cleaned up ChangeLogs and fixed typo in FeatureDefinesCairo.vsprops as reviewed by Julien.
Comment 19 Bruno Abinader (history only) 2012-08-15 13:48:55 PDT
Comment on attachment 158627 [details]
Patch

Re-requesting review flag as I mistakenly erased Julien's r+ by misusing webkit-patch tool :/
Comment 20 WebKit Review Bot 2012-08-15 15:31:43 PDT
Comment on attachment 158627 [details]
Patch

Clearing flags on attachment: 158627

Committed r125716: <http://trac.webkit.org/changeset/125716>
Comment 21 WebKit Review Bot 2012-08-15 15:31:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Kent Tamura 2012-08-15 17:50:13 PDT
Please update http://trac.webkit.org/wiki/FeatureFlags
Comment 23 Bruno Abinader (history only) 2012-08-15 19:20:55 PDT
(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!