Closed
Bug 743680
Opened 12 years ago
Closed 12 years ago
don't export nsARIAMap.h
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
26.88 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
353 bytes,
patch
|
davidb
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #613293 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 1•12 years ago
|
||
fix build issues
Attachment #613293 -
Attachment is obsolete: true
Attachment #613293 -
Flags: review?(trev.saunders)
Attachment #613868 -
Flags: review?(trev.saunders)
Comment 2•12 years ago
|
||
Comment on attachment 613868 [details] [diff] [review] patch2 >diff --git a/accessible/src/base/nsAccUtils.cpp b/accessible/src/base/nsAccUtils.cpp >--- a/accessible/src/base/nsAccUtils.cpp >+++ b/accessible/src/base/nsAccUtils.cpp > #include "nsARIAMap.h" > #include "nsDocAccessible.h" >+#include "nsCoreUtils.h" shouldn't it come before nsDocAccessible.h? >+ bool HasARIARole() const >+ { return mRoleMapEntry; } without checking the callers, wouldn't it make more sense for this to check mUseMappedRole here too? >+#ifndef _nsAccessible_inl_H_ >+#define _nsAccessible_inl_H_ I gues that name sort of makes sense for now, but MOZILLA_A11Y_ACCESSIBLE_INL_H_ seems like it makes more sense. > #import "mozView.h" > #import "nsRoleMap.h" > >+#include "Accessible-inl.h" >+#include "nsIAccessible.h" couldn't you get rid of that one? > #import "mozActionElements.h" > > #import "MacUtils.h" >- >-#import "nsIAccessible.h" >+#import "nsAccessible-inl.h" how it work? > enum CheckboxValue { > // these constants correspond to the values in the OS >diff --git a/accessible/src/mac/nsAccessibleWrap.mm b/accessible/src/mac/nsAccessibleWrap.mm >--- a/accessible/src/mac/nsAccessibleWrap.mm >+++ b/accessible/src/mac/nsAccessibleWrap.mm >@@ -36,16 +36,17 @@ > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsDocAccessible.h" > #include "nsObjCExceptions.h" > > #import "nsRoleMap.h" > >+#include "Accessible-inl.h" > #include "Role.h" > > #import "mozAccessible.h" > #import "mozActionElements.h" > #import "mozHTMLAccessible.h" > #import "mozTextAccessible.h" > > using namespace mozilla::a11y; >diff --git a/accessible/src/xul/nsXULColorPickerAccessible.cpp b/accessible/src/xul/nsXULColorPickerAccessible.cpp >--- a/accessible/src/xul/nsXULColorPickerAccessible.cpp >+++ b/accessible/src/xul/nsXULColorPickerAccessible.cpp >@@ -33,16 +33,17 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsXULColorPickerAccessible.h" > >+#include "Accessible-inl.h" > #include "nsAccUtils.h" > #include "nsAccTreeWalker.h" > #include "nsCoreUtils.h" > #include "nsDocAccessible.h" > #include "Role.h" > #include "States.h" > > #include "nsIDOMElement.h" >diff --git a/accessible/src/xul/nsXULComboboxAccessible.cpp b/accessible/src/xul/nsXULComboboxAccessible.cpp >--- a/accessible/src/xul/nsXULComboboxAccessible.cpp >+++ b/accessible/src/xul/nsXULComboboxAccessible.cpp >@@ -35,16 +35,17 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsXULComboboxAccessible.h" > >+#include "Accessible-inl.h" > #include "nsAccessibilityService.h" > #include "nsDocAccessible.h" > #include "nsCoreUtils.h" > #include "Role.h" > #include "States.h" > > #include "nsIAutoCompleteInput.h" > #include "nsIDOMXULMenuListElement.h" >diff --git a/accessible/src/xul/nsXULFormControlAccessible.cpp b/accessible/src/xul/nsXULFormControlAccessible.cpp >--- a/accessible/src/xul/nsXULFormControlAccessible.cpp >+++ b/accessible/src/xul/nsXULFormControlAccessible.cpp >@@ -34,16 +34,17 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsXULFormControlAccessible.h" > >+#include "Accessible-inl.h" > #include "nsAccUtils.h" > #include "nsAccTreeWalker.h" > #include "nsCoreUtils.h" > #include "nsDocAccessible.h" > #include "Relation.h" > #include "Role.h" > #include "States.h" > >diff --git a/accessible/src/xul/nsXULListboxAccessible.cpp b/accessible/src/xul/nsXULListboxAccessible.cpp >--- a/accessible/src/xul/nsXULListboxAccessible.cpp >+++ b/accessible/src/xul/nsXULListboxAccessible.cpp >@@ -35,16 +35,17 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsXULListboxAccessible.h" > >+#include "Accessible-inl.h" > #include "nsAccessibilityService.h" > #include "nsAccUtils.h" > #include "nsDocAccessible.h" > #include "Role.h" > #include "States.h" > > #include "nsComponentManagerUtils.h" > #include "nsIAutoCompleteInput.h" >diff --git a/accessible/src/xul/nsXULMenuAccessible.cpp b/accessible/src/xul/nsXULMenuAccessible.cpp >--- a/accessible/src/xul/nsXULMenuAccessible.cpp >+++ b/accessible/src/xul/nsXULMenuAccessible.cpp >@@ -33,16 +33,17 @@ > * and other provisions required by the GPL or the LGPL. If you do not delete > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsXULMenuAccessible.h" > >+#include "Accessible-inl.h" > #include "nsAccessibilityService.h" > #include "nsAccUtils.h" > #include "nsDocAccessible.h" > #include "nsXULFormControlAccessible.h" > #include "Role.h" > #include "States.h" > > #include "nsIDOMElement.h" >diff --git a/accessible/src/xul/nsXULTextAccessible.cpp b/accessible/src/xul/nsXULTextAccessible.cpp >--- a/accessible/src/xul/nsXULTextAccessible.cpp >+++ b/accessible/src/xul/nsXULTextAccessible.cpp >@@ -35,16 +35,17 @@ > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > // NOTE: groups are alphabetically ordered > #include "nsXULTextAccessible.h" > >+#include "Accessible-inl.h" > #include "nsAccUtils.h" > #include "nsBaseWidgetAccessible.h" > #include "nsCoreUtils.h" > #include "nsTextEquivUtils.h" > #include "Relation.h" > #include "Role.h" > #include "States.h" >
Attachment #613868 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #2) > >+ bool HasARIARole() const > >+ { return mRoleMapEntry; } > > without checking the callers, wouldn't it make more sense for this to check > mUseMappedRole here too? maybe, don't want to go into aria-activedescendant logic. > >+#ifndef _nsAccessible_inl_H_ > >+#define _nsAccessible_inl_H_ > > I gues that name sort of makes sense for now, but > MOZILLA_A11Y_ACCESSIBLE_INL_H_ seems like it makes more sense. I can do that, btw, why do you like to keep them in capslock since it doens't look super nice. For example, Element.h does the same http://mxr.mozilla.org/mozilla-central/source/content/base/public/Element.h. I think I'd go with mozilla_a11y_Accessible_inl_h_ > >-#import "nsIAccessible.h" > >+#import "nsAccessible-inl.h" > > how it work? #import and #include is mostly the same thing, #import makes sure to not include the file twice. I get back to #include for internal header file.
Comment 4•12 years ago
|
||
(In reply to alexander :surkov from comment #3) > (In reply to Trevor Saunders (:tbsaunde) from comment #2) > > >+ bool HasARIARole() const > > >+ { return mRoleMapEntry; } > > > > without checking the callers, wouldn't it make more sense for this to check > > mUseMappedRole here too? > > maybe, don't want to go into aria-activedescendant logic. ok > > >+#ifndef _nsAccessible_inl_H_ > > >+#define _nsAccessible_inl_H_ > > > > I gues that name sort of makes sense for now, but > > MOZILLA_A11Y_ACCESSIBLE_INL_H_ seems like it makes more sense. > > I can do that, btw, why do you like to keep them in capslock since it > doens't look super nice. For example, Element.h does the same > http://mxr.mozilla.org/mozilla-central/source/content/base/public/Element.h. > I think I'd go with mozilla_a11y_Accessible_inl_h_ ok, I think its just habit to make macros all caps, but I don't think it really matters so long as its unique which I think it is either way. > > >-#import "nsIAccessible.h" > > >+#import "nsAccessible-inl.h" > > > > how it work? > > #import and #include is mostly the same thing, #import makes sure to not > include the file twice. I get back to #include for internal header file. sure, but "nsAccessible-inl.h"?
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #4) > > #import and #include is mostly the same thing, #import makes sure to not > > include the file twice. I get back to #include for internal header file. > > sure, but "nsAccessible-inl.h"? yep, I noticed that and fixed, it didn't work I assume.
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0975fb1e62d
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c0975fb1e62d
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 8•12 years ago
|
||
This commit broke Linux/clang builds: make[5]: Entering directory `/home/george/dev/mozilla-central/obj-linux-clang-rel/toolkit/library' ../../accessible/src/atk/nsAccessibleWrap.o: In function `getRoleCB': /home/george/dev/mozilla-central/accessible/src/atk/nsAccessibleWrap.cpp:738: undefined reference to `nsAccessible::Role()' ../../accessible/src/atk/nsMaiInterfaceAction.o: In function `getKeyBindingCB': /home/george/dev/mozilla-central/accessible/src/atk/nsMaiInterfaceAction.cpp:111: undefined reference to `nsAccessible::Role()' /home/george/dev/mozilla-central/accessible/src/atk/nsMaiInterfaceAction.cpp:126: undefined reference to `nsAccessible::Role()' clang: error: linker command failed with exit code 1 (use -v to see invocation)
Comment 9•12 years ago
|
||
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=746004 to request a buildbot with clang.
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #615556 -
Flags: review?(trev.saunders)
Comment 11•12 years ago
|
||
Comment on attachment 615556 [details] [diff] [review] clang followup rs=me
Attachment #615556 -
Flags: review?(trev.saunders) → review+
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #11) > Comment on attachment 615556 [details] [diff] [review] > clang followup > > rs=me https://hg.mozilla.org/integration/mozilla-inbound/rev/975341acf9f1
Comment 13•12 years ago
|
||
Thanks for landing the clang fix! The nsAccessible::Role() error is now gone. However I still get: /usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZN12nsAccessible4RoleEv' isn't defined /usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output
Assignee | ||
Comment 14•12 years ago
|
||
Jeff, do you have any ideas?
Comment 15•12 years ago
|
||
I don't have time to go spec diving atm, but my first thoughts are the following linker errors in the case that you don't include Accessible-inl.h some where sort of makes sense if the compiler has the right to not emit a function for methods declared inline since then the uses in files that don't incude Accessible-inl.h will have calls to nsAccessible::Role() which was never emitted as a function and always inlined. Which I think would lead to things like comment 13 On the other hand I can't say that I get these compile errors at all, Alex's patch would probably be my first guess, but I have no idea what the problem was off hand.
Comment 16•12 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #13) > Thanks for landing the clang fix! The nsAccessible::Role() error is now > gone. However I still get: > > /usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZN12nsAccessible4RoleEv' > isn't defined > /usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output I assume you've disabled debug symbols, since that looks like a mangled method name? I wonder if either using gold instead of ld.bfd or building with debug symbols provides more info?
Comment 18•12 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #16) > (In reply to Mihai Sucan [:msucan] from comment #13) > > Thanks for landing the clang fix! The nsAccessible::Role() error is now > > gone. However I still get: > > > > /usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZN12nsAccessible4RoleEv' > > isn't defined > > /usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output > > I assume you've disabled debug symbols, since that looks like a mangled > method name? I wonder if either using gold instead of ld.bfd or building > with debug symbols provides more info? For optimized builds I disable debug symbols to make builds faster. I now re-enabled debug symbols and the errors stays the same. I did a debug build as well: same error. I installed gold linker and the error is gone with the patch surkov submitted. If I qpop his patch gold linker gives more info: make[5]: entrant dans le répertoire « /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library » /usr/bin/ld.gold.real: error: /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/src/atk/nsAccessibleWrap.o: requires dynamic R_X86_64_PC32 reloc against '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with -fPIC /usr/bin/ld.gold.real: error: /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/src/atk/nsMaiInterfaceAction.o: requires dynamic R_X86_64_PC32 reloc against '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with -fPIC /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsAccessibleWrap.cpp:738: error: undefined reference to 'nsAccessible::Role()' /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp:111: error: undefined reference to 'nsAccessible::Role()' /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp:126: error: undefined reference to 'nsAccessible::Role()' clang: error: linker command failed with exit code 1 (use -v to see invocation) make[5]: *** [libxul.so] Erreur 1 This is on ubuntu 12.04 lts, up-to-date, with debug enabled in mozconfig, with clang 3.0 from the ubuntu repos. I also did an optimized build. Things work well now. Please let me know how can I further help. Thanks!
Comment 19•12 years ago
|
||
(In reply to Hub Figuiere [:hub] from comment #9) > Filed https://bugzilla.mozilla.org/show_bug.cgi?id=746004 > > to request a buildbot with clang. Good. Note if we haven't fixed the clang issues we should back out the cause until we can find the fix IMO. I use clang on my Linux box and am testing now...
Comment 20•12 years ago
|
||
I hit the same problem as comment 13. (Ubuntu)
Comment 21•12 years ago
|
||
Rafael any ideas? (See Linux/clang pain above)
Comment 22•12 years ago
|
||
> I now re-enabled debug symbols and the errors stays the same. debug symbols don't let ld.bfd give you unmangled names? :( > I installed gold linker and the error is gone with the patch surkov > submitted. If I qpop his patch gold linker gives more info: > > make[5]: entrant dans le répertoire « > /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library » > /usr/bin/ld.gold.real: error: > /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/ > src/atk/nsAccessibleWrap.o: requires dynamic R_X86_64_PC32 reloc against > '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with > -fPIC > /usr/bin/ld.gold.real: error: > /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/ > src/atk/nsMaiInterfaceAction.o: requires dynamic R_X86_64_PC32 reloc against > '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with > -fPIC > /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsAccessibleWrap.cpp:738: > error: undefined reference to 'nsAccessible::Role()' > /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp: > 111: error: undefined reference to 'nsAccessible::Role()' > /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp: > 126: error: undefined reference to 'nsAccessible::Role()' > clang: error: linker command failed with exit code 1 (use -v to see > invocation) > make[5]: *** [libxul.so] Erreur 1 ok, and his patch should have addressed all of those I think. > This is on ubuntu 12.04 lts, up-to-date, with debug enabled in mozconfig, > with clang 3.0 from the ubuntu repos. I also did an optimized build. Things > work well now. > > > Please let me know how can I further help. Thanks! I'd start by including Accessible-inl.h into random files that don't already include it hoping I'd find one that made things link (Since I can't think of a possible explanation or better idea) > Note if we haven't fixed the clang issues we should back out the cause until > we can find the fix IMO. I use clang on my Linux box and am testing now... I'm not sure I agree, should we also back out anything that breaks solaris, freebsd, or win32 with mingw?
Comment 23•12 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #22) > > Note if we haven't fixed the clang issues we should back out the cause until > > we can find the fix IMO. I use clang on my Linux box and am testing now... > > I'm not sure I agree, should we also back out anything that breaks solaris, > freebsd, or win32 with mingw? If our developers a finding those significantly useful yes, but I don't think that's the case.
I have started a build at 7467925b9648f76b7b7dc796ee041c8e6cedff1e using clang 155207. The mozconfig is browser/config/mozconfigs/linux64/debug with ac_add_options --enable-accessibility added
The build is still going, but from what the error is, it looks like clang is inlining nsAccessible::Role and dropping the symbol. So if some file is using it without including Accessible-inc.h, it will get an undefined reference.
Comment 26•12 years ago
|
||
Oh I just noticed I'm barfing with: /usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZNK14nsWrapperCache10GetWrapperEv' isn't defined /usr/bin/ld.bfd.real: final link failed: Bad value Note: davidb@ubuntu:~/moz/mozilla-inbound$ cd objdir/ davidb@ubuntu:~/moz/mozilla-inbound/objdir$ find . -name *.o |xargs nm |grep _ZNK14nsWrapperCache10GetWrapperEv U _ZNK14nsWrapperCache10GetWrapperEv davidb@ubuntu:~/moz/mozilla-inbound/objdir$
It looks like the missing reference is coming from clang not optimizing away the call in return EnsureInnerWindow() ? GetWrapper() : nsnull; I have no idea how gold created the library with the missing symbol. In any case, just including nsWrapperCacheInlines.h in nsGlobalWindow.h fixes the problem for me.
Attachment #617064 -
Flags: review?(dbolter)
Comment 28•12 years ago
|
||
Comment on attachment 617064 [details] [diff] [review] proposed patch Review of attachment 617064 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Thanks!
Attachment #617064 -
Flags: review?(dbolter) → review+
Comment 29•12 years ago
|
||
(In reply to David Bolter [:davidb] from comment #28) > Comment on attachment 617064 [details] [diff] [review] > proposed patch > > Review of attachment 617064 [details] [diff] [review]: > ----------------------------------------------------------------- > > r=me. Thanks! it seems like this fixes your issue in comment 26, but I have a hard time seeing how this bug introduced that, and it seems like it shouldn't fix comment 13?
I was only ever able to reproduce the failure in comment 26, are you seeing others?
Comment 31•12 years ago
|
||
(In reply to Trevor Saunders (:tbsaunde) from comment #22) > > I now re-enabled debug symbols and the errors stays the same. > > debug symbols don't let ld.bfd give you unmangled names? :( It seems that no, it doesn't. (In reply to David Bolter [:davidb] from comment #26) > Oh I just noticed I'm barfing with: > /usr/bin/ld.bfd.real: libxul.so: hidden symbol > `_ZNK14nsWrapperCache10GetWrapperEv' isn't defined > /usr/bin/ld.bfd.real: final link failed: Bad value I have also seen this error in some of my tests. Can't remember which.
You need to log in
before you can comment on or make changes to this bug.
Description
•