1343516 - Clean up the mess of PropertyProvider and its implementations

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement)

Description

[Jeremy Chen ([e-mail])]

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

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: (wip).

MozReview-Commit-ID: J6VNxGye6ET

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

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

Comment 3

[Jeremy Chen ([e-mail])]

Attachment: clean up PropertyProvider and its implementations.

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 4

[Jeremy Chen ([e-mail])]

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 5

[Jeremy Chen ([e-mail])]

Comment on attachment 8847017 [details] [diff] [review]
(wip).

Ting-Yu, thank you for the feedback. :)

Comment 6

[Jonathan Kew [:jfkthame]]

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

[Jeremy Chen ([e-mail])]

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.

Comment 8

[Jeremy Chen ([e-mail])]

Attachment: Bug 1343516 - make all methods 'const' for the abstract class of PropertyProvider.

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

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Bug 1343516 - make all methods 'const' and 'final' for nsFontMetrics's StubPropertyProvider.

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

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Bug 1343516 - make overridden methods 'const' and 'final' for nsTextFrame's PropertyProvider.

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.

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

Comment 11

[Jeremy Chen ([e-mail])]

Attachment: Bug 1343516 - 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.

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

Comment 12

[Jeremy Chen ([e-mail])]

Attachment: Bug 1343516 - coding style fix for nsTextFrame's PropertyProvider.

I'm doing this at the last part of the patchset because we couldn't know
the length for horizontal alignment until we finish the refactoring work,
which is presented in the previous parts of the patchset.

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

Comment 13

[Jeremy Chen ([e-mail])]

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

Comment 14

[Jonathan Kew [:jfkthame]]

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 15

[Jonathan Kew [:jfkthame]]

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 16

[Jonathan Kew [:jfkthame]]

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 17

[Jonathan Kew [:jfkthame]]

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 18

[Jonathan Kew [:jfkthame]]

Comment on attachment 8847501 [details]
Bug 1343516 - coding style fix for nsTextFrame's PropertyProvider.

https://reviewboard.mozilla.org/r/120482/#review123366

Comment 19

[Jeremy Chen ([e-mail])]

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

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/156ee3ddc810
clean up the mess of PropertyProvider and its implementations. r=jfkthame

Comment 21

[Jeremy Chen ([e-mail])]

Attachment: 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 22

[Jeremy Chen ([e-mail])]

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

This is the final folded version.

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/156ee3ddc810