Bug 122128 – [CSS Regions] Helper functions for selection layout tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Manuel Rego Casasnovas]

As agreed in bug #121841 it seems a good idea to move common JavaScript functions related to CSS Regions selection tests to a common JavaScript file.

fast/regions/selection/ tests will be refactored in order to use these new functions.

[Manuel Rego Casasnovas]

Created attachment 213039 [details]
Patch

[Zoltan Horvath]

Comment on attachment 213039 [details]
Patch

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

Looks okay, some minor comments:


> LayoutTests/fast/regions/resources/helper.js:206
> +function selectCoordinates(fromX, fromY, toX, toY) {

I'd call this selectContentByRange.

> LayoutTests/fast/regions/resources/helper.js:207
> +    if (window.testRunner) {

You should rather use early return to avoid unnecessary indentation.

> LayoutTests/fast/regions/resources/helper.js:216
> +function selectIds(fromId, toId) {

I'd prefer selectContentByIds.

> LayoutTests/fast/regions/resources/helper.js:220
> +    selectCoordinates(fromRect.left, fromRect.top + fromRect.height / 2,

I'd make 2 temporary variables (fromRectVerticalCenter, ... ?) for the computations to make the code more readable.

> LayoutTests/fast/regions/resources/helper.js:233
> +    if (window.testRunner) {

Just return early.

Also, it seems it wasn't applied on the TOT.

[Manuel Rego Casasnovas]

Created attachment 213062 [details]
Patch

Thanks for the review, I've applied the changes. This doesn't apply in trunk because of it depends on bug #122099, which moves the CSS Regions layout tests related to selection to a specific folder "fast/regions/selection/".

[Alexandru Chiculita]

Comment on attachment 213062 [details]
Patch

Cool! r=me

[WebKit Commit Bot]

Comment on attachment 213062 [details]
Patch

Rejecting attachment 213062 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 213062, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
Tools/Scripts/update-webkit']" exit_code: 2

Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 122.

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 122.

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 122.

Full output: http://webkit-queues.appspot.com/results/3053007

[Manuel Rego Casasnovas]

Comment on attachment 213062 [details]
Patch

Set again cq+ due to some issues with commit-queue.

[WebKit Commit Bot]

Comment on attachment 213062 [details]
Patch

Clearing flags on attachment: 213062

Committed r156807: <http://trac.webkit.org/changeset/156807>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.