Bug 36649 – operator== and operator!= for Position is not used

• Component: DOM
• Status: RESOLVED INVALID

[Hajime Morrita]

operator== and operator!= for Position, defined in Position.h, is no longer used
and should be removed.
As Bug 24763, there is disagreement how these operators behave.
That also encourage us to remove them to avoid confusion. (As long as they aren't used.)

[Hajime Morrita]

Created attachment 51721 [details]
v0; just removed them.

[Eric Seidel (no email)]

I think I wrote them in hopes of wider position deployment.

[Eric Seidel (no email)]

Comment on attachment 51721 [details]
v0; just removed them.

But OK.

[Ryosuke Niwa]

(In reply to comment #2)
> I think I wrote them in hopes of wider position deployment.

I see your point that VisiblePosition might need to go away at some point, but we probably need to other refactoring in advance. In particular, we should first get rid of functions / editing commands (IndentOutdentCommand, InsertListCommmand, moveParagraph, etc...) that change selection to modify ranges.  It'll probably takes years to this refactoring because they're heavily depended on by a large portion of editing code.  If we had started replacing VisiblePosition by Position now, we'll end up mixing code that directly manipulates Position, VisiblePosition, and Selection simultaneously and that's very nasty.

For example, moveParagraphWithClones currently uses setEndSelection at http://trac.webkit.org/browser/trunk/WebCore/editing/CompositeEditCommand.cpp#L851 but if we had a version of deleteSelection that didn't rely on selection, we would be able to call this function without having to restore the selection afterwards.

[Eric Seidel (no email)]

I don't think that VisiblePosition should go away.  It should be used less in the core editing code, yes.  It's mostly a user-interaction concept.  It embodies the set of positions where the user could actually place the cursor.  But the internals of the editing engine needs to deal with Positions which are anywhere in the DOM.

[Eric Seidel (no email)]

Comment on attachment 51721 [details]
v0; just removed them.

This patch seems to be hanging the commit-queue.  Specifically DumpRenderTree runs forever.  I think ReportCrash is trying to make a crash log but failing.  I suspect that this change causes tests to fail.

Marking r- until we're sure this doesn't cause failures on mac.

[Eric Seidel (no email)]

Created attachment 51848 [details]
crash log from commit bot

This caused commit-bot to spin because it was crashing DRT for every test (or so it seems.  I have so many crash logs from DRT that the system started culling logs!

[Eric Seidel (no email)]

I expect we'll end up closing this bug as invalid.

[Hajime Morrita]

Eric, thank you for reviewing and I'm sorry for my slow reply.

> I expect we'll end up closing this bug as invalid.
Although I could not figure out what make stuck the test, 
But I understand that this fix is not essential and 
We need some discussion around Position/VisiblePosition design as Ryosuke mentioned.
So I'll close this at this time.