1252211 - Convert remaining XUL DOM classes to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

Which classes is this referring to?

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Rename nsXULControllers to XULControllers v1

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: Convert XULControllers to WebIDL v1

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Rename nsXULCommandDispatcher to XULCommandDispatcher v1

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: Convert XULCommandDispatcher to WebIDL v1

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: Convert XUL template builders to WebIDL v1

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #1)
> Which classes is this referring to?

All of the XUL classes remaining (XULControllers, XULCommandDispatcher, XULTemplateBuilder, XULTreeBuilder, TreeContentView). I also have a a patch for TreeSelection, but it looks like there might be implementations in JS of that in Thunderbird :-(. Then there's the DOMCI for various XUL elements, which afaict we can just remove.
Want to review? :-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Want to review? :-)

Sure.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers v1

Comment 10

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4ec8a0e104c

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Move nsXULTreeBuilder declaration into its own header v1

Comment 12

[Peter Van der Beken [:peterv]]

Attachment: Convert XUL template builders to WebIDL v1

Comment 13

[Peter Van der Beken [:peterv]]

Attachment: Convert TreeContentView to WebIDL v1

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831284 [details] [diff] [review]
Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers v1

Can you explain why this change is OK?  Do these not get used from XBL or random other places when remote XUL is allowed?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831285 [details] [diff] [review]
Move nsXULTreeBuilder declaration into its own header v1

r=me

Comment 16

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #14)
> Can you explain why this change is OK?  Do these not get used from XBL or
> random other places when remote XUL is allowed?

We discussed this way back on irc. These are objects that have only one interface in their DOMCI, so interface flattening is not an issue (just need to make sure we expose them in other WebIDL with that one interface). We're losing the interface object, but none of these have constants so I decided that was not a huge issue. Not sure what else you're worried about?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Without the classinfo, XPConnect will no longer create XPCWrappedNatives for these in non-system scopes, because nsScriptSecurityManager::CanCreateWrapper will return false.  In particular, these will no longer be usable from XBL attached to web pages, afaict (though they _will_ be usable in remote-XUL-whitelisted things...)

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK, I talked to bholley about comment 17.  He would like CanCreateWrapper to keep denying XPCNWs in content XBL scopes, long-term.

So I think we should do the following, in order:

1)  As part of the patches in this bug, loosen CanCreateWrapper in the "xbl scope" case to allow those three specific things we're removing classinfo for.

2)  Once we no longer have extensions able to do this sort of thing, check that none of the XBL we ourselves attach to content uses these APIs (I just did; it does not) and if so just remove those special cases.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831284 [details] [diff] [review]
Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers v1

So looking at addons DXR I see one addon doing "t instanceof aWindow.XULControllers": the "Enhanced Middle Click" addon.  And unfortunately, doing "instanceof undefined" will throw.  So we need to either get this addon fixed or keep a window.XULControllers around.  Jorge, thoughts?

I also see a bunch of addons that have a "javascript.jsm" file that exports the "XULControllers" name.  Kris, you seem to know something about that file; would it be horribly broken if window.XULControllers stops existing?

Comment 20

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #19)
> I also see a bunch of addons that have a "javascript.jsm" file that exports
> the "XULControllers" name.  Kris, you seem to know something about that
> file; would it be horribly broken if window.XULControllers stops existing?

No, that's just a list of builtin properties that don't (or didn't) show up in property enumeration until touched, and tl check the existence of. They don't cause any harm if they don't exist.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831284 [details] [diff] [review]
Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers v1

>-  NS_DEFINE_CHROME_XBL_CLASSINFO_DATA(XULControllers, nsNonDOMObjectSH,

Ah.  So this one doesn't have DOM_OBJECT classinfo to start with.  Hence no need to do anything interesting with it in CanCreateWrapper; non-system code couldn't touch it anyway.  I just verified that trying to do .controllers on a XULElement in videocontrols.xml throws "can't create wrapper" exceptions.

Given that, please mark XULElements's "controllers" attribute [ChromeOnly] in XULElement.webidl.  The attrs on Window, HTMLInputElement, and HTMLTextareaElement are already thus marked.

Depending on what Kris and Jorge say, we may need to have some sort of shim to make "instanceof XULControllers" work.

For XULCommandDispatcher and TreeSelection, please add hacks in CanCreateWrapper, per comment 18.

With this change, nsNonDOMObjectSH is unused and can go away, right?

>-    "Controllers",

Hrm.  So looking at addons mxr, a bunch of them have an ace.js that does this:

  t.isGecko=t.isMozilla=(window.Controllers||window.controllers)&&window.navigator.product==="Gecko",

This looks like <https://ace.c9.io/> at first glance.  And indeed, https://github.com/ajaxorg/ace/blob/f757c8568fba3e59392383e332519e5570f54e1e/lib/ace/lib/useragent.js#L85 shows exactly that code there as of today.  :(

Looks like this boolean is then used at least in https://github.com/ajaxorg/ace/blob/f757c8568fba3e59392383e332519e5570f54e1e/lib/ace/keyboard/textinput.js#L446 but also possibly by whatever sites are embedding this thing.  :(

So we may need to keep a truthy window.Controllers for the moment, and then look into removing it in a separate bug.  I _think_ it doesn't need any particularly nice instanceof behavior.  I hope.

r=me with the above bits fixed.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831287 [details] [diff] [review]
Convert XUL template builders to WebIDL v1

The commit message here is really confusing.  Maybe just drop the first sentence of it?

This applies to the other patches in this bug too.

>+++ b/dom/webidl/TreeView.webidl
>+  DOMString getColumnProperties(TreeColumn column);

Please fix the docs on this to clearly say it just returns an empty string (which I think is true for the IDL version, though maybe not the XPCOM version, right?).

>+  [Throws]
>+  void setCellValue(long row, TreeColumn column, DOMString value);
...
>+  [Throws]
>+  void setCellText(long row, TreeColumn column, DOMString value);

Please fix the docs to clearly documet these as no-ops.

Also, they don't need to be [Throws].

>-  void performAction(in wstring action);
>+  void performAction(DOMString action);

Technically changes behavior for strings with embedded null, but probably OK...

>+++ b/dom/webidl/XULTemplateBuilder.webidl
>+    readonly attribute Element root;

"Element?", please; the C++ has null-check all over.

>      * Force the template builder to rebuild its content. All existing content

The rebuild() method ended up all hidden.  It needs to be [Throws], no?

So does refresh().

>      * @throws NS_ERROR_NULL_POINTER if aResult or aQueryNode are null

They can't be; bindings will enforce that.  Please fix the comment.  (This is for addResult).

>+    boolean canDrop(long index, long orientation, DataTransfer dataTransfer);

Last arg should be nullable, no?  Certainly it's nullable on TreeView's TreeView's canDrop!

>+    void onDrop(long row, long orientation, DataTransfer dataTransfer);

Again, last arg should be nullable.

>+    void onCycleHeader(DOMString colID, Element elt);

Are we guaranteed the second arg can't be null?  Probably safer to make it nullable.

>+    MozRDFResource getResourceAtIndex(long aRowIndex);

Are we sure the return value can't be null?  Safer to mark it nullable.

>+++ b/dom/xul/templates/nsXULContentBuilder.cpp
> nsXULContentBuilder::HasGeneratedContent(nsIRDFResource* aResource,
>+        return aTag.IsVoid() || mRoot->NodeInfo()->LocalName().Equals(aTag);

DOMStringIsNull(aTag) might be more idiomatic than IsVoid().

+                if (aTag.IsVoid() || content->NodeInfo()->LocalName().Equals(aTag)) {

And here.

>+++ b/dom/xul/templates/nsXULTemplateBuilder.cpp
>+nsXULTemplateBuilder::AddResult(nsIXULTemplateResult* aResult,
>+    if (!aResult) {

Can't happen except via the XPCOM method.  Please move the null-check there.

>+nsXULTemplateBuilder::RemoveResult(nsIXULTemplateResult* aResult,
>+    if (!aResult) {

Same thing: move to the XPCOM method.

>+nsXULTemplateBuilder::ReplaceResult(nsIXULTemplateResult* aOldResult,
>+    if (!aOldResult || !aNewResult) {

And same here.

>+nsXULTemplateBuilder::ResultBindingChanged(nsIXULTemplateResult* aResult,
>+    if (!aResult) {

And here.

>+nsXULTemplateBuilder::AddRuleFilter(nsINode& aRule,
>+    if (!aFilter) {

And here.

> nsXULTemplateBuilder::AddListener(nsIXULBuilderListener* aListener)
> nsXULTemplateBuilder::RemoveListener(nsIXULBuilderListener* aListener)

Please file a followup to remove this XPCOM API and just store XULBuilderListeners.

>+++ b/dom/xul/templates/nsXULTreeBuilder.cpp
>+nsXULTreeBuilder::HasNextSibling(int32_t aRow, int32_t aAfterIndex,
>+                               ErrorResult& aError)

Please fix indent.

>+nsXULTreeBuilder::GetIndexOfResource(nsIRDFResource* aResource,
>+    if (!aResource) {

This can only happen when called from the XPCOM signature, right?  Please move the null-check there.

> NS_IMETHODIMP
> nsXULTreeBuilder::AddObserver(nsIXULTreeBuilderObserver* aObserver)

This is dead code after this patch, I think.  Please file a followup bug to remove it (and switch to storing XULTreeBuilderObserver instances directly instead of converting them to nsIXULTreeBuilderObserver).

In general, a bunch of the XPCOM API might be dead code now.

> nsXULTreeBuilder::RemoveObserver(nsIXULTreeBuilderObserver* aObserver)
>-    return mObservers.RemoveObject(aObserver) ? NS_OK : NS_ERROR_FAILURE;
>+    mObservers.RemoveElement(aObserver);

This changes behavior when someone tries to remove a nonexistent observer from throwing to not throwing.  I think that's OK, though...

>+nsXULTreeBuilder::GetTemplateActionCellFor(int32_t aRow, nsTreeColumn& aCol)
>+    nsCOMPtr<nsIAtom> colAtom;
>+    int32_t colIndex;
>+    aCol.GetAtom(getter_AddRefs(colAtom));
>+    aCol.GetIndex(&colIndex);

Please use the webidl getters here, not the xpcom ones.  Will look nicer.

>+    nsIContent* result;

You need to init it to nullptr.  Otherwise you can end up returning random values.

>+nsXULTreeBuilder::CanDrop(int32_t aRow, int32_t aOrientation,
>+            bool canDrop;

Need to init to false, in case observer->CanDrop() returns error... that would at least match the old code.

> NS_IMETHODIMP
> nsXULTreeBuilder::SelectionChanged()

Please add a comment in the header about how the XPCOM SelectionChanged is OK for webidl too.

>+nsXULTreeBuilder::IsEditable(int32_t aRow, nsTreeColumn& aColumn,
>+    return editable.EqualsLiteral("false");

Isn't this backwards?  Should be:

  return !editable.EqualsLiteral("false");

Looks like we have missing test coverage here?  :(

>+nsXULTreeBuilder::IsSelectable(int32_t aRow, nsTreeColumn& aColumn,
>+    return selectable.EqualsLiteral("false");

Again, this is backwards.  Should be:

  return !selectable.EqualsLiteral("false");

>+nsXULTreeBuilder::PerformAction(const nsAString& aAction)
>+            observer->OnPerformAction(nsString(aAction).get());

PromiseFlatString(aAction).get(), please.

> nsXULTreeBuilder::PerformAction(const char16_t* aAction)
>+    PerformAction(nsDependentString(aAction));

This will crash if someone passes null (which was a valid input before).

The good news is that this is dead code right now.  How about we just return error here and have a followup to remove it?  Failing that, return error at least when !aAction, before doing the nsDependentString thing.

>+nsXULTreeBuilder::PerformActionOnRow(const nsAString& aAction, int32_t aRow)
>+            observer->OnPerformActionOnRow(nsString(aAction).get(), aRow);

PromiseFlatString().

> nsXULTreeBuilder::PerformActionOnRow(const char16_t* aAction, int32_t aRow)
>+    PerformActionOnRow(nsDependentString(aAction), aRow);

Again, will crash if null is passed.

>+nsXULTreeBuilder::PerformActionOnCell(const nsAString& aAction, int32_t aRow,
>+            observer->OnPerformActionOnCell(nsString(aAction).get(), aRow, id.get());

PromiseFlatString().

> nsXULTreeBuilder::PerformActionOnCell(const char16_t* aAction, int32_t aRow, nsITreeColumn* aCol)
>+    PerformActionOnCell(nsDependentString(aAction), aRow, *col);

Will crash.

>+nsXULTreeBuilder::GetResourceAtIndex(int32_t aRowIndex, ErrorResult& aError)
>+    if (aRowIndex < 0 || aRowIndex >= mRows.Count()) {

Why not IsValidRowIndex?

>+++ b/dom/xul/templates/nsXULTreeBuilder.h
>+    bool IsSorted()
>+    {
>+        return false;

This doesn't match the XPCOM implementation.  Please fix!

>+++ b/layout/xul/tree/nsTreeColumns.h
>+    RefPtr<nsTreeColumn> col;
>+    if (aColumn) {
>+      CallQueryInterface(aColumn,
>+                         static_cast<nsTreeColumn**>(getter_AddRefs(col)));
>+    }

  RefPtr<nsTreeColumn> col = do_QueryObject(aColumn);

r=me with the above issues fixed.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8831289 [details] [diff] [review]
Convert TreeContentView to WebIDL v1

>-  is(view.getCellProperties(1, { id: "title" }), "icon",

OK, so this code was nonsense, I think (in that it ended up with a null atom and index == 0)... but it did work.  But didn't it end up passing in the id="restore" treecol, effectively (because index == 0), not the id="title" one?  Or did this not end up calling nsTreeContentView::GetCell?

Also, are we pretty sure there are no extensions relying on this behavior?  Because we _could_ have getCellProperties take a (TreeColumn or IndexDict) where IndexDict is a dictionary type that has a single "index" property, and thus preserve the old behavior, right?  Wouldn't help for this { id: "title" } case, again unless it's not actually calling nsTreeContentView to start with.

>+++ b/layout/xul/tree/nsTreeContentView.cpp
> NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(nsTreeContentView)

This needs addition of NS_WRAPPERCACHE_INTERFACE_MAP_ENTRY afaict.

>+nsTreeContentView::IsSeparator(int32_t aRow, ErrorResult& aError)
>+  return mRows[aRow]->IsEmpty();

Should call IsSeparator(), not IsEmpty().

> nsTreeContentView::SelectionChanged()

Should document in the header that the XPCOM version is OK for webidl.

>+nsTreeContentView::GetItemAtIndex(int32_t aIndex, ErrorResult& aError)
>+  return mRows[aIndex]->mContent->AsElement();

What guarantees that mRows[aIndex]->mContent->IsElement()?  Better to check and return null if not.

> nsTreeContentView::GetItemAtIndex(int32_t aIndex, nsIDOMElement** _retval)
>+  return CallQueryInterface(element, _retval);

"element" can be null here.   Please add a null-check and assign null as needed.  Or just QI into an nsCOMPtr<nsIDOMElement> and then forget() into _retval.

>+nsTreeContentView::GetIndexOfItem(Element& aItem)

Why does this require non-null?  The xpidl API allowed null, afaict, returning -1 for it.

> nsTreeContentView::GetIndexOfItem(nsIDOMElement* aItem, int32_t* _retval)
>+  NS_ENSURE_ARG(element);

Same here: why the API change?

>+nsTreeContentView::GetCell(nsIContent* aContainer, nsTreeColumn& aCol)
>+  aCol.GetAtom(getter_AddRefs(colAtom));
>+  aCol.GetIndex(&colIndex);

Can use the webidl accessors on aCol instead.

r=me with the above nits fixed.

This all scares the heck out of me given our apparent lack of test coverage and the worries around extension compat, but I don't really have any better ideas, except the ones I mention above.  :(

Comment 24

[Boris Zbarsky [:bzbarsky]]

Thank you again for doing this, by the way!

Comment 25

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #19)
> So looking at addons DXR I see one addon doing "t instanceof
> aWindow.XULControllers": the "Enhanced Middle Click" addon.  And
> unfortunately, doing "instanceof undefined" will throw.  So we need to
> either get this addon fixed or keep a window.XULControllers around.  Jorge,
> thoughts?

I don't think one add-on warrants a shim. I'll notify the developer. Any suggestions on what they should do instead?

Comment 26

[Boris Zbarsky [:bzbarsky]]

> Any suggestions on what they should do instead?

  t instanceof Components.interfaces.nsIControllers

should do the equivalent thing, I think.

Comment 27

[Tom S]

Can this land?

Comment 28

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64811af102dcd18be103c62e5b81e945ffca88b2

Comment 29

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #22)

> >+++ b/dom/webidl/TreeView.webidl
> >+  DOMString getColumnProperties(TreeColumn column);
> 
> Please fix the docs on this to clearly say it just returns an empty string
> (which I think is true for the IDL version, though maybe not the XPCOM
> version, right?).

> Please fix the docs to clearly documet these as no-ops.

> Also, they don't need to be [Throws].

I didn't do these, because TreeContentView (converted in the next patch) implements TreeView and needs this.

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #23)
> Comment on attachment 8831289 [details] [diff] [review]
> Convert TreeContentView to WebIDL v1
> 
> >-  is(view.getCellProperties(1, { id: "title" }), "icon",
> 
> OK, so this code was nonsense, I think (in that it ended up with a null atom
> and index == 0)... but it did work.  But didn't it end up passing in the
> id="restore" treecol, effectively (because index == 0), not the id="title"
> one?  Or did this not end up calling nsTreeContentView::GetCell?
> 
> Also, are we pretty sure there are no extensions relying on this behavior? 
> Because we _could_ have getCellProperties take a (TreeColumn or IndexDict)
> where IndexDict is a dictionary type that has a single "index" property, and
> thus preserve the old behavior, right?  Wouldn't help for this { id: "title"
> } case, again unless it's not actually calling nsTreeContentView to start
> with.

So it turns out that this code is calling a nsITreeView implementation in JS, not nsTreeContentView::GetCellProperties. I undid the change to the test.
I didn't see an extension doing something like this either.

> What guarantees that mRows[aIndex]->mContent->IsElement()?  Better to check
> and return null if not.

I've made Row's mContent an Element, all the callers already checked that it is.

Did the other changes.

Comment 30

[Peter Van der Beken [:peterv]]

Attachment: Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers

Comment 31

[Peter Van der Beken [:peterv]]

Attachment: Move nsXULTreeBuilder declaration into its own header

Comment 32

[Peter Van der Beken [:peterv]]

Attachment: Convert XUL template builders to WebIDL

Comment 33

[Peter Van der Beken [:peterv]]

Attachment: Convert TreeContentView to WebIDL

Comment 34

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c51cdd4283493b1ee4a3f8dcc75481fa06094ac
Bug 1252211 - Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/715142a051a0f9339c1c441a281da91b38c0713e
Bug 1252211 - Move nsXULTreeBuilder declaration into its own header. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82c5f20d6ee239c5e52b62440471c8a85ac6763f
Bug 1252211 - Convert XUL template builders to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/629ace67da3bec76161266bcccdbcb96561ec96f
Bug 1252211 - Convert TreeContentView to WebIDL. r=bz.

Comment 35

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=106639541&repo=mozilla-inbound

Comment 36

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54ce8da73fbb
Backed out changeset 629ace67da3b 
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7f52437c99
Backed out changeset 82c5f20d6ee2 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3936f4d38eaf
Backed out changeset 715142a051a0 
https://hg.mozilla.org/integration/mozilla-inbound/rev/23edf58d766b
Backed out changeset 2c51cdd42834 for bustage

Comment 37

[Peter Van der Beken [:peterv]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b770df8863b8165639c4a7a0eaabd5463c1090

Comment 38

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a358c0a7c665a0760a3089f4dd73f4bb9effa061
Bug 1252211 - Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b94ef88fc7ecae5829cb62c677083ac4c552942
Bug 1252211 - Move nsXULTreeBuilder declaration into its own header. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/535ff8df62de4dbe986c72128b7a36096f0b1110
Bug 1252211 - Convert XUL template builders to WebIDL. r=bz.

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bcd8780fd8847919b8aacaede3e74c1f392219a
Bug 1252211 - Convert TreeContentView to WebIDL. r=bz.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a358c0a7c665
https://hg.mozilla.org/mozilla-central/rev/9b94ef88fc7e
https://hg.mozilla.org/mozilla-central/rev/535ff8df62de
https://hg.mozilla.org/mozilla-central/rev/5bcd8780fd88

Comment 40

[Francesco Lodolo [:flod]]

You landed a string without updating the string ID, meaning that nobody will notice, and we'll keep shipping the wrong message in 90 or so languages.

Any technical reason preventing you from using a different string ID.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

Comment 41

[Boris Zbarsky [:bzbarsky]]

Sorry about that; that part wasn't in the patches I reviewed.  :(

> Any technical reason preventing you from using a different string ID

We could switch to a different id, I think.  We'd need to update the name in nsDeprecatedOperationList.h and the callsite that does the WarnOnceAbout.

We'd have a discontinuity in the use counter, because the use counter name would change.  I'm not sure whether that's a problem in practice.

Comment 42

[Francesco Lodolo [:flod]]

(In reply to Boris Zbarsky [:bz] (if a patch has no decent message, automatic r-) from comment #41)
> We could switch to a different id, I think.  We'd need to update the name in
> nsDeprecatedOperationList.h and the callsite that does the WarnOnceAbout.

Thanks (I also forgot the question mark in my previous comment).

I'm aware of some edge cases where it's really hard to change the ID, because they're dynamically generated. If it's not the case here, I will file a follow-up bug to update the ID.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Yeah, this ID is dynamically generated, but from a string we fully control and can change.  Followup would be great, thank you!