Convert remaining XUL DOM classes to WebIDL

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: peterv, Assigned: peterv, NeedInfo)

Tracking

(Blocks: 2 bugs, {addon-compat})

Trunk
mozilla56
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox56 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(4 attachments, 9 obsolete attachments)

31.65 KB, patch
peterv
: review+
Details | Diff | Splinter Review
59.46 KB, patch
peterv
: review+
Details | Diff | Splinter Review
137.15 KB, patch
peterv
: review+
Details | Diff | Splinter Review
48.91 KB, patch
peterv
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Whiteboard: btpp-active
Which classes is this referring to?
Flags: needinfo?(peterv)
(Assignee)

Comment 2

8 months ago
Created attachment 8818253 [details] [diff] [review]
Rename nsXULControllers to XULControllers v1
(Assignee)

Comment 3

8 months ago
Created attachment 8818254 [details] [diff] [review]
Convert XULControllers to WebIDL v1
(Assignee)

Comment 4

8 months ago
Created attachment 8818255 [details] [diff] [review]
Rename nsXULCommandDispatcher to XULCommandDispatcher v1
(Assignee)

Comment 5

8 months ago
Created attachment 8818256 [details] [diff] [review]
Convert XULCommandDispatcher to WebIDL v1
(Assignee)

Comment 6

8 months ago
Created attachment 8818257 [details] [diff] [review]
Convert XUL template builders to WebIDL v1
(Assignee)

Comment 7

8 months ago
(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? :-)
Flags: needinfo?(peterv)
> Want to review? :-)

Sure.
Blocks: 1207321
(Assignee)

Comment 9

7 months ago
Created attachment 8831284 [details] [diff] [review]
Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers v1
Attachment #8818253 - Attachment is obsolete: true
Attachment #8818254 - Attachment is obsolete: true
Attachment #8818255 - Attachment is obsolete: true
Attachment #8818256 - Attachment is obsolete: true
Attachment #8818257 - Attachment is obsolete: true
Attachment #8831284 - Flags: review?(bzbarsky)
(Assignee)

Comment 10

7 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4ec8a0e104c
(Assignee)

Comment 11

7 months ago
Created attachment 8831285 [details] [diff] [review]
Move nsXULTreeBuilder declaration into its own header v1
Attachment #8831285 - Flags: review?(bzbarsky)
(Assignee)

Comment 12

7 months ago
Created attachment 8831287 [details] [diff] [review]
Convert XUL template builders to WebIDL v1
Attachment #8831287 - Flags: review?(bzbarsky)
(Assignee)

Comment 13

7 months ago
Created attachment 8831289 [details] [diff] [review]
Convert TreeContentView to WebIDL v1
Attachment #8831289 - Flags: review?(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?
Flags: needinfo?(peterv)
Comment on attachment 8831285 [details] [diff] [review]
Move nsXULTreeBuilder declaration into its own header v1

r=me
Attachment #8831285 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 16

6 months ago
(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?
Flags: needinfo?(peterv)
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...)
Flags: needinfo?(peterv)
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 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?
Flags: needinfo?(kmaglione+bmo)
Flags: needinfo?(jorge)
(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.
Flags: needinfo?(kmaglione+bmo)
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.
Attachment #8831284 - Flags: review?(bzbarsky) → review+
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.
Attachment #8831287 - Flags: review?(bzbarsky) → review+
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.  :(
Attachment #8831289 - Flags: review?(bzbarsky) → review+
Thank you again for doing this, by the way!
(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?
Flags: needinfo?(jorge)
Keywords: addon-compat
> Any suggestions on what they should do instead?

  t instanceof Components.interfaces.nsIControllers

should do the equivalent thing, I think.
Can this land?
(Assignee)

Comment 28

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=64811af102dcd18be103c62e5b81e945ffca88b2
(Assignee)

Comment 29

2 months ago
(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.
Flags: needinfo?(peterv)
(Assignee)

Comment 30

2 months ago
Created attachment 8877070 [details] [diff] [review]
Remove DOMCI for TreeSelection, XULCommandDispatcher and XULControllers
Attachment #8831284 - Attachment is obsolete: true
Attachment #8877070 - Flags: review+
(Assignee)

Comment 31

2 months ago
Created attachment 8877071 [details] [diff] [review]
Move nsXULTreeBuilder declaration into its own header
Attachment #8831285 - Attachment is obsolete: true
Attachment #8877071 - Flags: review+
(Assignee)

Comment 32

2 months ago
Created attachment 8877072 [details] [diff] [review]
Convert XUL template builders to WebIDL
Attachment #8831287 - Attachment is obsolete: true
Attachment #8877072 - Flags: review+
(Assignee)

Comment 33

2 months ago
Created attachment 8877076 [details] [diff] [review]
Convert TreeContentView to WebIDL
Attachment #8831289 - Attachment is obsolete: true
Attachment #8877076 - Flags: review+
(Assignee)

Comment 34

2 months ago
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.
backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=106639541&repo=mozilla-inbound
Flags: needinfo?(peterv)

Comment 36

2 months ago
Backout by cbook@mozilla.com:
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
(Assignee)

Comment 37

2 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78b770df8863b8165639c4a7a0eaabd5463c1090
(Assignee)

Comment 38

2 months ago
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

2 months ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
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.
(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.
Yeah, this ID is dynamically generated, but from a string we fully control and can change.  Followup would be great, thank you!
Depends on: 1372969
You need to log in before you can comment on or make changes to this bug.