1281099 - Merge three bidi frame properties into one

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

Currently, there are three bidi-related frame properties: BaseLevelProperty, EmbeddingLevelProperty, and ParagraphDepthProperty. They are usually set and read together, and they are all 8bit integers.

We can convert them into a structure, and continue using small value property, so that we don't add any overhead. Actually it would likely make things faster because fewer properties lead to fewer calls to linear search procedure looking for properties.

This would also make it more extendable (so that we can store other infomation into this structure easier).

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1281099 part 1 - Convert some macros to functions to help later change.

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

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1281099 part 2 - Merge three bidi frame properties into one.

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8763780 [details]
Bug 1281099 part 2 - Merge three bidi frame properties into one.

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

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 8763779 [details]
Bug 1281099 part 1 - Convert some macros to functions to help later change.

https://reviewboard.mozilla.org/r/59906/#review56890

::: layout/generic/nsTextFrame.cpp:1609
(Diff revision 1)
>    if (mBidiEnabled &&
> -      (NS_GET_EMBEDDING_LEVEL(aFrame1) != NS_GET_EMBEDDING_LEVEL(aFrame2) ||
> -       NS_GET_PARAGRAPH_DEPTH(aFrame1) != NS_GET_PARAGRAPH_DEPTH(aFrame2)))
> +      (nsBidi::GetEmbeddingLevel(aFrame1) !=
> +       nsBidi::GetEmbeddingLevel(aFrame2) ||
> +       nsBidi::GetParagraphDepth(aFrame1) !=
> +       nsBidi::GetParagraphDepth(aFrame2)))
>      return false;

While you're here, might as well add the missing braces for the if-statement body -- thanks.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

https://reviewboard.mozilla.org/r/59906/#review56890

> While you're here, might as well add the missing braces for the if-statement body -- thanks.

They would be added in the second patch.

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8763780 [details]
Bug 1281099 part 2 - Merge three bidi frame properties into one.

https://reviewboard.mozilla.org/r/59908/#review56894

Nice!

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5)
> https://reviewboard.mozilla.org/r/59906/#review56890
> 
> > While you're here, might as well add the missing braces for the if-statement body -- thanks.
> 
> They would be added in the second patch.

So I just saw - fine, thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b53390fe3d19
part 1 - Convert some macros to functions to help later change. r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c11f8a65526
part 2 - Merge three bidi frame properties into one. r=jfkthame

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b53390fe3d19
https://hg.mozilla.org/mozilla-central/rev/1c11f8a65526