The Set/GetBidiProperty api is troublesome. (see part of the problem in bug 116976) We need to change it. I suggest we change it in the following way: 1. Chaneg the signature of GetBidiProperty from 1141 NS_IMETHOD GetBidiProperty(nsIPresContext* aPresContext, 1142 nsIAtom* aPropertyName, 1143 void** aPropertyValue, 1144 size_t aSize ) const = 0; to 1141 NS_IMETHOD GetBidiProperty(nsIPresContext* aPresContext, 1142 nsIAtom* aPropertyName, 1143 void** aPropertyValue) const = 0; and change the caller from (example) res = frameBefore->GetBidiProperty(context, embeddingLevel, (void**)&levelBefore,sizeof(PRUint8)); to void* bidiproperty; res = frameBefore->GetBidiProperty(context, embeddingLevel, &bidiproperty); levelBefore = (PRUint8) bidiproperty;

The following files have GetBidiProperty .h file /layout/base/public/nsIFrame.h /layout/html/base/src/nsFrame.h .cpp files /editor/libeditor/text/nsTextEditRulesBidi.cpp /content/base/src/nsSelection.cpp /layout/base/src/nsBidiPresUtils.cpp /layout/base/src/nsCaret.cpp /layout/html/base/src/nsBlockFrame.cpp /layout/html/base/src/nsContainerFrame.cpp /layout/html/base/src/nsFrame.cpp /layout/html/base/src/nsTextFrame.cpp

mass move unimportant m9.8 bug to m9.9 for now.

move to future.

Taking. (In reply to comment #0) > 1. Chaneg the signature of GetBidiProperty from > 1141 NS_IMETHOD GetBidiProperty(nsIPresContext* aPresContext, > 1142 nsIAtom* aPropertyName, > 1143 void** aPropertyValue, > 1144 size_t aSize ) const = 0; > > to > 1141 NS_IMETHOD GetBidiProperty(nsIPresContext* aPresContext, > 1142 nsIAtom* aPropertyName, > 1143 void** aPropertyValue) const = 0; I'd like to go a step further and change it to: virtual void* GetBidiProperty(nsIPresContext* aPresContext, nsIAtom* aPropertyName) const = 0; > and change the caller from (example) > res = frameBefore->GetBidiProperty(context, embeddingLevel, > (void**)&levelBefore,sizeof(PRUint8)); > > > to > void* bidiproperty; > res = frameBefore->GetBidiProperty(context, embeddingLevel, &bidiproperty); > levelBefore = (PRUint8) bidiproperty; roc, If I change the method as described above, which of the following forms do you think would be better for the caller? void* bidiproperty = frameBefore->GetBidiProperty(context, embeddingLevel); levelBefore = (PRUint8)bidiproperty; or simply levelBefore = (PRUint8)frameBefore->GetBidiProperty(context, embeddingLevel);

The latter. By the way, you should remove the prescontext parameter. The prescontext can be obtained efficiently by doing GetPresContext on the frame. Why does Get/SetBidiProperty exist? Can't we just use Get/SetProperty? Possibly with deCOMtamination/cleanup of those functions?

(In reply to comment #5) > Why does Get/SetBidiProperty exist? Can't we just use Get/SetProperty? Possibly > with deCOMtamination/cleanup of those functions? My main reason for not wanting to go all the way down that route is so as not to complicate optimization of Get/SetBidiProperty for frames with no Bidi content, as in bug 230609.

OK, then I'd like to see something like this: void* GetBidiProperty(nsIAtom* aPropertyName) const { return (mState & NS_FRAME_IS_BIDI) ? GetProperty(aPropertyName) : nsnull; } void* RemoveBidiProperty(nsIAtom* aPropertyName) { return (mState & NS_FRAME_IS_BIDI) ? RemoveProperty(aPropertyName) : nsnull; } nsresult SetBidiProperty(nsIAtom* aPropertyName, void* aValue) { NS_ASSERTION(mState & NS_FRAME_IS_BIDI, "can't set bidi property on non-bidi frame"); return SetProperty(aPropertyName, aValue); } void* GetProperty(nsIAtom* aPropertyName, nsresult* aStatus = nsnull) const; void* RemoveProperty(nsIAtom* aPropertyName, nsresult* aStatus = nsnull); nsresult SetProperty(nsIAtom* aPropertyName, void* aValue, DestructorFunc aDestructor = nsnull); As a first step you might want to just implement the first set without changing the normal property signatures.

ALthough I think that in some ways it might be better to fix bug 230609 by having the callers do the IS_BIDI check.

(In reply to comment #8) > ALthough I think that in some ways it might be better to fix bug 230609 by > having the callers do the IS_BIDI check. Actually, I'm leaning that way myself after looking through all the callers. Most of them are in any case inside a large block with an IS_BIDI or IsBidiEnabled) check already, so doing the check internally will be a waste of time more often than not.

right, that's what I suspected.

(In reply to comment #4) > levelBefore = (PRUint8)frameBefore->GetBidiProperty(context, embeddingLevel); or rather, to avoid a slew of warnings: levelBefore = NS_PTR_TO_INT32(frameBefore->GetBidiProperty(embeddingLevel));

The hard part of a patch like this is knowing where to stop. Here's an overview of what it contains: Set/GetBidiProperty is merged into deCOMtaminated Set/GetProperty, which look more or less like in comment 7: void* GetProperty(nsIAtom* aPropertyName, nsresult* aStatus = nsnull) const; virtual void* GetPropertyExternal(nsIAtom* aPropertyName, nsresult* aStatus) const; void* RemoveProperty(nsIAtom* aPropertyName, nsresult* aStatus = nsnull) const; nsresult SetProperty(nsIAtom* aPropertyName, void* aValue, NSFramePropertyDtorFunc aDestructor = nsnull); I've added a couple of macros for the most commonly used Bidi properties: #define NS_GET_BASE_LEVEL(frame) \ NS_PTR_TO_INT32(frame->GetProperty(nsLayoutAtoms::baseLevel)) #define NS_GET_EMBEDDING_LEVEL(frame) \ NS_PTR_TO_INT32(frame->GetProperty(nsLayoutAtoms::embeddingLevel)) I've added a check on NS_FRAME_IS_BIDI to some callsites accessing the nextBidi property, but not for those accessing baseLevel and embeddingLevel. They would need to check IsBidiEnabled() on the presContext, and I think that would be more expensive than just getting the property. I've removed the presContext parameter up the callchain in a few places. More of that could probably be done in due course. I also removed some #defines and other stuff that wasn't being used.

Comment on attachment 149690 [details] [diff] [review] Patch A lot of minor nits, but basically fine. + ((nsIFrame*)(prevInFlow->GetProperty(nsLayoutAtoms::nextBidi)) != NS_STATIC_CAST + ((nsIFrame*)(prevInFlow->GetProperty(nsLayoutAtoms::nextBidi)) == ditto I think you should leave in the #ifdef IBMBIDIs. I think people decided it was worth having them there just to delineate the bidi code. + return GetPresContext()->FrameManager()->SetFrameProperty(this, + aPropName, + aPropValue, + aPropDtorFunc); personally I hate this style :-) how about + return GetPresContext()->FrameManager() + ->SetFrameProperty(this, aPropName, aPropValue, aPropDtorFunc); + void* nextBidiFrame; This can be moved inside the loop and #ifdef IBMBIDI + PRBool isOddLevel = (NS_GET_EMBEDDING_LEVEL(this) & 1); unnecessary parens + (frame = (nsIFrame*)GetProperty(nsLayoutAtoms::nextBidi))) { NS_STATIC_CAST + PRBool isOddLevel = (NS_GET_EMBEDDING_LEVEL(this) & 1); unnecessary parens + nextInFlow = (nsIFrame*)GetProperty(nsLayoutAtoms::nextBidi); NS_STATIC_CAST + nextBidi = (nsTextFrame*)GetProperty(nsLayoutAtoms::nextBidi); ditto + currentLevel = NS_GET_EMBEDDING_LEVEL(aCurrentFrame); Declare currentLevel here + bidiLevel = NS_GET_EMBEDDING_LEVEL(aSelectFrame); Declare bidiLevel here + level = NS_GET_EMBEDDING_LEVEL(focusFrame); declare level here + levelBefore = declare levelBefore here

Fix checked in with the nits picked.