Daniel Holbert [:dholbert] Reporter
	Description • 7 years ago

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.)

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 7 years ago

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?)

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 2 • 7 years ago

(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 :)

	Daniel Holbert [:dholbert] Reporter
	Comment 4 • 7 years ago
mozreview-review

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".

	Ting-Yu Lin [:TYLin] (UTC-8) Assignee
	Comment 6 • 7 years ago
mozreview-review-reply

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!

	Pulsebot
	Comment 7 • 7 years ago

Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/76ad07a4e3a5
Change caller of RestrictCaretDraggingOffsets() to use getter_AddRefs. r=dholbert

	Carsten Book [:Tomcat]
	Comment 8 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/76ad07a4e3a5