1494230 - replace #text-base binding by webidl control

Status: RESOLVED FIXED

(Core :: XUL, task, P3)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Since CE applied to description and labels ruins down performance, then it's advisable to implement description/label properties in c++. Here's a shot for description element.

It doesnt' seem working though. I don't see 'disabled'/'value' properties on description/label elements.

Olli, could you take a look what may be wrong here?

Comment 1

[Neil Deakin]

You also need to add to nsXULElement::Construct.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Neil Deakin from comment #1)
> You also need to add to nsXULElement::Construct.

thank you, Neil.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[Brian Grinstead [:bgrins]]

After looking into this, do you think this approach will extend to the text-label and label-control bindings?

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> After looking into this, do you think this approach will extend to the
> text-label and label-control bindings?

I guess it can be reused for text-label as well, see bug 1448213 comment #31. Not sure about text-label/link bindings. If those implemented as CE don't hit performance, then I would go with CE, since it's the easiest way. Otherwise, we should look into merging text-label and nsTextBoxFrame I'd say.

Comment 6

[Brian Grinstead [:bgrins]]

From a try push:

1) It looks like there's a devtools test which needs an assertion to be updated to check for XULTextElement: https://treeherder.mozilla.org/#/jobs?repo=try&revision=651140290e609ae496a0089fcf3518a901ae9d1b&selectedJob=201864770 
2) test_interfaces.html is failing: https://treeherder.mozilla.org/#/jobs?repo=try&revision=651140290e609ae496a0089fcf3518a901ae9d1b&selectedJob=201863742

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Which performance issue is comment 0 talking about?

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from comment #7)
> Which performance issue is comment 0 talking about?

The problem is that we have a ton of hidden labels in the markup (and XBL anon content) in browser.xul, which don't get XBL bindings attached but do get Custom Elements connected. I've been working on at least deferring initialization until after DOMContentLoaded (in Bug 1481949) which should help, but perf would surely be better using a XULElement subclass than either CE or XBL.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

>   if (ns == kNameSpaceID_XUL) {
>-    if (definition->mLocalName == nsGkAtoms::menupopup ||
>+    if (definition->mLocalName == nsGkAtoms::description ||
>+        definition->mLocalName == nsGkAtoms::label) {
>+      cb = XULTextElement_Binding::GetConstructorObject;
>+    } else if (definition->mLocalName == nsGkAtoms::menupopup ||
>         definition->mLocalName == nsGkAtoms::popup ||
>         definition->mLocalName == nsGkAtoms::panel ||
>         definition->mLocalName == nsGkAtoms::tooltip) {
could you align these line under definition->mLocalName == nsGkAtoms::menupopup ||


>+#include "nsXULElement.h"
>+#include "Units.h"
What is Units.h? and what has that to do with this file?

>+
>+namespace mozilla {
>+namespace dom {
>+
>+
>+class XULTextElement final : public nsXULElement
>+{
>+public:
>+  explicit XULTextElement(already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo)
>+    : nsXULElement(std::move(aNodeInfo))
>+  {
>+  }
>+
>+  MOZ_CAN_RUN_SCRIPT bool Disabled()
>+  {
>+    return GetXULBoolAttr(nsGkAtoms::disabled);
>+  }
How can this run script? Or any of the getter methods? Please remove MOZ_CAN_RUN_SCRIPT or explain what would run script
(that would mean a bug somewhere I think).


Could we please have a small browser test or such using xul:description and xul:label elements and ensuring
that those elements'  .__proto__  == XULTextElement.prototype



I'd like to see a new patch after these small nits fixed.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> From a try push:
> 
> 1) It looks like there's a devtools test which needs an assertion to be
> updated to check for XULTextElement:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=651140290e609ae496a0089fcf3518a901ae9d1b&selectedJob=2
> 01864770 
> 2) test_interfaces.html is failing:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=651140290e609ae496a0089fcf3518a901ae9d1b&selectedJob=2
> 01863742

yeah, thank you, I found those yesterday after I filed the patch. Btw, how do you make a try build for this kind of changes, ./mach try fuzzy ctrl+A provides a lot of noise of intermittents.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to alexander :surkov (:asurkov) from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> > From a try push:
> > 
> > 1) It looks like there's a devtools test which needs an assertion to be
> > updated to check for XULTextElement:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=651140290e609ae496a0089fcf3518a901ae9d1b&selectedJob=2
> > 01864770 
> > 2) test_interfaces.html is failing:
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=651140290e609ae496a0089fcf3518a901ae9d1b&selectedJob=2
> > 01863742
> 
> yeah, thank you, I found those yesterday after I filed the patch. Btw, how
> do you make a try build for this kind of changes, ./mach try fuzzy ctrl+A
> provides a lot of noise of intermittents.

I just use ./mach try directly and have a few usual commands. Depends on the type of patch - frontend and browser-only changes are usually covered by `./mach try -b do -p linux64,macosx64,win64 -u mochitests,xpcshell -t none`. Lots of times for platform changes it's more common to do `./mach try -b do -p all -u all -t none` but it's more costly and also surfaces a bunch more intermittents as you've noticed, so YMMV. On https://mozilla-releng.net/trychooser/ you can select stuff more specifically.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] (r- if the bug doesn't explain what the change(s) are about.) from comment #9)

> >   if (ns == kNameSpaceID_XUL) {
> >-    if (definition->mLocalName == nsGkAtoms::menupopup ||
> >+    if (definition->mLocalName == nsGkAtoms::description ||
> >+        definition->mLocalName == nsGkAtoms::label) {
> >+      cb = XULTextElement_Binding::GetConstructorObject;
> >+    } else if (definition->mLocalName == nsGkAtoms::menupopup ||
> >         definition->mLocalName == nsGkAtoms::popup ||
> >         definition->mLocalName == nsGkAtoms::panel ||
> >         definition->mLocalName == nsGkAtoms::tooltip) {
> could you align these line under definition->mLocalName ==
> nsGkAtoms::menupopup ||

right

> >+#include "nsXULElement.h"
> >+#include "Units.h"
> What is Units.h? and what has that to do with this file?

copy/paste, removed

> >+  MOZ_CAN_RUN_SCRIPT bool Disabled()
> >+  {
> >+    return GetXULBoolAttr(nsGkAtoms::disabled);
> >+  }
> How can this run script? Or any of the getter methods? Please remove
> MOZ_CAN_RUN_SCRIPT or explain what would run script
> (that would mean a bug somewhere I think).

oh, that was a copied from other file. I don't think those can run any scripts (I guess JS mutation change listeners don't count). I will remove those.

> Could we please have a small browser test or such using xul:description and
> xul:label elements and ensuring
> that those elements'  .__proto__  == XULTextElement.prototype
> 
> 
> 
> I'd like to see a new patch after these small nits fixed.

sure

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Mutation listeners definitely count, but getters shouldn't cause mutations, only setters.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 9012770 [details] [diff] [review]
patch2

>+  bool Disabled()
>+  {
>+    return GetXULBoolAttr(nsGkAtoms::disabled);
>+  }
>+  void SetDisabled(bool aValue)
>+  {
>+    SetXULBoolAttr(nsGkAtoms::disabled, aValue);
>+  }
>+  void GetValue(DOMString& aValue) const
>+  {
>+    GetXULAttr(nsGkAtoms::value, aValue);
>+  }
>+  void SetValue(const nsAString& aValue)
>+  {
>+    SetAttr(kNameSpaceID_None, nsGkAtoms::value, aValue, true);
>+  }
Set* methods may run scripts, so don't remove the annotations from those.


>     void SetXULAttr(nsAtom* aName, const nsAString& aValue,
>                     mozilla::ErrorResult& aError)
>     {
>         SetAttr(aName, aValue, aError);
>     }
>+    bool GetXULBoolAttr(nsAtom* aName) const
>+    {
>+        nsAutoString value;
>+        GetAttr(kNameSpaceID_None, aName, value);
>+        return value.EqualsLiteral("true");
This should use AttrValueIs
Similar to https://searchfox.org/mozilla-central/rev/819cd31a93fd50b7167979607371878c4d6f18e8/dom/xul/nsXULElement.cpp#1466-1469

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/274e2637419e
replace #text-base binding by webidl control, r=smaug

Comment 17

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/274e2637419e