1358007 - Use nsTArray for Gecko's -moz-border-*-colors storage

Status: RESOLVED INVALID

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Jeremy Chen ([e-mail])]

At present, we use double pointers to nsBorderColors to store -moz-border-*-colors. We should be able to use nsTArray instead.

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

At present, we use double pointers to nsBorderColors (as an 1-D Array)
to store -moz-border-*-colors. For the simplicity reason, we shall
just use nsTArray instead.

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

Comment 2

[Jeremy Chen ([e-mail])]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

Hi Ting-Yu, since you always provide great comments while I'm doing refactoring stuff, could you take a look at this and give me some feedbacks? Thanks.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

https://reviewboard.mozilla.org/r/131848/#review134720

::: layout/style/nsStyleStruct.h:1149
(Diff revision 1)
>  
>    nsChangeHint CalcDifference(const nsStyleBorder& aNewData) const;
>  
>    void EnsureBorderColors() {
> -    if (!mBorderColors) {
> -      mBorderColors = new nsBorderColors*[4];
> +    if (mBorderColors.IsEmpty()) {
> +      if (mBorderColors.AppendElements(4)) {

We use the magic number 4 in some of the mBorderColor code while some of them iterate by using `mozilla::Side`. 

Ideally, we should define `constexpr int eSideCount = 4;` for `Side` like `eCornerCount` [1] for `Corner`, and use it instead of 4. This could be done another followup though.

[1] http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/gfx/2d/Types.h#417

::: layout/style/nsStyleStruct.h:1268
(Diff revision 1)
>      }
>      return nullptr;
>    }
>  
>  public:
> -  nsBorderColors** mBorderColors;     // [reset] composite (stripe) colors
> +  nsTArray<nsBorderColors*> mBorderColors; // [reset] composite (stripe) colors

If I'm reading the code correctly, we don't always want to append elements to mBorderColors. We call `EnsureBorderColors()` to allocate needed elements to save memory, so it's OK to use `AppendElements()` in `EnsureBorderColors()` instead of using `AutoTArray<nsBorderColors*, 4>` here.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

f+ per comment 3.

Comment 5

[Jeremy Chen ([e-mail])]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

https://reviewboard.mozilla.org/r/131848/#review134720

> We use the magic number 4 in some of the mBorderColor code while some of them iterate by using `mozilla::Side`. 
> 
> Ideally, we should define `constexpr int eSideCount = 4;` for `Side` like `eCornerCount` [1] for `Corner`, and use it instead of 4. This could be done another followup though.
> 
> [1] http://searchfox.org/mozilla-central/rev/d8ac097a1de2544f0e51d186bc850662c5b7430a/gfx/2d/Types.h#417

Sounds good to me.

> If I'm reading the code correctly, we don't always want to append elements to mBorderColors. We call `EnsureBorderColors()` to allocate needed elements to save memory, so it's OK to use `AppendElements()` in `EnsureBorderColors()` instead of using `AutoTArray<nsBorderColors*, 4>` here.

Yeah, that's right. Thank you for the feedback. :-)

Comment 6

[Jeremy Chen ([e-mail])]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

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

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

https://reviewboard.mozilla.org/r/131848/#review134734

::: commit-message-20dff:3
(Diff revision 2)
> +Bug 1358007 - use nsTArray for -moz-border-*-colors storage.
> +
> +At present, we use double pointers to nsBorderColors (as an 1-D Array)

s/an/a/

::: commit-message-20dff:4
(Diff revision 2)
> +Bug 1358007 - use nsTArray for -moz-border-*-colors storage.
> +
> +At present, we use double pointers to nsBorderColors (as an 1-D Array)
> +to store -moz-border-*-colors. For the simplicity reason, we shall

"For reasons of simplicity" or just "For simplicity"

::: layout/painting/nsCSSRendering.cpp:2041
(Diff revision 2)
> -  if (aBorder.mBorderColors)
> +  if (!aBorder.mBorderColors.IsEmpty()) {
>      return false;
> +  }

Is the existing code here wrong?  Why isn't it checking whether aBorder.mBorderColors is null (or, in the new code, whether the array is empty), and returning true if so?

::: layout/style/nsStyleStruct.h:1268
(Diff revision 2)
>      }
>      return nullptr;
>    }
>  
>  public:
> -  nsBorderColors** mBorderColors;     // [reset] composite (stripe) colors
> +  nsTArray<nsBorderColors*> mBorderColors; // [reset] composite (stripe) colors

I think it would be useful to use an nsTArray for the list of border colors for each of the four sides, too, rather than have the manually managed linked list.

(As for the type of the outer array, another option would be to do something like:

  typedef mozilla::Array<nsTArray<nscolor>, eSideCount> BorderColors;
  UniquePtr<BorderColors> mBorderColors;

That would restrict us to either having mBorderColors be null, or to be a pointer to a fixed-length array of four elements.  And it would save us the space of the nsTArray's header.  But that doesn't really matter, so I think the nsTArray as you have it is fine.)

WDYT?

Comment 8

[Jeremy Chen ([e-mail])]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

https://reviewboard.mozilla.org/r/131848/#review134734

> Is the existing code here wrong?  Why isn't it checking whether aBorder.mBorderColors is null (or, in the new code, whether the array is empty), and returning true if so?

This should've been taken care of in the last line, no?

If aBorder.mBorderColors is null (or, in the new code, whether the array is empty), the existing logic will further check if all four sides are opaque. If all four sides are also opaque, then IsOpaqueBorder() will return true (see the last line of IsOpaqueBorder()).

> I think it would be useful to use an nsTArray for the list of border colors for each of the four sides, too, rather than have the manually managed linked list.
> 
> (As for the type of the outer array, another option would be to do something like:
> 
>   typedef mozilla::Array<nsTArray<nscolor>, eSideCount> BorderColors;
>   UniquePtr<BorderColors> mBorderColors;
> 
> That would restrict us to either having mBorderColors be null, or to be a pointer to a fixed-length array of four elements.  And it would save us the space of the nsTArray's header.  But that doesn't really matter, so I think the nsTArray as you have it is fine.)
> 
> WDYT?

I like your suggestion. Will fix/write patches accordingly. Thank you.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

https://reviewboard.mozilla.org/r/131848/#review134734

> This should've been taken care of in the last line, no?
> 
> If aBorder.mBorderColors is null (or, in the new code, whether the array is empty), the existing logic will further check if all four sides are opaque. If all four sides are also opaque, then IsOpaqueBorder() will return true (see the last line of IsOpaqueBorder()).

But if aBorder.mBorderColors is non-null, that means we might have some border colors specified.  I don't think we want to return early in that case, we want to continue and check to see if the colors are opaque.  If mBorderColors is null, then we know there are no border colors, and we can return early.

Comment 10

[Jeremy Chen ([e-mail])]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

https://reviewboard.mozilla.org/r/131848/#review134734

> But if aBorder.mBorderColors is non-null, that means we might have some border colors specified.  I don't think we want to return early in that case, we want to continue and check to see if the colors are opaque.  If mBorderColors is null, then we know there are no border colors, and we can return early.

Oh, I see. So, we might need to further check if there exists at least one -moz-border-*-bolors is non-opaque before return `false`.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8859872 [details]
Bug 1358007 - use nsTArray for -moz-border-*-colors storage.

https://reviewboard.mozilla.org/r/131848/#review135494

Cancelling review for now.

Comment 12

[Jeremy Chen ([e-mail])]

Busy with stylo, so unassign myself for now.

Comment 13

[j.j.]

-moz-border-*-colors was removed in bug 1429723.