Bug 90807 - WebSurroundingText layout tests should use the same code path as the rest of the feature
Summary: WebSurroundingText layout tests should use the same code path as the rest of ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Leandro Graciá Gil
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-09 11:39 PDT by Leandro Graciá Gil
Modified: 2012-07-10 09:37 PDT (History)
9 users (show)

See Also:


Attachments
Patch (12.63 KB, patch)
2012-07-10 07:11 PDT, Leandro Graciá Gil
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Leandro Graciá Gil 2012-07-09 11:39:40 PDT
Currently WebSurroundingText has a custom initialize method based on node offsets that is used only by the LayoutTestController. Instead of this we should make the tests work with coordinates and use the same code path as the rest of the feature.
Comment 1 Leandro Graciá Gil 2012-07-10 07:11:43 PDT
Created attachment 151455 [details]
Patch
Comment 2 Leandro Graciá Gil 2012-07-10 07:17:19 PDT
Comment on attachment 151455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151455&action=review

> Source/WebKit/chromium/src/WebSurroundingText.cpp:43
> +void WebSurroundingText::initialize(const WebHitTestResult& hitTestResult, size_t maxLength)

This method is currently in use by Chromium. However, we can switch to the new one and remove this with a 3-sided patch.

> LayoutTests/platform/chromium/editing/surrounding-text/surrounding-text.html:-36
> -    shouldBeEqualToString('surroundingText(\'<button>.</button>12345<p id="here">6789 12345</p>6789<button>.</button>\', 100, 5)', '');

Invalid offsets throw a JS exception in findOffsetCoordinates now.
Comment 3 WebKit Review Bot 2012-07-10 07:19:44 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 4 Adam Barth 2012-07-10 08:08:50 PDT
Comment on attachment 151455 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151455&action=review

>> Source/WebKit/chromium/src/WebSurroundingText.cpp:43
>> +void WebSurroundingText::initialize(const WebHitTestResult& hitTestResult, size_t maxLength)
> 
> This method is currently in use by Chromium. However, we can switch to the new one and remove this with a 3-sided patch.

If you like, you can add a FIXME comment to the public header to this effect.
Comment 5 WebKit Review Bot 2012-07-10 09:01:44 PDT
Comment on attachment 151455 [details]
Patch

Clearing flags on attachment: 151455

Committed r122225: <http://trac.webkit.org/changeset/122225>
Comment 6 WebKit Review Bot 2012-07-10 09:01:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Leandro Graciá Gil 2012-07-10 09:37:20 PDT
Committed r122230: <http://trac.webkit.org/changeset/122230>