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.
Created attachment 158181 [details] Patch Proposed patch.
Comment on attachment 158181 [details] Patch Attachment 158181 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13492343
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 ?
Created attachment 158187 [details] Patch Fixed typo in Source/cmake/WebKitFeatures.cmake.
(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 on attachment 158187 [details] Patch Attachment 158187 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13482985
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)
Created attachment 158192 [details] Patch Fixed another typo in Source/WebCore/GNUmakefile.am.
(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?
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 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?).
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 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 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
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
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 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.
Created attachment 158627 [details] Patch Cleaned up ChangeLogs and fixed typo in FeatureDefinesCairo.vsprops as reviewed by Julien.
Comment on attachment 158627 [details] Patch Re-requesting review flag as I mistakenly erased Julien's r+ by misusing webkit-patch tool :/
Comment on attachment 158627 [details] Patch Clearing flags on attachment: 158627 Committed r125716: <http://trac.webkit.org/changeset/125716>
All reviewed patches have been landed. Closing bug.
Please update http://trac.webkit.org/wiki/FeatureFlags
(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!