a next round after bug 853340

Goodbye kTodo :)

Comment on attachment 730759 [details] [diff] [review] patch >+ case BOUNDARY_WORD_START: { >+ if (offset == 0) { >+ *aStartOffset = *aEndOffset = 0; >+ return NS_OK; >+ } >+ >+ int32_t midOffset = FindWordBoundary(offset, eDirPrevious, eStartWord); >+ *aEndOffset = FindWordBoundary(midOffset, eDirNext, eStartWord); >+ if (*aEndOffset == offset) { >+ *aStartOffset = midOffset; >+ return GetText(*aStartOffset, *aEndOffset, aText); >+ } >+ >+ *aStartOffset = FindWordBoundary(midOffset, eDirPrevious, eStartWord); >+ *aEndOffset = midOffset; >+ return GetText(*aStartOffset, *aEndOffset, aText); this all could really use comments explaining cases and such. >+ } >+ >+ case BOUNDARY_WORD_END: { >+ if (offset == 0) { >+ *aStartOffset = *aEndOffset = 0; >+ return NS_OK; >+ } >+ >+ *aEndOffset = FindWordBoundary(offset, eDirPrevious, eEndWord); >+ *aStartOffset = FindWordBoundary(*aEndOffset, eDirPrevious, eEndWord); >+ return GetText(*aStartOffset, *aEndOffset, aText); same, the offset == 0 part is especially suprising here >+ (moveForwardThenBack ? eDirNext : eDirPrevious), >+ wordMovementType); >+ if (moveForwardThenBack) > *aEndOffset = offset; > else > *aStartOffset = offset; maybe it would be nice to have type to return that is tupple start end text? what is the story with the test regressions?

(In reply to Trevor Saunders (:tbsaunde) from comment #3) > >+ case BOUNDARY_WORD_END: { > >+ if (offset == 0) { > >+ *aStartOffset = *aEndOffset = 0; > >+ return NS_OK; > >+ } > >+ > >+ *aEndOffset = FindWordBoundary(offset, eDirPrevious, eEndWord); > >+ *aStartOffset = FindWordBoundary(*aEndOffset, eDirPrevious, eEndWord); > >+ return GetText(*aStartOffset, *aEndOffset, aText); > > same, the offset == 0 part is especially suprising here "The returned string will contain the word before the offset" so empty string. An alternative is failure. I will add comments. > >+ (moveForwardThenBack ? eDirNext : eDirPrevious), > >+ wordMovementType); > >+ if (moveForwardThenBack) > > *aEndOffset = offset; > > else > > *aStartOffset = offset; > > maybe it would be nice to have type to return that is tupple start end text? you mean struct TextRange { int startOffset; int endOffset; nsAString& name; } ? I could separate these on separate 'case's too as I did in getTextBeforeOffset if it make code nicer (I professedly leaved two different approaches) > what is the story with the test regressions? which regressions? This patch fixes exiting regressions but it doesn't seem introduce new ones.

(In reply to alexander :surkov from comment #4) > (In reply to Trevor Saunders (:tbsaunde) from comment #3) > > > >+ case BOUNDARY_WORD_END: { > > >+ if (offset == 0) { > > >+ *aStartOffset = *aEndOffset = 0; > > >+ return NS_OK; > > >+ } > > >+ > > >+ *aEndOffset = FindWordBoundary(offset, eDirPrevious, eEndWord); > > >+ *aStartOffset = FindWordBoundary(*aEndOffset, eDirPrevious, eEndWord); > > >+ return GetText(*aStartOffset, *aEndOffset, aText); > > > > same, the offset == 0 part is especially suprising here > > "The returned string will contain the word before the offset" so empty > string. An alternative is failure. I will add comments. yeah, I figured it out eventually it just wasn't anything like obvious from just reading. > > >+ (moveForwardThenBack ? eDirNext : eDirPrevious), > > >+ wordMovementType); > > >+ if (moveForwardThenBack) > > > *aEndOffset = offset; > > > else > > > *aStartOffset = offset; > > > > maybe it would be nice to have type to return that is tupple start end text? > > you mean > > struct TextRange { > int startOffset; > int endOffset; > nsAString& name; > } > > ? that's more or less what I was thinking of > I could separate these on separate 'case's too as I did in > getTextBeforeOffset if it make code nicer (I professedly leaved two > different approaches) not sure what you mean off hand > > what is the story with the test regressions? > > which regressions? This patch fixes exiting regressions but it doesn't seem > introduce new ones. hm ok, maybe its just the diff being weird

(In reply to Trevor Saunders (:tbsaunde) from comment #5) > > struct TextRange { > > int startOffset; > > int endOffset; > > nsAString& name; > > } > > > > ? > > that's more or less what I was thinking of maybe as followup, as dexpcomination part > > I could separate these on separate 'case's too as I did in > > getTextBeforeOffset if it make code nicer (I professedly leaved two > > different approaches) > > not sure what you mean off hand compare getTextBeforeOffset case word_start: // logic break; case word_end: // logic; break; getTextAtOffset case word_start: // variables setting break; case word_end // variables setting break; // shared logic for both word start and end which one do you prefer?

(In reply to alexander :surkov from comment #6) > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > > > struct TextRange { > > > int startOffset; > > > int endOffset; > > > nsAString& name; > > > } > > > > > > ? > > > > that's more or less what I was thinking of > > maybe as followup, as dexpcomination part sure > > > I could separate these on separate 'case's too as I did in > > > getTextBeforeOffset if it make code nicer (I professedly leaved two > > > different approaches) > > > > not sure what you mean off hand > > compare > > getTextBeforeOffset > case word_start: > // logic > break; > case word_end: > // logic; > break; > > getTextAtOffset > case word_start: > // variables setting > break; > case word_end > // variables setting > break; > > // shared logic for both word start and end > > which one do you prefer? probably the first, but only slightly and it depends on the exact case.

1) comments were added 2) getTextAtOffset code was reformatted per comment #7