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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=110221





--- Comment #4 from Mario Sanchez Prada <mario at webkit.org>  2013-02-21 05:36:48 PST ---
(From update of attachment 189088)
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 :-)

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.