Accessibility needs to be notified about text selection/focus for EFL port.
Created attachment 189083 [details] patch
Attachment 189083 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/GNUmakefile.list.am', u'Source/WebCore/PlatformEfl.cmake', u'Source/WebCore/editing/FrameSelection.h', u'Source/WebCore/editing/atk/FrameSelectionAtk.cpp', u'Source/WebCore/editing/gtk/FrameSelectionGtk.cpp']" exit_code: 1 Source/WebCore/editing/atk/FrameSelectionAtk.cpp:20: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 189088 [details] updated style
Comment on attachment 189088 [details] updated style View in context: https://bugs.webkit.org/attachment.cgi?id=189088&action=review Informal review: This patch looks good to me overall. The only suggestion I would make is that perhaps you would be interested in adding some unit test at some point for EFL at some point, so you make sure this doesn't get broken in the future for your port, although I guess that's probably something you're already thinking of doing in a later stage. As for the GTK port, this patch should be fine as it is as long as the patch does not break any layout/unit test. Last, see some minor comments below... > Source/WebCore/ChangeLog:14 > + * GNUmakefile.list.am: > + * PlatformEfl.cmake: > + * editing/FrameSelection.h: > + (WebCore): As long as it's possible and make sense, you should provide a description in the ChangeLog for each of these items. > Source/WebCore/ChangeLog:19 > + * editing/atk/FrameSelectionAtk.cpp: Renamed from Source/WebCore/editing/gtk/FrameSelectionGtk.cpp. > + (WebCore): > + (WebCore::emitTextSelectionChange): > + (WebCore::maybeEmitTextFocusChange): > + (WebCore::FrameSelection::notifyAccessibilityForSelectionChange): Maybe for this ones is not needed as you are just renaming a path > Source/WebCore/editing/atk/FrameSelectionAtk.cpp:3 > + * Copyright (C) 2013 Samsung Electronics I think it's a bit overkill to add the Copyright line here if you're just renaming the file :-)
(In reply to comment #4) > > Source/WebCore/editing/atk/FrameSelectionAtk.cpp:3 > > + * Copyright (C) 2013 Samsung Electronics > > I think it's a bit overkill to add the Copyright line here if you're just renaming the file :-) There is small change in includes there. #if PLATFORM(EFL)....#endif
(In reply to comment #5) > (In reply to comment #4) > > > > Source/WebCore/editing/atk/FrameSelectionAtk.cpp:3 > > > + * Copyright (C) 2013 Samsung Electronics > > > > I think it's a bit overkill to add the Copyright line here if you're just renaming the file :-) > > There is small change in includes there. #if PLATFORM(EFL)....#endif Yes, but that's just about including files, not actual code in the file unit, so I still think it's a bit overkill to add the copyright line.
Created attachment 189519 [details] applied Mario's comments
Comment on attachment 189519 [details] applied Mario's comments View in context: https://bugs.webkit.org/attachment.cgi?id=189519&action=review > Source/WebCore/editing/atk/FrameSelectionAtk.cpp:104 > + // Emit relatedsignals. Nit: relatedsignals -> related signals. In actuality, all of the comments in this function are pretty redundant though and can be removed.
Created attachment 190064 [details] Applied Martin's suggestions.
Comment on attachment 190064 [details] Applied Martin's suggestions. Clearing flags on attachment: 190064 Committed r143937: <http://trac.webkit.org/changeset/143937>
All reviewed patches have been landed. Closing bug.