1287951 - stylo: Implement restyle hints

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez (:emilio)]

This allows an actual decent way of doing incremental restyling the same way Servo does.

I know there are a lot of optimizations possible, but I'd want to get a bit of feedback before.

Relevant Servo PR is in the URL field of the bug.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1287951: stylo: Add basic support for taking element Snapshots

Review commit: https://reviewboard.mozilla.org/r/65366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65366/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1287951: stylo: Add support for computing style hints from Servo.

Review commit: https://reviewboard.mozilla.org/r/65368/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65368/

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1287951: stylo: Implement ServoRestyleManager::AttributeWillChange.

Review commit: https://reviewboard.mozilla.org/r/65370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65370/

Comment 4

[Cameron McCormack (:heycam)]

Comment on attachment 8772611 [details]
Bug 1287951: stylo: Add basic support for taking element Snapshots

https://reviewboard.mozilla.org/r/65366/#review62438

::: dom/events/EventStates.h:162
(Diff revision 1)
>    InternalType GetInternalValue() const {
>      return mStates;
>    }
>  
> +  /**
> +   * Method used to get the appropriate state representation for servo.

Nit: "Servo" (which is the official capitalization, AIUI).

::: layout/base/ServoRestyleManager.h:73
(Diff revision 1)
> -    if (MOZ_UNLIKELY(IsDisconnected())) {
> -      return false;
> +    // XXX IsEmpty isn't public here, consider implementing it in
> +    // nsClassHashTable?

Sounds good.  (Don't need the comment in here, though.)

::: layout/base/ServoRestyleManager.h:106
(Diff revision 1)
>     * Propagates the IS_DIRTY flag down to the tree, setting
>     * HAS_DIRTY_DESCENDANTS appropriately.
>     */
> -  static void DirtyTree(nsIContent* aContent);
> +  static void DirtyTree(nsIContent* aContent, bool aIncludingRoot = true);
> +
> +  static void NoteRestyleHint(Element* aElement, nsRestyleHint aRestyleHint);

Can you add a comment on this explaning what it does?

::: layout/base/ServoRestyleManager.h:115
(Diff revision 1)
> +  void AddElementSnapshot(Element* aElement,
> +                          ServoElementSnapshot::Flags aWhatToCapture);
> +
> +  void AddElementSnapshot(Element* aElement,
> +                          ServoElementSnapshot::Flags aWhatToCapture,
> +                          nsRestyleHint aRestyleHint,
> +                          nsChangeHint aMinChangeHint);

And on these?  (Or one of them, that describes both?)

::: layout/base/ServoRestyleManager.cpp:20
(Diff revision 1)
>    : RestyleManagerBase(aPresContext)
>  {
>  }
>  
>  /* static */ void
> -ServoRestyleManager::DirtyTree(nsIContent* aContent)
> +ServoRestyleManager::DirtyTree(nsIContent* aContent, bool aIncludingRoot)

Ignoring issues with needing to use FlattenedChildIterator (and we'll need to figure that out on the Servo side at some point, so that we do restyle into shadow trees), I wonder if this DirtyTree work should be done by Servo so we can set all these bits in parallel.

(I still also wonder whether we can move to a model where we don't have to traverse down the tree setting dirty bits, but instead just use the fact that we have particular restyle hints to know whether we need to keep traversing down the tree to restyle, which is what Gecko does.)

::: layout/base/ServoRestyleManager.cpp:71
(Diff revision 1)
> +    // Note that we could have been called just after adding an element to the
> +    // table, for example.
> +    //
> +    // Other way to structure this would be to duplicate the logic in
> +    // AddElementSnapshot, at the (very minor) cost of possibly doing things
> +    // twice.
> +    needsRestyle = HasPendingRestyles();

Can you explain why we need this branch (i.e. why we need to do something when aRestyleHint and aMinChangeHint are both 0)?  If HasPendingRestyles() is true, won't we already have observed the refresh driver on the previous PostRestyleEvent, and so we can just return early?

::: layout/base/ServoRestyleManager.cpp:175
(Diff revision 1)
> +    // XXX Self must imply Subtree, at least for Servo, because of style
> +    // structs inheritance. Would that be taken care by the SetStyleContext
> +    // call?

Self implies Subtree in as much as it means descendants must be restyled, but it doesn't mean the same thing as the Subtree restyle hint, which means "re-run selector matching on descendants".

SetStyleContext would be too late, with our current setup of having a second pass over the tree to generate nsStyleContexts which we then pass to nsIFrame::SetStyleContext.

::: layout/base/ServoRestyleManager.cpp:179
(Diff revision 1)
> +
> +    // XXX Self must imply Subtree, at least for Servo, because of style
> +    // structs inheritance. Would that be taken care by the SetStyleContext
> +    // call?
> +    aHint |= eRestyle_Subtree;
> +    // MarkParentsAsHavingDirtyDescendants(aElement);

Remove this?

::: layout/base/ServoRestyleManager.cpp:195
(Diff revision 1)
> +        DirtyTree(cur->AsContent(), /* aIncludingRoot = */ true);
> +      }
> +    }
> +  }
> +
> +  // TODO: detect restyle for animations/transitions/etc, and act properly.

Nit: maybe reword that comment to say "Handle all other nsRestyleHint values".

::: layout/base/ServoRestyleManager.cpp:217
(Diff revision 1)
> +      // TODO: avoid this if we already have the highest restyle hint in the
> +      // subtree.

Avoid what?  I think you mean avoid the NoteRestyleHint call, or maybe the ComputeRestyleHint call, depending on what the value of snapshot->RestyleHint() is.  If so, make the comment a little more explicit.

::: layout/base/ServoRestyleManager.cpp:221
(Diff revision 1)
> +
> +      // TODO: avoid this if we already have the highest restyle hint in the
> +      // subtree.
> +      nsRestyleHint hint = styleSet->ComputeRestyleHint(element, snapshot);
> +      hint = hint | snapshot->RestyleHint();
> +      // nsRestyleHint hint = eRestyle_Self;

Remove this?

::: layout/base/ServoRestyleManager.cpp:341
(Diff revision 1)
> +  ServoElementSnapshot* existingSnapshot = mModifiedElements.LookupOrAdd(aElement);
> +  MOZ_ASSERT(existingSnapshot);

Nit: given we might create a new entry here, maybe just "snapshot" rather than "existingSnapshot" would be a better name.

::: layout/base/ServoRestyleManager.cpp:342
(Diff revision 1)
> +                                        ServoElementSnapshot::Flags aWhatToCapture,
> +                                        nsRestyleHint aRestyleHint,
> +                                        nsChangeHint aMinChangeHint)
> +{
> +  ServoElementSnapshot* existingSnapshot = mModifiedElements.LookupOrAdd(aElement);
> +  MOZ_ASSERT(existingSnapshot);

LookOrAdd is infallible, so no need to call MOZ_ASSERT.

::: layout/style/ServoBindings.h:259
(Diff revision 1)
> +uint32_t Servo_StyleWorkerThreadCount();
> +

This crept in from rebasing again. :-)

::: layout/style/ServoElementSnapshot.h:30
(Diff revision 1)
> +  explicit ServoAttrSnapshot(const nsAttrName& aName,
> +                             const nsAttrValue& aValue)

No need for explicit on a constructor that can't take one argument.  (Well, in C++11 apparently it prevents you from converting initializer lists into constructor calls, but that's not a concern here.)

::: layout/style/ServoElementSnapshot.h:46
(Diff revision 1)
> + * This means the attributes, and the element state, such as :hover, :active,
> + * etc...
> + */
> +class ServoElementSnapshot
> +{
> +  typedef mozilla::dom::Element Element;

Nit: just "dom::Element" since we're in mozilla.

::: layout/style/ServoElementSnapshot.h:53
(Diff revision 1)
> +public:
> +  /**
> +   * A bitflags enum class used to determine what data does a ServoElementSnapshot
> +   * contain, if either only State, only Attributes, or everything.
> +   */
> +  class Flags {

Rather than defining this Flags class, can we use an enum class and then the MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATOR macro from TypedEnumBits.h to defined the operators?

::: layout/style/ServoElementSnapshot.h:114
(Diff revision 1)
> +  explicit ServoElementSnapshot(Element* aElement,
> +                                Flags aWhatToCapture);

No need for explicit.

::: layout/style/ServoElementSnapshot.h:120
(Diff revision 1)
> +                                Flags aWhatToCapture);
> +
> +  /**
> +   * Empty snapshot, with no data at all.
> +   */
> +  explicit ServoElementSnapshot()

Or here.

::: layout/style/ServoElementSnapshot.h:147
(Diff revision 1)
> +  /**
> +   * Captures the given element attributes (if not previously captured).
> +   *
> +   * Equivalent to call Add(aElement, Flags::Attributes).
> +   */

The comment says that it snapshots the attributes if not previously captured, but the function definition doesn't seem to check whether we have attributes captured already.  Will this cause multiple attribute changes on the same element between processing restyles to break?

::: layout/style/ServoElementSnapshot.h:163
(Diff revision 1)
> +  // TODO: Profile, a 1 or 2 element AutoTArray could be worth it, given we know
> +  // we're dealing with attribute changes when we take snapshots of attributes,
> +  // though it can be wasted space if we deal with a lot of state-only
> +  // snapshots.

My intuition is that it won't matter, but we can always measure later.  (And space isn't such a big concern since these are short lived objects.)

::: layout/style/ServoElementSnapshot.h:174
(Diff revision 1)
> +  ServoStateType mState;
> +  nsRestyleHint mExplicitRestyleHint;
> +  nsChangeHint mExplicitChangeHint;
> +};
> +
> +} // namespace mozilla

Nit: blank line after this line.

::: layout/style/ServoElementSnapshot.cpp:6
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "mozilla/ServoElementSnapshot.h"

Nit: blank line before this line.

::: layout/style/ServoElementSnapshot.cpp:13
(Diff revision 1)
> +
> +namespace mozilla {
> +
> +ServoElementSnapshot::ServoElementSnapshot(Element* aElement,
> +                                           Flags aWhatToCapture)
> +  : mContains(Flags(0))

Initialize mState, mExplicitRestyleHint and mExplicitChangeHint here too?  (They won't be zero initialized.)

::: layout/style/ServoElementSnapshot.cpp:23
(Diff revision 1)
> +  Add(aElement, aWhatToCapture, nsRestyleHint(0), nsChangeHint(0));
> +
> +  MOZ_ASSERT(mContains == aWhatToCapture, "What happened here?");
> +}
> +
> +void ServoElementSnapshot::Add(Element* aElement,

Nit: newline after "void".

::: layout/style/ServoElementSnapshot.cpp:44
(Diff revision 1)
> +  if (!HasAny(Flags::State)) {
> +    mState = aElement->StyleState().ServoValue();
> +    mContains |= Flags::State;
> +  }

Why do we check that we haven't recorded state here, but we don't do it in Add()?

::: layout/style/ServoElementSnapshot.cpp:57
(Diff revision 1)
> +    attrName = aElement->GetAttrNameAt(i);
> +    const nsAttrValue* attrValue =
> +      aElement->GetParsedAttr(attrName->LocalName(), attrName->NamespaceID());

(Hmm, there's no way to get attributes by index to iterate over them without looking them up by their name?)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/65366/#review62438

> Ignoring issues with needing to use FlattenedChildIterator (and we'll need to figure that out on the Servo side at some point, so that we do restyle into shadow trees), I wonder if this DirtyTree work should be done by Servo so we can set all these bits in parallel.
> 
> (I still also wonder whether we can move to a model where we don't have to traverse down the tree setting dirty bits, but instead just use the fact that we have particular restyle hints to know whether we need to keep traversing down the tree to restyle, which is what Gecko does.)

Yes, I planned to do that as a later effort on the servo side (basically be allowed to say in a work unit: force the restyle all the way up). Though I guess I can also do it as part of this patch.

> Can you explain why we need this branch (i.e. why we need to do something when aRestyleHint and aMinChangeHint are both 0)?  If HasPendingRestyles() is true, won't we already have observed the refresh driver on the previous PostRestyleEvent, and so we can just return early?

Sure, so, when I add some data to the snapshot, I later call `PostRestyleEvent`, with maybe an empty explicit change or restyle hint. We still need to do the appropriate stuff later, though it's probably better if I factor that out and make an explicit call.

> Avoid what?  I think you mean avoid the NoteRestyleHint call, or maybe the ComputeRestyleHint call, depending on what the value of snapshot->RestyleHint() is.  If so, make the comment a little more explicit.

Will do. I meant the ComputeRestyleHint call. If know we're not computing a restyle hint that requires more effort than the explicit one, it seems appropriate to skip that.

> This crept in from rebasing again. :-)

Grr... I need to start looking at m-c instead of trying to remember what is and what not in rebases :)

> No need for explicit on a constructor that can't take one argument.  (Well, in C++11 apparently it prevents you from converting initializer lists into constructor calls, but that's not a concern here.)

I just usually prefer to do it, but well, you're right.

> (Hmm, there's no way to get attributes by index to iterate over them without looking them up by their name?)

I'll double-check.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/65366/#review62438

> And on these?  (Or one of them, that describes both?)

These go away in the following commits.

> Remove this?

That went away in the following commits.

> I'll double-check.

It doesn't seem to be: http://searchfox.org/mozilla-central/search?q=GetAttrNameAt&path=, the most similar thing is GetAttrInfo, and it seems easy to implement GetAttrInfoAt(uint32_t index).

Does that sound sensible?

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8772612 [details]
Bug 1287951: stylo: Add support for computing style hints from Servo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65368/diff/1-2/

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8772613 [details]
Bug 1287951: stylo: Implement ServoRestyleManager::AttributeWillChange.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65370/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1287951: stylo: Know if a snapshot belongs to a HTML element in an HTML document at construction time.

Review commit: https://reviewboard.mozilla.org/r/65702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65702/

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1287951: stylo: Implement ServoRestyleManager::AttributeWillChange.

Review commit: https://reviewboard.mozilla.org/r/65704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65704/

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Sorry for the noise, but mozreview didn't like the squash of the two first commits (I did that so there were less things going away).

Pending things are:

 1 - Avoid dirtying the tree all the way down.
 2 - Add an efficient way of iterating over arguments, that btw can be also reused by the normal attribute matching code.

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> It doesn't seem to be:
> http://searchfox.org/mozilla-central/search?q=GetAttrNameAt&path=, the most
> similar thing is GetAttrInfo, and it seems easy to implement
> GetAttrInfoAt(uint32_t index).
> 
> Does that sound sensible?

bz suggested on IRC a GetAttrValueAt, which should be fine too.  So feel free to file a bug to add that, and also have a look at the existing callers of GetAttrNameAt, most of which probably can be simplified to use this new GetAttrValueAt call too, to get the values.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8773062 [details]
Bug 1287951: stylo: Know if a snapshot belongs to a HTML element in an HTML document at construction time.

https://reviewboard.mozilla.org/r/65702/#review62778

r=me with these things addressed, assuming bholley was happy with the dirty bits approach after your discussion on IRC.

::: layout/base/ServoRestyleManager.h:107
(Diff revision 1)
>     * HAS_DIRTY_DESCENDANTS appropriately.
>     */
> -  static void DirtyTree(nsIContent* aContent);
> +  static void DirtyTree(nsIContent* aContent, bool aIncludingRoot = true);
> +
> +  /**
> +   * Marks the tree with the appropriate flags for the given restyle hint.

Maybe "dirty flags"?

::: layout/base/ServoRestyleManager.cpp:56
(Diff revision 1)
> -  if (aRestyleHint == 0 && !aMinChangeHint) {
> -    // Nothing to do here
> +  // NOTE: We defer the processing of restyle/change hints until
> +  // ProcessPendingRestyles.

So, just to be really nit-picky: this comment makes it sound like this is something out of the ordinary, but it actually matches the times when Gecko handles these things.  If the note is to point out the differences from Servo, maybe say that: "Note that unlike in Servo, we don't mark elements as dirty until we process the restyle hints in ProcessPendingRestyles."  The processing of change hints must be at that point -- it doesn't make sense to process them before we restyle the elements -- so probably don't need mentioning.

::: layout/base/ServoRestyleManager.cpp:65
(Diff revision 1)
> +    // Note that we could have been called just after adding an element to the
> +    // modified elements hashmap, for example.
> +    //
> +    // Other way to structure this would be to duplicate the logic in
> +    // AddElementSnapshot, at the (very minor) cost of possibly doing things
> +    // twice.
> +    needsRestyle = HasPendingRestyles();

Now that I understand why we get in here with a 0 restyle hint, I think this way (PostRestyleEvent being in charge of adding the refresh driver observer, if we do have something in mModifiedElements) is fine.  So I think we can remove the second part of the comment about the alternative approach.

(Although it might be clearer to just return early if aRestyleHint and aMinChangeHint are both 0 and there is nothing in mModifiedElements, and then we don't need needsRestyle.  Up to you.)

::: layout/base/ServoRestyleManager.cpp:168
(Diff revision 1)
> +ServoRestyleManager::NoteRestyleHint(Element* aElement, nsRestyleHint aHint)
> +{
> +  if (aHint & eRestyle_Self) {
> +    aElement->SetIsDirtyForServo();
> +    MarkParentsAsHavingDirtyDescendants(aElement);
> +    aHint |= eRestyle_Subtree;

I think it might be worth keeping a comment in here about why we add eRestyle_Subtree, since readers of this code who are familiar with Gecko's use of restyle hints may be confused about why we are seemingly causing more work to be done than necessary.

::: layout/base/ServoRestyleManager.cpp:184
(Diff revision 1)
> +        DirtyTree(cur->AsContent(), /* aIncludingRoot = */ true);
> +      }
> +    }
> +  }
> +
> +  // TODO: Handle all other nsRestyleHint values.

Maybe NS_ERROR("stylo: ...") in here if we have a hint we don't handle?

::: layout/base/ServoRestyleManager.cpp:323
(Diff revision 1)
>  
> +ServoElementSnapshot*
> +ServoRestyleManager::SnapshotForElement(Element* aElement)
> +{
> +  ServoElementSnapshot* snapshot = mModifiedElements.LookupOrAdd(aElement);
> +  if (!snapshot->HasAny(ServoElementSnapshot::Flags::HTMLElementInHTMLDocument)) {

Is there a reason we need to handle snapshots that don't have the mIsHTMLElementInHTMLDocument flag set accurately?  If every snapshot has that flag set when it is created, I wonder what the benefit is of recording whether it has been set.

::: layout/style/ServoBindings.h:37
(Diff revision 1)
>  using mozilla::FontFamilyList;
>  using mozilla::FontFamilyType;
>  using mozilla::dom::Element;
> +using mozilla::ServoElementSnapshot;

(The bindgen handles namespaced types now, so we should be able to remove these |using| statements and instead just use the fully qualified names in the function prototypes below, yes?  If so can you file a followup to do that so that we don't pollute the namespace of files that #include "ServoBindings.h".)

::: layout/style/ServoBindings.h:109
(Diff revision 1)
>  nsIAtom* Gecko_LocalName(RawGeckoElement* element);
>  nsIAtom* Gecko_Namespace(RawGeckoElement* element);
>  nsIAtom* Gecko_GetElementId(RawGeckoElement* element);
>  
>  // Attributes.
> -bool Gecko_HasAttr(RawGeckoElement* element, nsIAtom* ns, nsIAtom* name);
> +#define SERVO_DECLARE_ELEMENT_ATTR_MATCHING_FUNCTIONS(prefix_, implementor_)   \

Nit: I think it's fine to drop the blank lines between the function declarations, and Gecko style is (probably) to not have spaces around the "##" token pasting operator.

Also, please #undef the macro after using it.

::: layout/style/ServoBindings.cpp:106
(Diff revision 1)
>  Gecko_GetDocumentElement(RawGeckoDocument* aDoc)
>  {
>    return aDoc->GetDocumentElement();
>  }
>  
> -uint8_t
> +EventStates::ServoType

Did the corresponding ServoBindings.h change get lost?

::: layout/style/ServoBindings.cpp:197
(Diff revision 1)
> +    return nullptr;
> +  }
> +  return attr->GetServoCSSDeclarationValue();
> +}
> +
> +namespace attribute_matching {

I don't think it's common to use namespaces to group functions like this in Gecko.  The function names are unique enough within this file, so let's just make them static and remove the namespace.

::: layout/style/ServoBindings.cpp:402
(Diff revision 1)
>    return atomArray->Length();
>  }
>  
> -ServoDeclarationBlock*
> -Gecko_GetServoDeclarationBlock(RawGeckoElement* aElement)
> -{
> +} // namespace attribute_matching
> +
> +#define SERVO_IMPL_ELEMENT_ATTR_MATCHING_FUNCTION(prefix_, implementor_)       \

#undef the macro after using it.  (Otherwise it'll be visible to other .cpp files included in the same translation unit due to Gecko's unified builds.)

::: layout/style/ServoElementSnapshot.h:28
(Diff revision 1)
> +/**
> + * A structure representing a single attribute name and value.
> + *
> + * This is pretty similar to the private nsAttrAndChildArray::InternalAttr.
> + */
> +struct ServoAttrSnapshot {

Nit: newline before "{".

::: layout/style/ServoElementSnapshot.h:43
(Diff revision 1)
> +
> +/**
> + * A bitflags enum class used to determine what data does a ServoElementSnapshot
> + * contains.
> + */
> +enum class ServoElementSnapshotFlags : uint8_t {

Nit: here too.

::: layout/style/ServoElementSnapshot.h:115
(Diff revision 1)
> +  const nsAttrName* GetAttrNameAt(uint32_t aIndex) const {
> +    return &mAttrs[aIndex].mName;
> +  }

In DoMatch, we call GetAttrNameAt assuming that nullptr is returned if aIndex is out of range.  I assume we should do the same here too.

::: layout/style/ServoElementSnapshot.h:156
(Diff revision 1)
> +
> +  bool HasAny(Flags aFlags) {
> +    return bool(mContains & aFlags);
> +  }
> +private:
> +

Nit: put the newline before the "private:".

::: xpcom/glue/nsClassHashtable.h:69
(Diff revision 1)
> +
> +  /**
> +   * Returns whether this table is empty. Re-exported as public from
> +   * nsBaseHashtable, where it's protected.
> +   */
> +  bool IsEmpty() { return base_type::IsEmpty(); }

Please file a separate bug for this, and ask :froydnj for review.  (Might be better to use a |using| statement than define a forwarding function here, though.)

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8773063 [details]
Bug 1287951: stylo: Implement ServoRestyleManager::AttributeWillChange.

https://reviewboard.mozilla.org/r/65704/#review62814

Comment 15

[Emilio Cobos Álvarez (:emilio)]

https://reviewboard.mozilla.org/r/65702/#review62778

> Maybe NS_ERROR("stylo: ...") in here if we have a hint we don't handle?

Yep, thought about that, but since NS_ERROR and friends don't handle formatted arguments we can't get more info that "Unhandled restyle hint", which is quite unuseful.

I'll add it though.

> Is there a reason we need to handle snapshots that don't have the mIsHTMLElementInHTMLDocument flag set accurately?  If every snapshot has that flag set when it is created, I wonder what the benefit is of recording whether it has been set.

Basically avoid doing extra work reaching the document when it's already set.

Might be a premature optimisation though, but it did fit well and avoids the double check in element and document. It seems a really cheap check for Gecko though.

> (The bindgen handles namespaced types now, so we should be able to remove these |using| statements and instead just use the fully qualified names in the function prototypes below, yes?  If so can you file a followup to do that so that we don't pollute the namespace of files that #include "ServoBindings.h".)

Sure, I'm doing three followups for this bug it seems, the attribute parsing one, the nsClassHashTable one, and this one :P

> Did the corresponding ServoBindings.h change get lost?

Nope, it's intentional. Bindgen uses "ServoType" here that is not defined (and the way return values are returned in bindgen would be a bit difficult to fix it), but keeping the change in the cpp file we get an error if ServoType changes.

> In DoMatch, we call GetAttrNameAt assuming that nullptr is returned if aIndex is out of range.  I assume we should do the same here too.

Huh, I don't know how I didn't catch this during testing... Anyway, you're right, fixed.

> Please file a separate bug for this, and ask :froydnj for review.  (Might be better to use a |using| statement than define a forwarding function here, though.)

Ok, I'll revert these changes and file a followup.

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8773062 [details]
Bug 1287951: stylo: Know if a snapshot belongs to a HTML element in an HTML document at construction time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65702/diff/1-2/

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8773063 [details]
Bug 1287951: stylo: Implement ServoRestyleManager::AttributeWillChange.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65704/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5daa5c050331
stylo: Add support for computing style hints from Servo. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b04110c70f15
stylo: Implement ServoRestyleManager::AttributeWillChange. r=heycam

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> Yep, thought about that, but since NS_ERROR and friends don't handle
> formatted arguments we can't get more info that "Unhandled restyle hint",
> which is quite unuseful.
> 
> I'll add it though.

Yeah, I think it's useful to get some console spew to indicate something we don't handle yet.

FWIW if you want to use a formatted message in NS_ERROR (or MOZ_ASSERT, etc.) you can write:

  NS_ERROR(nsPrintfCString("stylo: Unhandled restyle hint %s",
                           RestyleManager::RestyleHintToString(aRestyleHint).get()).get());

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #15)
> > Is there a reason we need to handle snapshots that don't have the mIsHTMLElementInHTMLDocument flag set accurately?  If every snapshot has that flag set when it is created, I wonder what the benefit is of recording whether it has been set.
> 
> Basically avoid doing extra work reaching the document when it's already set.
> 
> Might be a premature optimisation though, but it did fit well and avoids the
> double check in element and document. It seems a really cheap check for
> Gecko though.

Every ServoElementSnapshot object that gets created immediately has mIsHTMLElementInHTMLDocument set accurately on it, in ServoRestyleManager::SnapshotForElement.  Can we just set it in the constructor, passing in the Element?  Something like this should work:

  auto entry = mModifiedElements.PutEntry(aElement);
  if (!entry->mData) {
    entry->mData = new ServoElementSnapshot(aElement);
  }

Because it seems like we don't need the IsHTMLElementInHTMLDocument flag at all.

> > Did the corresponding ServoBindings.h change get lost?
> 
> Nope, it's intentional. Bindgen uses "ServoType" here that is not defined
> (and the way return values are returned in bindgen would be a bit difficult
> to fix it), but keeping the change in the cpp file we get an error if
> ServoType changes.

OK.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5daa5c050331
https://hg.mozilla.org/mozilla-central/rev/b04110c70f15

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba8cc31b9cf0
followup: use nsClassHashTable's IsEmpty instead of Count. r=me

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba8cc31b9cf0

Comment 24

[Cameron McCormack (:heycam)]

Did you miss this (or disagree)?

(In reply to Cameron McCormack (:heycam) from comment #20)
> Every ServoElementSnapshot object that gets created immediately has
> mIsHTMLElementInHTMLDocument set accurately on it, in
> ServoRestyleManager::SnapshotForElement.  Can we just set it in the
> constructor, passing in the Element?  Something like this should work:
> 
>   auto entry = mModifiedElements.PutEntry(aElement);
>   if (!entry->mData) {
>     entry->mData = new ServoElementSnapshot(aElement);
>   }
> 
> Because it seems like we don't need the IsHTMLElementInHTMLDocument flag at
> all.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8773062 [details]
Bug 1287951: stylo: Know if a snapshot belongs to a HTML element in an HTML document at construction time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65702/diff/2-3/

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Whoops, PutEntry was private, so I needed to make it public, and forgot to fill the bug for that. Just filled 1290524

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e1f1a9c666d
followup: Know if a snapshot belongs to a HTML element in an HTML document at construction time. r=heycam

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0e1f1a9c666d