88509 - accessible/src/nsHTMLSelectAccessible.cpp fails to build with --disable-bidi

Status: RESOLVED FIXED

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

Description

[pocemit]

Tt uses one of HTML layout atoms which is only defined when IBMBIDI is enabled.
This causes a build failure.
Patch follows.

Comment 1

[pocemit]

Attachment: patch to fix the problem

Comment 2

[Mike Kaply [:mkaply]]

Two things here:

1. Is accessible dependendant on this function to work? If so, it should not be 
IBMBIDI in layout.

2. Why are you building with --disable-bidi? The intent was that once BIDI was 
turned on, it would never be turned off. We have even talked about starting to 
remove the #ifdefs.

Comment 3

[pocemit]

I cannot answer 1) since I did not write that code, and made a "educated guess"
as far as that function needing to be wrapped in #ifdef IBMBIDI because the atom
it was using was also in IBMBIDI

and to answer 2), no reason really, just a default build line carried over from
a few months ago. I don't have any arabic/hebrew fonts anyway, I am not sure
having it enabled would allow me any extra features...

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think the correct solution here is to move the listControlFrame atom out of
|#ifdef IBMBIDI| and do the same for nsListControlFrame::GetFrameType.

Comment 5

[John Gaunt (redfive)]

I agree with dbaron, and since it was my regression I will take care of that. I
think this actually points to a bigger issue of why all the layout frames don't
have atoms defined for them. But that is a discussion for the layout folks, and
not really appropriate for this bug.

feel free to re-assign to me.

Comment 6

[John Gaunt (redfive)]

Attachment: patch that also moves the atom into the non-ifdef'd space

Comment 7

[timeless]

.

Comment 8

[John Gaunt (redfive)]

*** Bug 88762 has been marked as a duplicate of this bug. ***

Comment 9

[John Gaunt (redfive)]

looking for r/sr  cc'ing attinasi

Comment 10

[Chris Abbey]

um, John... that patch lands me with:

c++ -o nsListControlFrame.o -c -DOSTYPE=\"Linux2.4\" -DOSARCH=\"Linux\" -DOJI 
-D_IMPL_NS_HTML -DENDER_LITE   -I../../../../dist/include 
-I../../../../dist/include -I/usr/src/moz/mozilla/dist/include/nspr      
-I./../../../base/src -I./../../base/src -I./../../style/src 
-I./../../../xul/base/src  -I/usr/X11R6/include   -fPIC  -I/usr/X11R6/include 
-fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith 
-Wbad-function-cast -Wcast-align -Woverloaded-virtual -Wsynth -pedantic 
-Wno-long-long -pipe -fshort-wchar -pthread -O3  -DNDEBUG -DTRIMMED 
-I/usr/X11R6/include -DMOZILLA_CLIENT -include ../../../../config-defs.h 
-Wp,-MD,.deps/nsListControlFrame.pp nsListControlFrame.cpp
nsListControlFrame.cpp: In method `nsresult nsListControlFrame::Paint
(nsIPresContext *, nsIRenderingContext &, const nsRect &,
nsFramePaintLayer)':
nsListControlFrame.cpp:454: warning: unused variable `nsIStyleContext
*sc'
nsListControlFrame.cpp: In method `void
nsListControlFrame::MultipleSelection (int, int)':
nsListControlFrame.cpp:1455: warning: `PRInt32 i' might be used
uninitialized in this function
nsListControlFrame.cpp: In method `void
nsListControlFrame::SetContentSelected (int, nsIContent *, int, int,
nsIPresShell *)':
nsListControlFrame.cpp:2028: warning: unused variable `PRBool
isSelected'
nsListControlFrame.cpp: In method `nsresult
nsListControlFrame::UpdateSelection (int, int, nsIContent *)':
nsListControlFrame.cpp:2803: warning: unused variable `PRBool changed'
nsListControlFrame.cpp: At top level:
nsListControlFrame.cpp:3207: no `nsresult
nsListControlFrame::GetFrameType (nsIAtom **) const' member function
declared in class `nsListControlFrame'
nsListControlFrame.cpp: In method `void
nsListControlFrame::GetScrollableView (nsIScrollableView *&)':
nsListControlFrame.cpp:3483: warning: unused variable `nsresult rv'
../../../../dist/include/nsBufferHandle.h: At top level:
nsListControlFrame.cpp:1948: warning: `int cnt' defined but not used
gmake[5]: *** [nsListControlFrame.o] Error 1

anyway, as mkaply points out, a better solution would appear to be removing 
the now outdated --disable-bidi from .mozconfig, which is where I'm headed 
now....

Comment 11

[John Gaunt (redfive)]

Attachment: v1.1 patch -- removed ifdef around declaration

Comment 12

[John Gaunt (redfive)]

Whoops, forgot to look into the .h, the declaration was wrapped in #ifdef's as 
well. 

posted a diff, although I'm suspicous of it because I have weird thing going on 
in my tree right now. Re-pulling to fix things.

Chris: I would suggest you file a new bug as "Remove -disable-bidi". Where you 
pull all the ifdef's in addition to disabling the option. In the context of this 
bug, currently, its kinda like swatting a fly with a buick.

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

r=dbaron on attachment 40976 [details] [diff] [review]

Comment 14

[Simon Fraser [no longer active]]

sr=sfraser

Comment 15

[Chris Abbey]

John, as much as I like taking out Mosquitos with a tactical nuke... see bug 
89203 for removal of --disable-bidi; all 105 files of it so far.

Comment 16

[lchiang]

It appears this bug has the r and the sr.  Can this be checked in?  Thanks. 
I'll update the status whiteboard with this status for a quick summary.

Comment 17

[John Gaunt (redfive)]

This has been checked into the 0.9.2 branch wed 4 july.

needs to go into trunk. Will get that in ASAP.

Comment 18

[John Gaunt (redfive)]

Checked into the trunk 7/10