Closed Bug 170852 Opened 22 years ago Closed 20 years ago

Remove #ifdef MOZ_UNICODE and MOZ_AIMM

Categories

(Core :: Internationalization, defect)

All
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tetsuroy, Assigned: smontagu)

Details

(Keywords: intl)

Attachments

(3 files, 2 obsolete files)

Now the flag (MOZ_UNICODE) is _ON_ by default, we need 
to remove them for improved readability and maintainability of the code.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.2beta
code issue, QA to yokoyama@netscape.com for now.
Keywords: intl
QA Contact: ruixu → yokoyama
MOZ_AIMM should be removed as well.
updating summary.  MOZ_UNICODE is no longer exclusive to widget module.
webshell/test app also contains the ifdef.
Summary: Remove #ifdef MOZ_UNICODE from /widget → Remove #ifdef MOZ_UNICODE and MOZ_AIMM
bulk milestone change
Target Milestone: mozilla1.2beta → mozilla1.3alpha
Attached patch cleanup (obsolete) — Splinter Review
shanjian: can you review?
Comment on attachment 105996 [details] [diff] [review]
cleanup

r=shanjian
Attachment #105996 - Flags: review+
   virtual LPCWSTR WindowClassW();
-#endif /* MOZ_UNICODE */
   virtual LPCTSTR WindowClass();

is the non-W version still needed?
Comment on attachment 105996 [details] [diff] [review]
cleanup

I just noticed that |WindowClass()| and |WindowPopupClass()| are implemented
like this:


LPCTSTR nsWindow::WindowClass()
{
  return (NS_ConvertUCS2toUTF8(WindowClassW()).get());
}

LPCTSTR nsWindow::WindowPopupClass()
{
  return (NS_ConvertUCS2toUTF8(WindowPopupClassW()).get());
}


I think this is a problem because it returns a pointer to the buffer in the
temporary NS_ConvertUCS2toUTF8 object, but the object will have it's destructor
called after returning from the function, so the caller will get a pointer to
memory that was already free'd ... right? This needs to be addressed.
that could be replaced with
return ToNewUTF8String(WindowClassW);
I think
I don't think calling the ToNewUTF8String(WindowClassW); is a good idea.
It will create a buffer in the heap and we need to explicitly free
the memory.  

We need to create the buffer in stack, just like the original 
nsWindow::WindowClass()
{
  const LPCTSTR className = "MozillaWindowClass";
  ...
  return className;
}

I think the following is better.
nsWindow::WindowClass()
{
  NS_ConvertUCS2toUTF8 className(WindowPopupClassW()).get());
  return (className.get());
}
This way className gets freed when function gets out of scope 
( ie. at '}' line )

kin: comment?
Oops bad syntax.  It should be
NS_ConvertUCS2toUTF8 className(WindowPopupClassW());
but your suggestion will not work. the string buffer is freed when the function
returns and its contents are undefined.
ah yeah, and to fix that, you either need a global variable, or you must
allocate from heap.
yokoyama, your proposed solution above, that uses a local className is identical
to what we have now so it doesn't fix anything.

The only reason why something like this works:

  {
    char *classname = "foobar";
    ...
    return classname;
  }

is because the "foobar" is a static constant string that never gets freed.
So like cbiesinger asked, do we still need non-W versions? Who is still calling
the non-W versions?

If for some reason they are still needed, perhaps a workaround would be
something like this:


LPCTSTR nsWindow::WindowClass()
{
  // Call into the wide version to make sure things get
  // registered properly.

  WindowClassW();

  // XXX: The class name used here must be kept in sync with
  //      the classname used in WindowClassW();

  return "MozillaWindowClass";
}
>do we still need non-W versions? 
I kept the non-W version for just being safe.  I believe we can
remove the non-W version. 

Thanks for comments, Christian & kin.  I'll post a new patch soon.
Remove WindowClass() and WindowPopupClass().

Rename WindowClassW() and WindowPopupClassW() to WindowClass() and
WindowPopupClass().

kin: can you super review?
Attachment #105996 - Attachment is obsolete: true
shanjian: can you review the latest patch?
hold on.  It looks like flash plugins are calling nsWindow::WindowClass() and
we shouldn't remove it.

It look like we have to do what kin suggested to do here:
http://bugzilla.mozilla.org/show_bug.cgi?id=170852#c15

I'll post a new patch soon.
Comment on attachment 106436 [details] [diff] [review]
remove non-W functions and rename W functions

invalidating
Attachment #106436 - Attachment is obsolete: true
Comment on attachment 108069 [details] [diff] [review]
Similar to the first reviewed patch except nsWindow::WindowPopupClass() and WindowClass()

can you review?
Attachment #108069 - Flags: review?(shanjian)
Attachment #108069 - Flags: review?(shanjian) → review+
Comment on attachment 108069 [details] [diff] [review]
Similar to the first reviewed patch except nsWindow::WindowPopupClass() and WindowClass()

I saw a weired behavior 
while I was debugging the 
plugin stuff when WindowClass()/WindowPopupClass()
are omitted.

It's better to keep them
for now.

kin: can you super review?
Attachment #108069 - Flags: superreview?(kin)
Comment on attachment 108069 [details] [diff] [review]
Similar to the first reviewed patch except nsWindow::WindowPopupClass() and WindowClass()

If you're looking for a review by days end to meet the 1.3alpha deadline ... I
won't be able to get to this.
Comment on attachment 108069 [details] [diff] [review]
Similar to the first reviewed patch except nsWindow::WindowPopupClass() and WindowClass()

stealing from kin's request queue.
sr=roc+moz
Attachment #108069 - Flags: superreview?(kin) → superreview+
yokoyama, can you checkin the patch or update it if needed? 
I am not actively working on mozilla at the moment.
assign to ftang
Assignee: yokoyama → ftang
Status: ASSIGNED → NEW
Target Milestone: mozilla1.3alpha → mozilla1.3final
ftang, can you checkin the patch after the 1.4a freeze cycle is over?
This is the above patch updated to trunk.
It doesn't include any Makefile changes, because NSPR header files have some
#ifdef MOZ_UNICODE parts, and I were afraid that those parts might be needed by
#ifdef MOZ_UNICODE mozilla code.

I plan to check this in tomorrow, assuming the above review/super-review still
applies.
the "patch updated, w/o makefile changes" attachment has been checked in.
I backed out the nsMsgCompose changes that were part of the latest patch here
because they broke attaching files with non-ascii filenames on non-windows
platforms (or at least the display of their names in the compose window)
Attachment #131118 - Flags: review?(leaf)
Attachment #131118 - Flags: review?(leaf) → review?(bryner)
Attachment #131118 - Flags: review?(bryner) → review+
-> to default owner (rather than ftang's WONTFIX)
Assignee: ftang → smontagu
QA Contact: tetsuroy → amyy
Target Milestone: mozilla1.3final → ---
Checked in the last patch (sr=neil on IRC)
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: