24755 – LayoutTests/http/tests/misc/url-in-utf16le.html regression

Bug 24755 - LayoutTests/http/tests/misc/url-in-utf16le.html regression

Summary: LayoutTests/http/tests/misc/url-in-utf16le.html regression

Status:	RESOLVED FIXED

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	New Bugs (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Mac OS X 10.5

Importance:	P2 Normal
Assignee:	Darin Adler

URL:
Keywords:

Depends on:
Blocks:

Reported:	2009-03-22 22:00 PDT by Nico Weber
Modified:	2009-03-23 16:50 PDT (History)
CC List:	3 users (show)

See Also:

Attachments
Patch. (1.25 KB, patch) 2009-03-22 22:04 PDT, Nico Weber	no flags	Details \| Formatted Diff \| Diff
Patch with better ChangeLog. (1.32 KB, patch) 2009-03-22 22:07 PDT, Nico Weber	no flags	Details \| Formatted Diff \| Diff
Patch with better changelog and no tabs. (1.32 KB, patch) 2009-03-22 22:09 PDT, Nico Weber	darin: review-	Details \| Formatted Diff \| Diff
patch that fixes regression tests without reintroducing the performance problem (4.58 KB, patch) 2009-03-23 09:17 PDT, Darin Adler	koivisto: review+	Details \| Formatted Diff \| Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Nico Weber 2009-03-22 22:00:12 PDT

WebKit change 41727 regressed query param handling for utf16 files. These fail:

    WebKitTools/Scripts/run-webkit-tests --debug LayoutTests/http/tests/misc/url-in-utf16le.html
    WebKitTools/Scripts/run-webkit-tests --debug LayoutTests/http/tests/misc/url-in-utf16be.html

Comment 1 Nico Weber 2009-03-22 22:04:48 PDT

Created attachment 28845 [details]
Patch.

Revert change 41727. Also fixes http://crbug.com/9067 .

Comment 2 Nico Weber 2009-03-22 22:05:33 PDT

With this patch,

    WebKitTools/Scripts/run-webkit-tests --debug LayoutTests/http/tests/misc/url-in-utf16le.html

works fine again.

Comment 3 Nico Weber 2009-03-22 22:07:48 PDT

Created attachment 28846 [details]
Patch with better ChangeLog.

Comment 4 Nico Weber 2009-03-22 22:09:44 PDT

Created attachment 28847 [details]
Patch with better changelog and no tabs.

Sorry for the noise. It's my first time.

Comment 5 Nico Weber 2009-03-22 22:16:16 PDT

Some comments on the change:

If `this` is an UTF16-encoding, change 41727 caused wrong results. Since `isNonByteBasedEncoding()` checks `noExtendedTextEncodingNameUsed()` already, and boolean operators are lazy, reverting 41727 does not cause any harm but fixes the regression.

Comment 6 David Kilzer (:ddkilzer) 2009-03-23 09:08:39 PDT

http://trac.webkit.org/changeset/41727

Comment 7 Darin Adler 2009-03-23 09:08:48 PDT

Comment on attachment 28847 [details]
Patch with better changelog and no tabs.

We can't just roll out this optimization, which was fixing a significant performance problem. We need a new fix rather than just rolling out the bad one. Let me take a look at this.

Comment 8 Darin Adler 2009-03-23 09:10:34 PDT

Comment on attachment 28847 [details]
Patch with better changelog and no tabs.

I'm about to post an improved patch for this.

Comment 9 Darin Adler 2009-03-23 09:17:12 PDT

Created attachment 28854 [details]
patch that fixes regression tests without reintroducing the performance problem

Comment 10 Antti Koivisto 2009-03-23 11:12:55 PDT

r=me

Comment 11 Darin Adler 2009-03-23 16:50:28 PDT

http://trac.webkit.org/changeset/41916