1227001 - Probably get rid of SetupBreakSinksFlags?

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+11]

It seems to me that SetupBreakSinksFlags completely makes no sense. Its three values:
SBS_DOUBLE_BYTE is set according to mDoubleByteText
SBS_EXISTING_TEXTRUN is never set
SBS_SUPPRESS_SINK is set according to mSkipIncompleteTextRuns

SetupBreakSinksForTextRun which accepts the flags is a method of BuildTextRunsScanner, which can access mDoubleByteText and mSkipIncompleteTextRuns. I don't see any reason why we need those flags there.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

These flags was added in bug 703100 without explanation why they were necessary.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

SBS_EXISTING_TEXTRUN never makes sense since its existing, because the only place set aExistingTextRun parameter of SetupBreakSinksForTextRun to true is removed in the bug created this constant.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1227001 part 1 - Remove SetupBreakSinksFlags from BuildTextRunsScanner.

Bug 1227001 part 1 - Remove SetupBreakSinksFlags from BuildTextRunsScanner.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1227001 part 2 - Remove no longer used mExistingTextRun from BreakSink.

Bug 1227001 part 2 - Remove no longer used mExistingTextRun from BreakSink.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Attachment: MozReview Request: Bug 1227001 part 3 - Remove no longer used mChangedBreaks from BreakSink.

Bug 1227001 part 3 - Remove no longer used mChangedBreaks from BreakSink.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8690598 [details]
MozReview Request: Bug 1227001 part 1 - Remove SetupBreakSinksFlags from BuildTextRunsScanner.

https://reviewboard.mozilla.org/r/25871/#review23311

Yeah, AFAICS this is fine. I don't remember anything specific about these flags; clearly they were just replacing existing bool parameters with a single flags word, but as you note, when we look a bit more closely there's no actual need to pass them at all. Thanks for the cleanup!

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8690599 [details]
MozReview Request: Bug 1227001 part 2 - Remove no longer used mExistingTextRun from BreakSink.

https://reviewboard.mozilla.org/r/25873/#review23313

Comment 8

[Jonathan Kew [:jfkthame]]

Comment on attachment 8690600 [details]
MozReview Request: Bug 1227001 part 3 - Remove no longer used mChangedBreaks from BreakSink.

https://reviewboard.mozilla.org/r/25875/#review23315

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/110de326fa1a77de38da6e4c088af5ec837cba06
Bug 1227001 part 1 - Remove SetupBreakSinksFlags from BuildTextRunsScanner. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e16f87eda8902ba514f240032c9a00a7835da95
Bug 1227001 part 2 - Remove no longer used mExistingTextRun from BreakSink. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/536455795509134faa9321924793a189c63ba104
Bug 1227001 part 3 - Remove no longer used mChangedBreaks from BreakSink. r=jfkthame

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/110de326fa1a
https://hg.mozilla.org/mozilla-central/rev/4e16f87eda89
https://hg.mozilla.org/mozilla-central/rev/536455795509