Bug 70248

Summary: Pass a Frame* parameter in EditorClient::respondToChangedSelection
Product: WebKit Reporter: Antonio Gomes <tonikitoo>
Component: HTML EditingAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, japhet, kenneth, mrobinson, rakuco, rniwa, webkit.review.bot, xan.lopez
Priority: P2    
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

Description Antonio Gomes 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...
Comment 1 Antonio Gomes 2011-10-17 11:09:56 PDT
Created attachment 111284 [details]
patch v1 - for EWS to build
Comment 2 WebKit Review Bot 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.
Comment 3 Antonio Gomes 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).
Comment 4 Gyuyoung Kim 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
Comment 5 Antonio Gomes 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 :/
Comment 6 Antonio Gomes 2011-10-17 12:07:25 PDT
Created attachment 111296 [details]
patch v1.1 - for EWS to build
Comment 7 WebKit Review Bot 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.
Comment 8 Gyuyoung Kim 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
Comment 9 Antonio Gomes 2011-10-17 12:16:52 PDT
Created attachment 111297 [details]
patch v1.2 - for EWS to build
Comment 10 WebKit Review Bot 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.
Comment 11 Gyuyoung Kim 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
Comment 12 Antonio Gomes 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
Comment 13 Darin Adler 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.
Comment 14 Darin Adler 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!
Comment 15 Ryosuke Niwa 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.
Comment 16 Antonio Gomes 2011-10-17 13:45:43 PDT
Created attachment 111314 [details]
patch v1.3 - for EWS to build
Comment 17 WebKit Review Bot 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.
Comment 18 Gyuyoung Kim 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
Comment 19 Antonio Gomes 2011-10-17 20:05:40 PDT
Created attachment 111372 [details]
patch v1.4 - for review
Comment 20 Antonio Gomes 2011-10-18 07:05:09 PDT
Created attachment 111439 [details]
patch 1.5
Comment 21 Ryosuke Niwa 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.
Comment 22 Gustavo Noronha (kov) 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
Comment 23 Antonio Gomes 2011-10-18 19:23:29 PDT
Created attachment 111553 [details]
patch 1.6
Comment 24 Antonio Gomes 2011-10-19 07:05:03 PDT
Created attachment 111610 [details]
(landed as r100874) - patch 1.7 - for EWS (r=rniwa)- do not review
Comment 25 Antonio Gomes 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
Comment 26 Antonio Gomes 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