117751 - Change Set/Get(Bidi)Property api

Status: RESOLVED FIXED

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

Description

[Frank Tang]

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;

Comment 1

[Frank Tang]

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

Comment 2

[Frank Tang]

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

Comment 3

[Frank Tang]

move to future. 

Comment 4

[Simon Montagu :smontagu]

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);

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 6

[Simon Montagu :smontagu]

(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.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 9

[Simon Montagu :smontagu]

(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.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

right, that's what I suspected.

Comment 11

[Simon Montagu :smontagu]

(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));

Comment 12

[Simon Montagu :smontagu]

Attachment: Patch

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 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 14

[Simon Montagu :smontagu]

Fix checked in with the nits picked.