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
: WebKit
WebKit Gtk
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
: 19578
:
  Show dependency treegraph
 
Reported: 2008-09-18 15:46 PST by
Modified: 2008-10-04 23:14 PST (History)


Attachments
Erroneous rendering of Acid2 test, Gtk/Linux r36623 (14.14 KB, image/png)
2008-09-18 15:48 PST, Clemmitt Sigler
no flags Details
Patch to fix reported acid2/acid3 failures (592 bytes, patch)
2008-09-24 17:37 PST, Clemmitt Sigler
no flags Review Patch | Details | Formatted Diff | Diff
Patch to revert r35954 to fix reported acid2/acid3 failures (1.99 KB, patch)
2008-09-27 11:03 PST, Clemmitt Sigler
alp: review+
Review Patch | 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 PST, Mario Bensi
no flags Review Patch | 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 PST, Mario Bensi
alp: review-
Review Patch | Details | Formatted Diff | Diff
Fix regressions and clean up the mess (5.94 KB, patch)
2008-09-30 08:38 PST, Alp Toker
hyatt: review+
Review Patch | Details | Formatted Diff | Diff


Note

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


Description From 2008-09-18 15:46:29 PST
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 From 2008-09-18 15:48:41 PST -------
Created an attachment (id=23542) [details]
Erroneous rendering of Acid2 test, Gtk/Linux r36623
------- Comment #2 From 2008-09-24 17:35:22 PST -------
(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 From 2008-09-24 17:37:07 PST -------
Created an attachment (id=23775) [details]
Patch to fix reported acid2/acid3 failures
------- Comment #4 From 2008-09-25 22:54:53 PST -------
(In reply to comment #2)

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

Clemmitt
------- Comment #5 From 2008-09-26 23:51:40 PST -------
hi Clemitt! Kindly add a ChangeLog to your patch.

Cheers
------- Comment #6 From 2008-09-27 10:58:20 PST -------
(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 From 2008-09-27 11:03:32 PST -------
Created an attachment (id=23878) [details]
Patch to revert r35954 to fix reported acid2/acid3 failures
------- Comment #8 From 2008-09-27 23:10:36 PST -------
(From update of attachment 23878 [details])
r=me

Thanks Clemmitt!
------- Comment #9 From 2008-09-28 08:57:24 PST -------
Created an attachment (id=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 From 2008-09-28 09:27:35 PST -------
Created an attachment (id=23893) [details]
fix regression Acid without remove all changes available on all other platform
------- Comment #11 From 2008-09-28 15:18:17 PST -------
(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 From 2008-09-29 16:58:25 PST -------
(From update of attachment 23893 [details])
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 From 2008-09-30 08:38:32 PST -------
Created an attachment (id=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 From 2008-10-01 04:24:20 PST -------
(In reply to comment #13)
> Created an attachment (id=23941) [edit] [details]
> 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 From 2008-10-04 04:55:47 PST -------
(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 From 2008-10-04 17:56:35 PST -------
(From update of attachment 23941 [details])
r=me
------- Comment #17 From 2008-10-04 23:14:06 PST -------
Landed in r37314