1298318 - Fix operator==() and Hash() function in SVGImageContext

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Kevin Chen[:kechen] (UTC + 8)]

In Bug 1264809, a member variable is added to SVGImageContext; however, as the comment[1] mentioned, operator==() and Hash() function in SVGImageContext aren't modified which may causes some errors.


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1264809#c97

Comment 1

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Add mIsPaintingSVGImageElement to operator==() and Hash()

Hi Daniel, can you help me to review this patch ?

And should I also uplift this to beta since we've already uplifted the patches in Bug 1264809.

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Review of attachment 8785182 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! Looks good. And yes, we should uplift this to Firefox Beta if possible, as you note, since it fixes a defect in bug 1264809's patches.

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1264809
[User impact if declined]:
  In some specific cases (Two SVG images have the same member variables except mIsPaintingSVGImageElement), SVG image will be drawn incorrectly.
[Describe test coverage new/current, TreeHerder]:
  No test.
[Risks and why]: 
  The risk is low, in this patch, the only modification is to add a member variable to operator== and Hash() function, the logic is simple and clear.

Comment 4

[Kevin Chen[:kechen] (UTC + 8)]

Try result : no SVG-related error
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fff37e07fd1&selectedJob=26397516

Comment 5

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

The patch in Bug 1264809 haven't been merged to beta yet, so a conflict will happen when applying this patch to beta.

I will mark the beta uplift request again after the merge.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31886ceb88e3
Add mIsPaintingSVGImageElement to operator==() and Hash() in SVGImageContext. r=dholbert

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31886ceb88e3

Comment 8

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Fixes a recent regression, Aurora50+

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a0e91dee1c6

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Another fix for SVG regression from 49. This can make the RC2 build today.

Comment 11

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Approval Request Comment
[Feature/regressing bug #]:
  Bug 1264809
[User impact if declined]:
  In some specific cases (Two SVG images have the same member variables except mIsPaintingSVGImageElement), SVG image will be drawn incorrectly.
[Describe test coverage new/current, TreeHerder]:
  No test.
[Risks and why]: 
  The risk is low, in this patch, the only modification is to add a member variable to operator== and Hash() function, the logic is simple and clear.

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Last minute fix for SVG regression. Uplifting for RC3

Comment 13

[Astley Chen (inactive)]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #0)
> In Bug 1264809, a member variable is added to SVGImageContext; however, as
> the comment[1] mentioned, operator==() and Hash() function in
> SVGImageContext aren't modified which may causes some errors.
> 
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1264809#c97

Could it be possible to ensure it's always valid by any particular test cases ? or at least we can have some comments in here so that others could be aware of this in future.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/113c5a385f5b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/113c5a385f5b

Comment 16

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Extra patch) Add a notice comment in SVGImageContext.

Hi Daniel,

According to comment 13, we should do something to ensure others wouldn't add new parameters without modifying these two methods.

I've done some researches but haven't found a way to verify this by test. So I add a notice comment in SVGImageContext.

Is the comment enough or a test is necessary ?

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8790520 [details] [diff] [review]
(Extra patch) Add a notice comment in SVGImageContext.

Review of attachment 8790520 [details] [diff] [review]:
-----------------------------------------------------------------

This comment seems sufficient to me.

r=me with the comment clarified a bit, like so:

::: layout/svg/SVGImageContext.h
@@ +76,5 @@
>      return aPAR.Hash();
>    }
>  
> +  // When adding the parameters, please also remember to modify Hash() and
> +  // operator ==.

Let's reword this a bit, to make it fit on one line & to make it more clearly a general note rather than something about the specific member-var(s) that it comes before. How about:
  // NOTE: When adding new member-vars, remember to update Hash() & operator==

Comment 18

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: (Extra patch) Add a notice comment in SVGImageContext.

Hi Daniel, I modified the comment as Comment 17 suggests, can you help me to review it ? Thank you.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8791065 [details] [diff] [review]
(Extra patch) Add a notice comment in SVGImageContext.

Review of attachment 8791065 [details] [diff] [review]:
-----------------------------------------------------------------

One final nit: you might add "followup" to the commit message, as in "Bug 1298318 followup - [...]"

(That makes it clearer that this isn't *the* patch for bug Bug 1298318.)

r=me

Comment 20

[Daniel Holbert [:dholbert]]

To save time, I'll just go ahead and make that tweak locally & push this on your behalf (as a DONTBUILD commit since it's comment-only & hence doesn't need tests/builds to be run).

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a387818ed08a
followup - Add notice comment in SVGImageContext. r=dholbert

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a387818ed08a