Bug 110221 - [EFL][GTK] Move text selection/focus notification for a11y from gtk to atk directory
Summary: [EFL][GTK] Move text selection/focus notification for a11y from gtk to atk di...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-19 07:13 PST by Mariusz Grzegorczyk
Modified: 2013-02-25 09:32 PST (History)
6 users (show)

See Also:


Attachments
patch (10.84 KB, patch)
2013-02-19 07:20 PST, Mariusz Grzegorczyk
no flags Details | Formatted Diff | Diff
updated style (10.84 KB, patch)
2013-02-19 07:28 PST, Mariusz Grzegorczyk
no flags Details | Formatted Diff | Diff
applied Mario's comments (11.05 KB, patch)
2013-02-21 06:40 PST, Mariusz Grzegorczyk
mrobinson: review+
Details | Formatted Diff | Diff
Applied Martin's suggestions. (10.91 KB, patch)
2013-02-25 08:11 PST, Mariusz Grzegorczyk
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mariusz Grzegorczyk 2013-02-19 07:13:41 PST
Accessibility needs to be notified about text selection/focus for EFL port.
Comment 1 Mariusz Grzegorczyk 2013-02-19 07:20:16 PST
Created attachment 189083 [details]
patch
Comment 2 WebKit Review Bot 2013-02-19 07:23:19 PST
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.
Comment 3 Mariusz Grzegorczyk 2013-02-19 07:28:25 PST
Created attachment 189088 [details]
updated style
Comment 4 Mario Sanchez Prada 2013-02-21 05:36:48 PST
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 :-)
Comment 5 Mariusz Grzegorczyk 2013-02-21 06:00:22 PST
(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
Comment 6 Mario Sanchez Prada 2013-02-21 06:09:03 PST
(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.
Comment 7 Mariusz Grzegorczyk 2013-02-21 06:40:50 PST
Created attachment 189519 [details]
applied Mario's comments
Comment 8 Martin Robinson 2013-02-25 07:24:40 PST
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.
Comment 9 Mariusz Grzegorczyk 2013-02-25 08:11:07 PST
Created attachment 190064 [details]
Applied Martin's suggestions.
Comment 10 WebKit Review Bot 2013-02-25 09:32:14 PST
Comment on attachment 190064 [details]
Applied Martin's suggestions.

Clearing flags on attachment: 190064

Committed r143937: <http://trac.webkit.org/changeset/143937>
Comment 11 WebKit Review Bot 2013-02-25 09:32:19 PST
All reviewed patches have been landed.  Closing bug.