Cameron McCormack (:heycam) Assignee
	Description • 10 years ago

Having nsStyleCoord::Calc be a style context allocated object makes the work in bug 931668 awkward.  As dbaron suggests in bug 931668 comment 75, let's make it refcounted to avoid problems with style contexts going away while style structs still have nsStyleCoords with Calc object pointers.

	Cameron McCormack (:heycam) Assignee
	Comment 2 • 10 years ago

Renaming because I want to use CalcValue in a later patch.

	Cameron McCormack (:heycam) Assignee
	Comment 5 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=ba4c6cb9b866

	David Baron :dbaron:
	Comment 6 • 10 years ago

Comment on attachment 8441249 [details] [diff] [review]
Part 1: Make nsStyleUnit a uint8_t-sized enum so we can use it on nsStyle{Sides,Corners}.

r=dbaron, although I'm not sure this portable yet.  (If it is, we should definitely start using it more, though.)

	David Baron :dbaron:
	Comment 7 • 10 years ago

Comment on attachment 8441251 [details] [diff] [review]
Part 2: Rename CalcValue to FloatCalcValue.

I probably prefer PixelCalcValue over FloatCalcValue.  So perhaps change it unless you prefer FloatCalcValue?

	David Baron :dbaron:
	Comment 8 • 10 years ago

Comment on attachment 8441252 [details] [diff] [review]
Part 3: Make nsStyleCoord::Calc refcounted and introduce a non-refcounted nsStyleCoord::CalcValue.

nsStyleCoord.h:

>+  private:
>+    Calc(const Calc&);
>+    ~Calc() {}
>+    Calc& operator=(const Calc&);

Please use MOZ_DELETE on the copy-ctor and operator=.


In nsStyleCoord::nsStyleCoord(const nsStyleCoord& aCopy) you may as
well use the 3-arg version of SetValue instead of the 4-arg version.


nsStyleCoord.cpp:


In nsStyleSides::nsStyleSides, could you set the 4 units to eStyleUnit_Null
instead of using memset?

Same for nsStyleCorners, except 8.


r=dbaron with that

	David Baron :dbaron:
	Comment 9 • 10 years ago

Comment on attachment 8441252 [details] [diff] [review]
Part 3: Make nsStyleCoord::Calc refcounted and introduce a non-refcounted nsStyleCoord::CalcValue.

Also, could you make Reset() assert that the unit given is in the range of valid units (I'm not worried about the holes) so that if there's a bad Reset() call on uninitialized memory, we're likely to catch it?

	David Baron :dbaron:
	Comment 10 • 10 years ago

Comment on attachment 8441252 [details] [diff] [review]
Part 3: Make nsStyleCoord::Calc refcounted and introduce a non-refcounted nsStyleCoord::CalcValue.

>   nsStyleCoord&  operator=(const nsStyleCoord& aOther)
>   {
>-    mUnit = aOther.mUnit;
>-    mValue = aOther.mValue;
>+    if (this != &aOther) {
>+      Reset();
>+      mUnit = aOther.mUnit;
>+      mValue = aOther.mValue;
>+      if (mUnit == eStyleUnit_Calc) {
>+        static_cast<Calc*>(mValue.mPointer)->AddRef();
>+      }
>+    }
>     return *this;
>   }

Also, could you make this just use SetValue (inside the this != &aOther test)?

	David Baron :dbaron:
	Comment 11 • 10 years ago

Comment on attachment 8441253 [details] [diff] [review]
Part 4: Make some style structs less memcpyable now that nsStyleCoords aren't POD types.

This should land before the other patches, not after.

Would you mind just getting rid of all the memcpy()s (including the other structs that you don't touch here), and just writing normal copy-constructors for everything (and not reordering members, and removing the comment in nsStylePosition in nsStyleStruct.h)?

r=dbaron with that


Also, this bug needs one additional patch -- to remove the entire mAllocations mechanism from nsStyleContext.

	David Baron :dbaron:
	Comment 12 • 10 years ago

(In reply to David Baron [:dbaron] (busy May 31-June 15) (UTC-7) from comment #11)
> Also, this bug needs one additional patch -- to remove the entire
> mAllocations mechanism from nsStyleContext.

Er, that was bug 1026345, so never mind.

One other note though -- it's worth mentioning in the commit message for what's currently part 3 that the change to SpecifiedCalcToComputedCalc means that computed calc() values are now stored in the rule tree.  (That's a pretty substantive memory/performance change.)

	Cameron McCormack (:heycam) Assignee
	Comment 13 • 10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #6)
> r=dbaron, although I'm not sure this portable yet.  (If it is, we should
> definitely start using it more, though.)

What do we define portability as?  If it's whether it works with the compilers we're using on our build infrastructure, then it is.  It's also listed in the don't-trust-this-too-much https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code wiki page.

(The problems I had recently with VS2010 and the changes to nsChangeHint.h was to do with 64-bit sized enums.)

	Nathan Froyd [:froydnj]
	Comment 14 • 10 years ago

Comment on attachment 8441249 [details] [diff] [review]
Part 1: Make nsStyleUnit a uint8_t-sized enum so we can use it on nsStyle{Sides,Corners}.

Review of attachment 8441249 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleCoord.h
@@ +11,4 @@
>  #include "nsCoord.h"
>  #include "nsStyleConsts.h"
>  
> +enum nsStyleUnit : uint8_t {

The portable alternative here is MOZ_ENUM_TYPE, though that severely pessimizes code on compilers that don't support MOZ_ENUM_TYPE, since nsStyleUnit will be significantly larger.

Theoretically, the syntax you've used ought to be supported everywhere, but see bug 952785 for adventures in trying to remove MOZ_ENUM_TYPE.  Clearly *something* doesn't quite work right...

	David Baron :dbaron:
	Comment 15 • 10 years ago

(In reply to Cameron McCormack (:heycam) from comment #13)
> (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from
> comment #6)
> > r=dbaron, although I'm not sure this portable yet.  (If it is, we should
> > definitely start using it more, though.)
> 
> What do we define portability as?  If it's whether it works with the
> compilers we're using on our build infrastructure, then it is.

Yeah, that's the definition I was concerned about.

	Cameron McCormack (:heycam) Assignee
	Comment 16 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=93f06e5e9a78

	Cameron McCormack (:heycam) Assignee
	Comment 17 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca674bb5687
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab1e552bcb90
https://hg.mozilla.org/integration/mozilla-inbound/rev/a58c76857f5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/27728193e4b4

	Ed Morley [:emorley]
	Comment 18 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/7ca674bb5687
https://hg.mozilla.org/mozilla-central/rev/ab1e552bcb90
https://hg.mozilla.org/mozilla-central/rev/a58c76857f5b
https://hg.mozilla.org/mozilla-central/rev/27728193e4b4