Bug 106003

Summary: [CSS Regions] Add tests for widows and orphans
Product: WebKit Reporter: Andrei Bucur <abucur>
Component: WebCore Misc.Assignee: Andrei Bucur <abucur>
Status: RESOLVED FIXED    
Severity: Normal CC: dino, WebkitBugTracker, webkit.review.bot
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
none
Patch none

Description Andrei Bucur 2013-01-03 00:43:16 PST
There are no regions tests for the widows/orphans feature that landed in http://trac.webkit.org/changeset/137200 .
Comment 1 Andrei Bucur 2013-01-03 07:35:07 PST
Created attachment 181174 [details]
Patch
Comment 2 Tony Chang 2013-01-03 09:48:07 PST
Comment on attachment 181174 [details]
Patch

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

> LayoutTests/fast/regions/regions-widows-and-orphans.html:45
> +    for (var i = 0; i < 3; i++) {

Nit: ++i

> LayoutTests/fast/regions/regions-widows-and-orphans.html:65
> +    for (var i = 1; i <= blocks.length; i++) {

Nit: ++i

> LayoutTests/fast/regions/regions-widows-and-orphans.html:69
> +        for (var j = 1; j <= numLines; j++) {

Nit: ++j

> LayoutTests/fast/regions/regions-widows-and-orphans.html:104
> +function log(msg) {
> +    if (!results)

Nit: I would have used js-test-pre.js instead of implementing my own logPass and logFail functions.

> LayoutTests/fast/regions/regions-widows-and-orphans.html:115
> +        var topOfContainer = document.getElementById(containerId).getBoundingClientRect().top;
> +    var topOfLine = document.getElementById(containerId + "-block-" + blockNumber + "-line-" + lineNumber).getBoundingClientRect().top;

Nit: Weird indent.

> LayoutTests/fast/regions/regions-widows-and-orphans.html:210
> +    testRunner.waitUntilDone();

Is this needed? The test harness should wait until the onload handler finishes running before dumping results.
Comment 3 Andrei Bucur 2013-01-03 09:54:51 PST
(In reply to comment #2)
> (From update of attachment 181174 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181174&action=review
> 
> > LayoutTests/fast/regions/regions-widows-and-orphans.html:45
> > +    for (var i = 0; i < 3; i++) {
> 
> Nit: ++i
> 
> > LayoutTests/fast/regions/regions-widows-and-orphans.html:65
> > +    for (var i = 1; i <= blocks.length; i++) {
> 
> Nit: ++i
> 
> > LayoutTests/fast/regions/regions-widows-and-orphans.html:69
> > +        for (var j = 1; j <= numLines; j++) {
> 
> Nit: ++j
> 
> > LayoutTests/fast/regions/regions-widows-and-orphans.html:104
> > +function log(msg) {
> > +    if (!results)
> 
> Nit: I would have used js-test-pre.js instead of implementing my own logPass and logFail functions.
> 
> > LayoutTests/fast/regions/regions-widows-and-orphans.html:115
> > +        var topOfContainer = document.getElementById(containerId).getBoundingClientRect().top;
> > +    var topOfLine = document.getElementById(containerId + "-block-" + blockNumber + "-line-" + lineNumber).getBoundingClientRect().top;
> 
> Nit: Weird indent.
> 
> > LayoutTests/fast/regions/regions-widows-and-orphans.html:210
> > +    testRunner.waitUntilDone();
> 
> Is this needed? The test harness should wait until the onload handler finishes running before dumping results.

Thx for the review! Will fix right away.
Comment 4 Andrei Bucur 2013-01-18 06:19:39 PST
Created attachment 183439 [details]
Patch
Comment 5 WebKit Review Bot 2013-01-18 06:42:43 PST
Comment on attachment 183439 [details]
Patch

Clearing flags on attachment: 183439

Committed r140139: <http://trac.webkit.org/changeset/140139>
Comment 6 WebKit Review Bot 2013-01-18 06:42:46 PST
All reviewed patches have been landed.  Closing bug.