1307728 - Convert LineReflowStatus to an enum class

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: Bug 1307728 - use LineReflowStatusToString to support debugging.

This is a pre-patch that uses LineReflowStatusToString() function instead of
LineReflowStatusNames array to support debugging. With this patch, we could
easily regain debugging support from LineReflowStatusToString() while converting
LineReflowStatus to an enum class.

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

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: Bug 1307728 - convert LineReflowStatus to an enum class.

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

Comment 3

[Jeremy Chen ([e-mail])]

Comment on attachment 8797964 [details]
Bug 1307728 - use LineReflowStatusToString to support debugging.

https://reviewboard.mozilla.org/r/83584/#review82160

There are few extra whitespaces that should be purged. I decided to purge them while converting LineReflowStatus to an enum class since the number is few. Hope this won't cause any pain while reviewing the patch.

Comment 4

[Mats Palmgren (inactive)]

Perhaps we should remove the "default:" branch in the switch so we'll
get a warning there if someone adds a new value without updating it?

Comment 5

[Jeremy Chen ([e-mail])]

(In reply to Mats Palmgren (:mats) from comment #4)
> Perhaps we should remove the "default:" branch in the switch so we'll
> get a warning there if someone adds a new value without updating it?

Hi Mats, shouldn't this be caught by the MOZ_ASSERT_UNREACHABLE() outside the switch?

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

An enum class cannot have a value other than defined, so "default:" branch is not needed.  And without the "default:", if some one adds a new value without a correspond switch-case added, compiler could output a warning says something like "enumeration value XXX not handled in switch."

Comment 7

[Jeremy Chen ([e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #6)
> An enum class cannot have a value other than defined, so "default:" branch
> is not needed.  And without the "default:", if some one adds a new value
> without a correspond switch-case added, compiler could output a warning says
> something like "enumeration value XXX not handled in switch."

Sounds great! Mats, TY, thank you for the pointer. :-)

BTW, Mats, since you've already taken a peek at these patches, could you help review them?

Comment 8

[Jeremy Chen ([e-mail])]

Comment on attachment 8797964 [details]
Bug 1307728 - use LineReflowStatusToString to support debugging.

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

Comment 9

[Jeremy Chen ([e-mail])]

Comment on attachment 8797965 [details]
Bug 1307728 - convert LineReflowStatus to an enum class.

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

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 8797964 [details]
Bug 1307728 - use LineReflowStatusToString to support debugging.

https://reviewboard.mozilla.org/r/83586/#review82464

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8797965 [details]
Bug 1307728 - convert LineReflowStatus to an enum class.

https://reviewboard.mozilla.org/r/83588/#review82462

::: layout/generic/nsBlockFrame.cpp:3953
(Diff revision 2)
>  
>    aState.mFlags.mIsLineLayoutEmpty = aLineLayout.LineIsEmpty();
>  
>    // We only need to backup if the line isn't going to be reflowed again anyway
>    bool needsBackup = aLineLayout.NeedsBackup() &&
> -    (lineReflowStatus == LINE_REFLOW_STOP || lineReflowStatus == LINE_REFLOW_OK);
> +    (lineReflowStatus == LineReflowStatus::Stop || lineReflowStatus == LineReflowStatus::OK);

This line is a tad long, could you wrap it please?

Comment 12

[Jeremy Chen ([e-mail])]

Comment on attachment 8797965 [details]
Bug 1307728 - convert LineReflowStatus to an enum class.

https://reviewboard.mozilla.org/r/83588/#review82462

> This line is a tad long, could you wrap it please?

Sure, will do. Thank you for the review.

Comment 13

[Jeremy Chen ([e-mail])]

Comment on attachment 8797964 [details]
Bug 1307728 - use LineReflowStatusToString to support debugging.

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

Comment 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8797965 [details]
Bug 1307728 - convert LineReflowStatus to an enum class.

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dc76dc5df6d9
use LineReflowStatusToString to support debugging. r=mats
https://hg.mozilla.org/integration/autoland/rev/6ffa724dba73
convert LineReflowStatus to an enum class. r=mats

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dc76dc5df6d9
https://hg.mozilla.org/mozilla-central/rev/6ffa724dba73