Bug 110944

Created attachment 190441 [details] Patch

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

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

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

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

(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

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

Created attachment 190534 [details] Patch for landing

Comment on attachment 190534 [details] Patch for landing Clearing flags on attachment: 190534 Committed r144214: <http://trac.webkit.org/changeset/144214>

All reviewed patches have been landed. Closing bug.

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

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.

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