Closed Bug 882281 Opened 7 years ago Closed 7 years ago

reorg getText* for line boundary

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

per bug 877974 comment #41: have a big bug (and patch) combining all bugs (patches) about getText* for line boundaries reorg.
Depends on: 882292
Depends on: 882602
Attached patch combined patchSplinter Review
contains all patches from dependent bugs
Assignee: nobody → surkov.alexander
Attachment #761935 - Flags: review?(trev.saunders)
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 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.
Attachment #761935 - Flags: review?(trev.saunders) → review+
(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?
(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
fixed as all blockings were fixed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.