Closed Bug 1028460 Opened 6 years ago Closed 6 years ago

Implement a proper SkipSides type for the GetSkipSides() result

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mats, Assigned: mats)

Details

Attachments

(4 files, 3 obsolete files)

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")
Attached patch wip1 (obsolete) — Splinter Review
This patch needs some polishing but let me know if you have some
feedback on the approach.
Attached patch wip2 (obsolete) — Splinter Review
Attachment #8443821 - Attachment is obsolete: true
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
Attachment #8443954 - Attachment is obsolete: true
Attachment #8447330 - Flags: review?(roc)
Attachment #8447334 - Flags: review?(roc)
Some minor tweaks to fix a "ambiguous type" compile error on Windows.
Attachment #8447334 - Attachment is obsolete: true
Attachment #8447334 - Flags: review?(roc)
Attachment #8447534 - Flags: review?(roc)
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
Attachment #8447534 - Flags: review?(roc) → review+
(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.
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.
(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.
You need to log in before you can comment on or make changes to this bug.