Bug 48347 – Calling the super class of RenderSVGImage::updateFromElement is missing

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Renata Hodovan]

Calling the super class of RenderSVGImage::updateFromElement is missing

[Renata Hodovan]

Created attachment 71914 [details]
Adding the missing super class call

The patch fixes the failed dynamic-update tests of feConvolveMatrix as well (because they have SVGImage objects)

[Andreas Kling]

(In reply to comment #1)
> The patch fixes the failed dynamic-update tests of feConvolveMatrix as well (because they have SVGImage objects)

This seems to imply that something should be unskipped and/or rebaselined.

[Nikolas Zimmermann]

Comment on attachment 71914 [details]
Adding the missing super class call

Looks good, great catch!
You need to write a better ChangeLog, saying what's fixed though...

[Renata Hodovan]

Created attachment 71988 [details]
Adding the missing super class call

[Dirk Schulze]

(In reply to comment #4)
> Created an attachment (id=71988) [details]
> Adding the missing super class call

I'd like to read a sentence in the ChangeLog that this is covered by dynamic updates of feConvolveMatrix, since every regression needs a regression test.

[Dirk Schulze]

Comment on attachment 71988 [details]
Adding the missing super class call

Sorry, but this sentence need to go in. It's WebKit policy.

[Renata Hodovan]

Created attachment 72039 [details]
Adding the missing super class call

[Dirk Schulze]

Comment on attachment 72039 [details]
Adding the missing super class call

r=me

[WebKit Commit Bot]

Comment on attachment 72039 [details]
Adding the missing super class call

Clearing flags on attachment: 72039

Committed r70654: <http://trac.webkit.org/changeset/70654>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

(In reply to comment #10)
> All reviewed patches have been landed.  Closing bug.

This patch affected a few pixel results, and they probably should be landed with the patch.

[Zoltan Herczeg]

> This patch affected a few pixel results, and they probably should be landed with the patch.

Chromium pixel tests are a difficult problem. Once I wanted to update some Chromium pixel tests, and asked Eric for help, but he said he use mac, and gave me some names. I wrote them, but they don't reply. The poblem is we couldn't compile a chromium port and run pixel tests, since we have no idea how it works :(

[Dimitri Glazkov (Google)]

(In reply to comment #12)
> > This patch affected a few pixel results, and they probably should be landed with the patch.
> 
> Chromium pixel tests are a difficult problem. Once I wanted to update some Chromium pixel tests, and asked Eric for help, but he said he use mac, and gave me some names. I wrote them, but they don't reply. The poblem is we couldn't compile a chromium port and run pixel tests, since we have no idea how it works :(

I'll take care of the Chromium pixel tests. However, I am positive this patch also affected platform/mac, for instance.

[Zoltan Herczeg]

> I'll take care of the Chromium pixel tests. However, I am positive this patch also affected platform/mac, for instance.

Exactly, in a good way. Those tests were working properly, until the patch mentioned in the ChangeLog made them wrong. So this patch just restores their oiginal state. Since the mac bots don't run pixel tests, this thing was hidden so far.

[Zoltan Herczeg]

To be more precise: if you run mac pixel tests on the mainline, you get 10 fails. This patch restores 8 of them, and you get less fails.

[Dimitri Glazkov (Google)]

(In reply to comment #15)
> To be more precise: if you run mac pixel tests on the mainline, you get 10 fails. This patch restores 8 of them, and you get less fails.

That's great! :)

[WebKit Review Bot]

http://trac.webkit.org/changeset/70667 might have broken GTK Linux 32-bit Debug