It's currently using 'int' with manual bit manipulation which is error prone, see bug 870606. Another bug that I found while working in this: http://hg.mozilla.org/mozilla-central/annotate/776f6d341b3f/layout/tables/nsTableColGroupFrame.cpp#l341 (it should be "skip |= LOGICAL_SIDE_B_START")

This patch needs some polishing but let me know if you have some feedback on the approach.

Comment on attachment 8443954 [details] [diff] [review] wip2 >+union SkipSides { >+ SkipSides() { mBits = 0; } >+ explicit SkipSides(SideBits aSideBits) >+ { [...] >+ mBits = aSideBits; >+ } [...] >+ bool Top() const { return mSides.top; } One issue with the union approach used here: C++ doesn't actually define what happens if you assign one member of a union, and then read from another one. Reference: it's undefined behavior to read from the member of the union that wasn't most recently written as it violates type aliasing. Many compilers implement, as a non-standard language extension, the ability to read inactive members of a union. http://en.cppreference.com/w/cpp/language/union So, this patch adds a dependency on undefined-in-C++ behavior, which theoretically could cause bizarre behavior (e.g. launching nethack[1]) in an optimizing compiler that took advantage of this undefined-ness. (Presumably it's not broken by any of the compilers we use, and we probably already have many other similar dependencies. but probably better to avoid them where possible.) [1] http://feross.org/gcc-ownage/

Good point. The union isn't really necessary here; a struct with a mBits field will do fine.

part 1: I decided to generalize the SkipSides type to a generic Sides type to represent a set of physical sides. I see that we have other code, besides GetSkipSides(), that could use this type. part 2: Unlike the earlier WIP patches, I'm also going to add an additional distinct LogicalSides type to represent logical sides to make any confusion between the two a compile error. The ApplySkipSides() methods in nsMargin/LogicalMargin will take a Sides/LogicalSides value respectively. This part also adds distinct enum types for the logical side values. part 3: Make GetSkipSides() return Sides and GetLogicalSkipSides() return LogicalSides. part 4: Remove the #define LOGICAL_SIDE* in favor of the LogicalSideBits enum values in part 2. In summary, we now have for physical sides: mozilla::Side, the ordinal value for each physical side, eSideTop etc mozilla::SideBits, the corresponding bit values, eSideBitsTop etc mozilla::Sides, struct to represent a set of sides, with methods taking SideBits and for logical sides: mozilla::LogicalSide, the ordinal value for each logical side, eLogicalSideBStart etc mozilla::LogicalSideBits, the corresponding bit values, eLogicalSideBitsBStart etc mozilla::LogicalSides, struct to represent a set of sides, with methods taking LogicalSideBits

Some minor tweaks to fix a "ambiguous type" compile error on Windows.

Comment on attachment 8447534 [details] [diff] [review] part 3 v2, Change the return type for Get*SkipSides() Review of attachment 8447534 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSRendering.h @@ +337,5 @@ > > > /** > * Render the outline for an element using css rendering rules > + * for borders. trailing whitespace

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > trailing whitespace Fixed. I also amended the doc comment for Get*SkipSides() slightly, pointing out the location of the Sides/LogicalSides types.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e237b2c61ea2 https://hg.mozilla.org/integration/mozilla-inbound/rev/316c8dfeca9b https://hg.mozilla.org/integration/mozilla-inbound/rev/ae01b3919c8c https://hg.mozilla.org/integration/mozilla-inbound/rev/663ff18cd4a1

I don't pretend to have any idea how this could be the case, but either this or bug 1031444 somehow broke /tests/dom/asmjscache/test/test_cachingBasic.html ("asm.js compilation is available") on Android 2.2 Armv6 Opt, https://tbpl.mozilla.org/php/getParsedLog.php?id=42694025&tree=Mozilla-Inbound Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ca361c8fea

From the failed test test_cachingBasic.html: <script> var jsFuns = SpecialPowers.Cu.getJSTestingFunctions(); ok(jsFuns.isAsmJSCompilationAvailable(), "asm.js compilation is available"); I don't see how my changes could have any effect on that. I pushed to Try: 1. just the patches in this bug 2. just the patch in bug 1031444 3. both together 4. both together + CLOBBER All are green, with multiple M4 runs on "Android 2.2 Armv6 Opt", which was the (only) platform that failed on -inbound. I compared which slaves did the job on m-i vs. Try and found a couple with the same name (I'm assuming it's the same actual machine). I'm going to push bug 1031444 only for now, since it's a trivial change. And then run the patches in this bug in ASAN to see if there's a memory violation or something in here.

Going with the theory of bug 1030446 comment 9, relanded this. https://hg.mozilla.org/integration/mozilla-inbound/rev/2c434107469b https://hg.mozilla.org/integration/mozilla-inbound/rev/49575c74cc7b https://hg.mozilla.org/integration/mozilla-inbound/rev/90afab231e68 https://hg.mozilla.org/integration/mozilla-inbound/rev/e8ed5f32c626

(In reply to Phil Ringnalda (:philor) from comment #15) > ..., relanded this. That's nice, thank you. And for the record, the ASAN Try run and local ASAN testing didn't find anything.

https://hg.mozilla.org/mozilla-central/rev/e8ed5f32c626 https://hg.mozilla.org/mozilla-central/rev/90afab231e68 https://hg.mozilla.org/mozilla-central/rev/49575c74cc7b https://hg.mozilla.org/mozilla-central/rev/2c434107469b