Bug 70248

Summary:

Pass a Frame* parameter in EditorClient::respondToChangedSelection

Product:

WebKit

Reporter:

Antonio Gomes <tonikitoo>

Component:

HTML Editing

Assignee:

Antonio Gomes <tonikitoo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

gustavo, japhet, kenneth, mrobinson, rakuco, rniwa, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

70253

Bug Blocks:

Attachments:

Description	Flags
patch v1 - for EWS to build	gyuyoung.kim: commit-queue-
patch v1.1 - for EWS to build	gyuyoung.kim: commit-queue-
patch v1.2 - for EWS to build	none
patch v1.3 - for EWS to build	gyuyoung.kim: commit-queue-
patch v1.4 - for review	tonikitoo: commit-queue-
patch 1.5	rniwa: review+, gustavo: commit-queue-
patch 1.6	none
(landed as r100874) - patch 1.7 - for EWS (r=rniwa)- do not review	none

Antonio Gomes

Reported 2011-10-17 10:47:11 PDT

Most of the port specific implementations of EditorClient::respondToChangedSelection (like EditorClient{Qt,Gtk,etc}) are wrongly relying on FocusController::focusedOrMainFrame method to get the Frame where the selection is happening on. It is not right, since a selection can be easily triggered in an inner frame that is not focused. Patch coming...

Attachments
patch v1 - for EWS to build (25.76 KB, patch) 2011-10-17 11:09 PDT, Antonio Gomes	gyuyoung.kim: commit-queue-	Details Formatted Diff Diff
patch v1.1 - for EWS to build (25.70 KB, patch) 2011-10-17 12:07 PDT, Antonio Gomes	gyuyoung.kim: commit-queue-	Details Formatted Diff Diff
patch v1.2 - for EWS to build (25.70 KB, patch) 2011-10-17 12:16 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
patch v1.3 - for EWS to build (27.00 KB, patch) 2011-10-17 13:45 PDT, Antonio Gomes	gyuyoung.kim: commit-queue-	Details Formatted Diff Diff
patch v1.4 - for review (27.00 KB, patch) 2011-10-17 20:05 PDT, Antonio Gomes	tonikitoo: commit-queue-	Details Formatted Diff Diff
patch 1.5 (27.12 KB, patch) 2011-10-18 07:05 PDT, Antonio Gomes	rniwa: review+ gustavo: commit-queue-	Details Formatted Diff Diff
patch 1.6 (27.12 KB, patch) 2011-10-18 19:23 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
(landed as r100874) - patch 1.7 - for EWS (r=rniwa)- do not review (27.13 KB, patch) 2011-10-19 07:05 PDT, Antonio Gomes	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Antonio Gomes

Comment 1 2011-10-17 11:09:56 PDT

Created attachment 111284 [details] patch v1 - for EWS to build

WebKit Review Bot

Comment 2 2011-10-17 11:12:03 PDT

Attachment 111284 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

Antonio Gomes

Comment 3 2011-10-17 11:15:09 PDT

cc'ed some port peers to be aware of the patch (and maybe review port specific parts effected).

Gyuyoung Kim

Comment 4 2011-10-17 11:16:09 PDT

Comment on attachment 111284 [details] patch v1 - for EWS to build Attachment 111284 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10085426

Antonio Gomes

Comment 5 2011-10-17 11:23:30 PDT

(In reply to comment #4) > (From update of attachment 111284 [details]) > Attachment 111284 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/10085426 bleh, obvious errors :/

Antonio Gomes

Comment 6 2011-10-17 12:07:25 PDT

Created attachment 111296 [details] patch v1.1 - for EWS to build

WebKit Review Bot

Comment 7 2011-10-17 12:09:09 PDT

Attachment 111296 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

Gyuyoung Kim

Comment 8 2011-10-17 12:12:01 PDT

Comment on attachment 111296 [details] patch v1.1 - for EWS to build Attachment 111296 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10098355

Antonio Gomes

Comment 9 2011-10-17 12:16:52 PDT

Created attachment 111297 [details] patch v1.2 - for EWS to build

WebKit Review Bot

Comment 10 2011-10-17 12:17:58 PDT

Attachment 111297 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

Gyuyoung Kim

Comment 11 2011-10-17 12:21:22 PDT

Comment on attachment 111297 [details] patch v1.2 - for EWS to build Attachment 111297 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10086572

Antonio Gomes

Comment 12 2011-10-17 12:31:53 PDT

(In reply to comment #10) > Attachment 111297 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 > > Updating OpenSource > Current branch master is up to date. > Updating chromium port dependencies using gclient... > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Re-trying 'depot_tools/gclient sync' > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Re-trying 'depot_tools/gclient sync' > Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. > Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. > Re-trying 'depot_tools/gclient sync' > No such file or directory at Tools/Scripts/update-webkit line 104. > > > If any of these errors are false positives, please file a bug against check-webkit-style. Ignore this. (In reply to comment #11) > (From update of attachment 111297 [details]) > Attachment 111297 [details] did not pass efl-ews (efl): > Output: http://queues.webkit.org/results/10086572 And this :) it is bug 70253

Darin Adler

Comment 13 2011-10-17 12:51:45 PDT

Comment on attachment 111297 [details] patch v1.2 - for EWS to build This is wrong. The editor client is per-frame and so can store a Frame* if needed. It’s not a per-Page thing.

Darin Adler

Comment 14 2011-10-17 12:56:41 PDT

Comment on attachment 111297 [details] patch v1.2 - for EWS to build Oops, my bad, it is per-Page, not per-Frame!

Ryosuke Niwa

Comment 15 2011-10-17 13:05:45 PDT

Comment on attachment 111297 [details] patch v1.2 - for EWS to build View in context: https://bugs.webkit.org/attachment.cgi?id=111297&action=review > Source/WebCore/ChangeLog:4 > + Missing bug url. > Source/WebCore/ChangeLog:19 > + * editing/Editor.cpp: Pass the Frame on where the selection is > + changing to the client. It seems like this can fit in on line.

Antonio Gomes

Comment 16 2011-10-17 13:45:43 PDT

Created attachment 111314 [details] patch v1.3 - for EWS to build

WebKit Review Bot

Comment 17 2011-10-17 13:48:07 PDT

Attachment 111314 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource Current branch master is up to date. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

Gyuyoung Kim

Comment 18 2011-10-17 15:02:44 PDT

Comment on attachment 111314 [details] patch v1.3 - for EWS to build Attachment 111314 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10106277

Antonio Gomes

Comment 19 2011-10-17 20:05:40 PDT

Created attachment 111372 [details] patch v1.4 - for review

Antonio Gomes

Comment 20 2011-10-18 07:05:09 PDT

Created attachment 111439 [details] patch 1.5

Ryosuke Niwa

Comment 21 2011-10-18 09:47:15 PDT

Comment on attachment 111439 [details] patch 1.5 Hopefully it won't break any tests on any port.

Gustavo Noronha (kov)

Comment 22 2011-10-18 15:01:10 PDT

Comment on attachment 111439 [details] patch 1.5 Attachment 111439 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10138094

Antonio Gomes

Comment 23 2011-10-18 19:23:29 PDT

Created attachment 111553 [details] patch 1.6

Antonio Gomes

Comment 24 2011-10-19 07:05:03 PDT

Created attachment 111610 [details] (landed as r100874) - patch 1.7 - for EWS (r=rniwa)- do not review

Antonio Gomes

Comment 25 2011-10-20 12:57:43 PDT

Comment on attachment 111610 [details] (landed as r100874) - patch 1.7 - for EWS (r=rniwa)- do not review landing it manually, just waiting for a green state on the bots

Antonio Gomes

Comment 26 2011-11-20 19:35:08 PST

Comment on attachment 111610 [details] (landed as r100874) - patch 1.7 - for EWS (r=rniwa)- do not review Clearing flags: r100874 > git svn dcommit Committing to http://svn.webkit.org/repository/webkit/trunk ... M Source/WebCore/ChangeLog M Source/WebCore/editing/Editor.cpp M Source/WebCore/loader/EmptyClients.h M Source/WebCore/page/EditorClient.h M Source/WebKit/chromium/ChangeLog M Source/WebKit/chromium/src/EditorClientImpl.cpp M Source/WebKit/chromium/src/EditorClientImpl.h M Source/WebKit/efl/ChangeLog M Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp M Source/WebKit/efl/WebCoreSupport/EditorClientEfl.h M Source/WebKit/gtk/ChangeLog M Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp M Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.h M Source/WebKit/mac/ChangeLog M Source/WebKit/mac/WebCoreSupport/WebEditorClient.h M Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm M Source/WebKit/qt/ChangeLog M Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp M Source/WebKit/qt/WebCoreSupport/EditorClientQt.h M Source/WebKit/win/ChangeLog M Source/WebKit/win/WebCoreSupport/WebEditorClient.cpp M Source/WebKit/win/WebCoreSupport/WebEditorClient.h M Source/WebKit/wx/ChangeLog M Source/WebKit/wx/WebKitSupport/EditorClientWx.cpp M Source/WebKit/wx/WebKitSupport/EditorClientWx.h M Source/WebKit2/ChangeLog M Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp M Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h Committed r100874 M Source/WebKit/chromium/ChangeLog M Source/WebKit/chromium/src/EditorClientImpl.cpp M Source/WebKit/chromium/src/EditorClientImpl.h M Source/WebKit/qt/WebCoreSupport/EditorClientQt.h M Source/WebKit/qt/WebCoreSupport/EditorClientQt.cpp M Source/WebKit/qt/ChangeLog M Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.h M Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp M Source/WebKit/gtk/ChangeLog M Source/WebKit/win/ChangeLog M Source/WebKit/win/WebCoreSupport/WebEditorClient.cpp M Source/WebKit/win/WebCoreSupport/WebEditorClient.h M Source/WebKit/wx/WebKitSupport/EditorClientWx.cpp M Source/WebKit/wx/WebKitSupport/EditorClientWx.h M Source/WebKit/wx/ChangeLog M Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm M Source/WebKit/mac/WebCoreSupport/WebEditorClient.h M Source/WebKit/mac/ChangeLog M Source/WebKit/efl/WebCoreSupport/EditorClientEfl.cpp M Source/WebKit/efl/WebCoreSupport/EditorClientEfl.h M Source/WebKit/efl/ChangeLog M Source/WebCore/editing/Editor.cpp M Source/WebCore/ChangeLog M Source/WebCore/page/EditorClient.h M Source/WebCore/loader/EmptyClients.h M Source/WebKit2/ChangeLog M Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.cpp M Source/WebKit2/WebProcess/WebCoreSupport/WebEditorClient.h r100874 = fdc7310332b67a164b63d03c3412604b5ea7a296 (refs/remotes/trunk) No changes between current HEAD and refs/remotes/trunk Resetting to the latest refs/remotes/trunk

Note You need to log in before you can comment on or make changes to this bug.