Bug 131023

Summary: [CSS Grid Layout] Clamping the number of repetitions in repeat()
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, hyatt, jfernandez, kling, macpherson, menard, rego, svillar
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60731    
Attachments:
Description Flags
Patch
none
Patch none

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.