[webkit-reviews] review requested: [Bug 27048] [Gtk] Implement STATE_FOCUSED, STATE_FOCUSABLE, and corresponding events for text objects : [Attachment 76411] Patch proposal + Layout Test

[bugzilla-daemon at webkit.org]

Mario Sanchez Prada <msanchez at igalia.com> has asked  for review:
Bug 27048: [Gtk] Implement STATE_FOCUSED, STATE_FOCUSABLE, and corresponding
events for text objects
https://bugs.webkit.org/show_bug.cgi?id=27048

Attachment 76411: Patch proposal + Layout Test
https://bugs.webkit.org/attachment.cgi?id=76411&action=review

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Attaching patch addressing all those issues. See details below...

(In reply to comment #8)
> (From update of attachment 74224 [details])
> View in context:
https://bugs.webkit.org/attachment.cgi?id=74224&action=review
> 
> Mostly looks good to me, just have some doubts/comments, see below.
> 
> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:506
> > +	 Settings* settings = coreObject->document()->frame()->settings();
> 
> Some of those can be in some situations NULL. At the very least frame(). If
this function can never be called in those cases then you should ASSERT.

Fixed, by adding extra checks to the function (better to be safe!)

> > WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:521
> > +	 return !selectionBelongsToObject(coreObject, selection) ||
selection.isNone() ? 0 : 1;
> 
> How is this grouping exactly? It's a quite confusing statement.

Fixed by replacing it for equivalent code.

> > WebCore/editing/gtk/SelectionControllerGtk.cpp:48
> > +	 static RefPtr<AccessibilityObject> oldObject = 0;
> 
> Mmm, I think you want to use DEFINE_STATIC_LOCAL here?

Fixed.

> > WebCore/editing/gtk/SelectionControllerGtk.cpp:82
> > +	 // Reset lastFocuseNode and return for no valid selections.
> 
> I don't see that reset happening?

I meant resetting it to 'zero'. Fixed comment

Now asking for review again then...