877974 - reorg getTextAtOffset for line boundary

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

regressions are fixed by follow ups

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 756395 [details] [diff] [review]
patch

>diff --git a/accessible/src/generic/HyperTextAccessible.cpp b/accessible/src/generic/HyperTextAccessible.cpp
>--- a/accessible/src/generic/HyperTextAccessible.cpp
>+++ b/accessible/src/generic/HyperTextAccessible.cpp
>@@ -777,18 +777,19 @@ HyperTextAccessible::GetRelativeOffset(n
>       -- hyperTextOffset;
>     }
>   }
> 
>   return hyperTextOffset;
> }
> 
> int32_t
>-HyperTextAccessible::FindWordBoundary(int32_t aOffset, nsDirection aDirection,
>-                                      EWordMovementType aWordMovementType)
>+HyperTextAccessible::FindBoundary(int32_t aOffset, nsDirection aDirection,
>+                                  nsSelectionAmount aAmount,
>+                                  EWordMovementType aWordMovementType)

why do we need both word movement type and selection amount?  What is selection stuff doing here anyway?

>   // Return hypertext offset of the boundary of the found word.
>   return GetRelativeOffset(mDoc->PresShell(), frameAtOffset, offsetInFrame,
>-                           accAtOffset, eSelectWord, aDirection,
>-                           (aWordMovementType == eStartWord),
>+                           accAtOffset, aAmount, aDirection,
>+                           (aWordMovementType == eStartWord || aAmount == eSelectBeginLine),
>                            aWordMovementType);

what is this about?

>+    case BOUNDARY_LINE_START: {
>+      // Home key, arrow down and if not on last line then home key.

doesn't really make sense or help me understand.

>+      *aStartOffset = FindLineBoundary(offset, eDirPrevious, eSelectBeginLine);
>+      *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectLine);
>+      int32_t tmpOffset = FindLineBoundary(*aEndOffset, eDirPrevious, eSelectBeginLine);
>+      if (tmpOffset != *aStartOffset)

how can that be true?  Please add comment giving case.

>+    case BOUNDARY_LINE_END: {
>+      // In contrast to word end boundary we follow the spec here. End key,
>+      // then up arrow and if not on first line then end key.

also not really useful.

>+      *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
>+      *aStartOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
>+      if (*aStartOffset != 0)
>+        *aStartOffset = FindLineBoundary(*aStartOffset, eDirNext, eSelectEndLine);

this makes no sense when can this if be needed?

>   int32_t FindWordBoundary(int32_t aOffset, nsDirection aDirection,
>-                           EWordMovementType aWordMovementType);
>+                           EWordMovementType aWordMovementType)
>+  {
>+    return FindBoundary(aOffset, aDirection, eSelectWord, aWordMovementType);
>+  }
>+
>+  /**
>+   * Return an offset of the found line boundary.
>+   */
>+  int32_t FindLineBoundary(int32_t aOffset, nsDirection aDirection,
>+                           nsSelectionAmount aAmount)

its annoying these have slightly different args

>   <script type="application/javascript">
>     if (navigator.platform.startsWith("Mac")) {
>-      SimpleTest.expectAssertions(0, 16);
>+      SimpleTest.expectAssertions(0, 12);
>     } else {
>-      SimpleTest.expectAssertions(16);
>+      SimpleTest.expectAssertions(12);
>     }

I bet you don't need mac check iirc assertion checking was introduced same time tests were turned on for mac so it's probably not actually intermittant there

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> >-HyperTextAccessible::FindWordBoundary(int32_t aOffset, nsDirection aDirection,
> >-                                      EWordMovementType aWordMovementType)
> >+HyperTextAccessible::FindBoundary(int32_t aOffset, nsDirection aDirection,
> >+                                  nsSelectionAmount aAmount,
> >+                                  EWordMovementType aWordMovementType)
> 
> why do we need both word movement type and selection amount?  What is
> selection stuff doing here anyway?

to reuse it for line boundaries

> >   // Return hypertext offset of the boundary of the found word.
> >   return GetRelativeOffset(mDoc->PresShell(), frameAtOffset, offsetInFrame,
> >-                           accAtOffset, eSelectWord, aDirection,
> >-                           (aWordMovementType == eStartWord),
> >+                           accAtOffset, aAmount, aDirection,
> >+                           (aWordMovementType == eStartWord || aAmount == eSelectBeginLine),
> >                            aWordMovementType);
> 
> what is this about?

something to make it working for line boundaries :) a code is not good shape, hopefully we will get it in the end

> >+    case BOUNDARY_LINE_START: {
> >+      // Home key, arrow down and if not on last line then home key.
> 
> doesn't really make sense or help me understand.

those FindLineBoundary calls corresponds to "home key", "arrow down" and "home key". It was meant being more descriptive than those calls :)

> >+      *aStartOffset = FindLineBoundary(offset, eDirPrevious, eSelectBeginLine);
> >+      *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectLine);
> >+      int32_t tmpOffset = FindLineBoundary(*aEndOffset, eDirPrevious, eSelectBeginLine);
> >+      if (tmpOffset != *aStartOffset)
> 
> how can that be true?  Please add comment giving case.

if we were at the last line then they equals (the comment above says that: "Home key, arrow down and if not on last line then home key."

> >+      *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
> >+      *aStartOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
> >+      if (*aStartOffset != 0)
> >+        *aStartOffset = FindLineBoundary(*aStartOffset, eDirNext, eSelectEndLine);
> 
> this makes no sense when can this if be needed?

a code was changed, I will attach a new patch.

> >   int32_t FindWordBoundary(int32_t aOffset, nsDirection aDirection,
> >+                           EWordMovementType aWordMovementType)

> >+  int32_t FindLineBoundary(int32_t aOffset, nsDirection aDirection,
> >+                           nsSelectionAmount aAmount)
> 
> its annoying these have slightly different args

PeekOffset is strange a bit. I'm not sure yet but I hope to wrap it nicely in a11y in the end

> >-      SimpleTest.expectAssertions(16);
> >+      SimpleTest.expectAssertions(12);
> >     }
> 
> I bet you don't need mac check iirc assertion checking was introduced same
> time tests were turned on for mac so it's probably not actually intermittant
> there

anyway they will go away when I finish text interface rework (I have a big layered pie of patches in my queue)

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > >-HyperTextAccessible::FindWordBoundary(int32_t aOffset, nsDirection aDirection,
> > >-                                      EWordMovementType aWordMovementType)
> > >+HyperTextAccessible::FindBoundary(int32_t aOffset, nsDirection aDirection,
> > >+                                  nsSelectionAmount aAmount,
> > >+                                  EWordMovementType aWordMovementType)
> > 
> > why do we need both word movement type and selection amount?  What is
> > selection stuff doing here anyway?
> 
> to reuse it for line boundaries

but why do we have something else for words?

> > >   // Return hypertext offset of the boundary of the found word.
> > >   return GetRelativeOffset(mDoc->PresShell(), frameAtOffset, offsetInFrame,
> > >-                           accAtOffset, eSelectWord, aDirection,
> > >-                           (aWordMovementType == eStartWord),
> > >+                           accAtOffset, aAmount, aDirection,
> > >+                           (aWordMovementType == eStartWord || aAmount == eSelectBeginLine),
> > >                            aWordMovementType);
> > 
> > what is this about?
> 
> something to make it working for line boundaries :) a code is not good
> shape, hopefully we will get it in the end

I... see, I guess you'll rewrite this too at some point?

> > >+    case BOUNDARY_LINE_START: {
> > >+      // Home key, arrow down and if not on last line then home key.
> > 
> > doesn't really make sense or help me understand.
> 
> those FindLineBoundary calls corresponds to "home key", "arrow down" and
> "home key". It was meant being more descriptive than those calls :)

it didn't make any sense to me.  I think comments that talk about spec or atleast boundaries would be better

> > >+      *aStartOffset = FindLineBoundary(offset, eDirPrevious, eSelectBeginLine);
> > >+      *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectLine);
> > >+      int32_t tmpOffset = FindLineBoundary(*aEndOffset, eDirPrevious, eSelectBeginLine);
> > >+      if (tmpOffset != *aStartOffset)
> > 
> > how can that be true?  Please add comment giving case.
> 
> if we were at the last line then they equals (the comment above says that:
> "Home key, arrow down and if not on last line then home key."

same

Comment 6

[Trevor Saunders (:tbsaunde)]

> > > >+      *aStartOffset = FindLineBoundary(offset, eDirPrevious, eSelectBeginLine);
> > > >+      *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectLine);
> > > >+      int32_t tmpOffset = FindLineBoundary(*aEndOffset, eDirPrevious, eSelectBeginLine);
> > > >+      if (tmpOffset != *aStartOffset)
> > > 
> > > how can that be true?  Please add comment giving case.
> > 
> > if we were at the last line then they equals (the comment above says that:
> > "Home key, arrow down and if not on last line then home key."

if you assume they can be equal then what is point of if, why not always just assign tmp to *aStartOffset at which point the first call is useless right?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #5)
> (In reply to alexander :surkov from comment #3)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > 
> > > >-HyperTextAccessible::FindWordBoundary(int32_t aOffset, nsDirection aDirection,
> > > >-                                      EWordMovementType aWordMovementType)
> > > >+HyperTextAccessible::FindBoundary(int32_t aOffset, nsDirection aDirection,
> > > >+                                  nsSelectionAmount aAmount,
> > > >+                                  EWordMovementType aWordMovementType)
> > > 
> > > why do we need both word movement type and selection amount?  What is
> > > selection stuff doing here anyway?
> > 
> > to reuse it for line boundaries
> 
> but why do we have something else for words?

I don't follow you. What is idea?

> > something to make it working for line boundaries :) a code is not good
> > shape, hopefully we will get it in the end
> 
> I... see, I guess you'll rewrite this too at some point?

one day I suppose

> > > >+      // Home key, arrow down and if not on last line then home key.
> > > 
> > > doesn't really make sense or help me understand.
> > 
> > those FindLineBoundary calls corresponds to "home key", "arrow down" and
> > "home key". It was meant being more descriptive than those calls :)
> 
> it didn't make any sense to me.  I think comments that talk about spec or
> atleast boundaries would be better

honestly I like this kind of comments because it is descriptive and really short. Also I don't think this style and thus comments are final, I want to change them after or before getTextAfterOffset change (currently my queue contains patches for getTextAtOffset and getTextBeforeOffset). Do you mind?

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #5)
> > (In reply to alexander :surkov from comment #3)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > > 
> > > > >-HyperTextAccessible::FindWordBoundary(int32_t aOffset, nsDirection aDirection,
> > > > >-                                      EWordMovementType aWordMovementType)
> > > > >+HyperTextAccessible::FindBoundary(int32_t aOffset, nsDirection aDirection,
> > > > >+                                  nsSelectionAmount aAmount,
> > > > >+                                  EWordMovementType aWordMovementType)
> > > > 
> > > > why do we need both word movement type and selection amount?  What is
> > > > selection stuff doing here anyway?
> > > 
> > > to reuse it for line boundaries
> > 
> > but why do we have something else for words?
> 
> I don't follow you. What is idea?

I'm not really sure what idea is, but its annoying its much less clear what theese selection amount constants mean that EWordMovementType ones.

> > > something to make it working for line boundaries :) a code is not good
> > > shape, hopefully we will get it in the end
> > 
> > I... see, I guess you'll rewrite this too at some point?
> 
> one day I suppose

do you have any idea why it makes sense to do it?

> > > > >+      // Home key, arrow down and if not on last line then home key.
> > > > 
> > > > doesn't really make sense or help me understand.
> > > 
> > > those FindLineBoundary calls corresponds to "home key", "arrow down" and
> > > "home key". It was meant being more descriptive than those calls :)
> > 
> > it didn't make any sense to me.  I think comments that talk about spec or
> > atleast boundaries would be better
> 
> honestly I like this kind of comments because it is descriptive and really
> short. Also I don't think this style and thus comments are final, I want to
> change them after or before getTextAfterOffset change (currently my queue
> contains patches for getTextAtOffset and getTextBeforeOffset). Do you mind?

I mind that they don't help me understand anything and seem more or less useless.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> I'm not really sure what idea is, but its annoying its much less clear what
> theese selection amount constants mean that EWordMovementType ones.

It's just a wrapper around nsFrame::PeekOffset. What can I do?

> > > > something to make it working for line boundaries :) a code is not good
> > > > shape, hopefully we will get it in the end
> > > 
> > > I... see, I guess you'll rewrite this too at some point?
> > 
> > one day I suppose
> 
> do you have any idea why it makes sense to do it?

it contains too much special cases

> > honestly I like this kind of comments because it is descriptive and really
> > short. Also I don't think this style and thus comments are final, I want to
> > change them after or before getTextAfterOffset change (currently my queue
> > contains patches for getTextAtOffset and getTextBeforeOffset). Do you mind?
> 
> I mind that they don't help me understand anything and seem more or less
> useless.

Sure, I can replace "end key" to "move to the end of line", "home key" to "move to the beginning of line", "up arrow" to "move to previous line". But that'd be too verbose perhaps and I'm not sure it helps much for understanding.

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> 
> > I'm not really sure what idea is, but its annoying its much less clear what
> > theese selection amount constants mean that EWordMovementType ones.
> 
> It's just a wrapper around nsFrame::PeekOffset. What can I do?

Ideally you'd try and make that nicer, though maybe having a11y enum and converting in FindBoundary() would be better for now

> > > > > something to make it working for line boundaries :) a code is not good
> > > > > shape, hopefully we will get it in the end
> > > > 
> > > > I... see, I guess you'll rewrite this too at some point?
> > > 
> > > one day I suppose
> > 
> > do you have any idea why it makes sense to do it?
> 
> it contains too much special cases

huh?

> > > honestly I like this kind of comments because it is descriptive and really
> > > short. Also I don't think this style and thus comments are final, I want to
> > > change them after or before getTextAfterOffset change (currently my queue
> > > contains patches for getTextAtOffset and getTextBeforeOffset). Do you mind?
> > 
> > I mind that they don't help me understand anything and seem more or less
> > useless.
> 
> Sure, I can replace "end key" to "move to the end of line", "home key" to
> "move to the beginning of line", "up arrow" to "move to previous line". But
> that'd be too verbose perhaps and I'm not sure it helps much for
> understanding.

nah, I'd like something more like

// if offset was in the last line then x is true so we need to to do y instead of z.

Comment 11

[alexander :surkov (:asurkov)]

Attachment: followup: comment fixes

sound better?

Comment 12

[Trevor Saunders (:tbsaunde)]

Comment on attachment 758345 [details] [diff] [review]
followup: comment fixes

>       // In contrast to word end boundary we follow the spec here. End key,
>-      // then up arrow and if not on first line then end key.
>+      // then up arrow and end key if not on the first line, Otherwise end key,
>+      // and up arrow.

I'd really rather not think about keys here, text is just a matrix or number line with some boundary points on it, so lets just think about that.

btw what can up arrow possibly mean if you're already on the top line?

Comment 13

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #12)

> I'd really rather not think about keys here, text is just a matrix or number
> line with some boundary points on it, so lets just think about that.

Ok, I don't care. Long comments won't harm me.

> btw what can up arrow possibly mean if you're already on the top line?

start of the line

Comment 14

[alexander :surkov (:asurkov)]

Attachment: followup: comment fixes2

that keeps you happier?

Comment 15

[Trevor Saunders (:tbsaunde)]

Comment on attachment 758356 [details] [diff] [review]
followup: comment fixes2

>-      // then up arrow and if not on first line then end key.
>+      // In contrast to word end boundary we follow the spec here.
>+      // End offset is end of the current line, start offset is end of
>+      // the previous line if any. Otherwise 0 offset.
>       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
>       int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
>       *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
>       if (*aStartOffset == *aEndOffset)

ok, but it seems the tricky bit here is the third call will return tmpOffset when it is not 0 or the start of thext, and will instead return *aEndOffset.  So commenting that would be good and asks the question why do we need the third call in the first place?

Comment 16

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #15)

> >-      // then up arrow and if not on first line then end key.
> >+      // In contrast to word end boundary we follow the spec here.
> >+      // End offset is end of the current line, start offset is end of
> >+      // the previous line if any. Otherwise 0 offset.
> >       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
> >       int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
> >       *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
> >       if (*aStartOffset == *aEndOffset)
> 
> ok, but it seems the tricky bit here is the third call will return tmpOffset
> when it is not 0 or the start of thext, and will instead return *aEndOffset.
> So commenting that would be good and asks the question why do we need the
> third call in the first place?

those calls do exactly what I wrote before end key to get end offset and up + end keys to get start offset.

Comment 17

[alexander :surkov (:asurkov)]

maybe this:

// End offset is end of the current line (as the end key was pressed),
// start offset is end of the previous line if any (up arrow end keys).
// Otherwise 0 offset (up arrow only).

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #16)
> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> 
> > >-      // then up arrow and if not on first line then end key.
> > >+      // In contrast to word end boundary we follow the spec here.
> > >+      // End offset is end of the current line, start offset is end of
> > >+      // the previous line if any. Otherwise 0 offset.
> > >       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
> > >       int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
> > >       *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
> > >       if (*aStartOffset == *aEndOffset)
> > 
> > ok, but it seems the tricky bit here is the third call will return tmpOffset
> > when it is not 0 or the start of thext, and will instead return *aEndOffset.
> > So commenting that would be good and asks the question why do we need the
> > third call in the first place?
> 
> those calls do exactly what I wrote before end key to get end offset and up
> + end keys to get start offset.

that's fine, but I don't understand how that answers my question. if you can detect with two calls you're in the first line case why make the third?

(In reply to alexander :surkov from comment #17)
> maybe this:
> 
> // End offset is end of the current line (as the end key was pressed),
> // start offset is end of the previous line if any (up arrow end keys).
> // Otherwise 0 offset (up arrow only).

that's fine, but it doesn't tell me why third call is needed.

btw its strange up arrow does anything when on top line, but not an a11y thing so *shrug*

Comment 19

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #18)

> > // End offset is end of the current line (as the end key was pressed),
> > // start offset is end of the previous line if any (up arrow end keys).
> > // Otherwise 0 offset (up arrow only).
> 
> that's fine, but it doesn't tell me why third call is needed.

1) end key - you find end offset
2) up key - brings you somewhere at previous line
3) end key - brings you to end of previous line

> btw its strange up arrow does anything when on top line, but not an a11y
> thing so *shrug*

it's a standard behavior I rely on here

Comment 20

[alexander :surkov (:asurkov)]

Attachment: followup: comment fixes3

Comment 21

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #19)
> (In reply to Trevor Saunders (:tbsaunde) from comment #18)
> 
> > > // End offset is end of the current line (as the end key was pressed),
> > > // start offset is end of the previous line if any (up arrow end keys).
> > > // Otherwise 0 offset (up arrow only).
> > 
> > that's fine, but it doesn't tell me why third call is needed.
> 
> 1) end key - you find end offset
> 2) up key - brings you somewhere at previous line
> 3) end key - brings you to end of previous line

That describes what you do, but that's not my question.  I want to know *why* you do that when it seems you could do something else that's simpler and faster while equivelent.

> > btw its strange up arrow does anything when on top line, but not an a11y
> > thing so *shrug*
> 
> it's a standard behavior I rely on here

sure, that doesn't mean its what I expect.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #21)

> > 1) end key - you find end offset
> > 2) up key - brings you somewhere at previous line
> > 3) end key - brings you to end of previous line
> 
> That describes what you do, but that's not my question.  I want to know
> *why* you do that when it seems you could do something else that's simpler
> and faster while equivelent.

I must missed it. What is that?

Comment 23

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #22)
> (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> 
> > > 1) end key - you find end offset
> > > 2) up key - brings you somewhere at previous line
> > > 3) end key - brings you to end of previous line
> > 
> > That describes what you do, but that's not my question.  I want to know
> > *why* you do that when it seems you could do something else that's simpler
> > and faster while equivelent.
> 
> I must missed it. What is that?

see comment 15?

Comment 24

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #23)
> (In reply to alexander :surkov from comment #22)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > 
> > > > 1) end key - you find end offset
> > > > 2) up key - brings you somewhere at previous line
> > > > 3) end key - brings you to end of previous line
> > > 
> > > That describes what you do, but that's not my question.  I want to know
> > > *why* you do that when it seems you could do something else that's simpler
> > > and faster while equivelent.
> > 
> > I must missed it. What is that?
> 
> see comment 15?

I didn't recognized it as algorithm, not sure I do this now.

(In reply to Trevor Saunders (:tbsaunde) from comment #15)
> Comment on attachment 758356 [details] [diff] [review]
> followup: comment fixes2
> 
> >-      // then up arrow and if not on first line then end key.
> >+      // In contrast to word end boundary we follow the spec here.
> >+      // End offset is end of the current line, start offset is end of
> >+      // the previous line if any. Otherwise 0 offset.
> >       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
> >       int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
> >       *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
> >       if (*aStartOffset == *aEndOffset)
> 
> ok, but it seems the tricky bit here is the third call will return tmpOffset
> when it is not 0 or the start of thext, and will instead return *aEndOffset.

3d call may return tmpOffset or may not (comment #19) it depends on this line and previous line length. It returns *aEndOffset if tmpOffset is 0 (we are at the first line).

Comment 25

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > (In reply to alexander :surkov from comment #22)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > > 
> > > > > 1) end key - you find end offset
> > > > > 2) up key - brings you somewhere at previous line
> > > > > 3) end key - brings you to end of previous line
> > > > 
> > > > That describes what you do, but that's not my question.  I want to know
> > > > *why* you do that when it seems you could do something else that's simpler
> > > > and faster while equivelent.
> > > 
> > > I must missed it. What is that?
> > 
> > see comment 15?
> 
> I didn't recognized it as algorithm, not sure I do this now.

I'm not sure its so much algorithm as claim that bits can be optimized out.

> (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > Comment on attachment 758356 [details] [diff] [review]
> > followup: comment fixes2
> > 
> > >-      // then up arrow and if not on first line then end key.
> > >+      // In contrast to word end boundary we follow the spec here.
> > >+      // End offset is end of the current line, start offset is end of
> > >+      // the previous line if any. Otherwise 0 offset.
> > >       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
> > >       int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
> > >       *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
> > >       if (*aStartOffset == *aEndOffset)
> > 
> > ok, but it seems the tricky bit here is the third call will return tmpOffset
> > when it is not 0 or the start of thext, and will instead return *aEndOffset.
> 
> 3d call may return tmpOffset or may not (comment #19) it depends on this
> line and previous line length. It returns *aEndOffset if tmpOffset is 0 (we
> are at the first line).

so, if this part is correct
if *aStart == *aEnd) *aStart = 0 it must be true that in this case *aStart and *aEnd are index of first line break after start of text otherwise 0 would not be correct start for line.  Therefore tmpoffset must be previous line boundary which must be 0 because we said this was first line boundary after zero.  Therefore we don't need third FindLieBoundary call if tmpOffset == 0 because we know *aStartOffset should also be zero in this case.

Comment 26

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #25)
> (In reply to alexander :surkov from comment #24)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > > (In reply to alexander :surkov from comment #22)
> > > > (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > > > 
> > > > > > 1) end key - you find end offset
> > > > > > 2) up key - brings you somewhere at previous line
> > > > > > 3) end key - brings you to end of previous line
> > > > > 
> > > > > That describes what you do, but that's not my question.  I want to know
> > > > > *why* you do that when it seems you could do something else that's simpler
> > > > > and faster while equivelent.
> > > > 
> > > > I must missed it. What is that?
> > > 
> > > see comment 15?
> > 
> > I didn't recognized it as algorithm, not sure I do this now.
> 
> I'm not sure its so much algorithm as claim that bits can be optimized out.
> 
> > (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > > Comment on attachment 758356 [details] [diff] [review]
> > > followup: comment fixes2
> > > 
> > > >-      // then up arrow and if not on first line then end key.
> > > >+      // In contrast to word end boundary we follow the spec here.
> > > >+      // End offset is end of the current line, start offset is end of
> > > >+      // the previous line if any. Otherwise 0 offset.
> > > >       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
> > > >       int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
> > > >       *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
> > > >       if (*aStartOffset == *aEndOffset)
> > > 
> > > ok, but it seems the tricky bit here is the third call will return tmpOffset
> > > when it is not 0 or the start of thext, and will instead return *aEndOffset.
> > 
> > 3d call may return tmpOffset or may not (comment #19) it depends on this
> > line and previous line length. It returns *aEndOffset if tmpOffset is 0 (we
> > are at the first line).
> 
> so, if this part is correct
> if *aStart == *aEnd) *aStart = 0 it must be true that in this case *aStart
> and *aEnd are index of first line break after start of text otherwise 0
> would not be correct start for line.

correct

>  Therefore tmpoffset must be previous
> line boundary

wrong, tmpOffset may be any offset on previous line. Maybe you miss how the up arrow works:

long line
line

if *aStartOffset is a text length, then tmpOffset is 4 offset (right after "long") since up arrow brings you there.

Comment 27

[alexander :surkov (:asurkov)]

I think I misread you again

Comment 28

[alexander :surkov (:asurkov)]

Attachment: followup4

Comment 29

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #26)
> (In reply to Trevor Saunders (:tbsaunde) from comment #25)
> > (In reply to alexander :surkov from comment #24)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #23)
> > > > (In reply to alexander :surkov from comment #22)
> > > > > (In reply to Trevor Saunders (:tbsaunde) from comment #21)
> > > > > 
> > > > > > > 1) end key - you find end offset
> > > > > > > 2) up key - brings you somewhere at previous line
> > > > > > > 3) end key - brings you to end of previous line
> > > > > > 
> > > > > > That describes what you do, but that's not my question.  I want to know
> > > > > > *why* you do that when it seems you could do something else that's simpler
> > > > > > and faster while equivelent.
> > > > > 
> > > > > I must missed it. What is that?
> > > > 
> > > > see comment 15?
> > > 
> > > I didn't recognized it as algorithm, not sure I do this now.
> > 
> > I'm not sure its so much algorithm as claim that bits can be optimized out.
> > 
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #15)
> > > > Comment on attachment 758356 [details] [diff] [review]
> > > > followup: comment fixes2
> > > > 
> > > > >-      // then up arrow and if not on first line then end key.
> > > > >+      // In contrast to word end boundary we follow the spec here.
> > > > >+      // End offset is end of the current line, start offset is end of
> > > > >+      // the previous line if any. Otherwise 0 offset.
> > > > >       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
> > > > >       int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
> > > > >       *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
> > > > >       if (*aStartOffset == *aEndOffset)
> > > > 
> > > > ok, but it seems the tricky bit here is the third call will return tmpOffset
> > > > when it is not 0 or the start of thext, and will instead return *aEndOffset.
> > > 
> > > 3d call may return tmpOffset or may not (comment #19) it depends on this
> > > line and previous line length. It returns *aEndOffset if tmpOffset is 0 (we
> > > are at the first line).
> > 
> > so, if this part is correct
> > if *aStart == *aEnd) *aStart = 0 it must be true that in this case *aStart
> > and *aEnd are index of first line break after start of text otherwise 0
> > would not be correct start for line.
> 
> correct
> 
> >  Therefore tmpoffset must be previous
> > line boundary
> 
> wrong, tmpOffset may be any offset on previous line. Maybe you miss how the
> up arrow works:
> 
> long line
> line
> 
> if *aStartOffset is a text length, then tmpOffset is 4 offset (right after
> "long") since up arrow brings you there.

accept we know this can't be true because searching forward from offset got us to the first line boundary after the start so offset must be at that boundary or before it which means searching backwards for the previous boundary to get tmpOffset can only have one possible result.

Comment 30

[Trevor Saunders (:tbsaunde)]

Comment on attachment 758926 [details] [diff] [review]
followup4

>+      // In contrast to word end boundary we follow the spec here.
>+      // End offset is end of the current line (as the end key was pressed).
>+      // Start offset is end of the previous line if any (up arrow and end keys),
>+      // otherwise 0 offset (up arrow only).
>       *aEndOffset = FindLineBoundary(offset, eDirNext, eSelectEndLine);
>-      int32_t tmpOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
>-      *aStartOffset = FindLineBoundary(tmpOffset, eDirNext, eSelectEndLine);
>-      if (*aStartOffset == *aEndOffset)
>-        *aStartOffset = 0;
>+      *aStartOffset = FindLineBoundary(offset, eDirPrevious, eSelectLine);
>+      if (*aStartOffset != 0)
>+        *aStartOffset = FindLineBoundary(*aStartOffset, eDirNext, eSelectEndLine);

so, is it true the middle call to FindLineBoundary actually does nothing of the sort and doesn't necessarily find the previous boundary?  That sounds like total madness, but I can't see why else you'd need the third call.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #30)

> so, is it true the middle call to FindLineBoundary actually does nothing of
> the sort and doesn't necessarily find the previous boundary?

it just moves you on previous line

Comment 32

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #31)
> (In reply to Trevor Saunders (:tbsaunde) from comment #30)
> 
> > so, is it true the middle call to FindLineBoundary actually does nothing of
> > the sort and doesn't necessarily find the previous boundary?
> 
> it just moves you on previous line

can we please rename that function or something?

Why can't we just have two function calls
1 find previous line boundary
2 find next line boundary
+ whatever edge cases exist

Comment 33

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #32)

> Why can't we just have two function calls
> 1 find previous line boundary
> 2 find next line boundary

Do you want simple inline wrappers around FindBoundary calls? I wanted to have them at some point.

Comment 34

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #33)
> (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> 
> > Why can't we just have two function calls
> > 1 find previous line boundary
> > 2 find next line boundary
> 
> Do you want simple inline wrappers around FindBoundary calls? I wanted to
> have them at some point.

what would they do?

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #34)
> (In reply to alexander :surkov from comment #33)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> > 
> > > Why can't we just have two function calls
> > > 1 find previous line boundary
> > > 2 find next line boundary
> > 
> > Do you want simple inline wrappers around FindBoundary calls? I wanted to
> > have them at some point.
> 
> what would they do?

like FindLineBoundary(ePrevLine, eLineStart), I didn't think how it fits, basically it is supposed to replace your 1-2nd items. Anyway, are you fine to deal with it in followup?

Comment 36

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #35)
> (In reply to Trevor Saunders (:tbsaunde) from comment #34)
> > (In reply to alexander :surkov from comment #33)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #32)
> > > 
> > > > Why can't we just have two function calls
> > > > 1 find previous line boundary
> > > > 2 find next line boundary
> > > 
> > > Do you want simple inline wrappers around FindBoundary calls? I wanted to
> > > have them at some point.
> > 
> > what would they do?
> 
> like FindLineBoundary(ePrevLine, eLineStart), I didn't think how it fits,

FindLineBoundary(ePrev / eNext) would be fine

> basically it is supposed to replace your 1-2nd items. Anyway, are you fine
> to deal with it in followup?

I'd really rather we didn't, what is currently called FindLineBoundary() is horribly missnamed, and I'd rather not review how code changes from one thing I don't understand to another that I don't really get and then maybe in the future to something better.

Comment 37

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #36)

> > like FindLineBoundary(ePrevLine, eLineStart), I didn't think how it fits,
> 
> FindLineBoundary(ePrev / eNext) would be fine

might be confusing, investigations are needed.

> > basically it is supposed to replace your 1-2nd items. Anyway, are you fine
> > to deal with it in followup?
> 
> I'd really rather we didn't, what is currently called FindLineBoundary() is
> horribly missnamed, and I'd rather not review how code changes from one
> thing I don't understand to another that I don't really get and then maybe
> in the future to something better.

Ok. Alternatively I could provide one huge patch that contains everything (my queue contains 10 patches already). But it shouldn't be much easier to review it and it's not friendly to regressions finding.

Making a changes in the first patch form my queue is definitely bad for me. So I would avoid changes like methods renaming/changing, at least until the logic is not broken.

Comment 38

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #37)
> (In reply to Trevor Saunders (:tbsaunde) from comment #36)
> 
> > > like FindLineBoundary(ePrevLine, eLineStart), I didn't think how it fits,
> > 
> > FindLineBoundary(ePrev / eNext) would be fine
> 
> might be confusing, investigations are needed.

how? especially when the alternative is a function named FindLineBoundary() that doesn't always return a line boundary?  just moving forward / back would also be more consistant with how words work.

> > > basically it is supposed to replace your 1-2nd items. Anyway, are you fine
> > > to deal with it in followup?
> > 
> > I'd really rather we didn't, what is currently called FindLineBoundary() is
> > horribly missnamed, and I'd rather not review how code changes from one
> > thing I don't understand to another that I don't really get and then maybe
> > in the future to something better.
> 
> Ok. Alternatively I could provide one huge patch that contains everything
> (my queue contains 10 patches already). But it shouldn't be much easier to
> review it and it's not friendly to regressions finding.

I sort of suspect one big patch might be easier to review, and I'm not sure how much splittingthis up will help with regressions.

> Making a changes in the first patch form my queue is definitely bad for me.
> So I would avoid changes like methods renaming/changing, at least until the
> logic is not broken.

Well I'm not sure I can tell if there is a logic error or not...

Also I really don't think risk of having the FindLineBoundary thing stay  as it is is ok.

I suspect I'd rather have one huge patch than go to what we have here and then something else.

Comment 39

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #38)
> (In reply to alexander :surkov from comment #37)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #36)
> > 
> > > > like FindLineBoundary(ePrevLine, eLineStart), I didn't think how it fits,
> > > 
> > > FindLineBoundary(ePrev / eNext) would be fine
> > 
> > might be confusing, investigations are needed.
> 
> how? especially when the alternative is a function named FindLineBoundary()
> that doesn't always return a line boundary?  just moving forward / back
> would also be more consistant with how words work.

for example, in case of getTextAtOffset start line boundary you need to get start of this line not depending where you are at this line, in case of getTextBeforeOffset you need to get start of prev line. Having plain FindLineBoundary(aPrev) doesn't seem enough for this.

> I sort of suspect one big patch might be easier to review, and I'm not sure
> how much splittingthis up will help with regressions.

that's evident but if you know you changed item a only then you check item a, if you changed items b and c as well then search area is bigger. Basically a big patch is just reorg how our line getting code works (at this point iirc I don't have layout fixes but if they were then a big patches would contained them as well).

> > Making a changes in the first patch form my queue is definitely bad for me.
> > So I would avoid changes like methods renaming/changing, at least until the
> > logic is not broken.
> 
> Well I'm not sure I can tell if there is a logic error or not...
> 
> Also I really don't think risk of having the FindLineBoundary thing stay  as
> it is is ok.

we really talk about inline method(s) introduction, I agree they should simplify the understanding of the code but it existing code shouldn't make a big problem to follow the logic.

> I suspect I'd rather have one huge patch than go to what we have here and
> then something else.

up to you, I hope to finish it this week

Comment 40

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #39)
> (In reply to Trevor Saunders (:tbsaunde) from comment #38)
> > (In reply to alexander :surkov from comment #37)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #36)
> > > 
> > > > > like FindLineBoundary(ePrevLine, eLineStart), I didn't think how it fits,
> > > > 
> > > > FindLineBoundary(ePrev / eNext) would be fine
> > > 
> > > might be confusing, investigations are needed.
> > 
> > how? especially when the alternative is a function named FindLineBoundary()
> > that doesn't always return a line boundary?  just moving forward / back
> > would also be more consistant with how words work.
> 
> for example, in case of getTextAtOffset start line boundary you need to get
> start of this line not depending where you are at this line, in case of
> getTextBeforeOffset you need to get start of prev line. Having plain
> FindLineBoundary(aPrev) doesn't seem enough for this.

why not? can't you do exactly the same thing we do for word boundaries?

> > I sort of suspect one big patch might be easier to review, and I'm not sure
> > how much splittingthis up will help with regressions.
> 
> that's evident but if you know you changed item a only then you check item
> a, if you changed items b and c as well then search area is bigger.
> Basically a big patch is just reorg how our line getting code works (at this
> point iirc I don't have layout fixes but if they were then a big patches
> would contained them as well).

true, but I think that would be nice to see big picture

Comment 41

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #40)
> (In reply to alexander :surkov from comment #39)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #38)
> > > (In reply to alexander :surkov from comment #37)
> > > > (In reply to Trevor Saunders (:tbsaunde) from comment #36)
> > > > 
> > > > > > like FindLineBoundary(ePrevLine, eLineStart), I didn't think how it fits,
> > > > > 
> > > > > FindLineBoundary(ePrev / eNext) would be fine
> > > > 
> > > > might be confusing, investigations are needed.
> > > 
> > > how? especially when the alternative is a function named FindLineBoundary()
> > > that doesn't always return a line boundary?  just moving forward / back
> > > would also be more consistant with how words work.
> > 
> > for example, in case of getTextAtOffset start line boundary you need to get
> > start of this line not depending where you are at this line, in case of
> > getTextBeforeOffset you need to get start of prev line. Having plain
> > FindLineBoundary(aPrev) doesn't seem enough for this.
> 
> why not?

say if you are at start line then FindStartLineBoundary(aPrev) return start line of this line or prev line? getTextAtOffset needs this line, getTextBeforeOffset needs prev line.

> can't you do exactly the same thing we do for word boundaries?

layout API is different so keeping the same logic doesn't make a big sense

> > > I sort of suspect one big patch might be easier to review, and I'm not sure
> > > how much splittingthis up will help with regressions.
> > 
> > that's evident but if you know you changed item a only then you check item
> > a, if you changed items b and c as well then search area is bigger.
> > Basically a big patch is just reorg how our line getting code works (at this
> > point iirc I don't have layout fixes but if they were then a big patches
> > would contained them as well).
> 
> true, but I think that would be nice to see big picture

if a big picture is only what you want then I can file a big patch for your review but I will keep the work landing by small parts.

Comment 42

[alexander :surkov (:asurkov)]

pulls r+ from bug 882281, https://hg.mozilla.org/integration/mozilla-inbound/rev/41355ae00835

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41355ae00835

Comment 44

[alexander :surkov (:asurkov)]

Comment on attachment 758926 [details] [diff] [review]
followup4

http://hg.mozilla.org/integration/mozilla-inbound/rev/cc5649624720

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc5649624720