Bug 63362 – convert editing/deleting/paragraph-in-preserveNewline.html to dumpAsText

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wyatt Carss]

convert editing/deleting/paragraph-in-preserveNewline.html to dumpAsText

[Wyatt Carss]

Created attachment 98555 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 98555 [details]
Patch

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

> LayoutTests/editing/deleting/paragraph-in-preserveNewline-expected.txt:14
> +
> +bar

We should use dump-as-markup so that we can see the markup in #test.  r- due to loss of test coverage.

[Wyatt Carss]

Created attachment 98770 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 98770 [details]
Patch

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

> LayoutTests/ChangeLog:11
> +        * editing/deleting/paragraph-in-preserveNewline-expected.txt: Added.

I don't see this file in the patch.

[Wyatt Carss]

Created attachment 98862 [details]
Patch

[Wyatt Carss]

Comment on attachment 98770 [details]
Patch

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

>> LayoutTests/ChangeLog:11
>> +        * editing/deleting/paragraph-in-preserveNewline-expected.txt: Added.
> 
> I don't see this file in the patch.

That's very odd - I should have caught that it was missing! I looked at a diff of the branch from earlier, and it shows my edits; somehow the script didn't recognize it. Anyway, new patch (with results) is attached.

[Ryosuke Niwa]

Comment on attachment 98862 [details]
Patch

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

> LayoutTests/editing/deleting/paragraph-in-preserveNewline-expected.txt:6
> +| <pre>
> +|   "<#selection-caret>
> +bar"

Huh, bar is outside of pre?  Please file a bug for this.

> LayoutTests/editing/deleting/paragraph-in-preserveNewline.html:20
> +if (window.layoutTestController)
> +     layoutTestController.dumpEditingCallbacks();

You need to do this before modifying selection & calling execCommand("Delete").  r- because of this.

[Wyatt Carss]

Created attachment 98956 [details]
Proposed patch

Hopefully this works - I manually made the patch file using 'git diff -M100 commit-number'. The upload script generates a diff somewhere, and git's dumb "smart rename tracking" which only comes to the surfaces when generating diffs causes the file not to be shown. Passing -M100 or --no-renames to the diff line keeps it from finding renames, which is great - but modifying the upload script to include either of these in calls to git diff didn't seem to have an effect.

Anyway, since webkit-patch doesn't have the ability to pretty-preview an arbitrary patch from a file, and I don't fully trust my ability to read raw diffs, I'll know after uploading this whether the manual diff worked quite the way I wanted.

[Wyatt Carss]

Comment on attachment 98862 [details]
Patch

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

>> LayoutTests/editing/deleting/paragraph-in-preserveNewline-expected.txt:6
>> +bar"
> 
> Huh, bar is outside of pre?  Please file a bug for this.

bar is in a string which has a \n in it -- so it appears on the next line without a new |

[Wyatt Carss]

Created attachment 98957 [details]
Proposed patch

Trying again with '--binary' in the git diff line.

[Ryosuke Niwa]

Comment on attachment 98862 [details]
Patch

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

>>> LayoutTests/editing/deleting/paragraph-in-preserveNewline-expected.txt:6
>>> +bar"
>> 
>> Huh, bar is outside of pre?  Please file a bug for this.
> 
> bar is in a string which has a \n in it -- so it appears on the next line without a new |

Ah ok.

[Wyatt Carss]

The attached patch works - I reapplied it to a clean local dir successfully. For future reference, 'git diff -M100 --binary commit-id > patch.diff' is what to use. I'm going to look into the scripts and see if I can get them to mimic this; it would be cool to have git do renames properly for our purposes.

[WebKit Review Bot]

Comment on attachment 98957 [details]
Proposed patch

Clearing flags on attachment: 98957

Committed r90031: <http://trac.webkit.org/changeset/90031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.