Bug 55189 – Add a USE() macro to control use of the built-in UTF8 codec

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

Add a USE() macro to control use of the built-in UTF8 codec

[James Robinson]

Created attachment 83733 [details]
Patch

[James Robinson]

The new builtin UTF8 codec should default to on for trunk, but it's still too crashy to use in releases.  This patch adds a USE() macro to control it so we don't have to go through crazy contortions on release branches.  Once the new code is stabilized and has sufficient test coverage we can remove this.

[Darin Adler]

Comment on attachment 83733 [details]
Patch

What’s the motivation here? Could you explain why Chrome doesn’t want to use the codec. This seems like a terrible idea to me, and not just because I wrote the new codec.

[James Robinson]

(In reply to comment #3)
> (From update of attachment 83733 [details])
> What’s the motivation here? Could you explain why Chrome doesn’t want to use the codec. This seems like a terrible idea to me, and not just because I wrote the new codec.

We do, but not yet.  It's still too crashy.

[Darin Adler]

(In reply to comment #2)
> The new builtin UTF8 codec should default to on for trunk, but it's still too crashy to use in releases.

What are the bug numbers for the crashes? I don’t know of any right now.

[James Robinson]

(In reply to comment #5)
> (In reply to comment #2)
> > The new builtin UTF8 codec should default to on for trunk, but it's still too crashy to use in releases.
> 
> What are the bug numbers for the crashes? I don’t know of any right now.

https://bugs.webkit.org/show_bug.cgi?id=54862#c19 says that the latest patch is still crashing on some platforms, and as far as I can tell no regression tests have been added for the crashes found so far.  I understand that you are still working on this code, but want to make sure that your bugfixes aren't tied to any port's release schedule as that's awkward and can lead to bad decisions.

There should be no change in behavior for any port (assuming no bugs in ICU or the builtin code) so from a platform perspective this should be a neutral change.

[Darin Adler]

(In reply to comment #6)
> (In reply to comment #5)
> > What are the bug numbers for the crashes? I don’t know of any right now.
> 
> https://bugs.webkit.org/show_bug.cgi?id=54862#c19 says that the latest patch is still crashing on some platforms

That confusing comment is about an assertion failure, covered by bug 55135. I fixed it this morning by removing the assertion but it’s still waiting in the commit queue.

> and as far as I can tell no regression tests have been added for the crashes found so far.

Yes, we should add the tests, but I don’t think the lack of regression tests is a significant factor in this decision.

> I understand that you are still working on this code

I’m not. I consider it done.

This patch is an overreaction. Would you be willing to wait a couple days to see if we find any crashes?

[James Robinson]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > What are the bug numbers for the crashes? I don’t know of any right now.
> > 
> > https://bugs.webkit.org/show_bug.cgi?id=54862#c19 says that the latest patch is still crashing on some platforms
> 
> That confusing comment is about an assertion failure, covered by bug 55135. I fixed it this morning by removing the assertion but it’s still waiting in the commit queue.
> 
> > and as far as I can tell no regression tests have been added for the crashes found so far.
> 
> Yes, we should add the tests, but I don’t think the lack of regression tests is a significant factor in this decision.
> 
> > I understand that you are still working on this code
> 
> I’m not. I consider it done.
> 
> This patch is an overreaction. Would you be willing to wait a couple days to see if we find any crashes?

I intend to leave the builtin UTF8 codec on by default in trunk.  For release branches, though, it needs a little bake time before shipping and it's too difficult to disable it on a branch right now.  Normally new code is developed behind a macro and then turned on when it seems to me that this patch is moving closer to the usual WebKit development pattern.

[Darin Adler]

(In reply to comment #8)
> Normally new code is developed behind a macro

There’s a lot of new code that is not done behind a macro, but I agree that major efforts usually do use one.

If you really want this, it’s OK with me.

But I think you’re wrong about the stage this code is at. I think it’s highly likely we are done with the new UTF-8 codec and there won’t be any further changes.

Please do remove this conditional soon once you see that we aren’t going to need it.

[James Robinson]

Thanks.  I hope we don't need this flexibility either but would rather have the option and not need it than need the option and not have it :)

[James Robinson]

Committed r79639: <http://trac.webkit.org/changeset/79639>

[Darin Adler]

It’s three weeks later and there have been no bug reports related to the UTF-8 decoder. Is it time to take this out now?

[Adam Barth]

(In reply to comment #12)
> It’s three weeks later and there have been no bug reports related to the UTF-8 decoder. Is it time to take this out now?

Hearing no objections, I'll post a patch removing the flag today.

[James Robinson]

Sounds good to me!

[Adam Barth]

Removal in https://bugs.webkit.org/show_bug.cgi?id=56508