Closed Bug 1281099 Opened 4 years ago Closed 4 years ago

Merge three bidi frame properties into one

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(2 files)

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 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/
Attachment #8763779 - Flags: review?(jfkthame) → review+
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.
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 on attachment 8763780 [details]
Bug 1281099 part 2 - Merge three bidi frame properties into one.

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

Nice!
Attachment #8763780 - Flags: review?(jfkthame) → review+
(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!
Pushed by xquan@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b53390fe3d19
https://hg.mozilla.org/mozilla-central/rev/1c11f8a65526
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.