75066 - bidi diffs for content and docshell

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Erik van der Poel]

This bug report will be used to attach diffs from the IBM bidi project for files
under content and docshell. Johnny, please review. Thanks.

Comment 1

[Erik van der Poel]

Attachment: diffs for files under content and docshell

Comment 2

[Johnny Stenback (:jst)]

nsIDocument::BidiEnabled() really needs to be renamed to GetBidiEnabled() and
the argument it takes really should be a pointer to a PRBool and not a reference
to follow XPCOM rules better, I know we violate the XPCOM rules alot in that
interface so the argument type doesn't really matter, but the name should be
changed. Why EnableBidi(void) and not SetBidiEnables(PRBool aEnable)? If you
only support enabling it for now then simply just implement that and assert, or
something, if someone tries to disable it, but don't limit what the users of
this API should be able to do in the future in the actual API.

Why is the structure:

+struct DirTable {
+  char*   name;
+  PRUint8 value;
+};

defined in the middle of nsDocument.cpp? Seems like it should either be above
the places where it's used, or in a header file. And it should use const char*,
and the names should be mName and mValue to match the other code around it.

I don't see nsBidiOptions defined anywhere, except in nsDocumentViewer.h marked
as a hack, what's the deal with that?

I'm assuming SET_BIDI_OPTION_CLIPBOARDTEXTMODE n' friends are macros somewhere,
but I don't see their definitions. Feels like this should be done with a little
inline helper method instead of a collection of macros. I'm looking at that code
and I have no idea what it does, so I don't know what to say about that.

Please point me at the nsTextFragment changes for this diff, I'm wondering what
this code does (in nsGenericDOMDataNode.cpp):

+  if (mDocument != nsnull) {
+    PRBool bidiEnabled = mText.SetTo(aBuffer, aLength);
+    if (bidiEnabled) {
+      mDocument->EnableBidi();
+    }
+  }

Comment 3

[Erik van der Poel]

Johnny, thanks for the review. You can see the bidi changes for nsTextFragment
here:

http://lxr.mozilla.org/seamonkey/source/content/shared/src/nsTextFragment.cpp#14
4

I'm working on addressing your other comments. Would you like to see diffs when
I'm done?

Comment 4

[Erik van der Poel]

Johnny, the SET_BIDI_OPTION_* macros are defined in nsIUBidiUtils.h:

http://lxr.mozilla.org/seamonkey/source/intl/unicharutil/public/nsIUBidiUtils.h#
265

nsBidiOptions used to be a struct, but we changed it to a PRUint32 so that we
could use it in a XPIDL file. The macros take care of bit shifting and masking.
I'd like to change nsBidiOptions to PRUint32 everywhere, and then continue to
use the macros, if that's OK with you.

If you insist on using inline methods, please let me know whether they should be
part of a class or global inlines that start with NS_*.

Comment 5

[Frank Tang]

jst, eriks's last diff include the following files. I understand there are still 
some outstanding issue. But can you tell us which of the following is OK so we 
don't need to take a look at them again?


content/base/public/nsIDocument.h
content/base/src/nsDocument.cpp
content/base/src/nsDocument.h
content/base/src/nsDocumentViewer.cpp
content/base/src/nsGenericDOMDataNode.cpp
content/html/content/src/nsGenericHTMLElement.cpp
content/html/document/src/nsHTMLDocument.cpp
content/html/document/src/nsHTMLDocument.h
content/xul/document/src/nsXULDocument.cpp
content/xul/document/src/nsXULDocument.h
docshell/base/nsDocShell.cpp
docshell/base/nsIMarkupDocumentViewer.idl

Comment 6

[Johnny Stenback (:jst)]

Now that I see the macro's I'm cool with leaving them as macros, I thought they
did much more than bit shifting...

Given that knowledge, I'm ok with the changes to the files:

content/base/src/nsDocumentViewer.cpp
content/base/src/nsGenericDOMDataNode.cpp
content/html/content/src/nsGenericHTMLElement.cpp
content/html/document/src/nsHTMLDocument.cpp
content/html/document/src/nsHTMLDocument.h
docshell/base/nsDocShell.cpp
docshell/base/nsIMarkupDocumentViewer.idl

I'd still like to see the nsIDocument changes I mentioned, and also, the
nsDocument.cpp weirdnes (Dirtable) cleared before checkin.

Comment 7

[Erik van der Poel]

Attachment: new diffs for the subset of files that jst had comments on

Comment 8

[Erik van der Poel]

Johnny, I've addressed all your comments, and re-submitted the diffs. Please
see the latest patch. May we have an r=<module-owner>?

Comment 9

[Frank Tang]

mark it as moz0.9

Comment 10

[Frank Tang]

jst- this is the last piece that we need to land before we turn on the bidi flag
on all platform. Could you please work w/ erik asap. Thanks.

Comment 11

[Johnny Stenback (:jst)]

Looks good now, r/a=jst, one nit remains:

...
+    nsCOMPtr<nsIPresContext> context;
+    shell->GetPresContext(getter_AddRefs(context) );
+    if (context.get() ) {

Why not simply "if (context) {"? Adding the .get() here only makes the code
harder to read.

Comment 12

[Erik van der Poel]

sr=erik

Comment 13

[Erik van der Poel]

Thanks for the reviews, Johnny. Removed the unnecessary .get().

Fixes checked in.

Comment 14

[Simon Montagu :smontagu]

For consistency, should we not make the same name changes from BidiEnabled to 
GetBidiEnabled and from EnableBidi to SetBidiEnabled in nsPresContext.cpp?

Comment 15

[Johnny Stenback (:jst)]

Yes, that would be a good thing to do, anyone up for doing that?

Comment 16

[Simon Montagu :smontagu]

I guess that would be me :-)

Comment 17

[Simon Montagu :smontagu]

Incidentally, I don't think we need to assert if SetBidiEnabled is called with 
PR_FALSE. It's not that we have any problems with the concept of disabling Bidi 
once it's been enabled, only that we can't see a way to determine when it 
should happen.

The only scenario that I can think of when this might be needed would be in the 
composer. Bidi is enabled as soon as an RTL character is entered, and in theory 
you could disable it once the last RTL character had been deleted, but how 
would you detect this?

Comment 18

[Frank Tang]

code landing bug. change qa to ftang
v=ftang

Comment 19

[Simon Montagu :smontagu]

Attachment: Patch to make the parallel name changes in nsPresContext.cpp as discussed above