nsAString is dead, long live nsAString. The _internal name has long been an eyesore when debugging, and, as I've discovered while writing this patch, people are actually using the internal names in code, which is pretty gross. The downside of this change is that crash signatures will shift, unfortunately.

Comment on attachment 8845705 [details] Bug 1346078: Remove nsAString_internal and just use the nsAString name directly. https://reviewboard.mozilla.org/r/118850/#review120782 ::: js/src/devtools/rootAnalysis/annotations.js:199 (Diff revision 1) > "void JSAutoCompartment::~JSAutoCompartment(int32)" : true, > > // The nsScriptNameSpaceManager functions can't actually GC. They > // just use a PLDHashTable which has function pointers, which makes the > // analysis think maybe they can. > - "nsGlobalNameStruct* nsScriptNameSpaceManager::LookupNavigatorName(nsAString_internal*)": true, > + "nsGlobalNameStruct* nsScriptNameSpaceManager::LookupName(nsAString*, uint16**)": true, The removal here is intentional; LookupNavigatorName disappeared sometime between 45 and now.

Xidorn: How do these bindings files work? Do I need to update them by hand? Is it done for me automatically? https://dxr.mozilla.org/mozilla-central/search?q=String_internal+path%3Aservo&redirect=false This will probably be more than just a search-and-replace, because I suspect these lines are no longer needed, for example: .hide_type("nsACString_internal") .hide_type("nsAString_internal") .raw_line("type nsACString_internal = nsACString;") .raw_line("type nsAString_internal = nsAString;")

I guess you don't need to touch anything inside servo/ directory for this change, given the existence of those four lines. Please do a try push with linux64-stylo build included, and if they build fine, then everything is fine. We can remove those lines and do other necessary changes in a followup.

Oh, maybe > "nsAString_internal_char_traits", > "nsAString_internal_incompatible_char_type", > "nsACString_internal_char_traits", > "nsACString_internal_incompatible_char_type", these lines could be problematic...

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #5) > Oh, maybe > > "nsAString_internal_char_traits", > > "nsAString_internal_incompatible_char_type", > > "nsACString_internal_char_traits", > > "nsACString_internal_incompatible_char_type", > these lines could be problematic... Those should probably become nsAString_incompatible_char_type etc. I think those bindings are meant to mirror the typdefs at: https://dxr.mozilla.org/mozilla-central/source/xpcom/string/nsTSubstring.h#72

By "problematic", I mean we cannot make it build successfully without touching files in servo directory. But you cannot checkin servo files via Gecko's repo, so we would need to have someone work on the servo side change, and coordinate landing that change with your change here. bholley, could you help on this?

I don't have cycles tonight, and have a pretty big pile for tomorrow. Michael, could you help out David?

Oh, I think we can land a patch to just add "nsAString_char_traits" things to Servo side, so we don't need any coordinated landing, the tree would not be busted.

Nm, don't need Michael. David, just generate a PR to servo with the change from comment 9 and ping somebody in #servo to stamp it.

Comment on attachment 8845705 [details] Bug 1346078: Remove nsAString_internal and just use the nsAString name directly. https://reviewboard.mozilla.org/r/118850/#review121144 Please send a heads-up to all the lists including stability@ that this is changing and to expect signature adjustments.

I'm going to rework this patch to use nsSubstring/nsCSubstring in light of bug 1346100 comment 4.

Before you land it, please do a try run with this servo pr https://github.com/servo/servo/pull/15922 and the following try syntax > try: -b do -p all -u all[linux64,linux64-stylo] -t none to see if stylo build continues to work properly. To apply that, you can use > hg import --prefix servo/ https://github.com/servo/servo/pull/15922.patch If everything goes well, please ping a stylo developer (could be me, bholley, heycam, Manishearth, or emilio) to land the servo side pr at the same time when you are going to land this bug.

> I'm going to rework this patch to use nsSubstring/nsCSubstring in light of > bug 1346100 comment 4. Using "nsSubstring" turned into a much bigger mess than I'd like. It's not as simple as reversing the "typedef nsAString nsSubstring;". All friend-ships and forward declarations must refer to the canonical name. And the forward declarations are pretty common in the dom code: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3AnsAC%3FString%3B I began to rewrite the class names in those DOM headers, and then I felt obligated to do so in the corresponding .cpp files as well. And then I discovered that the notion of nsA[C]String is baked into a bunch of XPIDL stuff too. At this point I don't think I have the time or desire to finish this patch. dbaron: If you think you or someone else will want to take on this massive rename in the near future, I'm happy to hold off on landing so that we avoid a double-change in crash signatures etc. But if you think this is unlikely to happen, how do you feel about just going ahead with the removal of _internal?

Yeah, I'm OK with just the removal of _internal. (Although I'm curious why there are a lot of changes outside of xpcom/string/ that can't be done with just s/\<nsAString\>/nsSubstring/g and s/\<nsACString\>/nsCSubstring/g.)

Throwing a regex at the whole tree would probably work. But I really don't want to go there until we have better blame capabilities. Tracing source history is already super frustrating and I don't want to add another giant rename. I was trying to just change the minimal amount to get things building, but it gets ugly, especially with things like dom/SomeHeader.h: class nsAString; void DoSomething(nsAString&); dom/SomeCompletelyDifferentFile.cpp: void DoSomething(nsAString& foo) { ... }

I'm going to let bug 1344629 land and merge around before pushing this. (And that will give dev-platform time to make noise if needed)

Pushed by dmajor@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0e21ef2001d3 Remove nsAString_internal and just use the nsAString name directly. r=bsmedberg