Closed Bug 1425954 Opened 2 years ago Closed 2 years ago

Grid chrome API incorrectly duplicates line names that appear both around and within a repeat auto declaration

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: bradwerth, Assigned: bradwerth)

References

Details

Attachments

(2 files)

The grid chrome API incorrectly reports line names in some cases. A template like:

grid-template-columns: [a b] repeat(auto-fit, [b] 10px [c]) [d]

will incorrectly report names of ("a", "b", "b") for line 2. Line 2 should have names ("a", "b").
This doesn't need to be blocked, and https://bugzilla.mozilla.org/show_bug.cgi?id=1423378#c9 proposes we solve this problem independently.
No longer depends on: 1423378
Attachment #8937823 - Flags: review?(mats)
Attachment #8937824 - Flags: review?(mats)
Comment on attachment 8937823 [details]
Bug 1425954 Part 1: Strip out duplicate names when reporting line names via grid chrome API.

https://reviewboard.mozilla.org/r/208518/#review214338

::: dom/grid/GridLines.cpp:65
(Diff revision 1)
> +void AddLineNameIfNotPresent(nsTArray<nsString>& aLineNames,
> +                             const nsString& aName)

This function should be declared static unless you plan to use it in other files.

::: dom/grid/GridLines.cpp:73
(Diff revision 1)
> +  if (!aLineNames.Contains(aName)) {
> +    aLineNames.AppendElement(aName);
> +  }
> +}
> +
> +void AddLineNamesIfNotPresent(nsTArray<nsString>& aLineNames,

ditto

::: dom/grid/GridLines.cpp:77
(Diff revision 1)
> +    if (!aLineNames.Contains(name)) {
> +      aLineNames.AppendElement(name);
> +    }

nit: calling AddLineNameIfNotPresent instead is slightly more readable; I'm pretty sure the compiler can inline it to generate identical code

::: dom/grid/GridLines.cpp:147
(Diff revision 1)
> -      lineNames = aLineInfo->mNames.SafeElementAt(i, nsTArray<nsString>());
> +      for (const auto& name : possiblyDuplicateLineNames) {
> +        AddLineNameIfNotPresent(lineNames, name);
> +      }

nit: this can be replaced with
"AddLineNamesIfNotPresent(lineNames, possiblyDuplicateLineNames)", right?
Attachment #8937823 - Flags: review?(mats) → review+
Comment on attachment 8937824 [details]
Bug 1425954 Part 2: Update grid tests to check reporting of duplicated line names.

https://reviewboard.mozilla.org/r/208520/#review214342
Attachment #8937824 - Flags: review?(mats) → review+
Comment on attachment 8937823 [details]
Bug 1425954 Part 1: Strip out duplicate names when reporting line names via grid chrome API.

https://reviewboard.mozilla.org/r/208518/#review214338

> nit: this can be replaced with
> "AddLineNamesIfNotPresent(lineNames, possiblyDuplicateLineNames)", right?

This construction is necessary to eliminate the duplicate names in possiblyDuplicateLineNames. It is an O(n^2) operation, and this is the outer loop. I've added an additional commment to make clear that this is intentional.
Pushed by bwerth@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7c248344b08b
Part 1: Strip out duplicate names when reporting line names via grid chrome API. r=mats
https://hg.mozilla.org/integration/autoland/rev/3b489b994f6d
Part 2: Update grid tests to check reporting of duplicated line names. r=mats
> This construction is necessary to eliminate the duplicate names in possiblyDuplicateLineNames.

But AddLineNamesIfNotPresent adds one name at a time and check Contains
before adding them so it will filter out any duplicates in
possiblyDuplicateLineNames, right?
(In reply to Mats Palmgren (:mats) from comment #10)
> > This construction is necessary to eliminate the duplicate names in possiblyDuplicateLineNames.
> 
> But AddLineNamesIfNotPresent adds one name at a time and check Contains
> before adding them so it will filter out any duplicates in
> possiblyDuplicateLineNames, right?

Ugh. Yes, you are of course correct. I should have thought it through more thoroughly before responding. I'll clean it up in Bug 1423378 as I rebase those changes on top of these.
Blocks: 1423378
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/7c248344b08b
https://hg.mozilla.org/mozilla-central/rev/3b489b994f6d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.