Timothy Nikkel (:tnikkel) Reporter
	Description • 15 years ago

TestWinTSF.cpp does not have MOZILLA_INTERNAL_API defined but it needs to access functions defined in headers that also include internal string types. It uses ugly header hacks to get around this that cause problems when changing unrelated code in perfectly reasonable ways (eg adding a header to nsPresContext.h in bug 495728). Instead we should just give it MOZILLA_INTERNAL_API and remove the hacks.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 15 years ago

Have you verified that this still builds, links, and runs in both libxul and non-libxul builds?

	Benjamin Smedberg
	Comment 2 • 15 years ago

Comment on attachment 382918 [details] [diff] [review]
patch

This will certainly break in release+tests builds. I don't think there's any good reason to make this use the internal API: we should try to fix the relevant headers if possible.

	Timothy Nikkel (:tnikkel) Reporter
	Comment 3 • 15 years ago

Fix the headers instead.

I tested this in a Windows VM debug build only.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 15 years ago

Comment on attachment 383151 [details] [diff] [review]
patch v2

>+++ b/content/base/public/nsContentUtils.h
>+typedef nsString nsAFlatString;

I'd really rather not be making that assumption here.  Why is it needed?

Also, is there a reason nsStringFwd.h doesn't do what you want here, instead of doing all these decls yourself?

Similar in the other content/layout headers you change.

>+++ b/layout/base/nsPresContext.h
>-  eWidgetType_Button      = 1,
>-  eWidgetType_Checkbox    = 2,
>-  eWidgetType_Radio            = 3,
>-  eWidgetType_Text            = 4
>+  eWidgetType_Button   = 1,
>+  eWidgetType_Checkbox = 2,
>+  eWidgetType_Radio    = 3,
>+  eWidgetType_Text     = 4

Please don't make unrelated whitespace changes?

>+++ b/layout/generic/nsIFrame.h
>-      INVALIDATE_IMMEDIATE = 0x1,
>-      INVALIDATE_CROSS_DOC = 0x2,
>-      INVALIDATE_NOTIFY_ONLY = 0x4
>+    INVALIDATE_IMMEDIATE = 0x1,
>+    INVALIDATE_CROSS_DOC = 0x2,
>+    INVALIDATE_NOTIFY_ONLY = 0x4

And here.

	Benjamin Smedberg
	Comment 5 • 15 years ago

Actually nsAFlatString is always nsString... he's just duplicating nsStringFwd.h into this file for a forward declaration.

	Timothy Nikkel (:tnikkel) Reporter
	Comment 6 • 15 years ago

(In reply to comment #4)
> Also, is there a reason nsStringFwd.h doesn't do what you want here, instead of
> doing all these decls yourself?

nsStringFwd.h generates a preprocessor error if MOZILLA_INTERNAL_API is not defined. So it can't be included in anything TestWinTSF.cpp needs.

> Please don't make unrelated whitespace changes?

Okay. For the future, is there a guideline for when and what types of unrelated minor cleanups to include or not? Do a separate patch or just let them go?

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 15 years ago

Benjamin just suggested moving that #error into nsAString.h instead.

As far as whitespace, I personally prefer separating cleanup and functionality patches completely.  Separate patch for cleanup is fine.

	Timothy Nikkel (:tnikkel) Reporter
	Comment 8 • 15 years ago

Moving the error doesn't allow me to include nsStringFwd.h because anything that includes the external nsStringAPI.h header will then conflict with anything that includes nsStringFwd.h because nsAutoString has different definitions -- 'typedef nsString nsAutoString;' in nsStringAPI.h, 'class nsAutoString;' in nsStringFwd.h.

What does work is including nsStringGlue.h, which just includes the internal or external headers based on MOZILLA_INTERNAL_API being defined or not. But this still forces me to do a few forward declarations for things not defined by the external headers.

Should I still move the error? I left it in. It might be useful to someone else even if I don't use it here.

Moved the minor unrelated cleanups to a separate patch.

Tested Windows debug only.

	Timothy Nikkel (:tnikkel) Reporter
	Comment 9 • 15 years ago

Get rid of some tabs and remove a duplicate makefile entry.

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 15 years ago

I'm still really unhappy with this, and especially with the typedef in nsContentUtils.h.  Too easy for that to get out of sync with the string headers...

Benjamin, can we just add more stuff to the "internal" branch in nsStringGlue.h?

	Benjamin Smedberg
	Comment 11 • 15 years ago

I'd be ok with globally replacing all nsAFlatString with nsString, if that seems better to you. Do you mean add `typedef nsString nsAFlatString` to the *external* branch of nsStringGlue.h? I'm pretty sure the internal branch already has that.

	Boris Zbarsky [:bzbarsky]
	Comment 12 • 15 years ago

Oh, right; this whole thing is about someone external including nsContentUtils (God, what a mess)....

I'd be fine with just nixing nsAFlatString, I think...  David, any objections?

	Benjamin Smedberg
	Comment 13 • 15 years ago

Comment on attachment 383566 [details] [diff] [review]
minor cleanups

The cleanup patch doesn't change APIs and doesn't require sr.

	Benjamin Smedberg
	Comment 14 • 15 years ago

Comment on attachment 383564 [details] [diff] [review]
patch v3

Clearing review pending decision about flatstring.

	David Baron :dbaron:
	Comment 15 • 15 years ago

(In reply to comment #11)
> I'd be ok with globally replacing all nsAFlatString with nsString, if that
> seems better to you. Do you mean add `typedef nsString nsAFlatString` to the
> *external* branch of nsStringGlue.h? I'm pretty sure the internal branch
> already has that.

I'm fine with either of those.

	Benjamin Smedberg
	Comment 16 • 15 years ago

I don't think it's necessary to remove nsAFlatString from every file in the tree (although that's not a bad idea either)... you could just change the instances in these headers you are including.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 14 years ago

Timothy, are you still waiting on my review here?  Or did we decide that we should get rid of nsAFlatString in general instead?

	Timothy Nikkel (:tnikkel) Reporter
	Comment 18 • 14 years ago

Comment on attachment 383564 [details] [diff] [review]
patch v3

No, I'm not waiting on your review. This bug has gone on to the great abyss of unattended bugs.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900)
	Comment 19 • 8 years ago

TestWinTSF.cpp was disabled at several years ago.

We should try to think new better test frame work for TSFTextStore. So, we don't need to fix this bug anymore.