Bug 20924 - [Gtk] Linux/Gtk: Recent tree revisions fail Acid2 and Acid3
: [Gtk] Linux/Gtk: Recent tree revisions fail Acid2 and Acid3
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
: Gtk
Depends on: 19578
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-18 15:46 PDT by Clemmitt Sigler
Modified: 2008-10-04 23:14 PDT (History)
1 user (show)

See Also:


Attachments
Erroneous rendering of Acid2 test, Gtk/Linux r36623 (14.14 KB, image/png)
2008-09-18 15:48 PDT, Clemmitt Sigler
no flags Details
Patch to fix reported acid2/acid3 failures (592 bytes, patch)
2008-09-24 17:37 PDT, Clemmitt Sigler
no flags Details | Formatted Diff | Diff
Patch to revert r35954 to fix reported acid2/acid3 failures (1.99 KB, patch)
2008-09-27 11:03 PDT, Clemmitt Sigler
alp: review+
Details | Formatted Diff | Diff
fix regression Acid without remove all changes available on all other platform (1.23 KB, patch)
2008-09-28 08:57 PDT, Mario Bensi
no flags Details | Formatted Diff | Diff
fix regression Acid without remove all changes available on all other platform (1.24 KB, patch)
2008-09-28 09:27 PDT, Mario Bensi
alp: review-
Details | Formatted Diff | Diff
Fix regressions and clean up the mess (5.94 KB, patch)
2008-09-30 08:38 PDT, Alp Toker
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Clemmitt Sigler 2008-09-18 15:46:29 PDT
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
Comment 1 Clemmitt Sigler 2008-09-18 15:48:41 PDT
Created attachment 23542 [details]
Erroneous rendering of Acid2 test, Gtk/Linux r36623
Comment 2 Clemmitt Sigler 2008-09-24 17:35:22 PDT
(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
Comment 3 Clemmitt Sigler 2008-09-24 17:37:07 PDT
Created attachment 23775 [details]
Patch to fix reported acid2/acid3 failures
Comment 4 Clemmitt Sigler 2008-09-25 22:54:53 PDT
(In reply to comment #2)

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

Clemmitt
Comment 5 Jan Alonzo 2008-09-26 23:51:40 PDT
hi Clemitt! Kindly add a ChangeLog to your patch.

Cheers
Comment 6 Clemmitt Sigler 2008-09-27 10:58:20 PDT
(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
Comment 7 Clemmitt Sigler 2008-09-27 11:03:32 PDT
Created attachment 23878 [details]
Patch to revert r35954 to fix reported acid2/acid3 failures
Comment 8 Alp Toker 2008-09-27 23:10:36 PDT
Comment on attachment 23878 [details]
Patch to revert r35954 to fix reported acid2/acid3 failures

r=me

Thanks Clemmitt!
Comment 9 Mario Bensi 2008-09-28 08:57:24 PDT
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 ?
Comment 10 Mario Bensi 2008-09-28 09:27:35 PDT
Created attachment 23893 [details]
fix regression Acid without remove all changes available on all other platform
Comment 11 Clemmitt Sigler 2008-09-28 15:18:17 PDT
(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
Comment 12 Alp Toker 2008-09-29 16:58:25 PDT
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.
Comment 13 Alp Toker 2008-09-30 08:38:32 PDT
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.
Comment 14 Clemmitt Sigler 2008-10-01 04:24:20 PDT
(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
Comment 15 Clemmitt Sigler 2008-10-04 04:55:47 PDT
(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
Comment 16 Dave Hyatt 2008-10-04 17:56:35 PDT
Comment on attachment 23941 [details]
Fix regressions and clean up the mess

r=me
Comment 17 Jan Alonzo 2008-10-04 23:14:06 PDT
Landed in r37314