RESOLVED FIXED 122128
[CSS Regions] Helper functions for selection layout tests 
https://bugs.webkit.org/show_bug.cgi?id=122128
Summary [CSS Regions] Helper functions for selection layout tests
Manuel Rego Casasnovas
Reported 2013-09-30 15:02:38 PDT
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.
Attachments
Patch (10.35 KB, patch)
2013-09-30 15:15 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Patch (10.49 KB, patch)
2013-10-01 00:40 PDT, Manuel Rego Casasnovas 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Manuel Rego Casasnovas
Comment 1 2013-09-30 15:15:07 PDT
Created attachment 213039 [details] Patch
Zoltan Horvath
Comment 2 2013-09-30 20:34:28 PDT
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
Comment 3 2013-10-01 00:40:48 PDT
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 4 2013-10-02 13:16:30 PDT
Comment on attachment 213062 [details] Patch Cool! r=me
WebKit Commit Bot
Comment 5 2013-10-02 14:51:51 PDT
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 6 2013-10-02 15:45:38 PDT
Comment on attachment 213062 [details] Patch Set again cq+ due to some issues with commit-queue.
WebKit Commit Bot
Comment 7 2013-10-02 16:27:54 PDT
Comment on attachment 213062 [details] Patch Clearing flags on attachment: 213062 Committed r156807: <http://trac.webkit.org/changeset/156807>
WebKit Commit Bot
Comment 8 2013-10-02 16:27:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.