Bug 61333 - Convert LayoutTests/editing/deleting/delete-block-table.html to dump-as-markup
Summary: Convert LayoutTests/editing/deleting/delete-block-table.html to dump-as-markup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Annie Sullivan
URL:
Keywords:
Depends on:
Blocks: 57148
  Show dependency treegraph
 
Reported: 2011-05-23 18:19 PDT by Annie Sullivan
Modified: 2011-05-24 20:39 PDT (History)
2 users (show)

See Also:


Attachments
Work in Progress (47.14 KB, patch)
2011-05-23 18:22 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (48.97 KB, patch)
2011-05-24 11:42 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff
Patch (49.45 KB, patch)
2011-05-24 12:08 PDT, Annie Sullivan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Annie Sullivan 2011-05-23 18:19:28 PDT
The output of this test would be easier to read as markup.
Comment 1 Annie Sullivan 2011-05-23 18:22:18 PDT
Created attachment 94533 [details]
Work in Progress
Comment 2 Annie Sullivan 2011-05-23 18:24:36 PDT
Whoops, looks like I didn't send the right arguments to webkit-patch. This patch should have been titled "work in progress". I used the rebaseline-for-dumpas-conv script on this test, and it made the changes to the html file in this patch. But the output is text, not markup. Ryosuke, any comments on the correct way to convert a test that uses editing.js like this?
Comment 3 Ryosuke Niwa 2011-05-23 20:00:52 PDT
Your approach is correct and would have worked if this test had #root like other editing.js.  See debugForDumpAsText in editing.js.
Comment 4 Annie Sullivan 2011-05-24 11:32:10 PDT
Thanks! I have this test converted, but before I post my patch, I wanted to add a description since I realized it doesn't have one. What's the correct way to put in a description for a test that calls runDumpAsTextEditingTest()? I see some tests have divs with classes "explanation", "scenario", "expected-results"--is that the right way to do it?
Comment 5 Ryosuke Niwa 2011-05-24 11:33:48 PDT
(In reply to comment #4)
> Thanks! I have this test converted, but before I post my patch, I wanted to add a description since I realized it doesn't have one. What's the correct way to put in a description for a test that calls runDumpAsTextEditingTest()? I see some tests have divs with classes "explanation", "scenario", "expected-results"--is that the right way to do it?

You can just add a p/div with explanation.  I don't think those class names mean anything.
Comment 6 Annie Sullivan 2011-05-24 11:42:22 PDT
Created attachment 94651 [details]
Patch
Comment 7 Ryosuke Niwa 2011-05-24 11:51:30 PDT
Comment on attachment 94651 [details]
Patch

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

Even though it's a minor change, I'd say r- since you're not a committer yet.

> LayoutTests/ChangeLog:8
> +        Converts delete-block-table.html to dump-as-markup by changing to use runDumpAsTextEditingTest.

runDumpAsTextEditingTest don't use dump-as-markup.js so you should either say text test or dumpAsTest test.
Comment 8 Ryosuke Niwa 2011-05-24 11:52:06 PDT
Comment on attachment 94651 [details]
Patch

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

> LayoutTests/editing/deleting/delete-block-table.html:4
>  <script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>

I would have stripped out language and type attributes.
Comment 9 Ryosuke Niwa 2011-05-24 11:52:46 PDT
Comment on attachment 94651 [details]
Patch

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

>> LayoutTests/editing/deleting/delete-block-table.html:4
>>  <script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
> 
> I would have stripped out language and type attributes.

Also, I would have put all three script elements at the end of document.
Comment 10 Annie Sullivan 2011-05-24 12:08:42 PDT
Created attachment 94658 [details]
Patch
Comment 11 Annie Sullivan 2011-05-24 12:08:50 PDT
Comment on attachment 94651 [details]
Patch

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

>> LayoutTests/ChangeLog:8

> 
> runDumpAsTextEditingTest don't use dump-as-markup.js so you should either say text test or dumpAsTest test.

Done.

>>> LayoutTests/editing/deleting/delete-block-table.html:4
>>>  <script src=../editing.js language="JavaScript" type="text/JavaScript" ></script>
>> 
>> I would have stripped out language and type attributes.
> 
> Also, I would have put all three script elements at the end of document.

Done. Since the two inline scripts are now next to each other, I combined them.
Comment 12 Ryosuke Niwa 2011-05-24 12:17:27 PDT
Comment on attachment 94658 [details]
Patch

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

The following comments for future references.

> LayoutTests/editing/deleting/delete-block-table.html:27
> +<script src=../editing.js></script>

I would prefer to wrap URL by (double) quotations but that's fine.

> LayoutTests/editing/deleting/delete-block-table.html:37
> +    for (i = 0; i < 3; i++) {
> +        deleteCommand();
> +    }

No curly brackets around a single statement.
Comment 13 WebKit Commit Bot 2011-05-24 19:35:15 PDT
Comment on attachment 94658 [details]
Patch

Rejecting attachment 94658 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 1

Last 500 characters of output:
autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain
    result = func(*args)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open
    return self.do_open(conn_factory, req)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open
    raise URLError(err)
urllib2.URLError: <urlopen error [Errno 60] Operation timed out>

Full output: http://queues.webkit.org/results/8732572
Comment 14 Ryosuke Niwa 2011-05-24 19:42:28 PDT
Comment on attachment 94658 [details]
Patch

Let's try again.
Comment 15 WebKit Commit Bot 2011-05-24 20:37:59 PDT
The commit-queue encountered the following flaky tests while processing attachment 94658 [details]:

media/video-playbackrate.html bug 58629 (author: eric.carlson@apple.com)
The commit-queue is continuing to process your patch.
Comment 16 WebKit Commit Bot 2011-05-24 20:39:39 PDT
Comment on attachment 94658 [details]
Patch

Clearing flags on attachment: 94658

Committed r87261: <http://trac.webkit.org/changeset/87261>
Comment 17 WebKit Commit Bot 2011-05-24 20:39:44 PDT
All reviewed patches have been landed.  Closing bug.