Closed Bug 833164 Opened 12 years ago Closed 10 months ago

code gen implementation of xpcom accessible events

Categories

(Core :: Disability Access APIs, defect)

Product:
Core
Component:
Disability Access APIs
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: tbsaunde, Assigned: tbsaunde)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

patch
59.44 KB, patch
ted
: review-
Details | Diff | Splinter Review
consolodate the various makeDepend things in the tree
12.58 KB, patch
ted
: review+
Details | Diff | Splinter Review
patch 2
58.38 KB, patch
surkov
: review+
ted
: review+
Details | Diff | Splinter Review
fix DOMi
1.70 KB, patch
neil
: review+
Details | Diff | Splinter Review
there's no good reason ot hand wirte this code when we can autogenerate it. It should be easier to add new event types since all you need to do is add idl and arrange for construction of the new event in a11y::MakeXPCEvent() as well as add event name to config file. I also want to do this so xpcom event doesn't hold onto internal event for bug 829320.
Attached patch patch — — Splinter Review
ted for the build goop and checking my python isn't totally insane if he likes and Alex for the rest of it.
Attachment #704731 - Flags: review?(surkov.alexander)
Attachment #704731 - Flags: review?(ted)
Comment on attachment 704731 [details] [diff] [review] patch Review of attachment 704731 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/src/xpcom/AccEventGen.py @@ +1,2 @@ > +#!/usr/bin/env python > +# header.py - Generate C++ header files from IDL. Not really, no. ::: accessible/src/xpcom/AccEvents.conf @@ +4,5 @@ > + file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > + The name of the event which real interface should have nsIDOM-prefix, > + and should be in nsIDOM<name>.idl file and which should have > + <name>Init dictionary for the event constructor. """ I haven't seen any of those.
Comment on attachment 704731 [details] [diff] [review] patch Review of attachment 704731 [details] [diff] [review]: ----------------------------------------------------------------- what is a point to change isMethod() to isProperty? ::: accessible/public/nsIAccessibleCaretMoveEvent.idl @@ +5,5 @@ > + > +#include "nsIAccessibleEvent.idl" > + > +[scriptable, builtinclass, uuid(5675c486-a230-4d85-a4bd-33670826d5ff)] > +interface nsIAccessibleCaretMoveEvent: nsIAccessibleEvent it'd be great to comment these interfaces shortly ::: accessible/src/base/AccEvent.cpp @@ +185,5 @@ > + uint32_t type = aEvent->GetEventType(); > + nsCOMPtr<nsIAccessibleEvent> xpEvent; > + > + switch (type) { > + case nsIAccessibleEvent::EVENT_STATE_CHANGE: { it seems it's more correct to be GetEventGroups based rather than event type based. @@ +189,5 @@ > + case nsIAccessibleEvent::EVENT_STATE_CHANGE: { > + AccStateChangeEvent* sc = downcast_accEvent(aEvent); > + bool extra = sc->GetState() > UINT32_MAX; > + bool enabled = sc->IsStateEnabled(); > + uint32_t state = extra ? sc->GetState() >> 31 : sc->GetState(); why do you reject to use nsAccUtils::To32States helper function? @@ +203,5 @@ > + bool insert = tc->IsTextInserted(); > + uint32_t start = tc->GetStartOffset(); > + uint32_t len = tc->GetLength(); > + xpEvent = new nsAccTextChangeEvent(type, acc, doc, domNode, fromUser, > + start, len, insert, text); In general the code seems getting more complicated that it was (taking into account AccEventGen.py). I hoped that autogeneration will handle these 'case' statements. ::: accessible/src/base/AccEvent.h @@ +466,5 @@ > +/** > + * Return a new xpcom accessible event for the given internal one. > + */ > +already_AddRefed<nsIAccessibleEvent> > +MakeXPCEvent(AccEvent* aEvent); shouldn't it be somewhere in xpcom directory? ::: accessible/src/xpcom/AccEventGen.py @@ +110,5 @@ > + fd.write("private:\n") > + for a in attributes: > + fd.write(" %s\n" % attributeVariableTypeAndName(a)) > + fd.write("};\n\n") > + nit: whitepspace @@ +140,5 @@ > + types = [] > + for e in conf.simple_events: > + idl = loadEventIDL(p, options.incdirs, e) > + types.extend(interfaceAttributeTypes(idl)) > + nit: same ::: accessible/src/xpcom/AccEvents.conf @@ +13,5 @@ > + 'TextChangeEvent', > + 'HideEvent', > + 'CaretMoveEvent', > + 'TableChangeEvent', > + 'VirtualCursorChangeEvent' 4 spaces indent is intentional style? ::: accessible/src/xpcom/Makefile.in @@ +13,5 @@ > MODULE = accessibility > LIBRARY_NAME = accessibility_xpcom_s > LIBXUL_LIBRARY = 1 > > +EXPORTS := nsAccEvents.h xpcAccEvents.h?
Attachment #704731 - Flags: review?(surkov.alexander)
> what is a point to change isMethod() to isProperty? there was no reason for it to be a method since it doesn't take any arguments or change any state. I changed it here so the code gen could avoid supporting things other than read only attributes. > ::: accessible/public/nsIAccessibleCaretMoveEvent.idl > @@ +5,5 @@ > > + > > +#include "nsIAccessibleEvent.idl" > > + > > +[scriptable, builtinclass, uuid(5675c486-a230-4d85-a4bd-33670826d5ff)] > > +interface nsIAccessibleCaretMoveEvent: nsIAccessibleEvent > > it'd be great to comment these interfaces shortly they already had no comments, but I guess I can just do it. > ::: accessible/src/base/AccEvent.cpp > @@ +185,5 @@ > > + uint32_t type = aEvent->GetEventType(); > > + nsCOMPtr<nsIAccessibleEvent> xpEvent; > > + > > + switch (type) { > > + case nsIAccessibleEvent::EVENT_STATE_CHANGE: { > > it seems it's more correct to be GetEventGroups based rather than event type > based. I'm not sure what case you think about here. > @@ +189,5 @@ > > + case nsIAccessibleEvent::EVENT_STATE_CHANGE: { > > + AccStateChangeEvent* sc = downcast_accEvent(aEvent); > > + bool extra = sc->GetState() > UINT32_MAX; > > + bool enabled = sc->IsStateEnabled(); > > + uint32_t state = extra ? sc->GetState() >> 31 : sc->GetState(); > > why do you reject to use nsAccUtils::To32States helper function? I thought there was a reason it didn't work here or something, but maybe there is no good reason. > @@ +203,5 @@ > > + bool insert = tc->IsTextInserted(); > > + uint32_t start = tc->GetStartOffset(); > > + uint32_t len = tc->GetLength(); > > + xpEvent = new nsAccTextChangeEvent(type, acc, doc, domNode, fromUser, > > + start, len, insert, text); > > In general the code seems getting more complicated that it was (taking into perhaps in some ways since c++ is now generated, but on other hand diff stat says there's less total code. Also it should now be much easier to implement new xpcom acc event types. > account AccEventGen.py). I hoped that autogeneration will handle these > 'case' statements. given there isn't a 1:1 mapping for classes, and there's that funny special case for hide events I'd don't really see how that could work, but if you have ideas autogenerating the switch would be nice. > ::: accessible/src/base/AccEvent.h > @@ +466,5 @@ > > +/** > > + * Return a new xpcom accessible event for the given internal one. > > + */ > > +already_AddRefed<nsIAccessibleEvent> > > +MakeXPCEvent(AccEvent* aEvent); > > shouldn't it be somewhere in xpcom directory? I don't really care, that was just a convenient dumping ground. > ::: accessible/src/xpcom/AccEvents.conf > @@ +13,5 @@ > > + 'TextChangeEvent', > > + 'HideEvent', > > + 'CaretMoveEvent', > > + 'TableChangeEvent', > > + 'VirtualCursorChangeEvent' > > 4 spaces indent is intentional style? since its python probably > ::: accessible/src/xpcom/Makefile.in > @@ +13,5 @@ > > MODULE = accessibility > > LIBRARY_NAME = accessibility_xpcom_s > > LIBXUL_LIBRARY = 1 > > > > +EXPORTS := nsAccEvents.h > > xpcAccEvents.h? sure
Comment on attachment 704731 [details] [diff] [review] patch Review of attachment 704731 [details] [diff] [review]: ----------------------------------------------------------------- r- just for the amount of code duplication, we need to start sharing code between all these code generators. I think longer-term we're going to need some magic Makefile (and eventually moz.build) variables to control code generation. Having custom Makefile rules like this is not fantastic. ::: accessible/src/xpcom/AccEventGen.py @@ +10,5 @@ > +# --makedepend-output support. > +make_dependencies = [] > +make_targets = [] > + > +# Copied from dombindingsgen.py That file doesn't seem to exist anymore. If you need to share code with other Python scripts in the tree now it's fairly simple to put it in a module and import it, since we have a virtualenv in the objdir. Considering that we have this code duplicated three times already it would be great to move it to a common module: http://mxr.mozilla.org/mozilla-central/search?string=writeMakeDependOutput @@ +16,5 @@ > + return filename.replace(' ', '\\ ') # enjoy! > + > +def writeMakeDependOutput(filename): > + print "Creating makedepend file", filename > + f = open(filename, 'w') While you're at it this could be changed to "with open(filename, 'w') as f:" and drop the try/finally bits. @@ +262,5 @@ > + writeMakeDependOutput(options.makedepend_output) > + if options.header_output is not None: > + outfd = open(options.header_output, 'w') > + print_header_file(outfd, conf) > + outfd.close() Can you move this whole block into a def main(): and then just call main() from the if __name__ == '__main__' bit? ::: accessible/src/xpcom/Makefile.in @@ +68,5 @@ > + $(srcdir)/AccEventGen.py \ > + $(LIBXUL_DIST)/sdk/bin/header.py \ > + $(LIBXUL_DIST)/sdk/bin/xpidl.py \ > + $(DEPTH)/js/src/js-confdefs.h > + $(PYTHON) $(topsrcdir)/config/pythonpath.py \ You don't need pythonpath anymore, we have a virtualenv in the objdir. If the PLY bits are not already installed into the virtualenv you can fix that here: http://mxr.mozilla.org/mozilla-central/source/build/virtualenv/packages.txt
Attachment #704731 - Flags: review?(ted) → review-
> I think longer-term we're going to need some magic Makefile (and eventually > moz.build) variables to control code generation. Having custom Makefile > rules like this is not fantastic. sounds nice, but I doubt I know make well enough to do that without a bit of help. > ::: accessible/src/xpcom/AccEventGen.py > @@ +10,5 @@ > > +# --makedepend-output support. > > +make_dependencies = [] > > +make_targets = [] > > + > > +# Copied from dombindingsgen.py > > That file doesn't seem to exist anymore. If you need to share code with > other Python scripts in the tree now it's fairly simple to put it in a > module and import it, since we have a virtualenv in the objdir. Considering > that we have this code duplicated three times already it would be great to > move it to a common module: hah, you think I actually know python ;) > @@ +262,5 @@ > > + writeMakeDependOutput(options.makedepend_output) > > + if options.header_output is not None: > > + outfd = open(options.header_output, 'w') > > + print_header_file(outfd, conf) > > + outfd.close() > > Can you move this whole block into a def main(): and then just call main() > from the if __name__ == '__main__' bit? sure, I just coppied it from the dom event code gen.
(In reply to Trevor Saunders (:tbsaunde) from comment #6) > > I think longer-term we're going to need some magic Makefile (and eventually > > moz.build) variables to control code generation. Having custom Makefile > > rules like this is not fantastic. > > sounds nice, but I doubt I know make well enough to do that without a bit of > help. Yeah, I'm not suggesting that you do that in this bug, we can file a followup bug on it.
Trevor and I talked on IRC, it's not currently possible to use xpidl.py from the virtualenv, so I filed bug 841713 on that, but won't block this patch on it.
Comment on attachment 713886 [details] [diff] [review] consolodate the various makeDepend things in the tree Review of attachment 713886 [details] [diff] [review]: ----------------------------------------------------------------- Just two nits that need to be fixed before landing, otherwise looks good. ::: python/codegen/makeUtils.py @@ +1,1 @@ > + Needs a MPL header. Also, normal Python module naming conventions don't use mixed case, so maybe "makeutils" (no caps) or "make_utils" here. @@ +1,3 @@ > + > +dependencies = [] > +targets = [] I'd probably feel a little more comfortable with this if this was all wrapped up in a class or something, but this is an improvement from what we already have.
Attachment #713886 - Flags: review?(ted) → review+
Whiteboard: [leave open]
Trev, do you need to update the patch after the landing?
(In reply to alexander :surkov from comment #13) > Trev, do you need to update the patch after the landing? a new patch will be coming yes
Attached patch patch 2 — — Splinter Review
Attachment #719659 - Flags: review?(ted)
Attachment #719659 - Flags: review?(surkov.alexander)
Comment on attachment 719659 [details] [diff] [review] patch 2 Review of attachment 719659 [details] [diff] [review]: ----------------------------------------------------------------- ::: accessible/public/nsIAccessibleTableChangeEvent.idl @@ +15,5 @@ > + > + /** > + * Return the number of rows or cols > + */ > + readonly attribute long numRowsOrCols; it'd be nice to get it renamed to rowOrColsCount ::: accessible/src/base/AccEvent.cpp @@ +181,5 @@ > + Accessible* acc = aEvent->GetAccessible(); > + nsINode* node = acc->GetNode(); > + nsIDOMNode* domNode = node ? node->AsDOMNode() : nullptr; > + bool fromUser = aEvent->IsFromUserInput(); > + uint32_t type = aEvent->GetEventType(); actually you don't really need to have locals for these items? @@ +189,5 @@ > + if (eventGroup & AccEvent::eStateChangeEvent) { > + AccStateChangeEvent* sc = downcast_accEvent(aEvent); > + bool extra = sc->GetState() > UINT32_MAX; > + bool enabled = sc->IsStateEnabled(); > + uint32_t state = extra ? sc->GetState() >> 31 : sc->GetState(); >> why do you reject to use nsAccUtils::To32States helper function? >I thought there was a reason it didn't work here or something, but maybe there is no >good reason. then pls use it, I'd like to keep code shared @@ +198,5 @@ > + > + if (eventGroup & AccEvent::eTextChangeEvent) { > + AccTextChangeEvent* tc = downcast_accEvent(aEvent); > + nsString text; > + tc->GetModifiedText(text); still unclear issue with me: nsString vs nsAutoString @@ +203,5 @@ > + bool insert = tc->IsTextInserted(); > + uint32_t start = tc->GetStartOffset(); > + uint32_t len = tc->GetLength(); > + xpEvent = new nsAccTextChangeEvent(type, acc, doc, domNode, fromUser, > + start, len, insert, text); it seems you don't need locals for these @@ +209,5 @@ > + } > + > + if (eventGroup & AccEvent::eHideEvent) { > + AccHideEvent* hideEvent = downcast_accEvent(aEvent); > + if (hideEvent) { how it can be null? @@ +240,5 @@ > + } > + > + xpEvent = new nsAccEvent(type, acc, doc, domNode, fromUser); > + return xpEvent.forget(); > + } nit: wrong offset for '}' ::: accessible/src/base/AccEvent.h @@ +471,5 @@ > +/** > + * Return a new xpcom accessible event for the given internal one. > + */ > +already_AddRefed<nsIAccessibleEvent> > +MakeXPCEvent(AccEvent* aEvent); it's nice to keep it in xpcom folder (I see xpcAccEvent.h(cpp) are in use) ::: accessible/src/jsat/Utils.jsm @@ +218,5 @@ > eventToString: function eventToString(aEvent) { > let str = Utils.AccRetrieval.getStringEventType(aEvent.eventType); > if (aEvent.eventType == Ci.nsIAccessibleEvent.EVENT_STATE_CHANGE) { > let event = aEvent.QueryInterface(Ci.nsIAccessibleStateChangeEvent); > + let stateStrings = (event.isExtraState) ? nit: you don't need braces ::: accessible/src/xpcom/AccEventGen.py @@ +53,5 @@ > + fd.write("#include \"nsIAccessible%s.h\"\n" % e) > + for e in conf.simple_events: > + idl = loadEventIDL(p, options.incdirs, e) > + for iface in filter(lambda p: p.kind == "interface", idl.productions): > + classname = ("nsAcc%s" % e) pls rename nsAcc -> xpcAcc @@ +79,5 @@ > + fd.write("private:\n") > + for a in attributes: > + fd.write(" %s\n" % attributeVariableTypeAndName(a)) > + fd.write("};\n\n") > + nit: whitespace @@ +109,5 @@ > + types = [] > + for e in conf.simple_events: > + idl = loadEventIDL(p, options.incdirs, e) > + types.extend(interfaceAttributeTypes(idl)) > + nit: whitespace ::: accessible/src/xpcom/Makefile.in @@ +84,5 @@ > + -I $(DEPTH)/dist/idl \ > + --header-output xpcAccEvents.h \ > + --stub-output xpcAccEvents.cpp \ > + --makedepend-output $(MDDEPDIR)/xpcAccEvents.pp \ > + $(srcdir)/AccEvents.conf nit: seems indents are bogus
Trev, please make sure DOMI is updated too
(In reply to alexander :surkov from comment #16) > Comment on attachment 719659 [details] [diff] [review] > patch 2 > > Review of attachment 719659 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: accessible/public/nsIAccessibleTableChangeEvent.idl > @@ +15,5 @@ > > + > > + /** > > + * Return the number of rows or cols > > + */ > > + readonly attribute long numRowsOrCols; > > it'd be nice to get it renamed to rowOrColsCount I'll file a bug. > ::: accessible/src/base/AccEvent.cpp > @@ +181,5 @@ > > + Accessible* acc = aEvent->GetAccessible(); > > + nsINode* node = acc->GetNode(); > > + nsIDOMNode* domNode = node ? node->AsDOMNode() : nullptr; > > + bool fromUser = aEvent->IsFromUserInput(); > > + uint32_t type = aEvent->GetEventType(); > > actually you don't really need to have locals for these items? I suppose, but its nicer, and the node silliiness is non trivial. > @@ +189,5 @@ > > + if (eventGroup & AccEvent::eStateChangeEvent) { > > + AccStateChangeEvent* sc = downcast_accEvent(aEvent); > > + bool extra = sc->GetState() > UINT32_MAX; > > + bool enabled = sc->IsStateEnabled(); > > + uint32_t state = extra ? sc->GetState() >> 31 : sc->GetState(); > > >> why do you reject to use nsAccUtils::To32States helper function? > > >I thought there was a reason it didn't work here or something, but maybe there is no > >good reason. > > then pls use it, I'd like to keep code shared actually I think my reason may have been that I want to assign to the same variable wether or not its an extra state, and I'm to lazy to check the spec to be absolutely sure passing the same pointer twice is legit. > @@ +198,5 @@ > > + > > + if (eventGroup & AccEvent::eTextChangeEvent) { > > + AccTextChangeEvent* tc = downcast_accEvent(aEvent); > > + nsString text; > > + tc->GetModifiedText(text); > > still unclear issue with me: nsString vs nsAutoString "buffers are hard :p" really though maybe we should just have that return nsString& and then pass that directly to the constructor? > @@ +203,5 @@ > > + bool insert = tc->IsTextInserted(); > > + uint32_t start = tc->GetStartOffset(); > > + uint32_t len = tc->GetLength(); > > + xpEvent = new nsAccTextChangeEvent(type, acc, doc, domNode, fromUser, > > + start, len, insert, text); > > it seems you don't need locals for these I suppose > @@ +209,5 @@ > > + } > > + > > + if (eventGroup & AccEvent::eHideEvent) { > > + AccHideEvent* hideEvent = downcast_accEvent(aEvent); > > + if (hideEvent) { > > how it can be null? I thing vesage from using event type. > ::: accessible/src/base/AccEvent.h > @@ +471,5 @@ > > +/** > > + * Return a new xpcom accessible event for the given internal one. > > + */ > > +already_AddRefed<nsIAccessibleEvent> > > +MakeXPCEvent(AccEvent* aEvent); > > it's nice to keep it in xpcom folder (I see xpcAccEvent.h(cpp) are in use) since those files are already taken a autogenerated what should it be called? Also it seems silly to have a set of files just for one function. Maybe just jam it all in Accessible::HandleAccEvent()? On the other hand it was in base/ before so leaving it there doesn't seem like getting worse. > ::: accessible/src/jsat/Utils.jsm > @@ +218,5 @@ > > eventToString: function eventToString(aEvent) { > > let str = Utils.AccRetrieval.getStringEventType(aEvent.eventType); > > if (aEvent.eventType == Ci.nsIAccessibleEvent.EVENT_STATE_CHANGE) { > > let event = aEvent.QueryInterface(Ci.nsIAccessibleStateChangeEvent); > > + let stateStrings = (event.isExtraState) ? > > nit: you don't need braces yeah, ok, generally I try to minimize changes I make to random js
(In reply to Trevor Saunders (:tbsaunde) from comment #18) > > > + readonly attribute long numRowsOrCols; > > > > it'd be nice to get it renamed to rowOrColsCount > > I'll file a bug. what's for? this interface is not in use. All you need is to change that name and interface uuid. bug filing might take longer. > > ::: accessible/src/base/AccEvent.cpp > > @@ +181,5 @@ > > > + Accessible* acc = aEvent->GetAccessible(); > > > + nsINode* node = acc->GetNode(); > > > + nsIDOMNode* domNode = node ? node->AsDOMNode() : nullptr; btw, it makes sense to change Accessible::DOMNode() to a raw pointer (gfb if you like) > > > + bool fromUser = aEvent->IsFromUserInput(); > > > + uint32_t type = aEvent->GetEventType(); > > > > actually you don't really need to have locals for these items? > > I suppose, but its nicer, and the node silliiness is non trivial. you can keep node stuff in locals, fromUser and type don't need locals. > > @@ +189,5 @@ > > > + if (eventGroup & AccEvent::eStateChangeEvent) { > > > + AccStateChangeEvent* sc = downcast_accEvent(aEvent); > > > + bool extra = sc->GetState() > UINT32_MAX; > > > + bool enabled = sc->IsStateEnabled(); > > > + uint32_t state = extra ? sc->GetState() >> 31 : sc->GetState(); > > > > >> why do you reject to use nsAccUtils::To32States helper function? > > > > >I thought there was a reason it didn't work here or something, but maybe there is no > > >good reason. > > > > then pls use it, I'd like to keep code shared > > actually I think my reason may have been that I want to assign to the same > variable wether or not its an extra state, and I'm to lazy to check the spec > to be absolutely sure passing the same pointer twice is legit. alternatively you can add a version uint32_t To32State(uint64_t, bool*) if we can't keep code shared then I prefer to keep the logic in one place > > > + tc->GetModifiedText(text); > > > > still unclear issue with me: nsString vs nsAutoString > > "buffers are hard :p" really though maybe we should just have that return > nsString& and then pass that directly to the constructor? I think const DependentString as return value should be nice > > > +MakeXPCEvent(AccEvent* aEvent); > > > > it's nice to keep it in xpcom folder (I see xpcAccEvent.h(cpp) are in use) > > since those files are already taken a autogenerated what should it be > called? Also it seems silly to have a set of files just for one function. > Maybe just jam it all in Accessible::HandleAccEvent()? On the other hand it > was in base/ before so leaving it there doesn't seem like getting worse. ok, we can keep it for now where you placed it since nothing nicer comes to my mind One day we will get xpcAccessible where this function can be hosted.
(In reply to alexander :surkov from comment #19) > (In reply to Trevor Saunders (:tbsaunde) from comment #18) > > > > > + readonly attribute long numRowsOrCols; > > > > > > it'd be nice to get it renamed to rowOrColsCount > > > > I'll file a bug. > > what's for? this interface is not in use. All you need is to change that > name and interface uuid. bug filing might take longer. > > > > ::: accessible/src/base/AccEvent.cpp > > > @@ +181,5 @@ > > > > + Accessible* acc = aEvent->GetAccessible(); > > > > + nsINode* node = acc->GetNode(); > > > > + nsIDOMNode* domNode = node ? node->AsDOMNode() : nullptr; > > btw, it makes sense to change Accessible::DOMNode() to a raw pointer (gfb if > you like) I suppose doesn't seem very important given its mostly if not all used by xul goo that wants to QI and stuff as well. > > > > + bool fromUser = aEvent->IsFromUserInput(); > > > > + uint32_t type = aEvent->GetEventType(); > > > > > > actually you don't really need to have locals for these items? > > > > I suppose, but its nicer, and the node silliiness is non trivial. > > you can keep node stuff in locals, fromUser and type don't need locals. what would be the point other than making things longer? > > > @@ +189,5 @@ > > > > + if (eventGroup & AccEvent::eStateChangeEvent) { > > > > + AccStateChangeEvent* sc = downcast_accEvent(aEvent); > > > > + bool extra = sc->GetState() > UINT32_MAX; > > > > + bool enabled = sc->IsStateEnabled(); > > > > + uint32_t state = extra ? sc->GetState() >> 31 : sc->GetState(); > > > > > > >> why do you reject to use nsAccUtils::To32States helper function? > > > > > > >I thought there was a reason it didn't work here or something, but maybe there is no > > > >good reason. > > > > > > then pls use it, I'd like to keep code shared > > > > actually I think my reason may have been that I want to assign to the same > > variable wether or not its an extra state, and I'm to lazy to check the spec > > to be absolutely sure passing the same pointer twice is legit. > > alternatively you can add a version > uint32_t To32State(uint64_t, bool*) > > if we can't keep code shared then I prefer to keep the logic in one place its simple and would be trivial but for the 31 instead of 32 weirdness, but ok even if its silly to have a function for one use. > > > > + tc->GetModifiedText(text); > > > > > > still unclear issue with me: nsString vs nsAutoString > > > > "buffers are hard :p" really though maybe we should just have that return > > nsString& and then pass that directly to the constructor? > > I think const DependentString as return value should be nice I'll file a bug
(In reply to Trevor Saunders (:tbsaunde) from comment #20) > > > I'll file a bug. > > > > what's for? this interface is not in use. All you need is to change that > > name and interface uuid. bug filing might take longer. > > btw, it makes sense to change Accessible::DOMNode() to a raw pointer (gfb if > > you like) > > I suppose doesn't seem very important given its mostly if not all used by > xul goo that wants to QI and stuff as well. just keep code nicer and it should be good bug for newcomers. > > you can keep node stuff in locals, fromUser and type don't need locals. > > what would be the point other than making things longer? lesser locals is better since we have a chance they won't be placed on stack when function is running
Comment on attachment 719659 [details] [diff] [review] patch 2 r=me with comments fixed
Attachment #719659 - Flags: review?(surkov.alexander) → review+
(In reply to alexander :surkov from comment #21) > (In reply to Trevor Saunders (:tbsaunde) from comment #20) > > > > > I'll file a bug. > > > > > > what's for? this interface is not in use. All you need is to change that > > > name and interface uuid. bug filing might take longer. > > > > btw, it makes sense to change Accessible::DOMNode() to a raw pointer (gfb if > > > you like) > > > > I suppose doesn't seem very important given its mostly if not all used by > > xul goo that wants to QI and stuff as well. > > just keep code nicer and it should be good bug for newcomers. > > > > you can keep node stuff in locals, fromUser and type don't need locals. > > > > what would be the point other than making things longer? > > lesser locals is better since we have a chance they won't be placed on stack > when function is running given that only one of the if branches is ever taken I can't see why a compiler would decide it needs to spill, and since the functions to get the variables are inline it could easily decide to not spill and just reorder functions / call the function again, so if a compiler decided fetching values and then spilling them was the best thing to do I'd trust it atleast some. and all that ignores that stack stuff is fast anyway because of stack caches and shadow register magic. Besides this code just isn't that important to wring for speed.
ok, up to you. I consider that as general rule that'd be nice to follow.
Comment on attachment 719659 [details] [diff] [review] patch 2 Review of attachment 719659 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine. Hopefully we'll be able to clean up some of the Makefile stuff in the future, but it's no worse than what we already have. Thanks for factoring out that makefile dependency generation code. ::: accessible/src/xpcom/AccEventGen.py @@ +231,5 @@ > + makeutils.writeMakeDependOutput(options.makedepend_output) > + if options.header_output is not None: > + outfd = open(options.header_output, 'w') > + print_header_file(outfd, conf) > + outfd.close() Can you factor out all this into a "def main()" so that this block just looks like: if __name__ == '__main__': main()
Attachment #719659 - Flags: review?(ted) → review+
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #25) > Comment on attachment 719659 [details] [diff] [review] > patch 2 > > Review of attachment 719659 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks fine. Hopefully we'll be able to clean up some of the Makefile > stuff in the future, but it's no worse than what we already have. Thanks for > factoring out that makefile dependency generation code. > > ::: accessible/src/xpcom/AccEventGen.py > @@ +231,5 @@ > > + makeutils.writeMakeDependOutput(options.makedepend_output) > > + if options.header_output is not None: > > + outfd = open(options.header_output, 'w') > > + print_header_file(outfd, conf) > > + outfd.close() > > Can you factor out all this into a "def main()" so that this block just > looks like: > if __name__ == '__main__': > main() done, but I need to then explicitly make p (the xpidl parser) and options globals because I don't want to refactor all this python :/ (this started out life as a copy of the dom event codegen)
Attached patch fix DOMi — — Splinter Review
Attachment #777127 - Flags: review?(neil)
Comment on attachment 777127 [details] [diff] [review] fix DOMi Don't bother with the ()s around the typeof condition. I found 25 probably unnecessary uses, and 235 cases where they weren't used at all, for example from toolkit/devtools/server/protocol.js -- return typeof(obj) === "function" ? obj.prototype : obj;
Attachment #777127 - Flags: review?(neil) → review+
Blocks: DOMi2.0.16
Blocks: DOMi2.0.15
Assignee: tbsaunde+mozbugs → nobody
Severity: normal → S3

I think this is all done. It just never got closed and leave-open was set earlier.

Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Assignee: nobody → tbsaunde+mozbugs
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: