Closed Bug 1284888 Opened 8 years ago Closed 7 years ago

Make TextOverflow::WillProcessLines() return a UniquePtr

Categories

(Core :: Layout: Text and Fonts, defect, P3)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: michael.li11702, Assigned: dholbert)

References

Details

Attachments

(2 files)

Bug 1284888 part 1: Annotate TextOverflow as a heap-only final class, to reflect reality.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Bug 1284888 part 2: Make TextOverflow::WillProcessLines() return a UniquePtr, for stronger lifetime guarantees.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details 
We're migrating nsAutoPtr to UniquePtr in nsBlockFrame.cpp, so we should make WillProcessLines() return a UniquePtr instead of returning a raw pointer.
Depends on: 1283273
See Also: → 1355531
This bug might become obsolete if we fix bug 1355531 (and convert to Maybe<> instead of UniquePtr<> for this type).
Priority: -- → P3
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
As supporting documentation that this is a sane thing to do (as a sanity-check for myself & to jog your memory in case you've forgotten), the UniquePtr documentation says:

 * It's perfectly okay for a function to return a UniquePtr. This transfers
 * the UniquePtr's sole ownership of the data, to the fresh UniquePtr created
 * in the calling function, that will then solely own that data. Such functions
 * can return a local variable UniquePtr, |nullptr|, |UniquePtr(ptr)| where
 * |ptr| is a |T*|, or a UniquePtr |Move()|'d from elsewhere.
https://dxr.mozilla.org/mozilla-central/rev/e0b0865639cebc1b5afa0268a4b073fcdde0e69c/mfbt/UniquePtr.h#163-167

(UniquePtr is smarter than nsAutoPtr in this respect -- no need for forget() etc.)
Comment on attachment 8887571 [details]
Bug 1284888 part 1: Annotate TextOverflow as a heap-only final class, to reflect reality.

https://reviewboard.mozilla.org/r/158432/#review163686
Attachment #8887571 - Flags: review?(jfkthame) → review+
Comment on attachment 8887572 [details]
Bug 1284888 part 2: Make TextOverflow::WillProcessLines() return a UniquePtr, for stronger lifetime guarantees.

https://reviewboard.mozilla.org/r/158434/#review163684

::: layout/generic/TextOverflow.h:65
(Diff revision 1)
> +  // Let MakeUnique invoke our (private) constructor:
> +  friend mozilla::detail::UniqueSelector<TextOverflow>::SingleObject
> +    mozilla::MakeUnique<TextOverflow>(nsDisplayListBuilder*&, nsIFrame*&);

This seems unfortunate to me; the TextOverflow header shouldn't need to know anything about this sort of detail of the implementation of UniquePtr.

I don't know what else to do, though -- aside from making our constructor public, which isn't ideal either as it's not supposed to be used except via our static WillProcessLines method.

(Personally, I think this is ugly enough that I'd be equally happy to r+ a patch that opts to make the constructor public. But I'll leave you to choose whichever you think is the lesser of the evils...)
Attachment #8887572 - Flags: review?(jfkthame) → review+
Comment on attachment 8887572 [details]
Bug 1284888 part 2: Make TextOverflow::WillProcessLines() return a UniquePtr, for stronger lifetime guarantees.

https://reviewboard.mozilla.org/r/158434/#review163684

> This seems unfortunate to me; the TextOverflow header shouldn't need to know anything about this sort of detail of the implementation of UniquePtr.
> 
> I don't know what else to do, though -- aside from making our constructor public, which isn't ideal either as it's not supposed to be used except via our static WillProcessLines method.
> 
> (Personally, I think this is ugly enough that I'd be equally happy to r+ a patch that opts to make the constructor public. But I'll leave you to choose whichever you think is the lesser of the evils...)

Yeah, I'll opt for the current strategy as the lesser evil -- I'd feel bad making the constructor public & relaxing something that we're currently strict about.  And this "friend" cruft feels slightly less crufty given that it's at least in the "private" section of the header.

(It'd be great if MakeUnique could just automagically have the same visibility & protections as the constructor, but I don't think that's possible.)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/39e9b0e7ae04
part 1: Annotate TextOverflow as a heap-only final class, to reflect reality. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/520df8c2a0ac
part 2: Make TextOverflow::WillProcessLines() return a UniquePtr, for stronger lifetime guarantees. r=jfkthame
Thanks KWierso.

> TextOverflow.h(67): error C2063: 'mozilla::MakeUnique': not a function

whelp, Windows apparently doesn't like that controversial "friend" declaration.

It's not the first one in our code-base -- I based it on this one:
https://dxr.mozilla.org/mozilla-central/rev/1b065ffd8a535a0ad4c39a912af18e948e6a42c1/dom/media/gmp/GMPLoader.cpp#203
...though that's in a class called "LinuxSandboxStarter", so MSVC probably never gets a chance to shoot that one down. :)

I'll take jfkthame's suggestion and switch to just making the constructor public here.
Green try run with updated patches, Linux + windows builds:
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc6156c6b74e8ef110dc95dff52617a0de0fcc7

Triggered autoland, so this should go back in once the tree reopens.
Flags: needinfo?(dholbert)
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ded6ed223ce5
part 1: Annotate TextOverflow as a heap-only final class, to reflect reality. r=jfkthame
https://hg.mozilla.org/integration/autoland/rev/4cc4ef8f7e73
part 2: Make TextOverflow::WillProcessLines() return a UniquePtr, for stronger lifetime guarantees. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/ded6ed223ce5
https://hg.mozilla.org/mozilla-central/rev/4cc4ef8f7e73
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: