The default bug view has changed. See this FAQ.

bidi diffs for content and docshell

VERIFIED FIXED in mozilla0.9

Status

()

Core
DOM
VERIFIED FIXED
16 years ago
6 years ago

People

(Reporter: Erik van der Poel, Assigned: Erik van der Poel)

Tracking

Trunk
mozilla0.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

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

Comment 1

16 years ago
Created attachment 30012 [details] [diff] [review]
diffs for files under content and docshell
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();
+    }
+  }
(Assignee)

Comment 3

16 years ago
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?
(Assignee)

Comment 4

16 years ago
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

16 years ago
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

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

Comment 7

16 years ago
Created attachment 30448 [details] [diff] [review]
new diffs for the subset of files that jst had comments on
(Assignee)

Comment 8

16 years ago
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

16 years ago
mark it as moz0.9
Target Milestone: --- → mozilla0.9

Comment 10

16 years ago
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.
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.
(Assignee)

Comment 12

16 years ago
sr=erik
(Assignee)

Comment 13

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

Fixes checked in.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
For consistency, should we not make the same name changes from BidiEnabled to 
GetBidiEnabled and from EnableBidi to SetBidiEnabled in nsPresContext.cpp?
Yes, that would be a good thing to do, anyone up for doing that?
I guess that would be me :-)
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

16 years ago
code landing bug. change qa to ftang
v=ftang
Status: RESOLVED → VERIFIED
QA Contact: lchiang → ftang
Created attachment 31442 [details] [diff] [review]
Patch to make the parallel name changes in nsPresContext.cpp as discussed above

Updated

6 years ago
Component: DOM: Abstract Schemas → DOM
You need to log in before you can comment on or make changes to this bug.