Bug 126427

Summary: [CSS Regions] Add performance tests for selection with mixed content
Product: WebKit Reporter: Manuel Rego Casasnovas <rego>
Component: Tools / TestsAssignee: Manuel Rego Casasnovas <rego>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jfernandez, mihnea, rniwa, WebkitBugTracker
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 118668    
Attachments:
Description Flags
Patch
none
Patch none

Description Manuel Rego Casasnovas 2014-01-03 02:32:46 PST
As agreed with Mihnea it would be great to have new performance tests for selection in CSS Regions but mixing content inside and outside regions. On top of that, it seems a good idea to have a test for "select all" command too.
Comment 1 Manuel Rego Casasnovas 2014-01-03 02:35:37 PST
Created attachment 220295 [details]
Patch
Comment 2 Ryosuke Niwa 2014-01-14 13:29:51 PST
Comment on attachment 220295 [details]
Patch

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

> PerformanceTests/Layout/RegionsSelectAllMixedContent.html:4
> +        <link rel="stylesheet" href="resources/regions.css" TYPE="text/css"></link>

Why is test capitalized?

> PerformanceTests/Layout/RegionsSelectAllMixedContent.html:16
> +            PerfTestRunner.measureTime(createRegionsSelectAllMixedContentTest(1000));

Why can't I see the actual test here?  It's really annoying that the test code is written elsewhere.

> PerformanceTests/Layout/RegionsSelectionMixedContent.html:4
> +        <link rel="stylesheet" href="resources/regions.css" TYPE="text/css"></link>

Ditto.

> PerformanceTests/Layout/RegionsSelectionMixedContent.html:16
> +            PerfTestRunner.measureTime(createRegionsSelectionMixedContentTest(100));

Ditto.

> PerformanceTests/Layout/resources/regions.js:165
> +    function createRegionsSelectionMixedContentTest(regionCount) {

This should be in .html fie so that we can see the test code in the single file.
Also, this function and the test should be named something like "ExtendingSelection".
"Selection" doesn't convey enough semantics.

> PerformanceTests/Layout/resources/regions.js:188
> +    function createRegionsSelectAllMixedContentTest(regionCount) {

Ditto about moving this into the html file.

> PerformanceTests/Layout/resources/regions.js:212
>      window.createRegionsTest = createRegionsTest;
>      window.createRegionsSelectionTest = createRegionsSelectionTest;

These other test functions should really be in the respective html files.
Comment 3 Manuel Rego Casasnovas 2014-01-15 02:48:51 PST
(In reply to comment #2)
> (From update of attachment 220295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=220295&action=review
> 
> > PerformanceTests/Layout/RegionsSelectAllMixedContent.html:4
> > +        <link rel="stylesheet" href="resources/regions.css" TYPE="text/css"></link>
> 
> Why is test capitalized?
> 
> > PerformanceTests/Layout/RegionsSelectAllMixedContent.html:16
> > +            PerfTestRunner.measureTime(createRegionsSelectAllMixedContentTest(1000));
> 
> Why can't I see the actual test here?  It's really annoying that the test code is written elsewhere.
> 
> > PerformanceTests/Layout/RegionsSelectionMixedContent.html:4
> > +        <link rel="stylesheet" href="resources/regions.css" TYPE="text/css"></link>
> 
> Ditto.
> 
> > PerformanceTests/Layout/RegionsSelectionMixedContent.html:16
> > +            PerfTestRunner.measureTime(createRegionsSelectionMixedContentTest(100));
> 
> Ditto.
> 
> > PerformanceTests/Layout/resources/regions.js:165
> > +    function createRegionsSelectionMixedContentTest(regionCount) {
> 
> This should be in .html fie so that we can see the test code in the single file.
> Also, this function and the test should be named something like "ExtendingSelection".
> "Selection" doesn't convey enough semantics.
> 
> > PerformanceTests/Layout/resources/regions.js:188
> > +    function createRegionsSelectAllMixedContentTest(regionCount) {
> 
> Ditto about moving this into the html file.

Attaching patch for landing fixing these issues.

> > PerformanceTests/Layout/resources/regions.js:212
> >      window.createRegionsTest = createRegionsTest;
> >      window.createRegionsSelectionTest = createRegionsSelectionTest;
> 
> These other test functions should really be in the respective html files.

Like this is related to other tests, it'll be fixed in a separated bug.
Comment 4 Manuel Rego Casasnovas 2014-01-15 02:50:08 PST
Created attachment 221252 [details]
Patch

Patch for landing.
Comment 5 WebKit Commit Bot 2014-01-15 03:22:21 PST
Comment on attachment 221252 [details]
Patch

Clearing flags on attachment: 221252

Committed r162065: <http://trac.webkit.org/changeset/162065>
Comment 6 WebKit Commit Bot 2014-01-15 03:22:23 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Manuel Rego Casasnovas 2014-01-15 04:37:22 PST
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 220295 [details] [details])
> > > PerformanceTests/Layout/resources/regions.js:212
> > >      window.createRegionsTest = createRegionsTest;
> > >      window.createRegionsSelectionTest = createRegionsSelectionTest;
> > 
> > These other test functions should really be in the respective html files.
> 
> Like this is related to other tests, it'll be fixed in a separated bug.

This has been done in bug #127041.