replace #text-base binding by webidl control

RESOLVED FIXED in Firefox 64

Status

()

P3
normal
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 months ago
Posted patch patch (obsolete) — Splinter Review
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?
Attachment #9012088 - Flags: feedback?(bugs)
(Assignee)

Updated

6 months ago
Assignee: nobody → surkov.alexander
Depends on: 1494167
Priority: -- → P3
Flags: needinfo?(enndeakin)
You also need to add to nsXULElement::Construct.
Flags: needinfo?(enndeakin)
(Assignee)

Comment 2

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

thank you, Neil.
(Assignee)

Comment 3

6 months ago
Posted patch patch (obsolete) — Splinter Review
Attachment #9012088 - Attachment is obsolete: true
Attachment #9012088 - Flags: feedback?(bugs)
Attachment #9012451 - Flags: review?(bugs)
After looking into this, do you think this approach will extend to the text-label and label-control bindings?
Flags: needinfo?(surkov.alexander)
(Assignee)

Comment 5

6 months ago
(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.
Flags: needinfo?(surkov.alexander)
Blocks: 1494529
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
Which performance issue is comment 0 talking about?
Flags: needinfo?(surkov.alexander)
(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.
Flags: needinfo?(surkov.alexander)
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.
Attachment #9012451 - Flags: review?(bugs) → review-
(Assignee)

Comment 10

6 months ago
(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.
(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.
(Assignee)

Comment 12

6 months ago
(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
(Assignee)

Comment 13

6 months ago
Posted patch patch2Splinter Review
Attachment #9012451 - Attachment is obsolete: true
Attachment #9012770 - Flags: review?(bugs)
Mutation listeners definitely count, but getters shouldn't cause mutations, only setters.
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
Attachment #9012770 - Flags: review?(bugs) → review+

Comment 16

6 months ago
Pushed by surkov.alexander@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/274e2637419e
replace #text-base binding by webidl control, r=smaug

Comment 17

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/274e2637419e
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.