Bug 53437 - Convert editing/deleting/5168598.html to dumpAsText test
Summary: Convert editing/deleting/5168598.html to dumpAsText test
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Chang Shu
URL:
Keywords:
Depends on:
Blocks: 53031
  Show dependency treegraph
 
Reported: 2011-01-31 10:18 PST by Chang Shu
Modified: 2011-02-10 13:06 PST (History)
2 users (show)

See Also:


Attachments
fix patch (10.94 KB, patch)
2011-01-31 10:25 PST, Chang Shu
rniwa: review-
Details | Formatted Diff | Diff
fix patch 2 (10.92 KB, patch)
2011-01-31 11:20 PST, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chang Shu 2011-01-31 10:18:39 PST
The intention of the test case is to check the existence of a link node and it should not crash. This can be done by text-only results, which is cross-platform.
Comment 1 Chang Shu 2011-01-31 10:25:05 PST
Created attachment 80656 [details]
fix patch
Comment 2 Ryosuke Niwa 2011-01-31 11:10:28 PST
Comment on attachment 80656 [details]
fix patch

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

> LayoutTests/ChangeLog:5
> +        Convert the test case to text-only so the expected result is cross-platform.

Please explicitly say we're converting this to a dumpAsText test.

> LayoutTests/editing/deleting/5168598.html:1
>  <body>

Can we add <!DOCTYPE html>?

> LayoutTests/editing/deleting/5168598.html:4
> +<div id="console"></div>

We won't need this if we make the following changes.

> LayoutTests/editing/deleting/5168598.html:14
> +function log(message) {
> +    var console = document.getElementById("console");
> +    var text = document.createTextNode(message);
> +    console.appendChild(text);
> +}

I'm not sure if it makes sense to add this log function just to print one line.

> LayoutTests/editing/deleting/5168598.html:21
> +log(document.getElementsByTagName('a').item(0) + "\n");

I would have done:
var numberOfElements = document.getElementsByTagName('a').length;
document.write(numberOfElements ? 'FAIL: there were ' + numberOfElements + ' anchor elements but expected none' : 'PASS');
Comment 3 Chang Shu 2011-01-31 11:20:02 PST
Created attachment 80663 [details]
fix patch 2
Comment 4 Ryosuke Niwa 2011-01-31 11:22:51 PST
Comment on attachment 80663 [details]
fix patch 2

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

r+ but you might want to consider hiding input element after printing PASS/FAIL.

> LayoutTests/editing/deleting/5168598-expected.txt:3
> + PASS

I think the whitespace before PASS will go away if you hid the input element after the test.
Comment 5 Chang Shu 2011-01-31 12:55:07 PST
Comment on attachment 80663 [details]
fix patch 2

Committed r77154: <http://trac.webkit.org/changeset/77154>
Comment 6 Chang Shu 2011-01-31 12:56:13 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Suresh Voruganti 2011-02-10 13:06:30 PST
Removing from Qtwebkit 2.1.1 Nice to have master bug.