382466 - in toolbar.xml, prevent lots of repeated work in updateChevron() by using a timer

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[(not reading, please use [e-mail] instead)]

in toolbar.xml's itemChanged(), prevent lots of repeated work in updateChevron() by using a timer

spun off from bug #379591, asaf wrote:

> Hrm, it might worth checking whether boxObject.width of the DOM node has
> changed (on a timeout) and only call updateChevron if so.

In addition to loading or reloading livemarks, there might be a lot of calls to updateChevron when adding or deleting lots of top level personal toolbar items from the bookmark manager dialog at once.

again, before we fix this, we should be convinced that we have a real performance problem.

Comment 1

[Dietrich Ayala (:dietrich)]

> 
> spun off from bug #379591, asaf wrote:
> 
> > Hrm, it might worth checking whether boxObject.width of the DOM node has
> > changed (on a timeout) and only call updateChevron if so.

is there a DOM mutation event we could listen for? that might be the most efficient way to go, if it's possible.

Comment 2

[Dão Gottwald [:dao]]

There's the underflow/overflow event, which could be used at least if the items were in a <xul:scrollbox> (which might be a good idea anyway).

Comment 3

[Marco Bonardo [:mak]]

i'm experimenting with the scrollbox, i've a working solution with some bug, i'm actually evaluating if we can at the same time reduce work calling updateChevron on a timer and fix bug 447571. Ideally i'd like to completely remove the need to call updateChevron manually, and let the task to dom listeners.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.0

This should be better than the previous impl, i'm using a stack instead of the vbox with hacked negative margins, the scrollbox allows to handle in a more efficient way overflow/underflow cases, resize handles window resizes.

All work is done on a timer, so that if we have many changes coming in a small timeframe we will delay the update to a single final one.

Finally this fixes a problem with the indicator bar that after a drag&drop was preventing the toolbar to shrink, causing the chevron to go out of scope.

From my tests this also solves bug 447571, the behavior looks similar to the one we had before it regressed, i can eventually provide a tryserver build to users having the issue and get feedback.

Comment 5

[Marco Bonardo [:mak]]

i also tried to get rid of the remainig calls to updateChevron, using further listeners, but i had to listen also on other 3 mutation events and filter them. this way update it's called just when it's needed and does not generate a bunch of useless calls to be filtered out.

Comment 6

[Dão Gottwald [:dao]]

You can probably get rid of toolbar-drop-indicator-bar altogether and at the same time avoid the stack. The latest patch in bug 347930 did this for the tab bar.

Comment 7

[Marco Bonardo [:mak]]

thanks, i'll look into that.

Comment 8

[Dão Gottwald [:dao]]

(In reply to comment #6)
> You can probably get rid of toolbar-drop-indicator-bar altogether and at the
> same time avoid the stack. The latest patch in bug 347930 did this for the tab
> bar.

I've filed bug 508499 for that, so you can see that patch without all the noise around it.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 392488 [details] [diff] [review]
patch v1.0

i had already a working solution based on it, there was not much noise. I still have to fix RTL and then it should be ready for review

Comment 10

[Marco Bonardo [:mak]]

i have a problem in RTL, the scrollbox overflows on the wrong side (right), i suspect i'm missing some align, any hint?

Comment 11

[(no longer active)]

(In reply to comment #10)
> i have a problem in RTL, the scrollbox overflows on the wrong side (right), i
> suspect i'm missing some align, any hint?

Have you tried playing with different alignments?  I'd expect the scrollbox to overflow on the left side in RTL mode by default...

Comment 12

[Marco Bonardo [:mak]]

yes, i've tried some alignment, could be i'm missing something obvious

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v1.1

this fixes the drop indicator, but scrollbox problem still exists.

Comment 14

[Marco Bonardo [:mak]]

+              else if (this.childNodes.length) {
this brace should not be there, fixed locally

replacing the scrollbox with an hbox overflows in the right direction. but i don't get anymore the overlow events clearly.

Comment 15

[Dão Gottwald [:dao]]

You may find this part of tabbrowser's addTab method interesting:

> if (this.tabContainer.mTabstrip._isRTLScrollbox) {
>   /* In RTL UI, the tab is visually added to the left side of the
>    * tabstrip. This means the tabstip has to be scrolled back in
>    * order to make sure the same set of tabs is visible before and
>    * after the new tab is added */
>
>   this.tabContainer.mTabstrip.scrollByPixels(this.mTabs[0].clientWidth);
> }

I also think this shouldn't be necessary.

Comment 16

[Marco Bonardo [:mak]]

Attachment: patch v2.0

So, for some reason i'm starting thinking scrollbox solution is not the best:
Scrollbox binding does not have an implementation, so i would have to use arrowscrollbox, and collapse the arrows.
Due to the above RTL bug on every update i should scroll back the box to position 0.
Since i'm adding 2 new listeners in the constructor could hurt startup by some ns.
I don't need to scroll and will never need, arrowscrollbox is bringing with it a quite complex binding that i'll never use.

the hbox solution on the other side works just as fine, the only drawback is that with overflow event i can directly uncollapse the chevron, and if it is collapsed i avoid doing any update to it. While, with the hbox solution, i have to check if we are overflowed during the update process.
Due to the fact the update now happens on a timer, the overhead should be mitigated for the most part, and largely better than what we actually have.

If we really want to try using a scrollbox, i'd move that to a followup and take current advantages from this patch.

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> So, for some reason i'm starting thinking scrollbox solution is not the best:
> Scrollbox binding does not have an implementation, so i would have to use
> arrowscrollbox, and collapse the arrows.

I don't understand what you mean. What implementation are you referring to?

Comment 18

[Marco Bonardo [:mak]]

well, i mean, what you can do is QI its boxObject to nsIScrollBoxObject and use the implemented scrolling methods. arrowscrollbox is fairly more complex and complete. but still other problems do not give me confidence.
I think we should file a followup about converting toolbar to a scrollbox, and make it depend on a bug on the scrollbox RTL issue, there are not many advantages in the end.

Comment 19

[Dão Gottwald [:dao]]

You can use scrollbox.scrollLeft. I don't think you need anything else, but maybe I'm missing something.

Comment 20

[Marco Bonardo [:mak]]

i did not try scrollLeft, so eventually i can try that too.
As a further reason i don't like this, it makes the scrollbox jumpy, since i will scroll on the same update timer that updates the chevron.
eventually i can provide both patches, there aren't many changes needed between the two, i had a working patch, just was not seeing the advantage in doing that right now, rather than waiting a proper fix for the scrollbox.

Comment 21

[Marco Bonardo [:mak]]

yes, i'll provide both patches so we can better evaluate the differences.

Comment 22

[Dão Gottwald [:dao]]

The rtl fix should probably look something like this:

//XXX bug [yet to be filed]
if (rtl)
 scrollBox.scrollLeft = scrollBox.scrollWidth;

Why should that happen in a timer callback?

Comment 23

[Marco Bonardo [:mak]]

Comment on attachment 392822 [details] [diff] [review]
patch v2.0

filed bug 508816 about the scrollbox issue.

I'm going with the scrollbox, i've provided some more cleanup of the code. There is still work needed but we plan to do another D&D code cleanup soon, so i won't go far more than this.

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch v3.0

Comment 25

[Marco Bonardo [:mak]]

// XXX (bug xxxxxx) Scrollbox does not handle correctly RTL mode.

this should be bug #, i was pretty sure to have replaced those xxx, but sounds like i forgot, fixed locally.

Comment 26

[Dão Gottwald [:dao]]

Comment on attachment 392956 [details] [diff] [review]
patch v3.0

>+          <xul:hbox pack="start">
>+            <xul:image class="toolbar-drop-indicator"
>+                       mousethrough="always"
>+                       collapsed="true"/>
>+          </xul:hbox>

mousethrough="always" can be removed, I think.

>       <method name="_rebuild">
>         <body><![CDATA[
>-          // Clear out references to existing nodes, since we'll be deleting and re-adding.
>+          // Clear out references to existing nodes, since they will be removed
>+          //  and re-added.

there's a superfluous space at the start of the second line

>       <method name="chevronPopupShowing">
>         <parameter name="aEvent"/>
>         <body><![CDATA[
>+          // Handle popupshowing only for the chevron popup.
>           var popup = aEvent.target;
>           if (popup != this._chevron.firstChild)
>             return;

make this if (aEvent.target != aEvent.currentTarget) ...?

>+      <method name="handleEvent">
>+        <parameter name="aEvent"/>
>         <body><![CDATA[
>+          switch (aEvent.type) {
>+            case "resize":
>+              // Ignore events that aren't on the document or the window
>+              // (html document, tooltips, etc)
>+              // Do not ignore content window resizes, because they may
>+              // be the result of the toolbar being shown/hidden
>+              if (aEvent.target != document && aEvent.target != window &&
>+                  aEvent.target != content)
>+                return;

It seems that aEvent.target != document will always be true, as the event is dispatched on the window.

Did you test if aEvent.target != content is still needed?

>+            case "overflow":
>+             // Attach the popup binding to the chevron popup if it has not yet
>+             // been initialized.

The comment needs another space before //.

>+      <method name="_updateChevronInternal">

This needs a better name, I think. Maybe _updateChevronTimerCallback.

>+        <body><![CDATA[
>+          var spaceLeft = this._scrollbox.getBoundingClientRect().width;
>           for (var i = 0; i < this.childNodes.length; i++) {
>             var child = this.childNodes[i];
>+            spaceLeft -= child.getBoundingClientRect().width;
>+            child.style.visibility = spaceLeft < 0 ? "hidden" : "visible";
>           }

What if the items have a margin? Shouldn't this rather work with getBoundingClientRect().right for ltr and .left for rtl?

>             if (aParentPopup._endMarker != -1) {
>-              aParentPopup.insertBefore(element,
>-                                        aParentPopup.childNodes[aParentPopup._endMarker]);
>+              var lastNode = aParentPopup.childNodes[aParentPopup._endMarker];
>+              aParentPopup.insertBefore(element, lastNode);
>             }

nit: use let

>+            var nodeIndex = this._getChildIndex(xulNode);

Array.indexOf(this.childNodes, xulNode);?

>             if (PlacesUtils.nodeIsFolder(xulNode.node) &&
>                 !PlacesUtils.nodeIsReadOnly(xulNode.node)) {
>+              var threshold = nodeBCR.width * 0.25;

>+              else {
>+                // Drop after this folder.
>+                var beforeIndex =
>+                  nodeIndex == this.childNodes.length - 1 ? -1 : nodeIndex + 1;

>             else {
>+              // This is a non-folder node or a read-only folder.
>+              // Drop before it with regards to RTL mode.
>+              var threshold = nodeBCR.width * 0.5;

>+              else {
>+                // Drop after this bookmark.
>+                var beforeIndex =

nit: use let

>-            // The _clearOverFolder() function will close the menu for _overFolder.node.
>-            // So null it out if we don't want to close it.
>+            // The _clearOverFolder() function will close the menu for
>+            //  _overFolder.node.  So null it out if we don't want to close it.

What's that change about?

>+          switch(direction) {
>+            case "ltr":
...
>+              break;
>+            case "rtl":
...
>+              break;
>           }

use if/else instead

>+/* Bookmarks toolbar */
> .toolbar-drop-indicator {
>   position: relative;
>+  z-index: 1;
>+  list-style-image: url(chrome://browser/skin/places/toolbarDropMarker.png);
> }

position: relative; and z-index: 1; belong in browser/base/content/browser.css.

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

It looks like this file hasn't been updated properly.

Comment 27

[Marco Bonardo [:mak]]

(In reply to comment #26)
> >+              if (aEvent.target != document && aEvent.target != window &&
> >+                  aEvent.target != content)
> >+                return;
> 
> It seems that aEvent.target != document will always be true, as the event is
> dispatched on the window.
> 
> Did you test if aEvent.target != content is still needed?

i'm testing it, hopefully some check can go


> >-            // The _clearOverFolder() function will close the menu for _overFolder.node.
> >-            // So null it out if we don't want to close it.
> >+            // The _clearOverFolder() function will close the menu for
> >+            //  _overFolder.node.  So null it out if we don't want to close it.
> 
> What's that change about?

just 80 char limit


> >+/* Bookmarks toolbar */
> > .toolbar-drop-indicator {
> >   position: relative;
> >+  z-index: 1;
> >+  list-style-image: url(chrome://browser/skin/places/toolbarDropMarker.png);
> > }
> 
> position: relative; and z-index: 1; belong in browser/base/content/browser.css.

would then not be better browser/components/places/content/places.css? This is only used in Places toolbar.xml

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #27)
> just 80 char limit

I think you can spare that change. The comment is harder to read after your reformatting, fwiw.

> would then not be better browser/components/places/content/places.css? This is
> only used in Places toolbar.xml

Yep, sounds right.

Comment 29

[Marco Bonardo [:mak]]

Attachment: patch 3.1

(In reply to comment #26)
> (From update of attachment 392956 [details] [diff] [review])
> >+          <xul:hbox pack="start">
> >+            <xul:image class="toolbar-drop-indicator"
> >+                       mousethrough="always"
> >+                       collapsed="true"/>
> >+          </xul:hbox>
> 
> mousethrough="always" can be removed, I think.

removing it causes indicator flickering when the mouse is dragging over the indicator. This could be an issue of our code that draws it, but i won't investigate it here. i'm leaving it in for now.

Fixed a bug in handling overflow/underflow events, the toolbar should not handle those events for scrollboxes in descendant menupopups.

Fixed remaining comments.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 393868 [details] [diff] [review]
patch 3.1

>+            <xul:toolbarbutton type="menu"
>+                               class="chevron"
>+                               mousethrough="never"
>+                               collapsed="true"
>+                               tooltiptext="&bookmarksToolbarChevron.tooltip;"
>+                               onpopupshowing="onChevronPopupShowing(event);">
>+              <xul:menupopup anonid="chevronPopup"
>+                             xbl:inherits="tooltip"

The onpopupshowing attribute should be on the menupopup element (and onChevronPopupShowing should use aEvent.currentTarget instead of aEvent.currentTarget.lastChild).

>+      <field name="_isRTL">
>+        // To test UI interactions in RTL mode you can set intl.uidirection.en
>+        // string pref to rtl.  Add-ons like ForceRTL won't work correctly since
>+        // this field could be initialized before them.

Note that _isRTL won't actually be updated when it was initialized before intl.uidirection.en changed.
In any case, intl.uidirection.* needs to be documented in a central place rather than adding such a comment to every piece of code that deals with rtl.

>+      <method name="_updateChevronPopupNodesVisibility">
>         <body><![CDATA[
>+        var chevronPopup = this._chevron.lastChild;

nit: wrongly indented line

>+          for (var i = 0; i < chevronPopup.childNodes.length; i++) {
>+            chevronPopup.childNodes[i].hidden =
>+              this.childNodes[i].style.visibility != "hidden";
>+          }

nit: "i" makes more sense as "let", as you don't want to use it outside of the loop.

>+      <method name="handleEvent">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          // Both overflow/underflow and resize events should not be handled
>+          // for descendant nodes.

Did you really get overflow/underflow events for nested nodes?

>+              //  This handles case when [...]

There's something wrong with that sentence's grammar.

>+      <method name="_updateChevronTimerCallback">
>+        <body><![CDATA[
>+          var sboxBCR = this._scrollbox.getBoundingClientRect();

Maybe just call this scrollRect?

>+            // Once a child overflows, all the next ones will.
>+            if (!childOverflowed) {
>+              var childBCR = child.getBoundingClientRect();

And similarly, childRect?
Also, a nice candidate for "let" instead of "var"...

>+              childOverflowed = (this._isRTL && childBCR.left < sboxBCR.left) ||
>+                                (!this._isRTL && childBCR.right > sboxBCR.right);

I would recommend:

childOverflowed = this._isRTL ? (A)
                              : (B);

instead of:

childOverflowed = (this._isRTL && A) ||
                  (!this._isRTL && B);

>       <method name="_getDropPoint">

>+          if (xulNode.node) {
>+            var nodeBCR = xulNode.getBoundingClientRect();

let nodeRect ...?

>+              if ((this._isRTL && aEvent.clientX > nodeBCR.right - threshold) ||
>+                  (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {

>+              else if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
>+                       (!this._isRTL && aEvent.clientX < nodeBCR.right - threshold)) {

>+              if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
>+                  (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {

if (this._isRTL ? (A)
                : (B)) {

instead of:

if ((this._isRTL && A) ||
    (!this._isRTL && B)) {

>+          else if (aTimer == this._ibTimer) {
>+
>+            this._dropIndicator.collapsed = true;
>             this._ibTimer = null;
>           }

That line break looks uninteded.

Comment 31

[Marco Bonardo [:mak]]

(In reply to comment #30)
> (From update of attachment 393868 [details] [diff] [review])
> >+            <xul:toolbarbutton type="menu"
> >+                               class="chevron"
> >+                               mousethrough="never"
> >+                               collapsed="true"
> >+                               tooltiptext="&bookmarksToolbarChevron.tooltip;"
> >+                               onpopupshowing="onChevronPopupShowing(event);">
> >+              <xul:menupopup anonid="chevronPopup"
> >+                             xbl:inherits="tooltip"
> 
> The onpopupshowing attribute should be on the menupopup element (and
> onChevronPopupShowing should use aEvent.currentTarget instead of
> aEvent.currentTarget.lastChild).

the popup is filled up in onpopupshowing, moving the onpopupshowing on the popup would execute it before the popup is populated, making this call useless. the function should then be enqueued, making the code far more complex than needed.

> >+      <field name="_isRTL">
> >+        // To test UI interactions in RTL mode you can set intl.uidirection.en
> >+        // string pref to rtl.  Add-ons like ForceRTL won't work correctly since
> >+        // this field could be initialized before them.
> 
> Note that _isRTL won't actually be updated when it was initialized before
> intl.uidirection.en changed.

sure, browser needs to be restarted between changes.

> In any case, intl.uidirection.* needs to be documented in a central place
> rather than adding such a comment to every piece of code that deals with rtl.

i agree, i put that in as a side note since people could try to use forceRTL method to work on the toolbar UI. i can remove the comment fwiw.

> >+      <method name="handleEvent">
> >+        <parameter name="aEvent"/>
> >+        <body><![CDATA[
> >+          // Both overflow/underflow and resize events should not be handled
> >+          // for descendant nodes.
> 
> Did you really get overflow/underflow events for nested nodes?

places popups can contain scrollboxes when the number of children is really big, and yes, i was getting overflow events from them.

> >+              childOverflowed = (this._isRTL && childBCR.left < sboxBCR.left) ||
> >+                                (!this._isRTL && childBCR.right > sboxBCR.right);
> 
> I would recommend:
> 
> childOverflowed = this._isRTL ? (A)
>                               : (B);
> 
> instead of:
> 
> childOverflowed = (this._isRTL && A) ||
>                   (!this._isRTL && B);

i tried that before since i prefer the ? : form usually, but makes the thing harder to read, being a condition that returns a condition (it's a = b ? c > d : e < f). Someone does not appreciate this kind of things.


> >+              if ((this._isRTL && aEvent.clientX > nodeBCR.right - threshold) ||
> >+                  (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {
> 
> >+              else if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
> >+                       (!this._isRTL && aEvent.clientX < nodeBCR.right - threshold)) {
> 
> >+              if ((this._isRTL && aEvent.clientX > nodeBCR.left + threshold) ||
> >+                  (!this._isRTL && aEvent.clientX < nodeBCR.left + threshold)) {
> 
> if (this._isRTL ? (A)
>                 : (B)) {
> 
> instead of:
> 
> if ((this._isRTL && A) ||
>     (!this._isRTL && B)) {

as before, i think that will make this unreadable, being these long conditions

Comment 32

[Dão Gottwald [:dao]]

(In reply to comment #31)
> the popup is filled up in onpopupshowing, moving the onpopupshowing on the
> popup would execute it before the popup is populated, making this call useless.

Really? The popup's popupshowing handler is capturing, yours isn't, so I think that should work.

> i agree, i put that in as a side note since people could try to use forceRTL
> method to work on the toolbar UI. i can remove the comment fwiw.

forceRTL could in fact be fixed to update the _isRTL field, so I don't think that comment makes a lot of sense anyway.

> i tried that before since i prefer the ? : form usually, but makes the thing
> harder to read, being a condition that returns a condition (it's a = b ? c > d
> : e < f). Someone does not appreciate this kind of things.

Well, I made that recommendation because I found the chained boolean expressions unnecessarily long-winded and thus harder to read. The ternary expression better represents what's going on: the rtl condition picks the expression that you're actually interested in.

Comment 33

[Marco Bonardo [:mak]]

(In reply to comment #32)
> (In reply to comment #31)
> > the popup is filled up in onpopupshowing, moving the onpopupshowing on the
> > popup would execute it before the popup is populated, making this call useless.
> 
> Really? The popup's popupshowing handler is capturing, yours isn't, so I think
> that should work.

yes, i thought the same initially, but does not work. the winning popupshowing (the one that fills up the popup) is menu.xml handler. it is also in the capturing phase, but the sequence i receive the notifications (moving popupshowing to the popup) is: toolbar.xml::popupshowing, toolbar.xml::chevronpopupshowing, menu.xml::popupshowing.

Comment 34

[Marco Bonardo [:mak]]

Attachment: patch 3.2

fixed all comments, apart popupshowing.
I could maybe add a popupshowing bubbling handler to the binding and use that instead of onpopupshowing, but won't change much.
i changed aEvent.currentTarget.lastChild to this._chevron.lastChild, it's more self-explained

Comment 35

[Dão Gottwald [:dao]]

(In reply to comment #33)
> > Really? The popup's popupshowing handler is capturing, yours isn't, so I think
> > that should work.
> 
> yes, i thought the same initially, but does not work. the winning popupshowing
> (the one that fills up the popup) is menu.xml handler. it is also in the
> capturing phase, but the sequence i receive the notifications (moving
> popupshowing to the popup) is: toolbar.xml::popupshowing,
> toolbar.xml::chevronpopupshowing, menu.xml::popupshowing.

Odd, I tested onpopupshowing="Cu.reportError(this._built)" on the menupopup and it seems to be executed after menu.xml's popupshowing handler.
toolbar.xml's popupshowing handler doesn't matter here, right?

Comment 36

[Marco Bonardo [:mak]]

(In reply to comment #35)
> Odd, I tested onpopupshowing="Cu.reportError(this._built)" on the menupopup and
> it seems to be executed after menu.xml's popupshowing handler.
> toolbar.xml's popupshowing handler doesn't matter here, right?

no, toolbar xml popupshowing handles autoopening and d&d issues with Linux, it's the inside menu binding that manages the popups contents.

i just added some console dump to the threee handlers after moving the onpopupshowing handler to the popup, and what i got was the above order. So i'm not sure why you see the reportError executed after :\

Comment 37

[Dão Gottwald [:dao]]

Comment on attachment 394001 [details] [diff] [review]
patch 3.2

The calculation of the indicator's position appears to be incorrect when there are other items on the toolbar.

>+          <xul:hbox pack="start">
>+            <xul:image class="toolbar-drop-indicator"
>+                       mousethrough="always"
>+                       collapsed="true"/>
>+          </xul:hbox>

This stretches the indicator vertically. You need to use align="center".
Why did you add pack="start"?

>         else {
>           // Dragging over a normal toolbarbutton,
>           // show indicator bar and move it to the appropriate drop point.
>+          var ind = this._dropIndicator;
>+          var halfInd = ind.getBoundingClientRect().width / 2;
>+          var direction = document.defaultView
>+                                  .getComputedStyle(this.parentNode, "")
>+                                  .direction;
>+          var translateX;

These should also be let.

Comment 38

[Marco Bonardo [:mak]]

Attachment: patch v3.3

(In reply to comment #37)
> (From update of attachment 394001 [details] [diff] [review])
> The calculation of the indicator's position appears to be incorrect when there
> are other items on the toolbar.

oh, nice catch.
I tried to add a toolbarbutton on the left and one on the right and the results are now correct. I also tried to move the home button on the toolbar, and everything looks fine.

> >+          <xul:hbox pack="start">
> >+            <xul:image class="toolbar-drop-indicator"
> >+                       mousethrough="always"
> >+                       collapsed="true"/>
> >+          </xul:hbox>
> 
> This stretches the indicator vertically. You need to use align="center".
> Why did you add pack="start"?

i merely copied that from your first patch for the tabbar and forget to check it.
Btw, i don't know if the effect was due to the stretching, but the indicator was looking better to me, clearer and crisper. I suppose that's because the dotted blue line was appearing like a continue line, and that was looking really cleaner and more similar to the tabbar drop indicator style. Maybe we could investigating replacing the image, i don't know why originally that was done as a dotted line. What do you think?

Comment 39

[Dão Gottwald [:dao]]

(In reply to comment #38)
> > This stretches the indicator vertically. You need to use align="center".
> > Why did you add pack="start"?
> 
> i merely copied that from your first patch for the tabbar and forget to check
> it.

I think my patches always used align="start", not pack.

> Maybe we
> could investigating replacing the image, i don't know why originally that was
> done as a dotted line. What do you think?

The dotted line looks fine to me. I'm not sure if a solid one would look better. Maybe ask Stephen or Alex about it...

Comment 40

[Dão Gottwald [:dao]]

Comment on attachment 394248 [details] [diff] [review]
patch v3.3

There's overflow: hidden; for hbox[type="places"] in places.css. That needs to be removed in order to prevent the indicator from being cut off.

> > mousethrough="always" can be removed, I think.
> 
> removing it causes indicator flickering when the mouse is dragging over the
> indicator.

I can't reproduce this. Please make sure it's really needed.

>+          <xul:scrollbox orient="horizontal"
>+                         class="bookmarks-toolbar-items"
>+                         style="min-width: 1px"

What's the min-width achieving? Can this be removed?

>+        this._scrollbox.addEventListener("overflow", this, false);
>+        this._scrollbox.addEventListener("underflow", this, false);
>+        window.addEventListener("resize", this, false);

Also remove these event listeners in the destructor?

>+      <field name="_chevron">
>+        document.getAnonymousElementByAttribute(this, "class", "chevron")
>+      </field>

Rather than doing this._chevron.lastChild over and over again, I think a _chevronPopup field should be added.

>+      <field name="_isRTL">
>+        document.defaultView.getComputedStyle(this.parentNode, "")
>+                                     .direction == "rtl"
>+      </field>

.direction is strangely indented.

>+      <method name="onChevronPopupShowing">

That name should probably start with an underscore.

>+          if (this._updateChevronTimer) {
>+            this._updateChevronTimer.cancel();
>+            this._updateChevronTimer = null;
>           }
>+          this._updateChevronTimer = this._setTimer(100);

this._updateChevronTimer = null; is redundant.

>+                // If the chevron popup is open, maintain it in sync.

s/maintain/keep/

>+          // This function returns informations about where to drop when

s/informations/information/

>+                let beforeIndex =
>+                  nodeIndex == this.childNodes.length - 1 ? -1 : nodeIndex + 1;

This could use some brackets, e.g. (nodeIndex == this.childNodes.length - 1)

Comment 41

[Marco Bonardo [:mak]]

Attachment: patch v3.4

(In reply to comment #40)
> > > mousethrough="always" can be removed, I think.
> > 
> > removing it causes indicator flickering when the mouse is dragging over the
> > indicator.
> 
> I can't reproduce this. Please make sure it's really needed.

as soon as i remove it, i see the flickering, so it's really needed

fixed all other comments.

Comment 42

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/518bfcb14683

Comment 43

[u88484]

Marco, is this going to land on the 1.9.2 branch?

Comment 44

[Marco Bonardo [:mak]]

(In reply to comment #43)
> Marco, is this going to land on the 1.9.2 branch?

after some baking, if no problems are reported, i think would make sense

Comment 45

[Marco Bonardo [:mak]]

Attachment: rollup patch for 1.9.2

Rollup patch including bug 508816

This patch greatly reduces work needed to update the bookmarks toolbar, produces cleaner code, and solves 2 common-issues often reported by users.

Comment 46

[Marco Bonardo [:mak]]

(In reply to comment #45)
> Rollup patch including bug 508816

oops, i meant including bug 510509.

Comment 47

[[:Cww]]

bug 447571 (fixed by this patch) can look like dataloss and is definitely buggy.  If this patch is low-risk, can it make 1.9.1 as well?

If not, can we make sure there's at least a relnote?

Comment 48

[Daniel Veditz [:dveditz]]

patches this large are generally not "low-risk". Maybe ask again after it's landed in 1.9.2 and gotten some beta testing.

Comment 49

[Samuel Sidler (old account; do not CC)]

... and by the time we get beta testing, 1.9.2 will be close to shipping and we likely will have run out of time to ship this in a 1.9.1.x release before 1.9.2 (a minor update, remember) ships. That makes this wontfix on the 1.9.1 branch.

Comment 50

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 395030 [details] [diff] [review]
rollup patch for 1.9.2

a192=beltzner

Comment 51

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bb13c127a460

Comment 52

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv