882281 - reorg getText* for line boundary

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

per bug 877974 comment #41: have a big bug (and patch) combining all bugs (patches) about getText* for line boundaries reorg.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: combined patch

contains all patches from dependent bugs

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 761935 [details] [diff] [review]
combined patch

>+  enum EWhichLineBoundary {
>+    ePrevLineBegin,
>+    ePrevLineEnd,
>+    eThisLineBegin,
>+    eThisLineEnd,
>+    eNextLineBegin,
>+    eNextLineEnd

it would be nice to comment this some, atleast cover how say eNextLineStart and eThisLineEnd are different I assume it has they  on different sides of the line end.

I want to take another look but doesn't seem to unreasonable, and all the test changes look ok.

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 761935 [details] [diff] [review]
combined patch

>+  int32_t AdjustCaretOffset(int32_t aOffset)

probably better to not put this in the header, esp since nothing outside of HyperTextAccessible uses it afaict

>+  bool IsEmptyLastLineOffset(int32_t aOffset)

probably the same, but I don't know this will force us to include any headers we wouldn't normally.

>+  enum EWhichLineBoundary {

comment what constants mean pls

nsSelectionAmount really needs comments too, but that's a different issue.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 761935 [details] [diff] [review]
> combined patch
> 
> >+  int32_t AdjustCaretOffset(int32_t aOffset)
> 
> probably better to not put this in the header, esp since nothing outside of
> HyperTextAccessible uses it afaict

it's under protected part, do we need really care?

> >+  bool IsEmptyLastLineOffset(int32_t aOffset)
> 
> probably the same, but I don't know this will force us to include any
> headers we wouldn't normally.

if we figure out that requires extra headers then we can move them into inl header. ok?

> >+  enum EWhichLineBoundary {
> 
> comment what constants mean pls

ok

> nsSelectionAmount really needs comments too, but that's a different issue.

nothing for this time, correct?

do you mind if I land patches one by one instead this big one?

Comment 5

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #3)
> > Comment on attachment 761935 [details] [diff] [review]
> > combined patch
> > 
> > >+  int32_t AdjustCaretOffset(int32_t aOffset)
> > 
> > probably better to not put this in the header, esp since nothing outside of
> > HyperTextAccessible uses it afaict
> 
> it's under protected part, do we need really care?
> 
> > >+  bool IsEmptyLastLineOffset(int32_t aOffset)
> > 
> > probably the same, but I don't know this will force us to include any
> > headers we wouldn't normally.
> 
> if we figure out that requires extra headers then we can move them into inl
> header. ok?I'm pretty sure we should need to be including nsFrameSelection.h other than the first of these functions and I don't see why they can't go in HyperTextAccessible.cpp, so I'd just do it, but *shrug*

> > nsSelectionAmount really needs comments too, but that's a different issue.
> 
> nothing for this time, correct?

true

> do you mind if I land patches one by one instead this big one?

seems fine I guess

Comment 6

[alexander :surkov (:asurkov)]

fixed as all blockings were fixed