Bug 88428 - REGRESSION (r115654): Opening many non-English WebArchives shows obvious encoding issues
Summary: REGRESSION (r115654): Opening many non-English WebArchives shows obvious enco...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-06-06 10:00 PDT by Brady Eidson
Modified: 2012-06-07 09:38 PDT (History)
5 users (show)

See Also:


Attachments
Patch v1 - Fix + test (3.56 KB, patch)
2012-06-06 16:45 PDT, Brady Eidson
japhet: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2012-06-06 10:00:41 PDT
REGRESSION (r115654): Opening many non-English WebArchives shows obvious encoding issues

http://trac.webkit.org/changeset/115654 is the change that caused this.

The claim in the patch was that this was just a refactor with no change in behavior, which is false.
If we don't figure this one out pretty soon, I'm going to try my best to roll it out.

In Radar as <rdar://problem/11575112>
Comment 1 Brady Eidson 2012-06-06 10:02:05 PDT
To reproduce:

1 - Go to http://www.dni.ru/ 
2 - Save as a WebArchive.
3 - In a WebKit nightly post-115654, open that WebArchive

The encoding issues will be obvious.
Comment 2 Nate Chapin 2012-06-06 12:28:11 PDT
(In reply to comment #1)
> To reproduce:
> 
> 1 - Go to http://www.dni.ru/ 
> 2 - Save as a WebArchive.
> 3 - In a WebKit nightly post-115654, open that WebArchive
> 
> The encoding issues will be obvious.

Yep. Looks like in r115654, I dropped the setEncoding() call for archive loads based on the the main archive resource's text encoding. Should be a 1-line fix, working on test now.
Comment 3 Nate Chapin 2012-06-06 13:04:36 PDT
(In reply to comment #2)
> (In reply to comment #1)
> > To reproduce:
> > 
> > 1 - Go to http://www.dni.ru/ 
> > 2 - Save as a WebArchive.
> > 3 - In a WebKit nightly post-115654, open that WebArchive
> > 
> > The encoding issues will be obvious.
> 
> Yep. Looks like in r115654, I dropped the setEncoding() call for archive loads based on the the main archive resource's text encoding. Should be a 1-line fix, working on test now.

Is there a way to load a .webarchive as a layout test? I can't seem to find an example, and the problem is with loading, not creating the webarchive.
Comment 4 Brady Eidson 2012-06-06 13:23:28 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > To reproduce:
> > > 
> > > 1 - Go to http://www.dni.ru/ 
> > > 2 - Save as a WebArchive.
> > > 3 - In a WebKit nightly post-115654, open that WebArchive
> > > 
> > > The encoding issues will be obvious.
> > 
> > Yep. Looks like in r115654, I dropped the setEncoding() call for archive loads based on the the main archive resource's text encoding. Should be a 1-line fix, working on test now.
> 
> Is there a way to load a .webarchive as a layout test? I can't seem to find an example, and the problem is with loading, not creating the webarchive.

Great question.  I'll poke around.
Comment 5 Brady Eidson 2012-06-06 13:47:09 PDT
(In reply to comment #3) 
> Is there a way to load a .webarchive as a layout test? I can't seem to find an example, and the problem is with loading, not creating the webarchive.

I can't find anything.

On the surface, adding such support would involve mucking around in the mess of perl and python that make up the run-webkit-tests infrastructure, in order to recognize webarchives as tests to be run.

But one shortcut idea would be to add a test which loads a webarchive in an iframe.  I haven't tested this yet, but it seems like it should work.
Comment 6 Brady Eidson 2012-06-06 16:45:53 PDT
Created attachment 146141 [details]
Patch v1 - Fix + test
Comment 7 Brady Eidson 2012-06-06 17:12:18 PDT
http://trac.webkit.org/changeset/119643
Comment 8 Brady Eidson 2012-06-06 17:12:28 PDT
(In reply to comment #7)
> http://trac.webkit.org/changeset/119643

I meant http://trac.webkit.org/changeset/119644
Comment 9 Ryosuke Niwa 2012-06-06 19:10:11 PDT
This test has been failing on Chromium.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fast%2Floader%2Fwebarchive-encoding-respected.html

(Sorry, there was a some TestExpectations error and the test was skipped momentarily)
Comment 10 Brady Eidson 2012-06-06 19:42:32 PDT
ACK - My apologies, it should basically be skipped on all non-Apple ports.

I'm trying to get a checkout up and running where I am right now, but if anyone can beat me to it please feel free.
Comment 11 Ryosuke Niwa 2012-06-06 19:48:06 PDT
(In reply to comment #10)
> ACK - My apologies, it should basically be skipped on all non-Apple ports.
> 
> I'm trying to get a checkout up and running where I am right now, but if anyone can beat me to it please feel free.

Ah, okay. Could you add them to relevant Skipped and TestExpectation files?
See http://trac.webkit.org/changeset/119665
Comment 12 Brady Eidson 2012-06-06 19:50:02 PDT
(In reply to comment #10)
> ACK - My apologies, it should basically be skipped on all non-Apple ports.

Much easier to just install the test in the platform/mac directory for now, which I'll do soon.
Comment 13 Brady Eidson 2012-06-06 19:52:33 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > ACK - My apologies, it should basically be skipped on all non-Apple ports.
> > 
> > I'm trying to get a checkout up and running where I am right now, but if anyone can beat me to it please feel free.
> 
> Ah, okay. Could you add them to relevant Skipped and TestExpectation files?
> See http://trac.webkit.org/changeset/119665

Since they're skipped for now and the bots are not longer imminently red, I'll work out of 88481 in the morning to move the test and cleanup the expectations that were checked in.
Comment 14 Ryosuke Niwa 2012-06-06 19:53:14 PDT
(In reply to comment #12)
> (In reply to comment #10)
> > ACK - My apologies, it should basically be skipped on all non-Apple ports.
> 
> Much easier to just install the test in the platform/mac directory for now, which I'll do soon.

Okay, could you revert my patch & close the bug 88481 when you do that? By the way, I think chromium port runs tests in platform/mac :(
Comment 15 Brady Eidson 2012-06-07 09:36:18 PDT
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #10)
> > > ACK - My apologies, it should basically be skipped on all non-Apple ports.
> > 
> > Much easier to just install the test in the platform/mac directory for now, which I'll do soon.
> 
> Okay, could you revert my patch & close the bug 88481 when you do that? By the way, I think chromium port runs tests in platform/mac :(

That's bizarre considering they don't use the "platform/mac" WebKit...  I'll leave Chromium/mac skipped.
Comment 16 Brady Eidson 2012-06-07 09:38:32 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > (In reply to comment #12)
> > > (In reply to comment #10)
> > > > ACK - My apologies, it should basically be skipped on all non-Apple ports.
> > > 
> > > Much easier to just install the test in the platform/mac directory for now, which I'll do soon.
> > 
> > Okay, could you revert my patch & close the bug 88481 when you do that? By the way, I think chromium port runs tests in platform/mac :(
> 
> That's bizarre considering they don't use the "platform/mac" WebKit...  I'll leave Chromium/mac skipped.

Nah, we're good:

// Run the Mac-specific platform tests, but only to check for crashes.
WONTFIX : platform/mac = FAIL PASS TIMEOUT