Asaf Romano (gone) Assignee
	Description • 20 years ago

stay tuned :)

	Asaf Romano (gone) Assignee
	Comment 1 • 20 years ago

It's amazing how broken is the old good TSM API.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 20 years ago

Comment on attachment 163955 [details] [diff] [review]
patch

It will take me probably at least a week to get to this review....  I'm a
little swamped with other reviews right now.

	jhp (no longer active)
	Comment 3 • 20 years ago

Comment on attachment 163955 [details] [diff] [review]
patch

Just some nits:

+typedef OSStatus (*fpKLGetKeyboardLayoutProperty_type) (KeyboardLayoutRef,
+    KeyboardLayoutPropertyTag, void**);

I would write it like this:
+typedef OSStatus (*fpKLGetKeyboardLayoutProperty_type)
+			 (KeyboardLayoutRef, KeyboardLayoutPropertyTag,
void**);

Try to keep the lines in the patch to 80 characters wide or less.

I would write the second code block in IsLangRTL with one 'return', like this:
+  nsresult rv = NS_OK;
+  if (fpKLGetCurrentKeyboardLayout) {
+    OSStatus err;
+    KeyboardLayoutRef currentKeyboard;
+    const void* currentKeyboardResID;
+  
+    err = fpKLGetCurrentKeyboardLayout(&currentKeyboard);
+
+    if (err == noErr) {
+      err = fpKLGetKeyboardLayoutProperty(currentKeyboard, kKLIdentifier,
+					   &currentKeyboardResID);
+      if (err == noErr) {
+	 *aIsRTL = IsRTLLangauge((SInt32)currentKeyboardResID);
+	 rv = NS_OK;
+      }
+    }
+  }
+
-   return NS_OK;
+  return rv;

Otherwise, the code looks good.

	Asaf Romano (gone) Assignee
	Comment 4 • 20 years ago

Comment on attachment 163955 [details] [diff] [review]
patch


bz?

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 20 years ago

I've still got 4 reviews in my queue that were requested before this one and
this code is really not something I know well, so I would need some time to
review this....

I'll try to get to this within the next 3-4 days, but it may not happen.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 20 years ago

Comment on attachment 163955 [details] [diff] [review]
patch

>Index: widget/src/mac/nsBidiKeyboard.cpp
>+  // Check if the resource id is BiDi associated (Arabic, Persion, Hebrew)
>+  // (Persion is included in the Arabic range)

"Persian", not "Persion".

sr=bzbarsky. Sorry this took so long... :(

	Asaf Romano (gone) Assignee
	Comment 7 • 20 years ago

thanks for reviews.

	Asaf Romano (gone) Assignee
	Comment 8 • 20 years ago

err, wrong diff.

	Simon Montagu :smontagu
	Comment 9 • 20 years ago

Patch checked in.

	Prognathous
	Comment 10 • 20 years ago

Verified with 1.8a6/20041215.

Thanks Asaf!

Prog.

	(no longer active)
	Comment 11 • 17 years ago

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).