Bug 15954 - Move DOM Selection operations out of SelectionController
Summary: Move DOM Selection operations out of SelectionController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-11-12 07:18 PST by Alexey Proskuryakov
Modified: 2007-11-12 22:26 PST (History)
0 users

See Also:


Attachments
proposed patch (41.48 KB, patch)
2007-11-12 07:28 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2007-11-12 07:18:11 PST
From bug 15922 comment 2:

> DOMSelection is the DOM's "selection object" API. It has the same relationship
> to SelectionController that DOMWindow has to Frame. Right now there's a problem
> in that we were "grooming" SelectionController to be this DOM object. So it has
> many functions that should simply be moved into DOMSelection.

Patch forthcoming.
Comment 1 Alexey Proskuryakov 2007-11-12 07:28:39 PST
Created attachment 17202 [details]
proposed patch
Comment 2 Darin Adler 2007-11-12 12:34:01 PST
Comment on attachment 17202 [details]
proposed patch

+    if (selectionController->isNone())
+        return "None";
+    else if (selectionController->isCaret())
+        return "Caret";
+    else
+        return "Range";

We normally don't do else after return.

+        || node->offsetInCharacters() && offset > caretMaxOffset(node)
+        || !node->offsetInCharacters() && offset > (int)node->childNodeCount()) {

Could probably rewrite this using ? : to not call offsetInCharacters twice. Or maybe there should be a helper function for this relatively common operation?

+        // Safari Selection Object API

Wow, I'm really surprised to learn that baseNode is Safari-specific!

Great patch! r=me
Comment 3 Alexey Proskuryakov 2007-11-12 22:26:19 PST
Committed revision 27744.

(In reply to comment #2)
> We normally don't do else after return.

Fixed.

> Could probably rewrite this using ? : to not call offsetInCharacters twice.

Done.

> Or maybe there should be a helper function for this relatively common operation?

That's something I'd like to avoid - currently, there seem to be too many editing helper functions that aren't quite clear about what they do, because the concept of a position is messed up (editing positions vs. DOM ones vs. rendering ones).