Remove code around IBMBIDI_SUPPORTMODE_*

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: xidorn, Assigned: dhanesh95, Mentored)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [good first bug][lang=c++])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

3 years ago
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_
I would like to work on this bug. Please assign it to me.
Reporter

Updated

3 years ago
Assignee: nobody → dhaneshsabane95
Just to be clear, all I need to do here is remove the code relating to IBM_BIDI_SUPPORTMODE_,right?
Reporter

Comment 3

3 years ago
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.
(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).
Reporter

Comment 6

3 years ago
mozreview-review
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

3 years ago
mozreview-review
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.
Reporter

Comment 8

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Reporter

Comment 10

3 years ago
mozreview-review
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.
Attachment #8799221 - Flags: review?(xidorn+moz)
Reporter

Comment 11

3 years ago
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 hidden (mozreview-request)
Attachment #8799221 - Attachment is obsolete: true
Reporter

Comment 13

3 years ago
mozreview-review
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!
Attachment #8799207 - Flags: review?(xidorn+moz) → review+
Reporter

Comment 14

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

Comment 15

3 years ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f7c9c23a2338
Remove code around IBMBIDI_SUPPORTMODE_*. r=xidorn
(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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f7c9c23a2338
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.