Closed
Bug 1346078
Opened 8 years ago
Closed 8 years ago
Remove nsAString_internal and just use the nsAString name directly
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: away, Assigned: away)
References
Details
Attachments
(1 file)
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 hidden (mozreview-request) |
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;")
Assignee: nobody → dmajor
Flags: needinfo?(xidorn+moz)
Comment 4•8 years ago
|
||
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.
Flags: needinfo?(xidorn+moz)
Comment 5•8 years ago
|
||
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
Comment 7•8 years ago
|
||
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?
Flags: needinfo?(bobbyholley)
Comment 8•8 years ago
|
||
I don't have cycles tonight, and have a pretty big pile for tomorrow. Michael, could you help out David?
Flags: needinfo?(bobbyholley) → needinfo?(michael)
Comment 9•8 years ago
|
||
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.
Comment 10•8 years ago
|
||
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.
Flags: needinfo?(michael)
Comment 11•8 years ago
|
||
mozreview-review |
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.
Attachment #8845705 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 12•8 years ago
|
||
I'm going to rework this patch to use nsSubstring/nsCSubstring in light of bug 1346100 comment 4.
Comment 13•8 years ago
|
||
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.
Assignee | ||
Comment 14•8 years ago
|
||
> 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?
Flags: needinfo?(dbaron)
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.)
Flags: needinfo?(dbaron)
Assignee | ||
Comment 16•8 years ago
|
||
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) { ... }
Assignee | ||
Comment 17•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 19•8 years ago
|
||
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
Comment 20•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•4 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•