Bug 53943 - VisibleSelection::setWithoutValidation() should allow caret selection.
Summary: VisibleSelection::setWithoutValidation() should allow caret selection.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 54060
  Show dependency treegraph
 
Reported: 2011-02-07 14:29 PST by Jia Pu
Modified: 2012-04-19 05:20 PDT (History)
8 users (show)

See Also:


Attachments
proposed patch (v1) (1.50 KB, patch)
2011-02-07 14:49 PST, Jia Pu
no flags Details | Formatted Diff | Diff
proposed patch (v2) (7.92 KB, patch)
2011-02-07 17:35 PST, Jia Pu
no flags Details | Formatted Diff | Diff
proposed patch (v2) (8.00 KB, patch)
2011-02-08 11:54 PST, Jia Pu
no flags Details | Formatted Diff | Diff
proposed patch (v3) (8.03 KB, patch)
2011-02-08 14:59 PST, Jia Pu
no flags Details | Formatted Diff | Diff
Patch for test. (7.65 KB, patch)
2011-02-09 09:07 PST, Jia Pu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jia Pu 2011-02-07 14:29:21 PST
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.
Comment 1 Jia Pu 2011-02-07 14:49:48 PST
Created attachment 81527 [details]
proposed patch (v1)
Comment 2 Eric Seidel (no email) 2011-02-07 15:07:44 PST
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.
Comment 3 Darin Adler 2011-02-07 15:13:56 PST
(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?
Comment 4 Jia Pu 2011-02-07 17:35:12 PST
Created attachment 81557 [details]
proposed patch (v2)
Comment 5 Jia Pu 2011-02-07 17:36:49 PST
(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.
Comment 6 WebKit Commit Bot 2011-02-07 23:31:37 PST
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
Comment 7 Jia Pu 2011-02-08 08:23:43 PST
(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?
Comment 8 Darin Adler 2011-02-08 10:10:42 PST
(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.
Comment 9 Jia Pu 2011-02-08 11:54:52 PST
Created attachment 81672 [details]
proposed patch (v2)
Comment 10 Jia Pu 2011-02-08 11:57:54 PST
(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.
Comment 11 Darin Adler 2011-02-08 13:21:48 PST
(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.
Comment 12 Jia Pu 2011-02-08 14:23:43 PST
(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.
Comment 13 WebKit Commit Bot 2011-02-08 14:42:07 PST
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
Comment 14 Darin Adler 2011-02-08 14:45:14 PST
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.
Comment 15 Jia Pu 2011-02-08 14:59:34 PST
Created attachment 81703 [details]
proposed patch (v3)
Comment 16 WebKit Commit Bot 2011-02-08 16:55:54 PST
Comment on attachment 81703 [details]
proposed patch (v3)

Clearing flags on attachment: 81703

Committed r77995: <http://trac.webkit.org/changeset/77995>
Comment 17 WebKit Commit Bot 2011-02-08 16:56:00 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 WebKit Review Bot 2011-02-08 17:24:21 PST
http://trac.webkit.org/changeset/77995 might have broken Qt Linux Release
Comment 19 Ryosuke Niwa 2011-02-08 20:20:38 PST
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
Comment 21 Jia Pu 2011-02-08 21:33:39 PST
(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?
Comment 22 Jia Pu 2011-02-08 21:50:00 PST
I guess I should have used "layoutTestController.dumpAsText". Will upload a patch tonight.
Comment 23 Jia Pu 2011-02-09 09:07:05 PST
Created attachment 81819 [details]
Patch for test.

This patch fixes the test introduced in previous patch.
Comment 24 Darin Adler 2011-02-09 09:51:00 PST
Comment on attachment 81819 [details]
Patch for test.

OK.
Comment 25 Jia Pu 2011-02-09 10:20:53 PST
Reopened to submit additional patch.
Comment 26 Brian Weinstein 2011-02-09 10:51:41 PST
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.
Comment 27 Jia Pu 2011-02-09 11:32:33 PST
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
Comment 28 Darin Adler 2011-02-09 11:46:41 PST
(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.
Comment 29 Darin Adler 2011-02-09 11:47:17 PST
We’ll see if changing the flags like this works or not.
Comment 30 WebKit Commit Bot 2011-02-09 12:08:57 PST
Comment on attachment 81819 [details]
Patch for test.

Clearing flags on attachment: 81819

Committed r78104: <http://trac.webkit.org/changeset/78104>
Comment 31 WebKit Commit Bot 2011-02-09 12:09:04 PST
All reviewed patches have been landed.  Closing bug.