Bug 192613 - Make HTMLConverter take two Positions in preparation to make it work with shadow DOM
Summary: Make HTMLConverter take two Positions in preparation to make it work with sha...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 192640
  Show dependency treegraph
 
Reported: 2018-12-11 21:36 PST by Ryosuke Niwa
Modified: 2018-12-12 20:05 PST (History)
6 users (show)

See Also:


Attachments
Cleanup (9.06 KB, patch)
2018-12-11 21:42 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-12-11 21:36:18 PST
In order to make HTMLConverter compatible with shadow DOM, we need to make it take two Positions instead of a Range
so that it can represent a selection which spans across shadow boundaries.
Comment 1 Ryosuke Niwa 2018-12-11 21:42:50 PST
Created attachment 357099 [details]
Cleanup
Comment 2 Darin Adler 2018-12-12 09:19:23 PST
Comment on attachment 357099 [details]
Cleanup

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

Wish we had a structure with two Positions instead of two separate ones.

If we take two positions, do we need to take precautions against traversing the entire document if end is not after start? Range takes care of that, but two positions don’t.

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2459
> +    HTMLConverter converter(range.startPosition(), range.endPosition());

I would have used { } syntax. And maybe not named the local variable.

> Source/WebCore/editing/cocoa/HTMLConverter.mm:2467
> +    HTMLConverter converter(range->startPosition(), range->endPosition());

Ditto.
Comment 3 Ryosuke Niwa 2018-12-12 13:11:04 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 357099 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357099&action=review
> 
> Wish we had a structure with two Positions instead of two separate ones.
> 
> If we take two positions, do we need to take precautions against traversing
> the entire document if end is not after start? Range takes care of that, but
> two positions don’t.

That is a good point. We have an early return for that in serializePreservingVisualAppearanceInternal too:

if (!comparePositions(start, end))
    return emptyString();

We should probably update it to:

if (comparePositions(start, end) >= 0)
    return emptyString();

I'm gonna add the following early return to HTMLConverter::convert():
if (comparePositions(m_start, m_end) > 0)
    return nil;

> 
> > Source/WebCore/editing/cocoa/HTMLConverter.mm:2459
> > +    HTMLConverter converter(range.startPosition(), range.endPosition());
> 
> I would have used { } syntax. And maybe not named the local variable.
> 
> > Source/WebCore/editing/cocoa/HTMLConverter.mm:2467
> > +    HTMLConverter converter(range->startPosition(), range->endPosition());
> 
> Ditto.

Fixed.
Comment 4 Ryosuke Niwa 2018-12-12 13:20:12 PST
Committed r239127: <https://trac.webkit.org/changeset/239127>
Comment 5 Radar WebKit Bug Importer 2018-12-12 13:21:28 PST
<rdar://problem/46673257>