Bug 110221 – [EFL][GTK] Move text selection/focus notification for a11y from gtk to atk directory

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Mariusz Grzegorczyk]

Accessibility needs to be notified about text selection/focus for EFL port.

[Mariusz Grzegorczyk]

Created attachment 189083 [details]
patch

[WebKit Review Bot]

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.

[Mariusz Grzegorczyk]

Created attachment 189088 [details]
updated style

[Mario Sanchez Prada]

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 :-)

[Mariusz Grzegorczyk]

(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

[Mario Sanchez Prada]

(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.

[Mariusz Grzegorczyk]

Created attachment 189519 [details]
applied Mario's comments

[Martin Robinson]

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.

[Mariusz Grzegorczyk]

Created attachment 190064 [details]
Applied Martin's suggestions.

[WebKit Review Bot]

Comment on attachment 190064 [details]
Applied Martin's suggestions.

Clearing flags on attachment: 190064

Committed r143937: <http://trac.webkit.org/changeset/143937>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.