Bug 110944 – Add ENABLE_CSS3_TEXT_LINE_BREAK flag.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Glenn Adams]

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

[Glenn Adams]

Created attachment 190441 [details]
Patch

[Eric Seidel (no email)]

I'm not sure I see the purpose of a flag.  We should just fix the perf problem and turn this on for everyone. :)

[Glenn Adams]

(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.

[Glenn Adams]

(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;

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

That said, I'm not going to object to this patch landing.  I just think this is added work for little gain.

[Glenn Adams]

(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

[Elliott Sprehn]

(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. :)

[Dean Jackson]

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.

[Glenn Adams]

Created attachment 190534 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 190534 [details]
Patch for landing

Clearing flags on attachment: 190534

Committed r144214: <http://trac.webkit.org/changeset/144214>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

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 :-) ).

[James Robinson]

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.

[Elliott Sprehn]

(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.