|ResetBStartStart| might introduce weird borders

Follow bug 1405929 comment 45 to further address the comments.

still have to make reftest.

Fail w/o patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d56c0e76be231b64414e1089dd216a82a7c0e965 Pass w/ patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c73bce60ad92d13034413f6fc04ac4254545856

Comment on attachment 8939715 [details] [diff] [review] 0001-Bug-1421887-fix-the-row-index-to-get-the-right-cell..patch >+// XXX eLogicalSideBStart, eLogicalSideIEnd and eLogicalSideIStart >+// have never been called. Consider to use them or remove them. Two comments here: The first is about the English. Rather than "Consider to use them or remove them." it should be "Consider using them or removing them." (Also, "have never been called" should really be "are not used".) The second is about the meaning. I don't think suggesting using them makes sense; we shouldn't add uses of a function just because it's unused. So I think the only reasonable suggestion is not use them. I'd suggest rewording to say something like: // FIXME: The only value callers pass for aSide is eLogicalSideBEnd. Consider removing support for the other three values. r=dbaron with that or similar

Comment on attachment 8939716 [details] [diff] [review] 0002-Bug-1421887-add-reftest.-r-dbaron.patch In the future, for cases like this, it might be better to make the reftest a little bit more different from the test, so that its rendering is more obviously correct, rather than using such similar tests. However, I'm struggling to come up with a good definition of what "cases like this" are -- in many cases, what you did here is the right approach, but I think it's probably a little riskier here due to the risk of both test and reference exercising a buggy codepath. But I'd need to think more about in what cases I think that's good advice, since it's certainly not true in general. Anyway, r=dbaron, since this is fine.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #7) > Comment on attachment 8939716 [details] [diff] [review] > 0002-Bug-1421887-add-reftest.-r-dbaron.patch > > In the future, for cases like this, it might be better to make the reftest a > little bit more different from the test, so that its rendering is more > obviously correct, rather than using such similar tests. However, I'm > struggling to come up with a good definition of what "cases like this" are > -- in many cases, what you did here is the right approach, but I think it's > probably a little riskier here due to the risk of both test and reference > exercising a buggy codepath. But I'd need to think more about in what cases > I think that's good advice, since it's certainly not true in general. I see. thank you.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60ecf9dcc4935deb32b20d1c2926eca87ef27cd9

Pushed by toros@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/888271e96fec fix the row index to get the right cell. r=dbaron https://hg.mozilla.org/integration/mozilla-inbound/rev/21060660b938 add reftest. r=dbaron

https://hg.mozilla.org/mozilla-central/rev/888271e96fec https://hg.mozilla.org/mozilla-central/rev/21060660b938