Closed Bug 839809 Opened 7 years ago Closed 7 years ago

make counters/lists that would go past our internal limit (int32_t) stay constant rather than wrapping

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: dbaron, Assigned: dbaron)

Details

Attachments

(1 file)

No description provided.
Per CSS WG resolution regarding counter-styles-3, afternoon of 2013-02-05:
http://krijnhoetmer.nl/irc-logs/css/20130205#l-1590

Note that this patch depends on signed integer overflow behavior in C++,
which I believe is portable despite being unspecified.
Attachment #712175 - Flags: review?(dholbert)
Comment on attachment 712175 [details] [diff] [review]
Make counter-increments and list counting that would go past our internal (int32_t) limit keep the counter at its current value rather than wrapping.

r=me -- just a few optional suggestions:

>+++ b/layout/base/nsCounterManager.cpp
>@@ -53,17 +53,25 @@ void nsCounterUseNode::Calc(nsCounterLis
>-        mValueAfter = aList->ValueBefore(this) + mChangeValue;
>+        int32_t valueBefore = aList->ValueBefore(this);
>+        int32_t valueAfter = valueBefore + mChangeValue;
>+        // The CSS Working Group resolved that a counter-increment that
>+        // exceeds internal limits should not increment at all.
>+        // http://krijnhoetmer.nl/irc-logs/css/20130205#l-1590
>+        if ((mChangeValue > 0) != (valueAfter > valueBefore)) {
>+          valueAfter = valueBefore;
>+        }
>+        mValueAfter = valueAfter;
[...]
>+++ b/layout/generic/nsBulletFrame.cpp
>+  int32_t nextOrdinal = mOrdinal + aIncrement;
>+
>+  // The CSS Working Group resolved that a counter-increment that
>+  // exceeds internal limits should not increment at all.
>+  // http://krijnhoetmer.nl/irc-logs/css/20130205#l-1590
>+  if ((aIncrement > 0) != (nextOrdinal > mOrdinal)) {
>+    nextOrdinal = mOrdinal;
>+  }
>+
>+  return nextOrdinal;

Rather than duplicating the comment & wraparound-checking-technique here, it might be better to separate out this code into a static helper method, (maybe on nsCounterManager or on nsLayoutUtils) -- e.g. int32_t IncrementCounterWithBoundsCheck(int32_t aCurCounterValue, int32_t aOffset).

Then both of these changes would just be one-liners.

It's also fine as-is, though.

>+++ b/layout/reftests/counters/counter-ua-limits-list-01-ref.html
>+
>+ <!--
>+    Work around our inability to parse -2147483648 as an HTML integer attribute
>+   -->

Probably worth mentioning bug 586761 in this comment. (looks like that covers this issue about parsing -2147483648)
Attachment #712175 - Flags: review?(dholbert) → review+
Comment on attachment 712175 [details] [diff] [review]
Make counter-increments and list counting that would go past our internal (int32_t) limit keep the counter at its current value rather than wrapping.

Also:
>@@ -6753,16 +6753,18 @@ nsBlockFrame::RenumberLists(nsPresContex
>     for (nsIContent* kid = mContent->GetFirstChild(); kid;
>          kid = kid->GetNextSibling()) {
>       if (kid->IsHTML(nsGkAtoms::li)) {
>+        // FIXME: This isn't right in terms of what CSS says to do for
>+        // overflow of counters.
>         ordinal -= increment;

AFAICT, we could only run into trouble here if we had a list with has more than 2^31 entries, right?  Might be worth mentioning that here, since that makes it significantly less worrisome.
er, s/has more than/has at least/
(In reply to Daniel Holbert [:dholbert] from comment #2)
> IncrementCounterWithBoundsCheck(int32_t aCurCounterValue, int32_t aOffset).

I plan to take this suggestion, but just call the function IncrementCounter.
Cool, that sounds good.
(Reftest failures were Mac OS X only; tests were fine on Windows, Linux, and Android if we run new reftests there by default.)
s/module/modulo/ in the new comment.
https://hg.mozilla.org/mozilla-central/rev/c46476d3892a
https://hg.mozilla.org/mozilla-central/rev/5c28fd3fd030
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.