Bug 110944

Summary: Add ENABLE_CSS3_TEXT_LINE_BREAK flag.
Product: WebKit Reporter: Glenn Adams <glenn>
Component: WebCore Misc.Assignee: Glenn Adams <glenn>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, dbates, eric, esprehn, gyuyoung.kim, jamesr, laszlo.gombos, leviw, oliver, rakuco, syoichi, vestbo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 89235    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Glenn Adams 2013-02-26 22:03:05 PST
In order to aid with the landing and incremental verification of CSS3 Text line-break support [1], an ENABLE_CSS3_TEXT_LINE_BREAK flag is helpful. When bug 89235 [2] is fully landed and port/platforms have been verified, it can be removed or merged with the ENABLE(CSS3_TEXT) flag. For initial commit, it defaults to 0 everywhere, but will shortly be targeted for default enabling on (apple) mac builds.

[1] https://lists.webkit.org/pipermail/webkit-dev/2012-September/022154.html
[2] https://bugs.webkit.org/show_bug.cgi?id=89235
Comment 1 Glenn Adams 2013-02-26 22:08:03 PST
Created attachment 190441 [details]
Patch
Comment 2 Eric Seidel (no email) 2013-02-26 22:46:45 PST
I'm not sure I see the purpose of a flag.  We should just fix the perf problem and turn this on for everyone. :)
Comment 3 Glenn Adams 2013-02-26 22:49:24 PST
(In reply to comment #2)
> I'm not sure I see the purpose of a flag.  We should just fix the perf problem and turn this on for everyone. :)

It was suggested by either jamesr or leviw today (I forget which) as a means to get from here to there.
Comment 4 Glenn Adams 2013-02-26 22:55:24 PST
(In reply to comment #3)
> (In reply to comment #2)
> > I'm not sure I see the purpose of a flag.  We should just fix the perf problem and turn this on for everyone. :)
> 
> It was suggested by either jamesr or leviw today (I forget which) as a means to get from here to there.

btw, there is already a CSS3_TEXT flag which a few of the other CSS3 Text features are built behind, but I thought this one would best be handled separately;

i like the idea of using this flag as a way to reduce the scope of what I might break, so that I can gradually widen that scope in a controlled fashion while I go through functional/performance test/fix cycles as needed on the different platforms;
Comment 5 Eric Seidel (no email) 2013-02-26 23:45:31 PST
Comment on attachment 190441 [details]
Patch

I still think you're largely just making work for yourself here.  Mostly we just need to solve the perf problem.  We can't turn it on anywhere until we do that, and once we solve it, then we want to turn it on everywhere. :)
Comment 6 Eric Seidel (no email) 2013-02-26 23:45:58 PST
That said, I'm not going to object to this patch landing.  I just think this is added work for little gain.
Comment 7 Glenn Adams 2013-02-26 23:51:30 PST
(In reply to comment #5)
> (From update of attachment 190441 [details])
> I still think you're largely just making work for yourself here.  Mostly we just need to solve the perf problem.  We can't turn it on anywhere until we do that, and once we solve it, then we want to turn it on everywhere. :)

perhaps, but i'm hoping this will allow me to avoid having to roll out the entire patch for each port that hits a snag; i also anticipate that the performance issue will not be the same on all platforms (or language sets), but will only learn the answer to this as i progress;

btw, i remembered it was esprehn that suggested the flag today
Comment 8 Elliott Sprehn 2013-02-27 08:24:40 PST
(In reply to comment #6)
> That said, I'm not going to object to this patch landing.  I just think this is added work for little gain.

I suggested the flag because Glenn had expressed frustration with getting his patch rolled out repeatedly. Instead of having his patch rolled in and out, he could land all his code behind the flag so he could turn this feature on per-platform, or even turn on sections of the code, and see what happened. Then he wouldn't need to deal with merges and the big patch rollouts since we'd just need to roll the flag value back and forth.

Another approach would be to do what I did for PseudoElement which was land a big patch that adds infrastructure for the feature but not the few lines of code that turns it on and then a much smaller patch that does that.

I don't feel strongly either way, or about a flag, I was just trying to get Glenn in a place where he could continue working. :)
Comment 9 Dean Jackson 2013-02-27 08:40:21 PST
Comment on attachment 190441 [details]
Patch

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

Patch looks ok to me (one small error) and Eric says he's ok with the flag landing.

> Source/WebCore/ChangeLog:8
> +        No new tests (OOPS!).

You'll have to remove this line before landing.
Comment 10 Glenn Adams 2013-02-27 08:49:52 PST
Created attachment 190534 [details]
Patch for landing
Comment 11 WebKit Review Bot 2013-02-27 10:47:55 PST
Comment on attachment 190534 [details]
Patch for landing

Clearing flags on attachment: 190534

Committed r144214: <http://trac.webkit.org/changeset/144214>
Comment 12 WebKit Review Bot 2013-02-27 10:47:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Laszlo Gombos 2013-02-27 13:43:43 PST
For future reference - I think future flags that do not touch .idl files can be defined in a single place in FeatureDefines.h and that will populate it everywhere in the tree. This should help with the churn (and I noticed that this flag is not introduced in FeatureDefines.h :-) ).
Comment 14 James Robinson 2013-02-27 13:46:03 PST
I don't think we should have added this at all - this isn't a feature and it won't be used to incrementally enable a feature on different ports/platforms, it's just guarding code that we know has bad performance across all ports/platforms.
Comment 15 Elliott Sprehn 2013-02-27 14:08:24 PST
(In reply to comment #14)
> I don't think we should have added this at all - this isn't a feature and it won't be used to incrementally enable a feature on different ports/platforms, it's just guarding code that we know has bad performance across all ports/platforms.

Webkit has a lot of flags you can turn off to get faster/less memory usage across all platforms. :) This isn't special, ex. SVG.

I don't know what you mean this isn't a feature though. Why is text-line-break not a feature? Shadow DOM, Mutation Observers, Regions, Exclusions, ... lots of things are in behind flags and had bad perf, memory, or security implications.