Bug 131023

Summary:

[CSS Grid Layout] Clamping the number of repetitions in repeat()

Product:

WebKit

Reporter:

Javier Fernandez <jfernandez>

Component:

Layout and Rendering

Assignee:

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:

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.