170852 - Remove #ifdef MOZ_UNICODE and MOZ_AIMM

Status: RESOLVED FIXED

(Core :: Internationalization, defect)

Description

[Roy Yokoyama]

Now the flag (MOZ_UNICODE) is _ON_ by default, we need 
to remove them for improved readability and maintainability of the code.

Comment 1

[Rui Xu]

code issue, QA to [e-mail] for now.

Comment 2

[Roy Yokoyama]

MOZ_AIMM should be removed as well.

Comment 3

[Roy Yokoyama]

updating summary.  MOZ_UNICODE is no longer exclusive to widget module.
webshell/test app also contains the ifdef.

Comment 4

[Roy Yokoyama]

bulk milestone change

Comment 5

[Roy Yokoyama]

Attachment: cleanup

shanjian: can you review?

Comment 6

[Shanjian Li]

Comment on attachment 105996 [details] [diff] [review]
cleanup

r=shanjian

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

   virtual LPCWSTR WindowClassW();
-#endif /* MOZ_UNICODE */
   virtual LPCTSTR WindowClass();

is the non-W version still needed?

Comment 8

[kinmoz]

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.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

that could be replaced with
return ToNewUTF8String(WindowClassW);
I think

Comment 10

[Roy Yokoyama]

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?

Comment 11

[Roy Yokoyama]

Oops bad syntax.  It should be
NS_ConvertUCS2toUTF8 className(WindowPopupClassW());

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

but your suggestion will not work. the string buffer is freed when the function
returns and its contents are undefined.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

ah yeah, and to fix that, you either need a global variable, or you must
allocate from heap.

Comment 14

[kinmoz]

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.

Comment 15

[kinmoz]

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";
}

Comment 16

[Roy Yokoyama]

>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.

Comment 17

[Roy Yokoyama]

Attachment: remove non-W functions and rename W functions

Remove WindowClass() and WindowPopupClass().

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

kin: can you super review?

Comment 18

[Roy Yokoyama]

shanjian: can you review the latest patch?

Comment 19

[Roy Yokoyama]

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 20

[Roy Yokoyama]

Comment on attachment 106436 [details] [diff] [review]
remove non-W functions and rename W functions

invalidating

Comment 21

[Roy Yokoyama]

Attachment: Similar to the first reviewed patch except nsWindow::WindowPopupClass() and WindowClass()

Comment 22

[Roy Yokoyama]

Comment on attachment 108069 [details] [diff] [review]
Similar to the first reviewed patch except nsWindow::WindowPopupClass() and WindowClass()

can you review?

Comment 23

[Roy Yokoyama]

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?

Comment 24

[kinmoz]

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 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 26

[Stephen Walker]

yokoyama, can you checkin the patch or update it if needed? 

Comment 27

[Roy Yokoyama]

I am not actively working on mozilla at the moment.
assign to ftang

Comment 28

[Stephen Walker]

ftang, can you checkin the patch after the 1.4a freeze cycle is over?

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch updated, w/o makefile changes

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.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

the "patch updated, w/o makefile changes" attachment has been checked in.

Comment 31

[Christian :Biesinger (don't email me, ping me on IRC)]

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)

Comment 32

[Stephen Walker]

Attachment: Remove MOZ_AIMM from the 2 Makefiles that define it, since the ifdefs for it were removed

Comment 33

[Adam Hauner]

-> to default owner (rather than ftang's WONTFIX)

Comment 34

[Simon Montagu :smontagu]

Checked in the last patch (sr=neil on IRC)