Bug 88428 – REGRESSION (r115654): Opening many non-English WebArchives shows obvious encoding issues

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

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>

[Brady Eidson]

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.

[Nate Chapin]

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

[Nate Chapin]

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

[Brady Eidson]

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

[Brady Eidson]

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

[Brady Eidson]

Created attachment 146141 [details]
Patch v1 - Fix + test

[Brady Eidson]

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

[Brady Eidson]

(In reply to comment #7)
> http://trac.webkit.org/changeset/119643

I meant http://trac.webkit.org/changeset/119644

[Ryosuke Niwa]

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)

[Brady Eidson]

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.

[Ryosuke Niwa]

(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

[Brady Eidson]

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

[Brady Eidson]

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

[Ryosuke Niwa]

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

[Brady Eidson]

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

[Brady Eidson]

(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