Closed
Bug 203406
Opened 22 years ago
Closed 21 years ago
Performace enhancement in CTL code
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.4beta
People
(Reporter: prabhat.hegde, Assigned: prabhat.hegde)
References
(Blocks 1 open bug)
Details
(Keywords: intl)
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
prabhat.hegde
:
review+
rbs
:
superreview+
|
Details | Diff | Splinter Review |
Current CTL code was developed as hack to support Thai and is quite inefficient
in logic. It appears that there are more users of the code than i initially
suspected. Hence cleaning logic, reducing code-size and memory consumption.
Assignee | ||
Comment 1•22 years ago
|
||
This is a fairly large patch but can't see more efficient way of filing this.
Assignee | ||
Comment 2•22 years ago
|
||
The following problems should also be addressed with this patch:
1. http://bugzilla.mozilla.org/show_bug.cgi?id=122552
2. Fixes warnings caused because nsULE.cpp did not use THREAD_SAFE XPCOM macros.
Assignee | ||
Comment 3•22 years ago
|
||
hi Roland or Masaki, could one of you check this out? If you need the source
files, let me know. (PS: I have run the patch through a tool that checks for bad
format which is the cause of a number of diffs)
OS: Windows 2000 → Linux
Hardware: Sun → PC
Comment 4•22 years ago
|
||
A bit off-topic here... currently, enable-ctl is only effective
on Unix, right? As it's discussed in bug 203052, Mozilla-Win
doesn't handle complex scripts as well as MS IE and I guess
it's also needed there (unless we decide to use Uniscribe/OTLS
very soon as MSIE does) and perhaps on other platforms (again
unless platform-specific complex script handling such as
Apple's AAT fonts and ATSUI can be utilized by Mozilla easily).
See also bug 177877, bug 176290 and bug 176315.
Keywords: intl
Comment 5•22 years ago
|
||
Prabhat, can you merge my patch for bug 204286 (attachment 122542 [details] [diff] [review] + the change I
mentioned
in bug 204286 comment #9) with your patch here? I believe both can(should) go in
before 1.4(beta).
Roland and Katakai-san, can we move this on?
Target Milestone: --- → mozilla1.4beta
Assignee | ||
Comment 6•22 years ago
|
||
Fixes warning for comparision between signed and unsigned from patch
(id=122976). Masaki or Roland, please commit. I can't do so myself.
Attachment #121744 -
Attachment is obsolete: true
Assignee | ||
Comment 7•22 years ago
|
||
hi Jungshik - I possibly missed your email regarding your change to add isWide.
Why do you require ASCII range to return UCS-2BE? - thanks. After clarification
i will update my Jumbo patch to incorporate your fixes.
Comment 8•22 years ago
|
||
Hi Prabhat.
Here's my rationale for the change I suggested.
As it stands, what nsUnicodeToSunIndic::Convert() returns is a bit odd
mixture of US-ASCII and characters in U+09xx and in U+F800(?: PUA). Suppose
that it's given an input string made up of US-ASCII and Devanagari letters,
say,'A B U+0935 U+0951 C D'. It returns (assuming no shaping for the sake of
demonstration) '0x41 0x42 0x09 0x35 0x09 0x51 0x43 0x44'. In this form without
further processing, I can't pass it to the rendering engine (e.g
nsFontMetricsXft) which expects the input to be either in *pure* ASCII (8bit
string : as XDrawString in X11 does) or in *pure* 16bit UCS-2BE (as
XDrawString16 in X11 does). As I wrote in bug 204286, I can write a simple
wrapper to turn the output of nsUnicodeToSunIndic::Convert() to 16bit UCS-2BE,
but that seems to be an uncessary overture because a simple change I
suggested in my patch to bug 204286 would make nsUnicodeToSunIndic::Convert()
return UCS-2BE string that can be directly fed to drawing-routine.
BTW, as you know too well, nsUnicodeToTIS620::Convert() is 'SingleByte'
converter so that I think adding 'aIsWide' parameter with the default set to
false makes sense. In the future, I expect nsUnicodeToTamilTTF (bug 204039)
and nsUnicodeToJamoTTF(bug 176315) to move to intl/ctl to take advantage of
CTL's selection/cursor movement support in nsTextFrame. They're
also 'wide'(double-byte) converters. Therefore, adding aIsWide is also to
prepare for the future :-)
Hope my explanation is clear :-). If not, please let me know and I'll try
again. Thanks.
Assignee | ||
Comment 9•22 years ago
|
||
Since the patch is too huge, attaching the diff source files. Masaki and
Roland, Can you look at it and putback? (The performance changes have been in
Sun Mozilla tree for sometime now and should hopefully be stopper free).
Comment 10•22 years ago
|
||
*** Bug 205260 has been marked as a duplicate of this bug. ***
Comment 11•22 years ago
|
||
This has to be applied on top of attachment 123155 [details]. It gets rid of another
signed vs unsigned comparison and adds extra braces around 'sub-arrays' of '2-d
arrays' to make gcc happy.
Comment 12•21 years ago
|
||
To move things forward (and make it easier for reviewers), I'm putting up a
combined patch
Attachment #123137 -
Attachment is obsolete: true
Attachment #123155 -
Attachment is obsolete: true
Attachment #123577 -
Attachment is obsolete: true
Comment 13•21 years ago
|
||
Comment on attachment 127498 [details] [diff] [review]
a combined patch (attachment 123155 [details] + attachment 123577 [details] [diff] [review])
Asking for r/sr on behalf of prabhat. Hope he doesn't mind :-)
Attachment #127498 -
Flags: superreview?(blizzard)
Attachment #127498 -
Flags: review?(katakai)
Assignee | ||
Comment 14•21 years ago
|
||
Definitely not! Thanks for the patch and help in moving the bug along.
prabhat.
Comment 15•21 years ago
|
||
Katakai-san, can you review the patch? Prabhat wrote that it'd been a part of
Sun's internal build for the last half year or so...
Comment 16•21 years ago
|
||
Bug 178735 regards cursor movement beeing broken in all textareas (textarea,
composer, mail etc) if compiling with --enable-ctl. It has been that way since
1.4. Does this huge patch solve that one too?
Assignee | ||
Comment 17•21 years ago
|
||
Possibly Louie Zhao can also review this. The perf enhancement part has been in
Sun's Mozilla codebase since long while - Louie?
prabhat
Updated•21 years ago
|
Attachment #127498 -
Flags: superreview?(rbs)
Attachment #127498 -
Flags: superreview?(blizzard)
Attachment #127498 -
Flags: review?(prabhat.hegde)
Attachment #127498 -
Flags: review?(katakai)
Assignee | ||
Comment 18•21 years ago
|
||
Comment on attachment 127498 [details] [diff] [review]
a combined patch (attachment 123155 [details] + attachment 123577 [details] [diff] [review])
Looks good to me. Original patch has been working uneventfully in Sun's Mozilla
for the past year.
Attachment #127498 -
Flags: review?(prabhat.hegde) → review+
Comment 19•21 years ago
|
||
Comment on attachment 127498 [details] [diff] [review]
a combined patch (attachment 123155 [details] + attachment 123577 [details] [diff] [review])
rs=rbs based on the year long testing at Sun.
Attachment #127498 -
Flags: superreview?(rbs) → superreview+
Comment 20•21 years ago
|
||
At very long last, fix checked into the trunk
thanks all.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Component: Layout: CTL → Layout: Text
QA Contact: arthit → layout.fonts-and-text
You need to log in
before you can comment on or make changes to this bug.
Description
•