1535725 - Port bug 1519948: Replace use of boxObject

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Jorg K (CEST = GMT+2)]

As per bug 1519948 comment #5:

element.boxObject.x/y/width/height can mostly be replaced with getBoundingClientRect(). It is almost identicial, although its behaviour in scrolling areas is different. If performing computations with the values, current code might add on scroll offsets and the like, which probably won't be needed with getBoundingClientRect.

element.boxObject.screenX/screenY can be replaced with element.screenX/screenY.

The property related functions would probably work with just properties on the element. I checked at one point and Thunderbird doesn't use them as far as I can tell anyway.

The element relation properties (at the end of BoxObject.webidl), have no exact equivalent but again I'm not sure if Thunderbird needs them. If so, I can suggest alternatives.

Comment 1

[Jorg K (CEST = GMT+2)]

Lots to replace:
https://searchfox.org/comm-central/search?q=.boxObject&case=false&regexp=false&path=

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-boxobject-x-y-height-width.patch

OK, here's the straight forward replacement of .x, .y, .width, .height, .screenX, .screenY.

I'll have to check what's left after that.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9058959 [details] [diff] [review]
1535725-boxobject-x-y-height-width.patch

Hold on, still a few left which need some manual treatment.

Comment 4

[Neil Deakin]

I notice there are some copies of some bindings in that link above (comment 1). Note a couple of special cases.

For richlistbox.xml see the changes in bug 1519967. It needs a different fix to fix scroll offsets. I didn't look at the Thunderbird code to see if this is relevant but thought I'd point them out. You may need similar changes in other cases, if the boxobject properties are being used in conjuction with scrollable areas.

For textbox.xml, see https://hg.mozilla.org/mozilla-central/rev/63908a1890a0

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-boxobject-x-y-height-width.patch (v2)

OK, this covers all the easy stuff, including richlistbox.xml. textbox.xml was moved to suite/ (SemMonkey), so we don't worry about it.

This is left:

mail:
.\base\content\folderDisplay.js:  get parentBox() { return this.domNode.boxObject.parentBox; },
.\base\content\folderDisplay.js:  get firstChild() { return this.domNode.boxObject.firstChild; },
.\base\content\folderDisplay.js:  get lastChild() { return this.domNode.boxObject.lastChild; },
.\base\content\folderDisplay.js:  get nextSibling() { return this.domNode.boxObject.nextSibling; },
.\base\content\folderDisplay.js:  get previousSibling() { return this.domNode.boxObject.previousSibling; },
.\base\content\folderDisplay.js:    return this.domNode.boxObject.getPropertyAsSupports(propertyName);
.\base\content\folderDisplay.js:    this.domNode.boxObject.setPropertyAsSupports(propertyName, value);
.\base\content\folderDisplay.js:    return this.domNode.boxObject.getProperty(propertyName);
.\base\content\folderDisplay.js:    return this.domNode.boxObject.setProperty(propertyName, value);
.\base\content\folderDisplay.js:    return this.domNode.boxObject.removeProperty(propertyName);
.\base\content\msgHdrView.js:      let name = attachmentitem.boxObject.firstChild
.\base\content\msgHdrView.js:      let label = item.boxObject.firstChild.nextSibling;

calendar:
.\base\content\calendar-multiday-view.xml:     let sbo = scrollbox.boxObject;
.\base\content\calendar-multiday-view.xml:     let propertyValue = scrollbox.boxObject.firstChild.boxObject[propertyName];

So in mail we have stuff with first/lastChild, nextSibling, etc.

In calendar we have those scroll stuff:
https://searchfox.org/comm-central/rev/b2c998eb41d696d31a87384487514e9b386f201c/calendar/base/content/calendar-multiday-view.xml#1373

So those are the non-trivial things that are left.

Should this patch work now? Then I can start a try.

Comment 6

[Jorg K (CEST = GMT+2)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0fca98077f6a18a6489c25e1a32bd8b29a99debb

Comment 7

[Neil Deakin]

.\base\content\folderDisplay.js: get parentBox() { return this.domNode.boxObject.parentBox; },
.\base\content\folderDisplay.js: get firstChild() { return this.domNode.boxObject.firstChild; },
.\base\content\folderDisplay.js: get lastChild() { return this.domNode.boxObject.lastChild; },
.\base\content\folderDisplay.js: get nextSibling() { return this.domNode.boxObject.nextSibling; },
.\base\content\folderDisplay.js: get previousSibling() { return this.domNode.boxObject.previousSibling; },
.\base\content\folderDisplay.js: return this.domNode.boxObject.getPropertyAsSupports(propertyName);
.\base\content\folderDisplay.js: this.domNode.boxObject.setPropertyAsSupports(propertyName, value);
.\base\content\folderDisplay.js: return this.domNode.boxObject.getProperty(propertyName);
.\base\content\folderDisplay.js: return this.domNode.boxObject.setProperty(propertyName, value);
.\base\content\folderDisplay.js: return this.domNode.boxObject.removeProperty(propertyName);

It looks like this is used to implement some fake BoxObject, but I bet it has no callers so they could just be removed.

.\base\content\msgHdrView.js: let name = attachmentitem.boxObject.firstChild
.\base\content\msgHdrView.js: let label = item.boxObject.firstChild.nextSibling;

This seems to have been added by a very recent patch (bug 1345167), so it is probably easier to determine why this was added. I suspect it is only used to get XBL anonymous children, so it should be using getAnonymousXXX type functions instead.

Comment 8

[Jorg K (CEST = GMT+2)]

I was just about to say that the lot in folderDisplay.js is for the fake box object.

Magnus and Alta88, can you suggest a replacement for:
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.720
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.923

So that leaves the Calendar stuff

calendar:
.\base\content\calendar-multiday-view.xml: let sbo = scrollbox.boxObject;
.\base\content\calendar-multiday-view.xml: let propertyValue = scrollbox.boxObject.firstChild.boxObject[propertyName];

https://searchfox.org/comm-central/rev/b2c998eb41d696d31a87384487514e9b386f201c/calendar/base/content/calendar-multiday-view.xml#1373
https://searchfox.org/comm-central/rev/b2c998eb41d696d31a87384487514e9b386f201c/calendar/base/content/calendar-multiday-view.xml#3687

For the first call site, we replace let sbo = scrollbox.boxObject; ... ; sbo.scrollBy( ... ); with scrollbox.scrollBy( ... ); ?

I'm happy for someone else to provide those tweaks after I've done the mass replacement.

EDIT:
BTW, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0fca98077f6a18a6489c25e1a32bd8b29a99debb is green, so the mass replacement was successful as far as we can tell.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-boxobject-x-y-height-width.patch (v2b)

Rebased. With every de-XBL patch that lands, this will need to be rebased :-(

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 9059029 [details] [diff] [review]
1535725-boxobject-x-y-height-width.patch (v2b)

Geoff, perhaps you can review this quickly before it rots. Green try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0fca98077f6a18a6489c25e1a32bd8b29a99debb

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-boxobject-x-y-height-width.patch (v2c)

Another rebase :-(

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e92087648287
Port bug 1519948: Replace use of boxObject: .x, .y, .height, .width, .screenX, .screenY. r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1e9b60bd5e34
Change test-session-store.js to use clientWidth/Height; rs=bustage-fix

Comment 14

[Geoff Lankow (:darktrojan)]

getBoundingClientRect uses floating-point numbers, which will probably cause a few problems like the test I just fixed. I don't know if clientWidth and clientHeight are also for the chop, but it'll do for now.

Comment 15

[Geoff Lankow (:darktrojan)]

Comment on attachment 9059112 [details] [diff] [review]
1535725-boxobject-x-y-height-width.patch (v2c)

Review of attachment 9059112 [details] [diff] [review]:
-----------------------------------------------------------------

Okay in principle, but please go back over it and fix the places where getBoundingClientRect is called multiple times on the same object. There's so many I gave up marking them all.

You can probably get away with clientLeft/Top/Width/Height in most places, but like I said, I don't know if those properties have a future.

::: calendar/base/content/calendar-multiday-view.xml
@@ +1353,3 @@
>              } else {
> +                diffStart = event.clientX - scrollbox.getBoundingClientRect().x;
> +                diffEnd = scrollbox.getBoundingClientRect().x + scrollbox.getBoundingClientRect().width - event.clientX;

Please refactor this so getBoundingClientRect is only called once.

@@ +1405,5 @@
>              // drag session, no sweeping.
> +            if (event.clientX < (event.target.getBoundingClientRect().x) ||
> +                event.clientX > (event.target.getBoundingClientRect().x + event.target.getBoundingClientRect().width) ||
> +                event.clientY < (event.target.getBoundingClientRect().y) ||
> +                event.clientY > (event.target.getBoundingClientRect().y + event.target.getBoundingClientRect().height)) {

And this.

@@ +3569,5 @@
> +                let element = document.getAnonymousElementByAttribute(col.column, "anonid", "boxstack");
> +                if (aClientX >= element.screenX &&
> +                    aClientX <= (element.screenX + element.getBoundingClientRect().width) &&
> +                    aClientY >= element.screenY &&
> +                    aClientY <= (element.screenY + element.getBoundingClientRect().height)) {

And this.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +70,5 @@
>          const cWidth = containerWidth % contentWidth;
>          let numHorizontal = (containerWidth - cWidth) / contentWidth;
>  
> +        let contentHeight = minimonth.getBoundingClientRect().height;
> +        let containerHeight = this.node.getBoundingClientRect().height;

Here.

::: calendar/test/mozmill/testTimezones.js
@@ +265,5 @@
>          for (let tzIdx = 0; tzIdx < timezones.length; tzIdx++) {
>              let [correctHour, minutes, day] = selectedTime[tzIdx];
>  
>              let timeNode = lookup(`${timeLine}/[${correctHour}]`).getNode();
> +            let timeY = timeNode.getBoundingClientRect().y + timeNode.getBoundingClientRect().height * (minutes / 60);

Here.

::: common/bindings/notificationbox.xml
@@ +351,5 @@
>                     content.localName != "browser")
>                  return;
>  
> +              var width = content.getBoundingClientRect().width;
> +              var height = content.getBoundingClientRect().height;

Here.

::: common/bindings/richlistbox.xml
@@ +672,4 @@
>  
>              // Partially visible items are also considered visible
> +            return (aItem.getBoundingClientRect().y + aItem.clientHeight > y) &&
> +                   (aItem.getBoundingClientRect().y < y + this.clientHeight);

Here.

::: common/src/customizeToolbar.js
@@ +114,5 @@
>    else
>      width = parseInt(document.documentElement.style.width);
> +  var screenX = gToolbox.screenX
> +                + ((gToolbox.getBoundingClientRect().width - width) / 2);
> +  var screenY = gToolbox.screenY + gToolbox.getBoundingClientRect().height;

Here.

@@ +673,5 @@
>    } else {
>      gCurrentDragOverItem = null;
>  
>      var direction = window.getComputedStyle(dropTarget.parentNode).direction;
> +    var dropTargetCenter = dropTarget.getBoundingClientRect().x + (dropTarget.getBoundingClientRect().width / 2);

And here.

::: mail/test/mozmill/shared-modules/test-mouse-event-helpers.js
@@ +176,5 @@
>    let screenY;
>    if (aArgs && ("screenY" in aArgs))
>      screenY = aArgs.screenY;
>    else
> +    screenY = aDispatcher.ScreenY;

Wait, what? I hope this code never runs, which it probably doesn't.

Comment 16

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #14)

getBoundingClientRect uses floating-point numbers, which will probably cause a few problems like the test I just fixed.

Thanks, I should have checked my try better :-( - screenY = aDispatcher.ScreenY; is a good one, but sed didn't notice. I'll go over it and replace the double calls, unless we want to switch to clientLeft/Top/Width/Height.

To avoid further passes through this code, maybe we can answer these questions first:

1. Neil, what about using clientLeft/Top/Width/Height, see comment #14 and comment #15.
2. Neil and Geoff, any suggestion for the calendar stuff in comment #8.
3. Alta88 and Magnus, any suggestion for the stuff in msgHdrView.js, see comment #8.

Comment 17

[Geoff Lankow (:darktrojan)]

.\base\content\calendar-multiday-view.xml: let sbo = scrollbox.boxObject;

scrollBy is also method on sbo.

.\base\content\calendar-multiday-view.xml: let propertyValue = scrollbox.boxObject.firstChild.boxObject[propertyName];

The best I've come up with to get the size of the scrollbar (unless we can get a handle for that scrollbar) is to calculate the size of the scrollbox minus the size of its contents. That seems a bit wrong.

Comment 18

[Neil Deakin]

(In reply to Jorg K (GMT+2) from comment #16)

To avoid further passes through this code, maybe we can answer these questions first:

1. Neil, what about using clientLeft/Top/Width/Height, see comment #14 and comment #15.

clientWidth/clientHeight return the padding box, whereas boxObject and getBoundingClientRect return the border box. You can use them, but you'll need to adjust any calculations to account for the border.

2. Neil and Geoff, any suggestion for the calendar stuff in comment #8.

You should be able to use scrollbox.scrollBy instead.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-follow-up.patch

OK, removed multiple retrievals of bounding rect on same object. Also fixed ScreenX and the scrollBy call site. I didn't fix notificationbox.xml to avoid more rebasing in bug 1536935 where that will be removed.

Still left:
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.720
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.923

.\base\content\calendar-multiday-view.xml: let propertyValue = scrollbox.boxObject.firstChild.boxObject[propertyName];

Comment 20

[alta88]

(In reply to Jorg K (GMT+2) from comment #8)

I was just about to say that the lot in folderDisplay.js is for the fake box object.

Magnus and Alta88, can you suggest a replacement for:
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.720
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.923

you can't programmatically access a node created in a binding without boxObject. querySelectorAll doesn't work nor are child nodes exposed (anymore). your options are:

1. set an attribute like |externalattachment| on the <richlistitem> and have a custom css selector for the node wanting .text-link styling (also with hover).
2. wait for de-xbl of attachment widgets. maybe even future proof it and use html.
3. looks like menuitem will continue haunting; as the note says the method in #1 above didn't work well. perhaps something similar can be tried again, otherwise you can remove setting |text-link| and the extra styling there. it was a nice visual bit indicating those menuitems were external links.

Comment 21

[Neil Deakin]

(In reply to alta88 from comment #20)

you can't programmatically access a node created in a binding without boxObject.

That's not correct. You can use document.getAnonymousNodes or document.getAnonymousElementByAttribute

A better way though is to add a method or property to the binding that does what you need it to do, rather than having code external to the binding accessing its internals.

Comment 22

[alta88]

(In reply to Neil Deakin from comment #21)

(In reply to alta88 from comment #20)

you can't programmatically access a node created in a binding without boxObject.

That's not correct. You can use document.getAnonymousNodes or document.getAnonymousElementByAttribute

how 'forever' are those methods going to be? are they in a spec?

A better way though is to add a method or property to the binding that does what you need it to do, rather than having code external to the binding accessing its internals.

i assume you mean custom element class, and not actually old deprecating 'binding'.

Comment 23

[Magnus Melin [:mkmelin]]

The attachment list is being de-xbl'd and is almost ready (pending a focus/select problem) - bug 1523607

Comment 24

[Jorg K (CEST = GMT+2)]

I did what I could here, so can someone please take over for the last two remaining bits (see comment #19).

Comment 25

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-follow-up.patch (v2)

Found some more:
https://searchfox.org/comm-central/search?q=.boxObject&case=false&regexp=false&path=

Remaining ones in
calendar/base/content/calendar-multiday-view.xml
mail/base/content/folderDisplay.js (fake stuff)
mail/base/content/msgHdrView.js

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 9059651 [details] [diff] [review]
1535725-follow-up.patch (v2)

OK, apparently there are more multiple retrievals of bounding rect on same object. I'll do a better job later.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 9059112 [details] [diff] [review]
1535725-boxobject-x-y-height-width.patch (v2c)

Review of attachment 9059112 [details] [diff] [review]:
-----------------------------------------------------------------

OK, checking what I have forgotten.

::: calendar/base/content/dialogs/calendar-event-dialog-recurrence.js
@@ +70,5 @@
>          const cWidth = containerWidth % contentWidth;
>          let numHorizontal = (containerWidth - cWidth) / contentWidth;
>  
> +        let contentHeight = minimonth.getBoundingClientRect().height;
> +        let containerHeight = this.node.getBoundingClientRect().height;

Forgot to fix this.

::: mail/base/content/mailWidgets.xml
@@ +151,5 @@
>            // ... or not far enough?
> +          else if (item.screenY + item.getBoundingClientRect().height >
> +                   box.screenY + box.getBoundingClientRect().height)
> +            box.scrollTop = item.getBoundingClientRect().y + item.getBoundingClientRect().height -
> +                            box.getBoundingClientRect().y - box.getBoundingClientRect().height;

More here.

::: mail/base/content/sanitizeDialog.js
@@ +80,5 @@
>      }
>  
>      // If clearing a specific time range
>      if (!warningBox.hidden) {
> +      window.resizeBy(0, -warningBox.getBoundingClientRect().height);

Here. EDIT: No saving here.

::: mail/base/content/tabmail.xml
@@ +2304,3 @@
>              return null;
>  
> +          if (event.screenX > tab.screenX + tab.getBoundingClientRect().width * .75)

Here.

@@ +2322,4 @@
>                  return i;
>            } else {
>              for (let i = 0; i < tabs.length; i++)
> +              if (event.screenX > (tabs[i].screenX + (tabs[i].getBoundingClientRect().width / 2)))

Here. EDIT: No saving here.

::: mail/base/modules/GlobalPopupNotifications.jsm
@@ +833,5 @@
>      this._refreshPanel(notificationsToShow);
>  
>      // If the anchor element is hidden or null, fall back to the identity icon.
> +    if (!anchorElement || (anchorElement.getBoundingClientRect().height == 0 &&
> +                           anchorElement.getBoundingClientRect().width == 0)) {

Here and twice below.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5657,5 @@
>      let bucket = document.getElementById("attachmentBucket");
>  
>      if (target == bucket) {
>        // Dragging or dropping at top/bottom border of the listbox
> +      if ((aEvent.screenY - target.screenY) / target.getBoundingClientRect().height < 0.5) {

Here and twice below. EDIT: No saving here.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +65,2 @@
>  
> +  msgHeadersToolbar.height = msgHeadersToolbar.getBoundingClientRect().height +

Here.

::: mail/test/mozmill/folder-display/test-opening-messages.js
@@ +193,5 @@
>    let childrenHeightsStr = "";
>    for (let child of messengerChildren) {
>      if ("boxObject" in child) {
> +      childrenHeightsSum += child.getBoundingClientRect().height;
> +      childrenHeightsStr += '"' + child.id + '": ' + child.getBoundingClientRect().height + ', ';

Here.

::: mail/test/mozmill/shared-modules/test-folder-display-helpers.js
@@ +1095,5 @@
>    aTree.focus();
>    // very important, gotta be able to see the row
>    aTree.ensureRowIsVisible(aViewIndex);
>    // coordinates of the upper left of the entire tree widget (headers included)
> +  let tx = aTree.getBoundingClientRect().x, ty = aTree.getBoundingClientRect().y;

Here and twice below.

::: mail/test/mozmill/shared-modules/test-mouse-event-helpers.js
@@ +58,5 @@
>    synthesize_drop(aDropWindow, aDropObject, dt,
> +      { screenX : aDropObject.screenX +
> +                    (aDropObject.getBoundingClientRect().width * aRelDropX),
> +        screenY : aDropObject.screenY +
> +                    (aDropObject.getBoundingClientRect().width * aRelDropY)

Here.

::: mail/test/mozmill/tabmail/test-tabmail-dragndrop.js
@@ +235,5 @@
>  
>    // notify tab1 drag has ended
>    synthesize_drag_end(mc.window, dropContent, tab1, dt,
> +      { screenX : (dropContent.screenX + dropContent.getBoundingClientRect().width / 2 ),
> +        screenY : (dropContent.screenY + dropContent.getBoundingClientRect().height / 2 ) });

Here.

Comment 28

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-follow-up.patch (v3)

I hope this is it. Note the bug here:

-      { screenX : aDropObject.screenX +
-                    (aDropObject.getBoundingClientRect().width * aRelDropX),
-        screenY : aDropObject.screenY +
-                    (aDropObject.getBoundingClientRect().width * aRelDropY)
                                                          ^^^^^

As you saw, I edited the previous comment to note the cases where no saving would occur.

Comment 29

[Geoff Lankow (:darktrojan)]

Comment on attachment 9060094 [details] [diff] [review]
1535725-follow-up.patch (v3)

Review of attachment 9060094 [details] [diff] [review]:
-----------------------------------------------------------------

This looks okay. It's probably worth running the linter across the changed files. I didn't spot anything but I'm not a machine.

r+ with these changes:

::: mail/base/content/mailWidgets.xml
@@ +153,5 @@
>            // ... or not far enough?
> +          else if (item.screenY + itemRect.height >
> +                   box.screenY + boxRect.height)
> +            box.scrollTop = itemRect.y + itemRect.height -
> +                            boxRect.y - boxRect.height;

While you're here, can you join these wrapped lines and/or add curly braces?

::: mail/base/modules/GlobalPopupNotifications.jsm
@@ +834,5 @@
>  
>      // If the anchor element is hidden or null, fall back to the identity icon.
> +    let r;
> +    if (!anchorElement || ((r = anchorElement.getBoundingClientRect()).height == 0 &&
> +                           r.width == 0)) {

Oh, ew. :-( 

M-C has a better solution: https://searchfox.org/mozilla-central/rev/ec489aa170b6486891cf3625717d6fa12bcd11c1/toolkit/modules/PopupNotifications.jsm#995

::: mail/test/mozmill/folder-display/test-opening-messages.js
@@ +191,5 @@
>    let messengerChildren = aWC.e("messengerWindow").children;
>    let childrenHeightsSum = 0;
>    let childrenHeightsStr = "";
>    for (let child of messengerChildren) {
>      if ("boxObject" in child) {

Hmm… that's not going to work. You should be able to remove this "if", all the children have getBoundingClientRect but only some have boxObject. I guess you'll know it doesn't work if the test stops passing.

Comment 30

[Jorg K (CEST = GMT+2)]

Attachment: 1535725-follow-up.patch (v4)

Thanks, I checked linting on try.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a7efaecb39cc
Follow-up: Fix multiple retrievals of bounding rect on same object. r=darktrojan

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e7f13928d361
Follow-up: fix broken test-tabmail-dragndrop.js; rs=bustage-fix DONTBUILD

Comment 33

[Geoff Lankow (:darktrojan)]

Linting doesn't help when the mistake is in an unlinted file and the reviewer is blind.

Comment 34

[Jorg K (CEST = GMT+2)]

Thanks for fixing that. Copy/paste error from here: https://hg.mozilla.org/comm-central/rev/a7efaecb39cc#l13.12.
I actually had a green try before those last changes.

Anyway, what's the way forward with the two remaining call sites, not counting the fake stuff?

Still left:
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.720
https://hg.mozilla.org/comm-central/rev/d167921e42eb#l6.923

.\base\content\calendar-multiday-view.xml: let propertyValue = scrollbox.boxObject.firstChild.boxObject[propertyName];

Comment 35

[Magnus Melin [:mkmelin]]

The ones still left should be pretty easy to convert at least once bug 1523607 lands (I hear it's figured out and should be finished soon). When it's not anonymous content you can just querySelector the child object you need.

Comment 36

[Jorg K (CEST = GMT+2)]

Magnus, bug 1523607 didn't fix anything as far as I can see, somehow the count even increased, I guess the ones in calendar-event-dialog-attendees-custom-elements.js were hiding in some pending patches and snuck back in :-(

https://searchfox.org/comm-central/search?q=.boxObject&case=false&regexp=false&path=

calendar/base/content/calendar-multiday-base-view.js
1549 const propertyValue = this.scrollbox.boxObject.firstChild.boxObject[widthOrHeight];
calendar/base/content/dialogs/calendar-event-dialog-attendees-custom-elements.js
3158 this.mRowHeight = items[i].boxObject.height;
3169 let listboxHeight = this.boxObject.height;

mail/base/content/folderDisplay.js - some fake tree junk that could just be removed.
2636 get parentBox() { return this.domNode.boxObject.parentBox; },
2637 get firstChild() { return this.domNode.boxObject.firstChild; },
2638 get lastChild() { return this.domNode.boxObject.lastChild; },
2639 get nextSibling() { return this.domNode.boxObject.nextSibling; },
2640 get previousSibling() { return this.domNode.boxObject.previousSibling; },
2642 return this.domNode.boxObject.getPropertyAsSupports(propertyName);
2645 this.domNode.boxObject.setPropertyAsSupports(propertyName, value);
2648 return this.domNode.boxObject.getProperty(propertyName);
2651 return this.domNode.boxObject.setProperty(propertyName, value);
2654 return this.domNode.boxObject.removeProperty(propertyName);

mail/base/content/msgHdrView.js
2352 let name = attachmentitem.boxObject.firstChild
2670 let label = item.boxObject.firstChild.nextSibling;

Comment 37

[Magnus Melin [:mkmelin]]

Attachment: bug1535725_boxobject_rem.patch

Comment 38

[Magnus Melin [:mkmelin]]

Attachment: bug1535725_boxobject_calendar_rem.patch

Comment 39

[alta88]

Comment on attachment 9067526 [details] [diff] [review]
bug1535725_boxobject_rem.patch

works for me.

Comment 40

[Jorg K (CEST = GMT+2)]

Great. Note that the "fake stuff" will go in bug 1518823, see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e24022251ffeae43ec05cc6a774693e396f0b5f2

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 9067526 [details] [diff] [review]
bug1535725_boxobject_rem.patch

Review of attachment 9067526 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgHdrView.js
@@ +2665,5 @@
>      } else {
>        // The text-link class must be added to the <label> and have a <menu>
>        // hover rule. Adding to <menu> makes hover overflow the underline to
>        // the popup items.
> +      let label = item.children[1];

Why not item.firstChild.nextSibling;? Isn't that the same as .children[1], only more efficient?

Comment 42

[Magnus Melin [:mkmelin]]

I can't imagine there would be any efficiency difference, it's a reference to the same thing. Just more readable.
(Well, children avoids any possible whitespace node problems in the future.)

Comment 43

[Jorg K (CEST = GMT+2)]

In general, .children[1] has a complexity of O(N) where N is the number of siblings, .firstChild.nextSibling has a complexity of O(1). That's because DOM nodes don't hold a child array any more, see bug 651120. So the all the children need to be traversed to fill an array which is later discarded :-( - Therefore direct access the first and second child in generally much faster.

How many children are you expecting? BTW, what's a "white-space node problem"?

Comment 44

[alta88]

To bypass possible text/comment nodes, .firstElementChild and .nextElementSibling could be used. I guess given that bug, these are infinitesimally slightly better ;) if you know for sure the .children array is only constructed lazily.

Comment 45

[Magnus Melin [:mkmelin]]

There's apparently 6 children in this case. firstChild could also include whitespace text nodes (depending on the document type).

I'd still go for readability over micro-optimization here.

Comment 46

[Jorg K (CEST = GMT+2)]

Well, for 6 children that we don't need, I'd most certainly go for the "proper" way that M-C promotes of not retrieving all children, also for the simple reason of not establishing a bad pattern that others could copy even if they are 60 children. So can we please revert this unnecessary change or do you insist?

Comment 47

[Jorg K (CEST = GMT+2)]

https://searchfox.org/comm-central/search?q=.firstChild.nextSibling&case=false&regexp=false&path=
584 hits, so you can't really say that it's badly readable.

https://searchfox.org/comm-central/search?q=.children%5B1%5D&case=false&regexp=false&path=
130 hits, mostly in 3rd party code, tests and devtools. Also, I have the impression that children is also used for style retrievals.

https://searchfox.org/comm-central/search?q=.childNodes%5B1%5D&case=false&regexp=false&path=
195 hits, mostly in 3rd party code, tests and devtools.

.children and .childNodes are different, the former doesn't return text or comment nodes. Is that desired?

Comment 48

[Magnus Melin [:mkmelin]]

.children is a standard API, and i don't see any de-promotion about it anywhere. (Yes, we want children, not childNodes).

I did a quick check on what kind of numbers we're looking at. Not very conclusive, but what we can say it there is no real difference. The numbers are so tiny any and all changes you make to code could result in such differences. In my test children[1] "won" 3/5 times actually.

children[1]:
0.006427000000257976 ms
0.006904000001668464 ms
0.009058999999979278 ms
0.022188000000824104 ms
0.023704999997789855 ms

firstElementChild.nextElementSibling:
0.004087999999683234 ms
0.0179069999994681 ms
0.010258999998768559 ms
0.01703700000052777 ms
0.00450500000079046 ms

Comment 49

[Jorg K (CEST = GMT+2)]

No one said that .children is being "de-promoted". All I said is that there is no child node array any more, so retrieving a child is potentially O(N). Surely for small N, you won't see a difference.

If you want to contribute some real insight here, please try with 600 or 6000 children (or post the test for others to try), doing timing on 6 children proves nothing. If for 6000 children .children[1] is still as fast as getting the next sibling, then you won indeed. While you're there, compare getting the 1st and the 6000th child, and also the 5950th, maybe there is optimisation to get the last one. Maybe retrieval is really smart and the linked list of children is only walked to the number you require.

Comment 50

[Magnus Melin [:mkmelin]]

But we only have 6 children, so what's the point? Unless it's very very top level, you won't find that many children.

Comment 51

[Paul Morris [:pmorris]]

Comment on attachment 9067528 [details] [diff] [review]
bug1535725_boxobject_calendar_rem.patch

Review of attachment 9067528 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. r+

Comment 52

[Jorg K (CEST = GMT+2)]

Comment on attachment 9067526 [details] [diff] [review]
bug1535725_boxobject_rem.patch

These might as well join their friends on the beta.

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bb64c0fb051b
Replace use of boxObject in msgHdrView.js. r=alta88
https://hg.mozilla.org/comm-central/rev/12e96412b2c8
Replace remaining use of boxObject in calendar. r=pmorris

Comment 54

[Jorg K (CEST = GMT+2)]

We started this in the 68 cycle.

Comment 55

[Jorg K (CEST = GMT+2)]

TB 68 beta:
https://hg.mozilla.org/releases/comm-beta/rev/38dd71d77389f5e0f68a6c668f7213e288184845
https://hg.mozilla.org/releases/comm-beta/rev/a3f862000ed8fb0d4d1ce976b5a3c9f665cfbcc0