Bug 1360508: Allow fixups on text styles to be reflected. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 28 Apr 2017 15:03:05 +0200
changeset 570171 cdbddf16aa9816371488e47e8139147b859b8d5c
parent 570170 1d61bb9fa88a0cb3c4c3e7fc5db6ec86ea9c5286
child 626422 f22d037f2df3fda65ea0ff7cf7650cf1e1b3ccc7
push id56414
push userbmo:emilio+bugs@crisal.io
push dateFri, 28 Apr 2017 13:28:07 +0000
reviewersheycam
bugs1360508
milestone55.0a1
Bug 1360508: Allow fixups on text styles to be reflected. r?heycam MozReview-Commit-ID: Eh6shYiv4RC
layout/style/nsStyleContext.h
--- a/layout/style/nsStyleContext.h
+++ b/layout/style/nsStyleContext.h
@@ -626,59 +626,41 @@ private:
             GetStyle##name_<aComputeData>(this, mBits);                 \
         /* always cache inherited data on the style context; the rule */\
         /* node set the bit in mBits for us if needed. */               \
         mCachedInheritedData.mStyleStructs[eStyleStruct_##name_] =      \
           const_cast<nsStyle##name_ *>(newData);                        \
         return newData;                                                 \
       }                                                                 \
       /**                                                               \
-       * Always forward to the parent to grab the inherited style struct\
-       * we're a text node.                                             \
+       * Also (conservatively) set the owning bit in the parent style   \
+       * context if we're a text node.                                  \
        *                                                                \
        * This causes the parent element's style context to cache any    \
        * inherited structs we request for a text node, which means we   \
        * don't have to compute change hints for the text node, as       \
        * handling the change on the parent element is sufficient.       \
        *                                                                \
-       * Note that adding the inherit bit is ok, because the struct     \
-       * pointer returned by the parent and the child is owned by       \
-       * Servo. This is fine if the pointers are the same (as it        \
-       * should, read below), because both style context sources will   \
-       * hold it.                                                       \
+       * Note, however, that we still need to request the style struct  \
+       * of the text node itself, since we may run some fixups on it,   \
+       * like for text-combine.                                         \
        *                                                                \
-       * In the case of a mishandled frame, we could end up with the    \
-       * pointer to and old parent style, but that's fine too, since    \
-       * the parent style context will remain alive until we reframe,   \
-       * in which case we'll discard both style contexts. Also, we      \
-       * hold a strong reference to the parent style context, which     \
-       * makes it a non-issue.                                          \
-       *                                                                \
-       * Also, note that the assertion below should be true, except     \
-       * for those frames we still don't handle correctly, like         \
-       * anonymous table wrappers, in which case the pointers will      \
-       * differ.                                                        \
+       * This model is sound because for the fixed-up values to change, \
+       * other properties on the parent need to change too, and we'll   \
+       * handle those change hints correctly.                           \
        *                                                                \
-       * That means we're not going to restyle correctly text frames    \
-       * of anonymous table wrappers, for example. It's kind of         \
-       * embarrassing, but I think it's not worth it to add more        \
-       * logic here unconditionally, given that's going to be fixed.    \
-       *                                                                \
-       * TODO(emilio): Convert to a strong assertion once we support    \
-       * all kinds of random frames. In fact, this can be a great       \
-       * assertion to debug them.                                       \
+       * TODO(emilio): Perhaps we should remove those fixups and handle \
+       * those in layout instead. Those fixups are kind of expensive    \
+       * for style sharing, and computed style of text nodes is not     \
+       * observable. If we do that, we could assert here that the       \
+       * inherited structs of both are the same.                        \
        */                                                               \
-      if (mPseudoTag == nsCSSAnonBoxes::mozText) {                      \
+      if (mPseudoTag == nsCSSAnonBoxes::mozText && aComputeData) {      \
         MOZ_ASSERT(mParent);                                            \
-        const nsStyle##name_* data =                                    \
-          mParent->DoGetStyle##name_<aComputeData>();                   \
-        NS_WARNING_ASSERTION(!data ||                                   \
-          data == Servo_GetStyle##name_(mSource.AsServoComputedValues()), \
-          "bad data");                                                  \
-        return data;                                                    \
+        mParent->AddStyleBit(NS_STYLE_INHERIT_BIT(name_));              \
       }                                                                 \
                                                                         \
       const bool needToCompute = !(mBits & NS_STYLE_INHERIT_BIT(name_));\
       if (!aComputeData && needToCompute) {                             \
         return nullptr;                                                 \
       }                                                                 \
                                                                         \
       const nsStyle##name_* data =                                      \