1295818 - Rewrite nsStyleChangeList to don't rewrite nsTArray.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

nsStyleChangeList is basically an nsTArray with a lot of manual memory management.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1295818: Port nsStyleChangeList to modern C++.

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

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8781777 [details]
Bug 1295818: Port nsStyleChangeList to modern C++.

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

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8781777 [details]
Bug 1295818: Port nsStyleChangeList to modern C++.

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

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1295818: Templatize nsTArray::RemoveElementsBy to avoid a spurious allocation.

mozilla::function involves an allocation of the FunctionImpl type, that we can
avoid rather easily in this case.

This commit is completely optional, I guess, though the code we're using
RemoveElementsBy with the previous patch is sort of hot.

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

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8781777 [details]
Bug 1295818: Port nsStyleChangeList to modern C++.

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

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Cameron, I'm aware that the change hints are processed in the reverse order now. After a quick look at the code it seems it was just done that way for convenience and shouldn't have any side effect, but let's see... That's revertible though.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=524d0e23e762

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8781893 [details]
Bug 1295818: Templatize nsTArray::RemoveElementsBy to avoid a spurious allocation.

https://reviewboard.mozilla.org/r/72216/#review69848

Comment 8

[Cameron McCormack (:heycam)]

Comment on attachment 8781777 [details]
Bug 1295818: Port nsStyleChangeList to modern C++.

https://reviewboard.mozilla.org/r/72112/#review70088

::: layout/base/nsStyleChangeList.h:24
(Diff revision 4)
>  class nsIContent;
>  
> -// XXX would all platforms support putting this inside the list?
>  struct nsStyleChangeData {
> -  nsIFrame*   mFrame;
> -  nsIContent* mContent;
> +  nsIFrame* mFrame; // weak
> +  RefPtr<nsIContent> mContent;

Use nsCOMPtr for interfaces.

::: layout/base/nsStyleChangeList.h:28
(Diff revision 4)
>  // Note:  nsStyleChangeList owns a reference to
>  //  nsIContent pointers in its list.

Can drop this comment now that it's obvious from the fact we're using a strong-reference-holding smart pointer for mContent.

::: layout/base/nsStyleChangeList.h:30
(Diff revision 4)
>  
> -static const uint32_t kStyleChangeBufferSize = 10;
> -
>  // Note:  nsStyleChangeList owns a reference to
>  //  nsIContent pointers in its list.
> -class nsStyleChangeList {
> +class nsStyleChangeList : public AutoTArray<nsStyleChangeData, 10> {

Nit: "{" on new line.


Overall, these changes make nsStyleChangeList a lot better.  I wonder whether we should inherit AutoTArray privately, and then only expose publicly the methods from it that consumers need, which I think would be:

  using AutoTArray<nsStyleChangeData, 10>::cbegin;
  using AutoTArray<nsStyleChangeData, 10>::cend;
  using AutoTArray<nsStyleChangeData, 10>::Clear;
  using AutoTArray<nsStyleChangeData, 10>::IsEmpty;

WDYT?

::: layout/base/nsStyleChangeList.h:32
(Diff revision 4)
>    nsStyleChangeList();
>    ~nsStyleChangeList();

Feel free to move the constructor / destructor bodies here, since they're now just the MOZ_COUNT_CTOR/DTOR calls.

::: layout/base/nsStyleChangeList.cpp:48
(Diff revision 4)
> -      for (int32_t index = mCount - 1; index >= 0; --index) {
> -        if (aContent == mArray[index].mContent) { // remove this change
> +      // NOTE: This is captured by reference to please static analysis.
> +      // Capturing it by value as a pointer is fine.

Does it trip the static analysis up to capture that specific variable by value?

  RemoveElementsBy([aContent](...

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f3d36db350d
Port nsStyleChangeList to modern C++. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/15405ca22e64
Templatize nsTArray::RemoveElementsBy to avoid a spurious allocation. r=froydnj

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8781777 [details]
Bug 1295818: Port nsStyleChangeList to modern C++.

https://reviewboard.mozilla.org/r/72112/#review70088

> Nit: "{" on new line.
> 
> 
> Overall, these changes make nsStyleChangeList a lot better.  I wonder whether we should inherit AutoTArray privately, and then only expose publicly the methods from it that consumers need, which I think would be:
> 
>   using AutoTArray<nsStyleChangeData, 10>::cbegin;
>   using AutoTArray<nsStyleChangeData, 10>::cend;
>   using AutoTArray<nsStyleChangeData, 10>::Clear;
>   using AutoTArray<nsStyleChangeData, 10>::IsEmpty;
> 
> WDYT?

Sounds good, just did that.

> Does it trip the static analysis up to capture that specific variable by value?
> 
>   RemoveElementsBy([aContent](...

Yep, that's how I wrote it the first time, but the static analyzer complained with: "error: Refcounted variable 'aContent' of type 'nsIContent' cannot be captured by a lambda".

Should I fill a bug about it?

Comment 11

[Cameron McCormack (:heycam)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #10)
> Yep, that's how I wrote it the first time, but the static analyzer
> complained with: "error: Refcounted variable 'aContent' of type 'nsIContent'
> cannot be captured by a lambda".
> 
> Should I fill a bug about it?

Well, it looks like it's designed to avoid people accidentally passing in raw pointers to lambdas that persist for a while (e.g. passed to NS_DispatchToMainThread) -- bug 1153304.  So I guess it'd be WONTFIXed if you did file it.  To me it is simple enough to see that the pointer won't persist beyond the nsTArray method call, so I'm fine with the reference workaround.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8f3d36db350d
https://hg.mozilla.org/mozilla-central/rev/15405ca22e64