Bug 25494 - Add more Position constructor functions and make them all inline
Summary: Add more Position constructor functions and make them all inline
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-04-30 15:00 PDT by Eric Seidel (no email)
Modified: 2009-09-09 16:26 PDT (History)
5 users (show)

See Also:


Attachments
Add PositionConstructors.h (19.84 KB, patch)
2009-04-30 15:00 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add PositionConstructors.h (20.20 KB, patch)
2009-04-30 15:37 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Rename positionBeforeNode to positionInParentBeforeNode (23.43 KB, patch)
2009-04-30 16:15 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add more position constructors (6.07 KB, patch)
2009-04-30 16:15 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add PositionCreationFunctions.h (20.42 KB, patch)
2009-06-01 17:19 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Rename positionBeforeNode to positionInParentBeforeNode (23.46 KB, patch)
2009-06-01 17:19 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add more position constructors (6.09 KB, patch)
2009-06-01 17:19 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Add more position constructors (5.33 KB, patch)
2009-07-30 12:07 PDT, Eric Seidel (no email)
abarth: review+
Details | Formatted Diff | Diff
Rename positionBeforeNode to positionInParentBeforeNode (25.97 KB, patch)
2009-07-30 12:07 PDT, Eric Seidel (no email)
abarth: review+
Details | Formatted Diff | Diff
Move them back to Position.h per mjs's request (7.45 KB, patch)
2009-07-30 12:07 PDT, Eric Seidel (no email)
abarth: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-04-30 15:00:06 PDT
add PositionConstructors.h in preparation for adding more position constructor functions

It seems no matter what I do, the Position constructors remain confusing.  So I think we'll likely move away from them in the future and move towards constructor functions.
Comment 1 Eric Seidel (no email) 2009-04-30 15:00:29 PDT
Created attachment 29923 [details]
Add PositionConstructors.h

 26 files changed, 142 insertions(+), 51 deletions(-)
Comment 2 Eric Seidel (no email) 2009-04-30 15:01:24 PDT
My next step will be to rename positionBeforeNode to positionInParentBeforeNode and add a positionBeforeNode which returns a neighbor anchored position (which is more efficient to construct and sufficient in many cases... and better in some others!)
Comment 3 Eric Seidel (no email) 2009-04-30 15:37:29 PDT
Created attachment 29924 [details]
Add PositionConstructors.h

 26 files changed, 148 insertions(+), 51 deletions(-)
Comment 4 Eric Seidel (no email) 2009-04-30 15:37:46 PDT
Comment on attachment 29923 [details]
Add PositionConstructors.h

Uploaded one with a slightly nicer changelog.
Comment 5 Eric Seidel (no email) 2009-04-30 16:15:12 PDT
Created attachment 29926 [details]
Rename positionBeforeNode to positionInParentBeforeNode

 15 files changed, 90 insertions(+), 39 deletions(-)
Comment 6 Eric Seidel (no email) 2009-04-30 16:15:13 PDT
Created attachment 29927 [details]
Add more position constructors

 6 files changed, 64 insertions(+), 8 deletions(-)
Comment 7 Maciej Stachowiak 2009-05-22 01:35:40 PDT
Comment on attachment 29924 [details]
Add PositionConstructors.h

I don't think calling this header PositionConstructors.h is right. "Constructor" means a specific thing in C++, and these aren't it, so it's kind of confusing.

I actually think it's fine for these functions to be in Position.h, since they are effectively part of the interface to Position, but if you can think of a good alternate header name, that's ok too.

Also, while I'm nitpicking, I don't think the names of functions that take nodes necessarily need Node in them.It could just be:

static inline Position positionBefore(const Node* node)
static inline Position positionAfter(const Node* node)
static inline Position firstDeepEditingPosition(Node* anchorNode)
static inline Position lastDeepEditingPosition(Node* anchorNode)

r- for the header name. Function renames are optional.
Comment 8 Maciej Stachowiak 2009-05-22 01:37:03 PDT
Comment on attachment 29926 [details]
Rename positionBeforeNode to positionInParentBeforeNode

I repeat my earlier comments about removing "Node" from the names of these functions. Otherwise looks ok, but this depends on the previous patch which I r-'d, so r- for this too.
Comment 9 Maciej Stachowiak 2009-05-22 01:38:36 PDT
Comment on attachment 29927 [details]
Add more position constructors

r- because this depends on the previous r-'d patches. Also: I would request that you not add unused functions. Please add them with at least one real use that has test coverage, so we don't have untested dead code in the tree.
Comment 10 Maciej Stachowiak 2009-05-22 01:39:42 PDT
Comment on attachment 29924 [details]
Add PositionConstructors.h

I forgot to mention before. Functions in headers should just be "inline", not "static inline". This also applies to the other two patches.
Comment 11 Eric Seidel (no email) 2009-06-01 16:00:30 PDT
(In reply to comment #7)
> (From update of attachment 29924 [details] [review])
> I don't think calling this header PositionConstructors.h is right.
> "Constructor" means a specific thing in C++, and these aren't it, so it's kind
> of confusing.
> 
> I actually think it's fine for these functions to be in Position.h, since they
> are effectively part of the interface to Position, but if you can think of a
> good alternate header name, that's ok too.

I think a separate header is better for keeping more classes out of Position.h
Comment 12 Eric Seidel (no email) 2009-06-01 17:16:54 PDT
(In reply to comment #9)
> (From update of attachment 29927 [details] [review])
> r- because this depends on the previous r-'d patches. Also: I would request
> that you not add unused functions. Please add them with at least one real use
> that has test coverage, so we don't have untested dead code in the tree.

These get used in later patches in this series.  But I've been trying to keep the patches small.
Comment 13 Eric Seidel (no email) 2009-06-01 17:18:07 PDT
(In reply to comment #8)
> (From update of attachment 29926 [details] [review])
> I repeat my earlier comments about removing "Node" from the names of these
> functions. Otherwise looks ok, but this depends on the previous patch which I
> r-'d, so r- for this too.

These get renamed in the next patch to positionInParentBeforeNode.  I could rename that to positionBeforeInParent, but I think that's more confusion.
Comment 14 Eric Seidel (no email) 2009-06-01 17:19:13 PDT
Ok, uploading a new first patch only changing the header name from PositionContructors to PositionCreationFunctions.h

No other changes to the patches (except making them apply to TOT).
Comment 15 Eric Seidel (no email) 2009-06-01 17:19:32 PDT
Created attachment 30846 [details]
Add PositionCreationFunctions.h

 26 files changed, 148 insertions(+), 51 deletions(-)
Comment 16 Eric Seidel (no email) 2009-06-01 17:19:34 PDT
Created attachment 30847 [details]
Rename positionBeforeNode to positionInParentBeforeNode

 15 files changed, 90 insertions(+), 39 deletions(-)
Comment 17 Eric Seidel (no email) 2009-06-01 17:19:36 PDT
Created attachment 30848 [details]
Add more position constructors

 6 files changed, 64 insertions(+), 8 deletions(-)
Comment 18 Eric Seidel (no email) 2009-06-02 13:16:37 PDT
Oh, I remember now why I split these out of Position.h... so that Position.h didn't have to include htmlediting.h for lastOffsetForEditing.  Eventually lastOffsetForEditing will probably move into some Position file.

Maybe I should just move first/lastDeepEditingPositionForNode into htmlediting.h and leave the rest in Position.h.

It sounds like both Justin and Maciej think these patches are just fine... I just need an r+ from someone. ;)
Comment 19 Maciej Stachowiak 2009-06-23 20:39:52 PDT
Comment on attachment 30846 [details]
Add PositionCreationFunctions.h

r=me but please apply your recently suggested plan to keep most of this in Position.h itself.
Comment 20 Maciej Stachowiak 2009-06-23 20:41:53 PDT
Comment on attachment 30847 [details]
Rename positionBeforeNode to positionInParentBeforeNode

This still has "static inline" functions in headers, which I am pretty sure is wrong. I think you missed this comment before.
Comment 21 Maciej Stachowiak 2009-06-23 20:42:36 PDT
Comment on attachment 30848 [details]
Add more position constructors

Once again I am pretty sure functions in headers should just be "inline", not "static inline".
Comment 22 Eric Seidel (no email) 2009-07-30 12:07:38 PDT
Created attachment 33799 [details]
Add more position constructors


---
 6 files changed, 62 insertions(+), 8 deletions(-)
Comment 23 Eric Seidel (no email) 2009-07-30 12:07:47 PDT
Created attachment 33800 [details]
Rename positionBeforeNode to positionInParentBeforeNode


---
 16 files changed, 96 insertions(+), 46 deletions(-)
Comment 24 Eric Seidel (no email) 2009-07-30 12:07:54 PDT
Created attachment 33801 [details]
Move them back to Position.h per mjs's request


---
 7 files changed, 68 insertions(+), 45 deletions(-)
Comment 25 Eric Seidel (no email) 2009-07-30 12:09:39 PDT
Comment on attachment 33799 [details]
Add more position constructors

A victim of bug 27849.
Comment 26 Eric Seidel (no email) 2009-07-30 12:09:44 PDT
Comment on attachment 33800 [details]
Rename positionBeforeNode to positionInParentBeforeNode

A victim of bug 27849.
Comment 27 Eric Seidel (no email) 2009-07-30 12:16:02 PDT
The patches are posted here in reverse order.  Again due to some minor troubles with post-commits. ;)
Comment 28 Maciej Stachowiak 2009-08-11 02:15:43 PDT
Reviewed Notification.cpp. Event listener logic looks flawless now. Kudos!
Comment 29 Maciej Stachowiak 2009-08-11 02:18:20 PDT
Oops, commented in wrong bug.
Comment 30 Adam Barth 2009-08-27 23:10:49 PDT
Comment on attachment 33799 [details]
Add more position constructors

yay for removing duplicate code
Comment 31 Adam Barth 2009-08-27 23:13:03 PDT
Comment on attachment 33800 [details]
Rename positionBeforeNode to positionInParentBeforeNode

This patch appears to be a pure alpha-transformation.
Comment 32 Eric Seidel (no email) 2009-09-09 16:26:37 PDT
Landed as r48233, r48234, r48235.