Bug 131023 - [CSS Grid Layout] Clamping the number of repetitions in repeat()
Summary: [CSS Grid Layout] Clamping the number of repetitions in repeat()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2014-04-01 02:20 PDT by Javier Fernandez
Modified: 2014-05-01 10:49 PDT (History)
13 users (show)

See Also:


Attachments
Patch (4.33 KB, patch)
2014-04-25 04:29 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff
Patch (4.73 KB, patch)
2014-05-01 09:28 PDT, Javier Fernandez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Javier Fernandez 2014-04-01 02:20:19 PDT
The ED suggests now to be able to clamp the number of repetitions when using the repeat() function, taking precautions about excessive memory usage.

See the mailing list thread for more details:

  - http://lists.w3.org/Archives/Public/www-style/2014Mar/0553.html
Comment 1 Javier Fernandez 2014-04-25 04:29:12 PDT
Created attachment 230164 [details]
Patch
Comment 2 Brent Fulgham 2014-04-25 09:54:06 PDT
Comment on attachment 230164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230164&action=review

> Source/WebCore/css/CSSParser.cpp:4930
> +        repetitions = MAX_REPETITIONS;

Should there be any kind of feedback to the inspector when this happens?
Comment 3 Brent Fulgham 2014-04-25 09:57:13 PDT
Comment on attachment 230164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230164&action=review

This patch seems fine overall. Please make make the MAX_REPETITIONS name more descriptive when you land.

>> Source/WebCore/css/CSSParser.cpp:4930
>> +        repetitions = MAX_REPETITIONS;
> 
> Should there be any kind of feedback to the inspector when this happens?

Also: I think you should reference the specification or draft that specifies this limit, and call the variable "MAX_GRID_TRACK_REPETITIONS".
Comment 4 Javier Fernandez 2014-05-01 09:15:41 PDT
(In reply to comment #2)
> (From update of attachment 230164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230164&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:4930
> > +        repetitions = MAX_REPETITIONS;
> 
> Should there be any kind of feedback to the inspector when this happens?

umm, good question. I think the limit of 10000 is high enough to now be reached in real webs, so the clamping operation won't be very frequent. On the other hand, when using values higher than 10000 it wont be easy to realize the value was clamped watching at the computed value. So, I'm not sure whether informing the inspector is worth or not. 

Personally, I think that the case of clamping values is so rare that I think it's not worth to deal with the inspector.
Comment 5 Javier Fernandez 2014-05-01 09:28:34 PDT
Created attachment 230584 [details]
Patch

Besides applying suggested changes, I've modified the tests to do also some boundary testing.
Comment 6 Brent Fulgham 2014-05-01 10:19:16 PDT
Comment on attachment 230584 [details]
Patch

r=me
Comment 7 WebKit Commit Bot 2014-05-01 10:49:07 PDT
Comment on attachment 230584 [details]
Patch

Clearing flags on attachment: 230584

Committed r168108: <http://trac.webkit.org/changeset/168108>
Comment 8 WebKit Commit Bot 2014-05-01 10:49:13 PDT
All reviewed patches have been landed.  Closing bug.