Bug 68333
| Summary: | [meta] TextIterator refactoring | ||
|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> |
| Component: | HTML Editing | Assignee: | Nobody <webkit-unassigned> |
| Status: | NEW | ||
| Severity: | Normal | CC: | ap, enrica, inferno |
| Priority: | P2 | ||
| Version: | 528+ (Nightly build) | ||
| Hardware: | Unspecified | ||
| OS: | Unspecified | ||
| Bug Depends on: | 68330, 68331, 68332 | ||
| Bug Blocks: | |||
Ryosuke Niwa
We should cleanup TextIterator.
| Attachments | ||
|---|---|---|
| Add attachment proposed patch, testcase, etc. |
Alexey Proskuryakov
Could you please describe the goals of this cleanup? From the bugs currently in blocking list, it's not clear to me what the problem is, and when this refactoring can be considered done.
Ryosuke Niwa
(In reply to comment #1)
> Could you please describe the goals of this cleanup? From the bugs currently in blocking list, it's not clear to me what the problem is, and when this refactoring can be considered done.
The goal of this refactoring is to make TextIterator code more readable. By definition, it's unclear when the factoring is done. We have to use our judgement to decide when it's done.
Alexey Proskuryakov
What specifically is the problem with current code that you intend to solve? Can you describe a general direction for the changes?
I haven't dealt with it too much, but it always seemed well written and understandable to me.
Ryosuke Niwa
(In reply to comment #3)
> What specifically is the problem with current code that you intend to solve? Can you describe a general direction for the changes?
>
> I haven't dealt with it too much, but it always seemed well written and understandable to me.
If TextIterator and its variants are really well-written, I wouldn't expect bugs listed below to exist:
https://bugs.webkit.org/show_bug.cgi?id=61110
https://bugs.webkit.org/show_bug.cgi?id=63334
https://bugs.webkit.org/show_bug.cgi?id=65296
https://bugs.webkit.org/show_bug.cgi?id=66086
Alexey Proskuryakov
Thanks. Could you please also give some insight into the first two questions?
> What specifically is the [readability] problem with current code that you intend to solve? Can you describe a general direction for the changes?
Ryosuke Niwa
(In reply to comment #5)
> Thanks. Could you please also give some insight into the first two questions?
Sure.
> > What specifically is the [readability] problem with current code that you intend to solve? Can you describe a general direction for the changes?
I think one of the issues is that TextIterator has a lot of member variables and so many different behaviors, so I'm hoping that we can simplify some of that.
Also functions like TextIterator::advance, TextIterator::handleTextNode, TextIterator::handleTextBox should be broken down into smaller pieces with descriptive function names. Maybe bundling some member functions into a struct/class with a bunch of helper functions (like what we did with line layout code) might help.
Finally, we should really consider sharing code between VisiblePosition, visible_units, TextIterator, etc... I think one of the issues we have now is that people working on rendering code don't necessary update all those files when they add new feature (first-letter is a good example) and some class/function gets out-of-date. Sharing code between those classes and centralizing it in one place will probably mitigate this issue to some extent.