Bug 53943 – VisibleSelection::setWithoutValidation() should allow caret selection.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jia Pu]

Currently setWithoutValidation() fails on "ASSERT(base != extent);" when undoing some paste operation. It also cause assertion failure in the patch for bug 52221. This function should be modified to allow caret selection.

[Jia Pu]

Created attachment 81527 [details]
proposed patch (v1)

[Eric Seidel (no email)]

Comment on attachment 81527 [details]
proposed patch (v1)

How do we test this?  Please add tests or explain why testing is imposible in the ChangeLog.

[Darin Adler]

(In reply to comment #2)
> How do we test this?  Please add tests or explain why testing is impossible in the ChangeLog.

Jia, can you create a test that exercises the case where you are undoing a paste operation that you mentioned in your description?

[Jia Pu]

Created attachment 81557 [details]
proposed patch (v2)

[Jia Pu]

(In reply to comment #3)
> (In reply to comment #2)
> > How do we test this?  Please add tests or explain why testing is impossible in the ChangeLog.
> 
> Jia, can you create a test that exercises the case where you are undoing a paste operation that you mentioned in your description?

I have added such test.

Also, comparing the test result on LayoutTests/fast and LayoutTest/editing, this patch doesn't introduce any new test failure.

[WebKit Commit Bot]

Comment on attachment 81557 [details]
proposed patch (v2)

Rejecting attachment 81557 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 1

Last 500 characters of output:
mands/roll.py
	M	Tools/Scripts/webkitpy/tool/commands/queries.py
	M	Tools/Scripts/webkitpy/tool/commands/prettydiff.py
	M	Tools/Scripts/webkitpy/tool/commands/upload.py
	M	Tools/Scripts/webkitpy/tool/bot/sheriffircbot.py
	M	Tools/Scripts/webkitpy/tool/bot/irc_command.py
	M	Tools/Scripts/webkitpy/tool/main.py
r77913 = c3ccf5a231c164debfa1e2a2727eed0a73815769 (refs/remotes/origin/master)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://queues.webkit.org/results/7817005

[Jia Pu]

(In reply to comment #6)
> (From update of attachment 81557 [details])
> Rejecting attachment 81557 [details] from commit-queue.
> 
> Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 1
> 
> Full output: http://queues.webkit.org/results/7817005

Darin, did I miss something in the ChangeLog, or is it something you need to do?

[Darin Adler]

(In reply to comment #7)
> Darin, did I miss something in the ChangeLog, or is it something you need to do?

You removed the Reviewed by NOBODY (OOPS) line from the change logs or wrote them yourself so those lines are not in there. That is not right, because those lines are edited by the script to put in the reviewer name. You should post a new patch with those lines added back in.

[Jia Pu]

Created attachment 81672 [details]
proposed patch (v2)

[Jia Pu]

(In reply to comment #8)
> (In reply to comment #7)
> > Darin, did I miss something in the ChangeLog, or is it something you need to do?
> 
> You removed the Reviewed by NOBODY (OOPS) line from the change logs or wrote them yourself so those lines are not in there. That is not right, because those lines are edited by the script to put in the reviewer name. You should post a new patch with those lines added back in.

Hmm, I'm almost certain I have received conflicting information from someone else. In fact, I did this to all the patches I have submitted. Somehow they all got landed. 

Anyway, I have uploaded a new patch. Thanks.

[Darin Adler]

(In reply to comment #10)
> Hmm, I'm almost certain I have received conflicting information from someone else.

Maybe you’re confusing the “no tests” OOPS with the “reviewed by” OOPS?

> In fact, I did this to all the patches I have submitted. Somehow they all got landed.

Maybe you can point me to an example if you’d like to unravel the mystery.

[Jia Pu]

(In reply to comment #11)
> (In reply to comment #10)
> > Hmm, I'm almost certain I have received conflicting information from someone else.
> 
> Maybe you’re confusing the “no tests” OOPS with the “reviewed by” OOPS?
> 
> > In fact, I did this to all the patches I have submitted. Somehow they all got landed.
> 
> Maybe you can point me to an example if you’d like to unravel the mystery.

Ah, my mistake. There're only a couple of patches without "OOPS" line. They seem to have been landed manually.

[WebKit Commit Bot]

Comment on attachment 81672 [details]
proposed patch (v2)

Rejecting attachment 81672 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'la..." exit_code: 2

Last 500 characters of output:
082b63c51f332a965beb1e6b8c20a14b16dbb719
r77969 = a9f4e27c5c3779f53df659dc5c1c1ca07213ce6c
r77970 = 9b1b5c360350975da272101220435203046d2be6
r77971 = 144b02c425132ff97badf9f13da23ba28ae4c5c3
r77972 = 5e5543c0edb14418274188508a2b0ee393cd0c7e
r77973 = 072e529dbed6ed8fd692c6c79394be23ecc2bd0a
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://queues.webkit.org/results/7763222

[Darin Adler]

New problem is this:

The following files contain tab characters:

        trunk/LayoutTests/editing/undo/undo-paste-when-caret-is-not-in-range.html

    Please use spaces instead to indent.

[Jia Pu]

Created attachment 81703 [details]
proposed patch (v3)

[WebKit Commit Bot]

Comment on attachment 81703 [details]
proposed patch (v3)

Clearing flags on attachment: 81703

Committed r77995: <http://trac.webkit.org/changeset/77995>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/77995 might have broken Qt Linux Release

[Ryosuke Niwa]

undo-paste-when-caret-is-not-in-range.html is failing on WebKit2.  I can't tell whether or not a missing DRT feature is causing the failure: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(WebKit2%20Tests)/r77995%20(8289)/editing/undo/undo-paste-when-caret-is-not-in-range-pretty-diff.html

[Ryosuke Niwa]

It's also failing on GTK, Qt, and Windows:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r78010%20(19127)/results.html
http://build.webkit.org/results/Qt%20Linux%20Release/r78010%20(27938)/results.html
http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r78010%20(9159)/results.html

[Jia Pu]

(In reply to comment #20)
> It's also failing on GTK, Qt, and Windows:
> http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r78010%20(19127)/results.html
> http://build.webkit.org/results/Qt%20Linux%20Release/r78010%20(27938)/results.html
> http://build.webkit.org/results/Windows%207%20Release%20(Tests)/r78010%20(9159)/results.html

hmm, does this mean I should generate platform specific expected file? how should I go about this if I don't easily have other platforms readily available.

Or can the test be written in a more platform independent way?

[Jia Pu]

I guess I should have used "layoutTestController.dumpAsText". Will upload a patch tonight.

[Jia Pu]

Created attachment 81819 [details]
Patch for test.

This patch fixes the test introduced in previous patch.

[Darin Adler]

Comment on attachment 81819 [details]
Patch for test.

OK.

[Jia Pu]

Reopened to submit additional patch.

[Brian Weinstein]

This test has been failing on Windows since it landed, added Windows-specific failing results in r78082.

If you could investigate and figure out the issue that is causing this test to fail on Windows, that would be great.

https://bugs.webkit.org/show_bug.cgi?id=54120 tracks the failure.

[Jia Pu]

The reason for test failure on other platforms is that I was dumping the render tree instead of plain text, hence the different result on different platforms. 

I have submitted a second patch to fix the test. Darin r+'ed it. But I forgot to change the bug status back to "REOPENED", so the build bot refused to process the patch. Can someone re-approve it and commit+ it? Thanks

[Darin Adler]

(In reply to comment #27)
> I have submitted a second patch to fix the test. Darin r+'ed it. But I forgot to change the bug status back to "REOPENED", so the build bot refused to process the patch. Can someone re-approve it and commit+ it? Thanks

I think a separate bug reports works best in cases like this.

[Darin Adler]

We’ll see if changing the flags like this works or not.

[WebKit Commit Bot]

Comment on attachment 81819 [details]
Patch for test.

Clearing flags on attachment: 81819

Committed r78104: <http://trac.webkit.org/changeset/78104>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.