Per bug 1056516 comment 90, ``` At present, PropertyProvider and its implementations are pretty messy. It would be nice to annotate the overrides to make it clear what they are, remove any redundant methods (e.g. there's already a StyleText() accessor in the nsTextFrame implementation that nobody currently uses, afaics), make methods and data members 'const' where possible, declare the implementations 'final', etc. ```

MozReview-Commit-ID: J6VNxGye6ET

Comment on attachment 8847017 [details] [diff] [review] (wip). Review of attachment 8847017 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/src/nsFontMetrics.cpp @@ +82,5 @@ > }; > > class StubPropertyProvider : public gfxTextRun::PropertyProvider { > public: > virtual void GetHyphenationBreaks(gfxTextRun::Range aRange, Delete "virtual" and the "override" in this declaration according to the coding style quoted below. Same for all other cases throughout this patch. Method declarations must use at most one of the following keywords: virtual, override, or final. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods

1. Explicitly make methods 'const' for PropertyProvider abstract class. 2. Explicitly make methods in the two sub-classes (nsTextFrame.cpp and nsFontMetrics.cpp) 'final'. 3. Remove the redundant method StyleText(). 4. Make methods of PropertyProvider in nsTextFrame.cpp 'const' where possible. MozReview-Commit-ID: J6VNxGye6ET

Comment on attachment 8847043 [details] [diff] [review] clean up PropertyProvider and its implementations. One thing that may not be perfect is, I can't make GetHyphenWidth() and GetSpacing() 'const', because they may update/access some member variables (ex. mHyphenWidth, mTabWidths, etc.). I thought about using 'volatile' for the member variables (and maybe some methods as well) so we can make all the methods in the abstract class 'const'. But, I'm not sure if this is something worth doing...

Comment on attachment 8847017 [details] [diff] [review] (wip). Ting-Yu, thank you for the feedback. :)

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #4) > Comment on attachment 8847043 [details] [diff] [review] > clean up PropertyProvider and its implementations. > > One thing that may not be perfect is, I can't make GetHyphenWidth() and > GetSpacing() 'const', because they may update/access some member variables > (ex. mHyphenWidth, mTabWidths, etc.). I thought about using 'volatile' for > the member variables (and maybe some methods as well) so we can make all the > methods in the abstract class 'const'. But, I'm not sure if this is > something worth doing... 'volatile' would not be appropriate; that tells the compiler that a variable might unexpectedly change at any time due to some outside effect. What you want here is 'mutable' for the member variables like mHyphenWidth that serve purely as internal caching within the class. That will allow them to be modified (mutated) even when the object is const (e.g. within a 'const' method). I think many of the member vars in nsTextFrame's PropertyProvider can also be made 'const', as they're set once (by the initializer list) during construction, and never changed after that.

Comment on attachment 8847043 [details] [diff] [review] clean up PropertyProvider and its implementations. Got great feedbacks from Jonathan, will update the patchset and re-request review then.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36a1fa7de7f4a36c2a8744a01884331706249ebe

Comment on attachment 8847497 [details] Bug 1343516 - make all methods 'const' for the abstract class of PropertyProvider. https://reviewboard.mozilla.org/r/120474/#review123362 Thanks for doing this, it cleans things up nicely (and might even help the compiler optimize better). The series of patches presents it in a logical way for review, but please fold the patches together before landing (as the tree wouldn't actually build in some of the intermediate stages, e.g. when the abstract PropertyProvider interface has been modified but the implementations haven't yet been fixed).

Comment on attachment 8847498 [details] Bug 1343516 - make all methods 'const' and 'final' for nsFontMetrics's StubPropertyProvider. https://reviewboard.mozilla.org/r/120476/#review123050 ::: gfx/src/nsFontMetrics.cpp:84 (Diff revision 1) > } > > RefPtr<gfxTextRun> mTextRun; > }; > > class StubPropertyProvider : public gfxTextRun::PropertyProvider { You could just do ``` class StubPropertyProvider final : public ... ``` rather than adding the `final` specifier to each of the individual methods.

Comment on attachment 8847499 [details] Bug 1343516 - make overridden methods 'const' and 'final' for nsTextFrame's PropertyProvider. https://reviewboard.mozilla.org/r/120478/#review123054 ::: gfx/thebes/gfxTextRun.cpp:2190 (Diff revision 1) > return MakeTextRun(&dash, 1, aDrawTarget, aAppUnitsPerDevUnit, > gfxFontGroup::TEXT_IS_PERSISTENT, nullptr); > } > > gfxFloat > -gfxFontGroup::GetHyphenWidth(gfxTextRun::PropertyProvider *aProvider) > +gfxFontGroup::GetHyphenWidth(const gfxTextRun::PropertyProvider *aProvider) While you're touching this, please move the `*` across to attach to the type instead of the parameter name. ::: layout/generic/nsTextFrame.cpp:3161 (Diff revision 1) > - virtual void GetSpacing(Range aRange, Spacing* aSpacing); > - virtual gfxFloat GetHyphenWidth(); > - virtual void GetHyphenationBreaks(Range aRange, HyphenType* aBreakBefore); > - virtual StyleHyphens GetHyphensOption() { > + void GetSpacing(Range aRange, Spacing* aSpacing) const final; > + gfxFloat GetHyphenWidth() const final; > + void GetHyphenationBreaks(Range aRange, HyphenType* aBreakBefore) const final; > + StyleHyphens GetHyphensOption() const final { > return mTextStyle->mHyphens; > } > > - virtual already_AddRefed<DrawTarget> GetDrawTarget() { > + already_AddRefed<DrawTarget> GetDrawTarget() const final { > return CreateReferenceDrawTarget(GetFrame()); > } > > - virtual uint32_t GetAppUnitsPerDevUnit() { > + uint32_t GetAppUnitsPerDevUnit() const final { > return mTextRun->GetAppUnitsPerDevUnit(); > } Like with StubPropertyProvider, you can just declare the class as `final` rather than each individual method.

Comment on attachment 8847500 [details] Bug 1343516 - make the member variables in nsTextFrame's PropertyProvider to be 'const'. https://reviewboard.mozilla.org/r/120480/#review123364

Comment on attachment 8847501 [details] Bug 1343516 - coding style fix for nsTextFrame's PropertyProvider. https://reviewboard.mozilla.org/r/120482/#review123366

Comment on attachment 8847497 [details] Bug 1343516 - make all methods 'const' for the abstract class of PropertyProvider. https://reviewboard.mozilla.org/r/120474/#review123362 No problem. Will fold the patchset together before landing. Thank you for the review. :)

Pushed by jichen@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/156ee3ddc810 clean up the mess of PropertyProvider and its implementations. r=jfkthame

The cleanup work includes: part 1: make all methods 'const' for the abstract class of PropertyProvider. part 2: make nsFontMetrics's StubPropertyProvider final. part 3: make nsTextFrame's PropertyProvider final. Make some methods 'const' and some variables 'mutable', so we could let all the overridden methods stay const. We also need to make the pass-in parameter of gfxFontGroup's GetHyphenWidth const. Note that the comment of GetHyphenWidth seem outdated, so I fixed it as well. part 4: make the member variables in nsTextFrame's PropertyProvider to be 'const'. Make all the member variables 'const' except mStart, mLength, mJustificationArrayStart, and mJustificationSpacings. The static function AdvanceToNextTab is fixed since we only use 2 of the 4 parameters. part 5: coding style fix for nsTextFrame's PropertyProvider. MozReview-Commit-ID: 1kbWPwx27aQ

Comment on attachment 8848470 [details] [diff] [review] clean up the mess of PropertyProvider and its implementations. r=jfkthame This is the final folded version.