1308137 - Remove code around IBMBIDI_SUPPORTMODE_*

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

It seems to me the bidi support mode is no longer used. Searching IBMBIDI_SUPPORTMODE_ in our codebase, I don't find anything using that. [1]

I think we can completely remove the related declaration and preference.


[1] https://dxr.mozilla.org/mozilla-central/search?q=IBMBIDI_SUPPORTMODE_

Comment 1

[Dhanesh Sabane [:dhanesh95] (UTC+5:30)]

I would like to work on this bug. Please assign it to me.

Comment 2

[Dhanesh Sabane [:dhanesh95] (UTC+5:30)]

Just to be clear, all I need to do here is remove the code relating to IBM_BIDI_SUPPORTMODE_,right?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Yes, including the pref "bidi.support" and several other related macros (GET_BIDI_OPTION_SUPPORT and SET_BIDI_OPTION_SUPPORT at least). I guess that's all.

Comment 4

[Dhanesh Sabane [:dhanesh95] (UTC+5:30)]

Attachment: Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*.

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

Comment 5

[Dhanesh Sabane [:dhanesh95] (UTC+5:30)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #3)
> Yes, including the pref "bidi.support" and several other related macros
> (GET_BIDI_OPTION_SUPPORT and SET_BIDI_OPTION_SUPPORT at least). I guess
> that's all.

I've tried my hands at it and I've submitted a [review request](https://reviewboard-hg.mozilla.org/gecko/rev/4dc6ead7ebe5).

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8799207 [details]
Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*.

https://reviewboard.mozilla.org/r/84500/#review83010

Please fix the following issues:

::: intl/unicharutil/util/nsBidiUtils.h
(Diff revision 1)
> -#define IBMBIDI_SUPPORTMODE_STR         "bidi.support"
> -

I don't think the empty line here should be removed.

::: intl/unicharutil/util/nsBidiUtils.h:193
(Diff revision 1)
>  //  ------------------
>  //  Support Mode
>  //  ------------------
>  //  bidi.support

This comment is no longer needed as well.

::: intl/unicharutil/util/nsBidiUtils.h:201
(Diff revision 1)
>           (IBMBIDI_NUMERAL_NOMINAL<<8)          | \
> -         (IBMBIDI_SUPPORTMODE_MOZILLA<<12))

You should remove the "| \" at the end of the line before as well.

This should lead to compiler error, but it doesn't, which means this macro is not used at all... Sounds like the corresponding member field is not initialized properly.

Could you add a line to nsIDocument's constructor to initialize mBidiOptions with this macro?

::: intl/unicharutil/util/nsBidiUtils.h
(Diff revision 1)
> -#define GET_BIDI_OPTION_SUPPORT(bo) (((bo)>>12) & 0x0000000F) /* 4 bits for SUPPORT */
> -

The empty line here probably shouldn't be removed as well.

::: layout/base/nsPresContext.cpp
(Diff revision 1)
> -  prefInt =
> -    Preferences::GetInt(IBMBIDI_SUPPORTMODE_STR,
> -                        GET_BIDI_OPTION_SUPPORT(bidiOptions));
> -  SET_BIDI_OPTION_SUPPORT(bidiOptions, prefInt);

You should also remove this pref from modules/libpref/init/all.js and anything related to that pref.

Also you may want to add "r?xidorn" at the end of your commit message so that this patch would be put into my review queue.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8799207 [details]
Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*.

https://reviewboard.mozilla.org/r/84500/#review83020

::: intl/unicharutil/util/nsBidiUtils.h:165
(Diff revision 1)
>  #define IBMBIDI_TEXTDIRECTION       1
>  #define IBMBIDI_TEXTTYPE            2
>  #define IBMBIDI_NUMERAL             4
> -#define IBMBIDI_SUPPORTMODE         5

While you're here... in the absence of any comments, it's not evident what these constants were supposed to mean; but AFAICS none of them are used at all, anywhere. So I suggest removing all four of them, not just IBMBIDI_NUMERAL.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8799207 [details]
Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*.

https://reviewboard.mozilla.org/r/84500/#review83020

> While you're here... in the absence of any comments, it's not evident what these constants were supposed to mean; but AFAICS none of them are used at all, anywhere. So I suggest removing all four of them, not just IBMBIDI_NUMERAL.

Hmmm, you're right. These four macros are not used either, although their corresponding prefs are still effective. I didn't notice that.

Comment 9

[Dhanesh Sabane [:dhanesh95] (UTC+5:30)]

Attachment: Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*r?xidorn

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8799221 [details]
Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*r?xidorn

https://reviewboard.mozilla.org/r/84502/#review83024

Please squash the two commits together. They are doing a single thing. If you're using hg, you can use "hg histedit" and make the second one "roll" to the first one. If using git, it should be "git rebase -i" I think.

There are still some issues, see below. In addition, for the commit message, please finish the message sentence with a full stop (".") before adding "r?" flag. It should generally be something like "Bug XXXXX - Do something. r?someone"

::: dom/base/nsIDocument.h:214
(Diff revision 1)
> -  nsIDocument();
> +  nsIDocument()
> +  {
> +    mBidiOptions = IBMBIDI_DEFAULT_BIDI_OPTIONS;
> +  }

I bet you cannot build successfully with this change. The constructor of nsIDocument is defined in nsDocument.cpp. Please add it there, in the member initializer list.

::: intl/unicharutil/util/nsBidiUtils.h:194
(Diff revision 1)
> -//  bidi.support
>  
>  #define IBMBIDI_DEFAULT_BIDI_OPTIONS              \
>          ((IBMBIDI_TEXTDIRECTION_LTR<<0)         | \
>           (IBMBIDI_TEXTTYPE_CHARSET<<4)          | \
> -         (IBMBIDI_NUMERAL_NOMINAL<<8)          | \
> +         (IBMBIDI_NUMERAL_NOMINAL<<8))         

Please also remove the tailing whitespaces.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

dhanesh95: re your question on IRC, I think the most proper way is to do it in the member initializer list, which is something like
> nsIDocument()
>   : mBidiOptions(IBMBIDI_DEFAULT_BIDI_OPTIONS)
> {
> }

Note that you need to take care of the order of initializers in the list. Compiler would warn (which may lead to error on our server build) if the order is different from the order they are defined in the class.

Comment 12

[Dhanesh Sabane [:dhanesh95] (UTC+5:30)]

Comment on attachment 8799207 [details]
Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*.

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

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8799207 [details]
Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*.

https://reviewboard.mozilla.org/r/84500/#review84918

Looks good to me now. Thanks for fixing this!

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

Please click "Fixed" for the issue raised by Jonathan Kew, otherwise I cannot land this patch.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7c9c23a2338
Remove code around IBMBIDI_SUPPORTMODE_*. r=xidorn

Comment 16

[Dhanesh Sabane [:dhanesh95] (UTC+5:30)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #13)
> Comment on attachment 8799207 [details]
> Bug 1308137 - Remove code around IBMBIDI_SUPPORTMODE_*.
> 
> https://reviewboard.mozilla.org/r/84500/#review84918
> 
> Looks good to me now. Thanks for fixing this!

Thanks for helping me!

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f7c9c23a2338