Closed Bug 1361072 Opened 3 years ago Closed 3 years ago

Callers of AccessibleCaretManager::RestrictCaretDraggingOffsets() should probably use getter_AddRefs

Categories

(Core :: Layout, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dholbert, Assigned: TYLin)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

AccessibleCaretManager::RestrictCaretDraggingOffsets() takes an arg of type nsINode**.  And its callsite calls it like so:


>  nsINode* node = nullptr;
>  [...]
>  nsIFrame* frame =
>    GetFrameForFirstRangeStartOrLastRangeEnd(dir, &offset, &node, &contentOffset);
>  [...]
>  nsCOMPtr<nsIContent> content = do_QueryInterface(node);
https://dxr.mozilla.org/mozilla-central/rev/5278e2a35fc8f2be390243db1e62858bf0982055/layout/base/AccessibleCaretManager.cpp#1122,1124-1125,1131

Could we just get rid of the variable |node|, and declare |content| higher up, and pass in getter_AddRefs(content) to this function instead of &node?  (And then have GetFrameForFirstRangeStartOrLastRangeEnd internally use a refcounted pointer, which it would forget() into that outparam.)

This is normally how we handle outparam pointers to refcounted types.  And bug 1359455 is formalizing this practice, and right now it's having to explicitly annotate this function (RestrictCaretDraggingOffsets) as violating that practice.

(This line of code was added in bug 1168891 -- adding dependency on that bug.)
In particular, I *think* everything would be equivalent if we:
 (1) passed getter_AddRefs(content) into GetFrameForFirstRangeStartOrLastRangeEnd here (instead of &node)

 (2) Change the aOutNode parameter to be nsIContent** instead of nsINode**.

 (3) Within the GetFrameForFirstRangeStartOrLastRangeEnd impl, at the place where we assign *aOutNode, replace this line...
      *aOutNode = startNode.get();
     ...with this:
      startContent.forget(aOutNode)

TYLin, does that make sense to you? Anything that I'm missing here?  (And would you be up for looking into this, since you know this code better than I do?)
Flags: needinfo?(tlin)
(In reply to Daniel Holbert [:dholbert] (AFK May 3-7, 13-21) from comment #1)
> TYLin, does that make sense to you? Anything that I'm missing here?  (And
> would you be up for looking into this, since you know this code better than
> I do?)

Yes, the fix is reasonable. I'll upload a patch later :)
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Flags: needinfo?(tlin)
Comment on attachment 8863630 [details]
Bug 1361072 - Change caller of RestrictCaretDraggingOffsets() to use getter_AddRefs.

https://reviewboard.mozilla.org/r/135422/#review138532

Thanks! r=me, with one nit:

::: layout/base/AccessibleCaretManager.cpp:1126
(Diff revision 1)
> -  nsIFrame* frame =
> -    GetFrameForFirstRangeStartOrLastRangeEnd(dir, &offset, &node, &contentOffset);
> +  nsIFrame* frame = GetFrameForFirstRangeStartOrLastRangeEnd(
> +    dir, &offset, getter_AddRefs(content), &contentOffset);

Please don't use this extreme-deindentation-of-args style, except when absolutely necessary to avoid going over 80 characters  (I don't believe the Coding Style doc supports it, and also I personally find it hard to skim.)

Let's just make this:
  nsIFrame* frame =
    GetFrameForFirstRangeStartOrLastRangeEnd(dir, &offset,
                                             getter_AddRefs(content),
                                             &contentOffset);
i.e. reverting back to how it was before this patch, with a linebreak before & after the 3rd arg because of the additional length from "getter_AddRefs".
Attachment #8863630 - Flags: review?(dholbert) → review+
Comment on attachment 8863630 [details]
Bug 1361072 - Change caller of RestrictCaretDraggingOffsets() to use getter_AddRefs.

https://reviewboard.mozilla.org/r/135422/#review138532

> Please don't use this extreme-deindentation-of-args style, except when absolutely necessary to avoid going over 80 characters  (I don't believe the Coding Style doc supports it, and also I personally find it hard to skim.)
> 
> Let's just make this:
>   nsIFrame* frame =
>     GetFrameForFirstRangeStartOrLastRangeEnd(dir, &offset,
>                                              getter_AddRefs(content),
>                                              &contentOffset);
> i.e. reverting back to how it was before this patch, with a linebreak before & after the 3rd arg because of the additional length from "getter_AddRefs".

Fixed. Thanks for the review!
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76ad07a4e3a5
Change caller of RestrictCaretDraggingOffsets() to use getter_AddRefs. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/76ad07a4e3a5
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1361889
No longer depends on: 1361889
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.