Remove remaining uses of bidi.direction pref

NEW
Assigned to

Status

()

Core
Layout: Text
5 years ago
4 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

(Depends on: 1 bug, {rtl})

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Since bug 151407 the pref bidi.direction is no longer used to determine the default direction of web pages. It has some other uses that in principle can be removed as well. Bug 454420 is I think the only case that needs a decision and resolution first.
(Assignee)

Comment 1

5 years ago
Created attachment 746804 [details] [diff] [review]
Patch

Assuming that bug 454420 will remove the use of the pref for determining scrollbar side, this removes the other uses and the pref itself, also the "bidi.support" pref, which is a fossil that I don't think we have ever had code support for.

FTR, I understand that the "bidi.texttype" pref is used in some terminal emulation webapps so I don't want to remove that.
Attachment #746804 - Flags: review?(ehsan)

Comment 2

5 years ago
Comment on attachment 746804 [details] [diff] [review]
Patch

Review of attachment 746804 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

::: docshell/base/nsIMarkupDocumentViewer.idl
@@ +103,3 @@
>     * Use this attribute to access all the Bidi options in one operation
>     */
> +  attribute octet bidiOptions;

Please change the uuid of this interface.

::: layout/base/nsBidiPresUtils.cpp
@@ +1662,5 @@
>          HandleNumbers(aText,aTextLength,IBMBIDI_NUMERAL_ARABIC);
>        break;
>  
>      case IBMBIDI_NUMERAL_PERSIANCONTEXT:
> +      if ( ( (aIsOddLevel) && (IS_ARABIC_DIGIT (aText[0])) ) || (eCharType_ArabicNumber == aCharType) )

Oh, looking at this brings back "good" memories of when I looked at this code before!
Attachment #746804 - Flags: review?(ehsan) → review+
(Assignee)

Updated

4 years ago
Duplicate of this bug: 946647
Comment on attachment 746804 [details] [diff] [review]
Patch

Why aren't all bidi-related methods removed from nsDocumentViewer.cpp? I didn't see callers for any of those methods in m-c or c-c.
(In reply to Simon Montagu :smontagu from comment #1)
> FTR, I understand that the "bidi.texttype" pref is used in some terminal
> emulation webapps so I don't want to remove that.

In what way do webapps use this? Does it have any effect whatsoever on Gecko behavior? I suppose it did in the past, but I suspect that's no longer true.
(Assignee)

Comment 6

4 years ago
Those are all interesting questions but for the time being they are all theoretical. If somebody really wants to add value to this bug it would be more useful to get some traction on 454420.
(Assignee)

Comment 7

4 years ago
(In reply to Henri Sivonen (:hsivonen) from comment #4)
> Why aren't all bidi-related methods removed from nsDocumentViewer.cpp? I
> didn't see callers for any of those methods in m-c or c-c.

I thought that if it implemented nsIMarkupDocumentViewer it had to retain getters and setters for all the attributes on the interface. Is that not the case?
(In reply to Simon Montagu :smontagu from comment #7)
> I thought that if it implemented nsIMarkupDocumentViewer it had to retain
> getters and setters for all the attributes on the interface. Is that not the
> case?

Not the case if we remove the attributes from the interface. And why wouldn't we?

(In reply to Simon Montagu :smontagu from comment #6)
> Those are all interesting questions but for the time being they are all
> theoretical. If somebody really wants to add value to this bug it would be
> more useful to get some traction on 454420.

If this bug isn't moving, how about we land the patch from bug 946647 to get rid of dead code we could get rid of right away?
You need to log in before you can comment on or make changes to this bug.