[webkit-reviews] review requested: [Bug 25677] [GTK] Implement support for get_character_extents and get_range_extents : [Attachment 45535] proposed fix, includes unit test

[bugzilla-daemon at webkit.org]

Joanmarie Diggs <joanmarie.diggs at gmail.com> has asked  for review:
Bug 25677: [GTK] Implement support for get_character_extents and
get_range_extents
https://bugs.webkit.org/show_bug.cgi?id=25677

Attachment 45535: proposed fix, includes unit test
https://bugs.webkit.org/attachment.cgi?id=45535&action=review

------- Additional Comments from Joanmarie Diggs <joanmarie.diggs at gmail.com>
Essentially [1] the same patch as before, but with a unit test added.

Notes:

1. The change to the code proper between the previous version and this version
is the addition of some sanity checking for two possible conditions:

a. A call to atk_text_get_range_extents() is made with an end offset of -1 (to
represent the last character). This is extremely likely to occur. I wasn't
handling it before. That having been said, now that it's being handled here, it
turns out that we're claiming some pretty bogus extents for end offset of -1.
After much hair pulling, I started examining the values from other applications
(e.g. Gedit, Firefox) under the same conditions. The results were equally (and
in some cases more) bogus. So this is not a WebKit bug. Once I track down the
true source of this bug (gail perhaps?) and it gets fixed, I believe we will
automatically start doing the right thing here.

b. A call to atk_text_get_range_extents() is made with an end offset that is
greater than the last character. Our choices are (0, 0, 0, 0) and to assume
that the caller erred but wants the extents for the range of text beginning
with the start offset provided through the final character.  Personally, I
think we should go with the latter, and have done so in this version of the
patch. If anyone feels strongly otherwise, I can rip that bit out.

2. Since unit tests are designed (as I understand it) to test platform-specific
API calls, this new functionality seemed to call out for a unit test rather
than a layout test. I believe you'll find that the unit test I wrote is quite
thorough and that it avoids (I hope) the potential for inconsistent results
based on the environment of the individual (or bot) running the test.

The disadvantage seems to be that we can't test the coordinate type of
ATK_XY_SCREEN; just ATK_XY_WINDOW. When I attempt to add ATK_XY_SCREEN to the
unit test, the test aborts with the following error:

~~~~~
Gdk-CRITICAL **: gdk_window_get_origin: assertion `GDK_IS_WINDOW (window)'
failed
aborting...
~~~~~

I presume this is because unit tests don't cause an actual window to be
rendered. :-)

a. Is it possible to write a unit test which forces an actual window to be
rendered?

b. Do we care?

On the latter front, I'm not convinced it's a big deal given that we will be
thoroughly testing the window-based extents via the included unit test. And I
think that it might be more trouble than it's worth to generate a layout test
which presumably has the potential for inconsistent results (i.e. failures)
based on the environment of the individual (or bot) running the test. But, I'll
of course do whatever y'all suggest.

Finally, I just added my test to testatk.c. I'm not sure at what point it might
be worth splitting the Atk unit tests up. If now is the time, please let me
know. If now is not the time.... I'm guessing that the style queue bot is going
to spit up on this patch due to the new test adhering to the style guidelines
for C++ and not the style guidelines for C. Last time this issue came up, Xan
said to not change the test and to disregard the style bot in this particular
instance. I'm sticking with that advice until I hear otherwise. :-)

Please review. Thanks!