Re: [webkit-dev] New runtime setting: css3TextDecoration
On Aug 14, 2012, at 5:41 PM, Elliott Sprehn espr...@chromium.org wrote: The point of the vendor prefix is to assert the instability of the feature. The WebKit project uses vendor prefixes to avoid compatibility conflict between a feature and a later incompatible standardized feature that would use the same name. This is not intended as a way to smuggle in low quality features. WebKit features should have high quality implementations even during the stage where they are vendor-prefixed. We don’t want to ship half-baked or low quality features in releases of WebKit because they can cause long term problems for people supporting content that has to work on multiple versions of WebKit. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Wed, Aug 15, 2012 at 12:55 PM, Darin Adler da...@apple.com wrote: On Aug 14, 2012, at 5:41 PM, Elliott Sprehn espr...@chromium.org wrote: The point of the vendor prefix is to assert the instability of the feature. The WebKit project uses vendor prefixes to avoid compatibility conflict between a feature and a later incompatible standardized feature that would use the same name. This is not intended as a way to smuggle in low quality features. WebKit features should have high quality implementations even during the stage where they are vendor-prefixed. We don’t want to ship half-baked or low quality features in releases of WebKit because they can cause long term problems for people supporting content that has to work on multiple versions of WebKit. -- Darin Sorry, but I honestly don't see the point of this discussion anymore, in terms that all the implementations are getting proper layout tests (property usage, repaint, getComputedStyle, etc) and pending review. The compile flag (now accepted by all) is being addresed in https://bugs.webkit.org/show_bug.cgi?id=93863 and thus won't make the feature exposed to web by default. As Ryosuke mentioned, layout tests aren't enough for properly testing editing features, so please point me to where I can actually stress test it to get confidence about the code. Best, -- Bruno de Oliveira Abinader ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Aug 14, 2012, at 10:05 AM, Bruno Abinader brunoabina...@gmail.com wrote: A series of CSS3 text decoration properties implementations are currently on the works. These adds new properties which specifications are not finished yet, thus subject to changes. In order to avoid early web exposure, a new setting (aka. runtime flag) is going to be added: css3TextDecoration. As discussed with Julien, this flag provides easier maintenance than adding a whole new compile flag (as seen in https://bugs.webkit.org/show_bug.cgi?id=93863 ). This new runtime flag comes disabled by default and is going to affect parsing of the following CSS3 text-decoration properties: -webkit-text-decoration-line ( https://bugs.webkit.org/show_bug.cgi?id=90959 ) -webkit-text-decoration-style ( https://bugs.webkit.org/show_bug.cgi?id=90958 ) -webkit-text-decoration-color ( https://bugs.webkit.org/show_bug.cgi?id=91638 ) -webkit-text-decoration-skip ( https://bugs.webkit.org/show_bug.cgi?id=92801 ) And finally CSS3 specification support for text-decoration ( https://bugs.webkit.org/show_bug.cgi?id=92000 ) The bug related to this implementation is https://bugs.webkit.org/show_bug.cgi?id=93966 . Any comments will be kindly appreciated! I don't think it's appropriate to add settings for CSS features that are under development, for a number of reasons: * If we did this for every feature, we'd end up with hundreds of Settings. * Traditionally, Settings don't tend to get removed, resulting in an ever-accumulating number of Settings. * If your feature is protected by an ENABLE flag, vendors that want to ship release software can turn it off. * If developing your feature in trunk is so disruptive that you need a Setting to turn it off for most people, then perhaps you should be working on a branch up to the point where your feature is mostly usable. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
I agree with Simon. Setting is a wrong mechanism to guard new features under development. On Tue, Aug 14, 2012 at 11:10 AM, Simon Fraser simon.fra...@apple.comwrote: On Aug 14, 2012, at 10:05 AM, Bruno Abinader brunoabina...@gmail.com wrote: A series of CSS3 text decoration properties implementations are currently on the works. These adds new properties which specifications are not finished yet, thus subject to changes. In order to avoid early web exposure, a new setting (aka. runtime flag) is going to be added: css3TextDecoration. As discussed with Julien, this flag provides easier maintenance than adding a whole new compile flag (as seen in https://bugs.webkit.org/show_bug.cgi?id=93863 ). This new runtime flag comes disabled by default and is going to affect parsing of the following CSS3 text-decoration properties: -webkit-text-decoration-line ( https://bugs.webkit.org/show_bug.cgi?id=90959 ) -webkit-text-decoration-style ( https://bugs.webkit.org/show_bug.cgi?id=90958 ) -webkit-text-decoration-color ( https://bugs.webkit.org/show_bug.cgi?id=91638 ) -webkit-text-decoration-skip ( https://bugs.webkit.org/show_bug.cgi?id=92801 ) And finally CSS3 specification support for text-decoration ( https://bugs.webkit.org/show_bug.cgi?id=92000 ) The bug related to this implementation is https://bugs.webkit.org/show_bug.cgi?id=93966 . Any comments will be kindly appreciated! I don't think it's appropriate to add settings for CSS features that are under development, for a number of reasons: * If we did this for every feature, we'd end up with hundreds of Settings. * Traditionally, Settings don't tend to get removed, resulting in an ever-accumulating number of Settings. * If your feature is protected by an ENABLE flag, vendors that want to ship release software can turn it off. * If developing your feature in trunk is so disruptive that you need a Setting to turn it off for most people, then perhaps you should be working on a branch up to the point where your feature is mostly usable. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
I don't think it's appropriate to add settings for CSS features that are under development, for a number of reasons: * If we did this for every feature, we'd end up with hundreds of Settings. * Traditionally, Settings don't tend to get removed, resulting in an ever-accumulating number of Settings. ENABLE has a slightly better track of record but I don't think we should push back on runtime flags just because of that. Having tons of #ifdef's is a lot more worrying from my perspective, on top of not being able to build / test the feature before you toggle the flag on one platform (with all the surprises coming from platform differences when a port maintainer decide to do so). * If your feature is protected by an ENABLE flag, vendors that want to ship release software can turn it off. That's true, except that the original thread didn't mention any form of feature flag [1]. Nobody objected at the time and thus patches that implemented part of the spec arrived on bugzilla: prefixed but not protected by any flag. Due to the bar to landing such a change and exposing directly, I requested that a feature flag be introduced so that patches could be split into atomic pieces that can be reviewed one at a time (and also checking that the testing is appropriate). This update isn't about bringing a discussion about ENABLE vs runtime, more about mentioning that there will be a new flag contrary to what was said previously. There is definitely a trend to use runtime flags on core features lately (region, grid, custom filters...) sometimes along with ENABLE flags. This should be discussed separately. Thanks, Julien [1] http://lists.webkit.org/pipermail/webkit-dev/2012-July/021715.html ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 3:06 PM, Ryosuke Niwa rn...@webkit.org wrote: On Tue, Aug 14, 2012 at 11:56 AM, Julien Chaffraix * If your feature is protected by an ENABLE flag, vendors that want to ship release software can turn it off. That's true, except that the original thread didn't mention any form of feature flag [1]. Nobody objected at the time and thus patches that implemented part of the spec arrived on bugzilla: prefixed but not protected by any flag. I had certainly assumed that this was done under a new build flag. If that were not the case, I expected relevant reviewers to r- those patches. Maybe this was a bad assumption to make. I've studied CSS Region implementation and basically it contains both compile and runtime flags: The former gets enabled by default while the latter gets disabled, so we can still catch compile-time code errors while not automatically exposing the feature to the web without explicitly enabling the setting. I guess we can implement this feature the same way? I do have a working patch with compile flag implementation [1] . The -webkit-text-decoration-line code works most of the time as an alias to original text-decoration, so it was assumed that no special build flag was required at that time. This is fixed also in [1]. The runtime flag patch [2] works on top of [1], so we end up with the same behavior as CSS region implements. Links: [1] https://bugs.webkit.org/show_bug.cgi?id=93863 [2] https://bugs.webkit.org/show_bug.cgi?id=93966 -- Bruno de Oliveira Abinader ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 12:42 PM, Bruno Abinader brunoabina...@gmail.comwrote: I do have a working patch with compile flag implementation [1] . The -webkit-text-decoration-line code works most of the time as an alias to original text-decoration, so it was assumed that no special build flag was required at that time. As I have said on the previous thread, adding this property may break editing in unexpected ways. In particular, we probably wouldn't preserve new values properly when modifying styles. So we need to fix that before shipping it on any browser. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 3:47 PM, Ryosuke Niwa rn...@webkit.org wrote: On Tue, Aug 14, 2012 at 12:42 PM, Bruno Abinader brunoabina...@gmail.com wrote: I do have a working patch with compile flag implementation [1] . The -webkit-text-decoration-line code works most of the time as an alias to original text-decoration, so it was assumed that no special build flag was required at that time. As I have said on the previous thread, adding this property may break editing in unexpected ways. In particular, we probably wouldn't preserve new values properly when modifying styles. So we need to fix that before shipping it on any browser. - Ryosuke This is exactly why we need a a flag, whether it is compile, runtime or both :) ps: I've run a bunch of tests with editing with and without the flag set and so far it shows no changes (ie. one underlined line can be copied/pasted with same style, and -webkit-text-decoration-line makes no difference to editing because it is not contained in WebCore::editingProperties vector. -- Bruno de Oliveira Abinader ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 1:08 PM, Bruno Abinader brunoabina...@gmail.comwrote: This is exactly why we need a a flag, whether it is compile, runtime or both :) ps: I've run a bunch of tests with editing with and without the flag set and so far it shows no changes (ie. one underlined line can be copied/pasted with same style, and -webkit-text-decoration-line makes no difference to editing because it is not contained in WebCore::editingProperties vector. In editing, unfortunately, just passing existing layout tests isn't enough. We need to make sure editing code works as expected when those new values of text decorations are used. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 12:06 PM, Ryosuke Niwa rn...@webkit.org wrote: On Tue, Aug 14, 2012 at 11:56 AM, Julien Chaffraix julien.chaffr...@gmail.com wrote: I don't think it's appropriate to add settings for CSS features that are under development, for a number of reasons: * If we did this for every feature, we'd end up with hundreds of Settings. * Traditionally, Settings don't tend to get removed, resulting in an ever-accumulating number of Settings. ENABLE has a slightly better track of record but I don't think we should push back on runtime flags just because of that. Having a runtime flag incurs runtime cost. Performance is one of our core goal and any WebKit hacker will agree that it's important. Here you forgot to mention that we care about is if it has a *notable* runtime cost in which case there is no proof that the current flags incur such a cost. We know of patches from feature under a flag that have been rolled out because of measurable performance regression [1] so we would know if that was the case. It's also better to catch such slow-down earlier rather than later. The most significant performance impact is probably the increase in the binary size which I won't deny and don't have insights into. Having tons of #ifdef's is a lot more worrying from my perspective, Having a runtime flag is significantly worse in that it affects end users. Having a compile flag is a painful for developers but has absolutely zero cost for end users. You will need to define end users here. WebKit is meant to be embedded and as such you can think of different end users. People embedding WebKit are expected to know about our flags. Browsers' end users are only impacted insofar as browsers vendors implement the right UI and this claim is making strong assumption about how this is done. Now, both options with respect to the flag have their trade-offs but there seems to be majority in favor of ENABLE flags. Brushing aside one option based on FUD is not really something I am supportive of though. Thanks, Julien [1] for example, see https://bugs.webkit.org/show_bug.cgi?id=76064 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 3:10 PM, Julien Chaffraix julien.chaffr...@gmail.com wrote: The most significant performance impact is probably the increase in the binary size which I won't deny and don't have insights into. This is only a part of the performance problem but it is actually fairly bad. We do not realize it that much because our development machines have huge CPU caches. Many CPUs in the wild do not have 8Mb of fast cache and sometime we pay a high price for the size of WebKit. I prefer compile flag over runtime flag for experimental features. Benjamin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 3:10 PM, Julien Chaffraix julien.chaffr...@gmail.com wrote: On Tue, Aug 14, 2012 at 12:06 PM, Ryosuke Niwa rn...@webkit.org wrote: On Tue, Aug 14, 2012 at 11:56 AM, Julien Chaffraix julien.chaffr...@gmail.com wrote: I don't think it's appropriate to add settings for CSS features that are under development, for a number of reasons: * If we did this for every feature, we'd end up with hundreds of Settings. * Traditionally, Settings don't tend to get removed, resulting in an ever-accumulating number of Settings. ENABLE has a slightly better track of record but I don't think we should push back on runtime flags just because of that. Having a runtime flag incurs runtime cost. Performance is one of our core goal and any WebKit hacker will agree that it's important. Here you forgot to mention that we care about is if it has a *notable* runtime cost in which case there is no proof that the current flags incur such a cost. http://en.wikipedia.org/wiki/Creeping_normalcy Given how many build flags we already have, I'm not excited about the prospect of having many runtime flags. Brushing aside one option based on FUD is not really something I am supportive of though. We have already had regressions like https://bugs.webkit.org/show_bug.cgi?id=90367 due to runtime flags so this is not a theoretical concern. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
I don't think editing code breaking when the page uses -webkit prefixed properties should block launching a feature. The point of the vendor prefix is to assert the instability of the feature. This wouldn't be a regression, it's just a missing feature. On Tue, Aug 14, 2012 at 3:22 PM, Ryosuke Niwa rn...@webkit.org wrote: ... ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] New runtime setting: css3TextDecoration
On Tue, Aug 14, 2012 at 5:41 PM, Elliott Sprehn espr...@chromium.orgwrote: I don't think editing code breaking when the page uses -webkit prefixed properties should block launching a feature. The point of the vendor prefix is to assert the instability of the feature. That's not the point of a vendor prefix. Other browser vendors have shown some real resentments over how we always ship half-baked features that only work in simple cases. See bugs like https://bugs.webkit.org/show_bug.cgi?id=93304. We really need to stop shipping half-baked or half-finished features. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
