Last Comment Bug 75066 - bidi diffs for content and docshell
: bidi diffs for content and docshell
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla0.9
Assigned To: Erik van der Poel
: Frank Tang
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2001-04-06 22:06 PDT by Erik van der Poel
Modified: 2011-01-20 05:01 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
diffs for files under content and docshell (23.96 KB, patch)
2001-04-06 22:07 PDT, Erik van der Poel
no flags Details | Diff | Review
new diffs for the subset of files that jst had comments on (9.00 KB, patch)
2001-04-11 12:55 PDT, Erik van der Poel
no flags Details | Diff | Review
Patch to make the parallel name changes in nsPresContext.cpp as discussed above (12.38 KB, patch)
2001-04-19 08:08 PDT, Simon Montagu :smontagu
no flags Details | Diff | Review

Description Erik van der Poel 2001-04-06 22:06:22 PDT
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 2001-04-06 22:07:09 PDT
Created attachment 30012 [details] [diff] [review]
diffs for files under content and docshell
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2001-04-10 19:43:49 PDT
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 2001-04-11 10:21:21 PDT
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 2001-04-11 10:46:43 PDT
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 2001-04-11 11:42:43 PDT
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, jst@mozilla.com) 2001-04-11 12:08:57 PDT
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 2001-04-11 12:55:29 PDT
Created attachment 30448 [details] [diff] [review]
new diffs for the subset of files that jst had comments on
Comment 8 Erik van der Poel 2001-04-11 12:57:23 PDT
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 2001-04-11 12:57:50 PDT
mark it as moz0.9
Comment 10 Frank Tang 2001-04-12 11:51:43 PDT
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, jst@mozilla.com) 2001-04-12 13:42:59 PDT
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 2001-04-12 16:35:28 PDT
sr=erik
Comment 13 Erik van der Poel 2001-04-12 16:45:58 PDT
Thanks for the reviews, Johnny. Removed the unnecessary .get().

Fixes checked in.
Comment 14 Simon Montagu :smontagu 2001-04-17 03:01:42 PDT
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, jst@mozilla.com) 2001-04-17 03:24:11 PDT
Yes, that would be a good thing to do, anyone up for doing that?
Comment 16 Simon Montagu :smontagu 2001-04-17 06:27:31 PDT
I guess that would be me :-)
Comment 17 Simon Montagu :smontagu 2001-04-17 07:44:18 PDT
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 2001-04-18 11:00:15 PDT
code landing bug. change qa to ftang
v=ftang
Comment 19 Simon Montagu :smontagu 2001-04-19 08:08:54 PDT
Created attachment 31442 [details] [diff] [review]
Patch to make the parallel name changes in nsPresContext.cpp as discussed above

Note You need to log in before you can comment on or make changes to this bug.