Bug 55189 - Add a USE() macro to control use of the built-in UTF8 codec
Summary: Add a USE() macro to control use of the built-in UTF8 codec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-24 15:36 PST by James Robinson
Modified: 2011-03-16 17:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.15 KB, patch)
2011-02-24 15:38 PST, James Robinson
fishd: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2011-02-24 15:36:16 PST
Add a USE() macro to control use of the built-in UTF8 codec
Comment 1 James Robinson 2011-02-24 15:38:08 PST
Created attachment 83733 [details]
Patch
Comment 2 James Robinson 2011-02-24 15:39:22 PST
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.
Comment 3 Darin Adler 2011-02-24 15:39:23 PST
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.
Comment 4 James Robinson 2011-02-24 15:39:40 PST
(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.
Comment 5 Darin Adler 2011-02-24 15:40:00 PST
(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.
Comment 6 James Robinson 2011-02-24 15:43:38 PST
(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.
Comment 7 Darin Adler 2011-02-24 15:48:01 PST
(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?
Comment 8 James Robinson 2011-02-24 15:55:03 PST
(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.
Comment 9 Darin Adler 2011-02-24 15:58:11 PST
(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.
Comment 10 James Robinson 2011-02-24 16:00:56 PST
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 :)
Comment 11 James Robinson 2011-02-24 16:04:20 PST
Committed r79639: <http://trac.webkit.org/changeset/79639>
Comment 12 Darin Adler 2011-03-14 13:58:09 PDT
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?
Comment 13 Adam Barth 2011-03-16 16:50:38 PDT
(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.
Comment 14 James Robinson 2011-03-16 16:51:47 PDT
Sounds good to me!
Comment 15 Adam Barth 2011-03-16 17:32:19 PDT
Removal in https://bugs.webkit.org/show_bug.cgi?id=56508