Closed Bug 1298318 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: SVG, defect)

49 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: kechen, Assigned: kechen)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

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
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.
Attachment #8785182 - Flags: review?(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.
Attachment #8785182 - Flags: review?(dholbert) → review+
Assignee: nobody → kechen
Blocks: 1264809
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.
Attachment #8785182 - Flags: approval-mozilla-beta?
Attachment #8785182 - Flags: approval-mozilla-aurora?
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.
Attachment #8785182 - Flags: approval-mozilla-beta?
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/31886ceb88e3
Add mIsPaintingSVGImageElement to operator==() and Hash() in SVGImageContext. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/31886ceb88e3
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Keywords: regression
Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Fixes a recent regression, Aurora50+
Attachment #8785182 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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 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.
Attachment #8785182 - Flags: approval-mozilla-beta?
Comment on attachment 8785182 [details] [diff] [review]
Add mIsPaintingSVGImageElement to operator==() and Hash()

Last minute fix for SVG regression. Uplifting for RC3
Attachment #8785182 - Flags: approval-mozilla-release+
Attachment #8785182 - Flags: approval-mozilla-beta?
Attachment #8785182 - Flags: approval-mozilla-beta+
(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.
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 ?
Attachment #8790520 - Flags: feedback?(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==
Attachment #8790520 - Flags: feedback?(dholbert) → feedback+
Hi Daniel, I modified the comment as Comment 17 suggests, can you help me to review it ? Thank you.
Attachment #8790520 - Attachment is obsolete: true
Attachment #8791065 - Flags: review?(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
Attachment #8791065 - Flags: review?(dholbert) → review+
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).
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a387818ed08a
followup - Add notice comment in SVGImageContext. r=dholbert
Version: unspecified → 49 Branch
You need to log in before you can comment on or make changes to this bug.