Open Bug 1340852 Opened 5 years ago Updated 5 years ago

investigate avoiding passing raw pointers for gfxTextRun::PropertyProvider::Spacing

Categories

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

defect

Tracking

()

People

(Reporter: chenpighead, Unassigned)

Details

Attachments

(1 file)

At present, we pass gfxTextRun::PropertyProvider::Spacing* raw pointers along with a specified Range parameter in couple functions like:

GetAdjustedSpacing()
gfxTextRun::GetAdjustedSpacingArray()
GetSpacing()
PropertyProvider::GetSpacingInternal()
TabWidthStore::ApplySpacing()

Since we start to use nsTArray<PropertyProvider::Spacing> in our codes (see [1]), I wonder if we could replace all the PropertyProvider::Spacing declarations with nsTArray/autoTArray? In this way, we could pass these arrays by references, which is a bit more safe from getting into undefined-behaviour (ex. accidentally passing a nullptr). We can probably remove the Range parameters in these functions as well.


[1] http://searchfox.org/mozilla-central/search?q=AutoTArray%3CPropertyProvider%3A%3ASpacing&path=
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #0)
> At present, we pass gfxTextRun::PropertyProvider::Spacing* raw pointers
> along with a specified Range parameter in couple functions like:
> 
> GetAdjustedSpacing()
> gfxTextRun::GetAdjustedSpacingArray()
> GetSpacing()
> PropertyProvider::GetSpacingInternal()
> TabWidthStore::ApplySpacing()
> 
> Since we start to use nsTArray<PropertyProvider::Spacing> in our codes (see
> [1]), I wonder if we could replace all the PropertyProvider::Spacing
> declarations with nsTArray/autoTArray? In this way, we could pass these
> arrays by references, which is a bit more safe from getting into
> undefined-behaviour (ex. accidentally passing a nullptr). We can probably
> remove the Range parameters in these functions as well.

Hmmm, re-checked again and found that Range parameters seem unavoidable.
Comment on attachment 8838921 [details]
Bug 1340852 - (wip) avoid passing raw pointers for gfxTextRun::PropertyProvider::Spacing.

Looks like we have a special need that only acquiring a sub-range of spacing information in GetAdjustedSpacingArray. To avoid passing the raw pointers, we need an extra for-loop to make GetAdjustedSpacingArray still working, which may hurt us a bit in performance. :(
Attachment #8838921 - Flags: feedback?(jfkthame)
Attachment #8838921 - Flags: feedback?(jfkthame)
Comment on attachment 8838921 [details]
Bug 1340852 - (wip) avoid passing raw pointers for gfxTextRun::PropertyProvider::Spacing.

Clear feedback request.

The additional for-loop is really a bad idea. Maybe passing a nsTArrayIterator for the must-use-pointer case could be a solution.

Some refactoring work for PropertyProvider::GetSpacing might be needed as well, since we pass a raw pointer to PropertyProvider::GetSpacing for both

1. a gfxFont::Spacing Array [1]
2. a single gfxFont::Spacing value [2]


[1] http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/gfx/thebes/gfxTextRun.cpp#347
[2] http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/gfx/thebes/gfxTextRun.cpp#303-306
Attachment #8838921 - Flags: feedback?(jfkthame)
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.