Bug 20924 – [Gtk] Linux/Gtk: Recent tree revisions fail Acid2 and Acid3

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Clemmitt Sigler]

It's been perhaps a month or more since Acid3 passed with a score of 100 for GTK builds under Linux.  In fact, even Acid2 fails with a background color error!  Both Acid2 and Acid3 used to pass.  From r36623, running Programs/GtkLauncher:

Acid2: Incorrectly rendered Acid2 test image attached next.

Acid3 fail message:
Failed 3 tests.
Test 26 passed, but took 65ms (less than 30fps)
Test 69 passed, but took 29 attempts (less than perfect).
Test 77 failed: expected '4776' but got '7189' - getComputedTextLength failed.
Test 78 failed: expected '90' but got '0' - getRotationOfChar(0) failed.
Test 97 failed: expected 'one' but got 'fail' - data: failed as escaped
Total elapsed time: 1.79s

HTH.

Clemmitt

[Clemmitt Sigler]

Created attachment 23542 [details]
Erroneous rendering of Acid2 test, Gtk/Linux r36623

[Clemmitt Sigler]

(In reply to comment #0)

Hope someone looks at this, even just to tell me the resolution is WFM...!

It took me a while, but I've tracked this down to a one-line change introduced in r35954.  I'm attaching a patch I made on r36873 to fix the acid2 rendering bug/acid3 failure on my system.

In his ChangeLog message, Mario Bensi said his patch fixed a problem where "the image test ( https://bugs.webkit.org/attachment.cgi?id=21726 ) is not drawn correctly," so my patch may break rendering of some other things???

Info that may be peculiar to my system: I'm running Gentoo with NLS *explicity*turned*off* for the packages I build/install (in /etc/make.conf).  I wonder if this would affect stuff like data.latin1().data() ...?

HTH.

Clemmitt

[Clemmitt Sigler]

Created attachment 23775 [details]
Patch to fix reported acid2/acid3 failures

[Clemmitt Sigler]

(In reply to comment #2)

Just for shites and giggles, would someone with perm please confirm/NEW this bug report?  TIA.

Clemmitt

[Jan Alonzo]

hi Clemitt! Kindly add a ChangeLog to your patch.

Cheers

[Clemmitt Sigler]

(In reply to comment #5)
> hi Clemitt! Kindly add a ChangeLog to your patch.

Hello Jan, and thanks.  My apologies for not following the "Contributing Code" guidelines.  I'll get this fixed up right now....

Done.  Will attach patch now and mark for review.  TIA.

Clemmitt

[Clemmitt Sigler]

Created attachment 23878 [details]
Patch to revert r35954 to fix reported acid2/acid3 failures

[Alp Toker]

Comment on attachment 23878 [details]
Patch to revert r35954 to fix reported acid2/acid3 failures

r=me

Thanks Clemmitt!

[Mario Bensi]

Created attachment 23892 [details]
fix regression Acid without remove all changes available on all other platform

this path keep all change of bug 19578 and add a fix to remove the regression of the old patch.

I think is better than remove the last changes needed by all platform except gtk, no ?

[Mario Bensi]

Created attachment 23893 [details]
fix regression Acid without remove all changes available on all other platform

[Clemmitt Sigler]

(In reply to comment #9)
> this path keep all change of bug 19578 and add a fix to remove the regression
> of the old patch.

I just tested this change against revision 37045, and it does address the GTK Acid2/Acid3 failures in my tree :^)  (Not sure if it meets the developers' coding standards for this module....)

Thanks for the revised patch!

Clemmitt

[Alp Toker]

Comment on attachment 23893 [details]
fix regression Acid without remove all changes available on all other platform

Mario,

Sorry about my terse reply earlier -- I may not have explained the issue fully.

The change in r35954 needs to be backed out not just because it regressed the GLib code path but because it's wrong on all platforms: it introduces the incorrect assumption that all data URLs are Base64-encoded, whereas we're meant to support non-encoded data URLs as well.

The logic in parseDataUrl() function is pretty messy/difficult to follow but it was essentially correct for both GLib and non-GLib before r35954 (except for the loss when going back to a string in the non-GLib code path which you noticed).

If you study this function (as it was before r35954) you'll see what I'm getting at and why your latest change isn't correct either. Personally I'd love to see this stupid function rewritten as it totally obfuscates a simple task.

[Alp Toker]

Created attachment 23941 [details]
Fix regressions and clean up the mess

This patch addresses the known issues in both the GLib and non-GLib parseDataUrl() paths.

[Clemmitt Sigler]

(In reply to comment #13)
> Created an attachment (id=23941) [edit]
> Fix regressions and clean up the mess

Hi Alp,

Many kudos!  Thanks for taking the time to grok this module and patch it up "right."  I've applied your patch to r37102 (the latest one attached) and it fixes the Linux/GTK Acid2 and Acid3 regressions I've noted.

Clemmitt

[Clemmitt Sigler]

(In reply to comment #14)

Is there any way to get some love for Alp's patch?  I'd love to see this misbehaviour get fixed -- a different patch to ResourceHandleManager.cpp just got landed.  I'd hope this could get landed so that conflicts with this patch can be avoided....  TIA.

Clemmitt

[Dave Hyatt]

Comment on attachment 23941 [details]
Fix regressions and clean up the mess

r=me

[Jan Alonzo]

Landed in r37314