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

Status: NEW

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

Description

[Jeremy Chen ([e-mail])]

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=

Comment 1

[Jeremy Chen ([e-mail])]

(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 2

[Jeremy Chen ([e-mail])]

Attachment: Bug 1340852 - (wip) avoid passing raw pointers for gfxTextRun::PropertyProvider::Spacing.

Review commit: https://reviewboard.mozilla.org/r/113688/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/113688/

Comment 3

[Jeremy Chen ([e-mail])]

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

Comment 4

[Jeremy Chen ([e-mail])]

Comment on attachment 8838921 [details]
Bug 1340852 - (wip) avoid passing raw pointers for gfxTextRun::PropertyProvider::Spacing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/113688/diff/1-2/

Comment 5

[Jeremy Chen ([e-mail])]

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