Closed
Bug 1298318
Opened 7 years ago
Closed 7 years ago
Fix operator==() and Hash() function in SVGImageContext
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: kechen, Assigned: kechen)
References
Details
(Keywords: regression)
Attachments
(2 files, 1 obsolete file)
1.50 KB,
patch
|
dholbert
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
888 bytes,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•7 years ago
|
||
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 2•7 years ago
|
||
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 | ||
Comment 3•7 years ago
|
||
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?
Assignee | ||
Comment 4•7 years ago
|
||
Try result : no SVG-related error https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fff37e07fd1&selectedJob=26397516
Assignee | ||
Comment 5•7 years ago
|
||
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
status-firefox50:
--- → affected
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/31886ceb88e3
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
status-firefox49:
--- → affected
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 9•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/3a0e91dee1c6
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.
Assignee | ||
Comment 11•7 years ago
|
||
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+
Comment 13•7 years ago
|
||
(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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/113c5a385f5b
Comment 15•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-release/rev/113c5a385f5b
Assignee | ||
Comment 16•7 years ago
|
||
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 17•7 years ago
|
||
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+
Assignee | ||
Comment 18•7 years ago
|
||
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 19•7 years ago
|
||
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+
Comment 20•7 years ago
|
||
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•7 years ago
|
||
Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a387818ed08a followup - Add notice comment in SVGImageContext. r=dholbert
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a387818ed08a
Updated•7 years ago
|
Version: unspecified → 49 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•