754695 - [Feature request] preference to control bindings load order

Status: RESOLVED WONTFIX

(Core :: XBL, enhancement)

Description

[lekma]

Attachment: control order of bindings load order

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120423122843

Steps to reproduce:

Currently bindings are loaded in reverse 'natural' order ('natural' order being the order in which they appear in a file (sorry I couldn't find a better word than 'natural').
The attached patch adds a boolean preference "layout.bindings.natural_load_order" (default: false, to preserve current behavior) which, if set to true, loads sibling bindings in the order they appear in file.

Comment 1

[Boris Zbarsky [:bzbarsky]]

When is this pref useful, exactly?  Seems like setting it would break the world (and in particular, break all sorts of XUL widgets that depend on the current order)...

Comment 2

[lekma]

Well, I've been testing this for a week now and didn't run into problems from toolkit widgets (it seems none of them rely on sibling bindings being loaded before or after themselves (I didn't test them all though)).

It's useful for an extension I'm writing (a widget wrapping a scripting language runtime). The thing is, if a user writes:

<binding src="script1" />
<binding src="script2" />

it seems reasonable to expect script1 to run before script2, isn't it? and this patch is the easiest way I found to do just that (I'm open to better suggestions/ideas) .

Comment 3

[Boris Zbarsky [:bzbarsky]]

You're changing parent/child order, not just sibling order, unless I'm missing something.

In case you care, the last time the order here got accidentally changed, we quickly ended up with bug 526178, which is what the code you're changing is all about.  As far as I can tell, your patch with the pref flipped would regress that bug.

Now you could make the argument that the order should be depth-first preorder with child lists walked forwards.  But it'd need to be a general change to help your extension, not a pref, since flipping the pref would likely break other extensions and almost certainly at least some toolkit widgets, your testing notwithstanding, so your extension would not be able flip the pref anyway.

As for your actual use case, I'm not quite sure what a good solution is.  You could watch for insertions of your elements and run the scripts, without using XBL, perhaps?

Comment 4

[lekma]

Attachment: control bindings load order and preserve parent/child ctor order

(In reply to Boris Zbarsky (:bz) from comment #3)
> You're changing parent/child order, not just sibling order, unless I'm
> missing something.
You're right, I completely missed the parent/children relationship (don't ask me why, but I was convinced it was taken care in the binding manager). Sorry.

> 
> In case you care, the last time the order here got accidentally changed, we
> quickly ended up with bug 526178, which is what the code you're changing is
> all about.  As far as I can tell, your patch with the pref flipped would
> regress that bug.
Yep (again, sorry).

> 
> Now you could make the argument that the order should be depth-first
> preorder with child lists walked forwards.  But it'd need to be a general
> change to help your extension, not a pref, since flipping the pref would
> likely break other extensions and almost certainly at least some toolkit
> widgets, your testing notwithstanding, so your extension would not be able
> flip the pref anyway.
This new patch keeps the pref (renamed to layout.bindings.forward_load_order): when the pref is false (default) nothing is changed, when it is true only sibling bindings are reordered.

Given a binding defined as:
<parentBinding>
  <content>
    <childBinding name="1" />
    <childBinding name="2" />
  </content>
</parentBinding>

and a xul file where you have:
<parentBinding name="A" />
<parentBinding name="B" />

Right now you get:
B
2
1
A
2
1

With the patch and the pref flipped to true, you get:
A
1
2
B
1
2

And I would also make the case that this seems more appropriate/logical even for the default behavior (but I don' want to upset anyone with unforeseen collateral damages, hence the pref).
> 
> As for your actual use case, I'm not quite sure what a good solution is. 
> You could watch for insertions of your elements and run the scripts, without
> using XBL, perhaps?
I tried that and it opened a whole new can of worms (being in an observer meant I couldn't propagate exceptions back to JavaScript correctly, etc.).

Anyway, if/when you get the time I'd be happy to hear your comments on this new approach.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> but I don' want to upset anyone with unforeseen collateral damages

See, this is the part I don't understand.  Installing your extension would flip the pref, right?  So it would in fact cause precisely such unforeseen collateral damage...

Comment 6

[lekma]

(In reply to Boris Zbarsky (:bz) from comment #5)
> > but I don' want to upset anyone with unforeseen collateral damages
> 
> See, this is the part I don't understand.  Installing your extension would
> flip the pref, right?  So it would in fact cause precisely such unforeseen
> collateral damage...

Well my extension isn't for firefox nor for any widely spread mozilla application, it's for a xulrunner application (a learning music thingy), that I'm developing as well, so I kinda can control the damages here.

But even if it were not the case, I'm just being overly cautious here. AFAICT (and fortunately) no toolkit widget cares if a sibling is loaded before or after them or if sibling children are loaded one before the other (unfortunately I cannot seem to be able to come up with a mechanical way to check them all), and if that would happen, it'd only be a matter of flipping position between bindings back to the (correct, might I add) order. That's why I think we can live with the pref for a little while and see what comes up (other extensions don't have to know about this pref for the time being :)).

Comment 7

[Boris Zbarsky [:bzbarsky]]

> it's for a xulrunner application (a learning music thingy), that I'm developing as well

Ah, ok.  In that case, I guess the pref approach makes sense.  I'll take a look at the patch later tonight, hopefully!

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 625616 [details] [diff] [review]
control bindings load order and preserve parent/child ctor order

I think the mCurrentIsParent setup is wrong.

Consider this DOM:

  <div id="A>
    <div id="1">
      <div id="a"></div>
    </div>
    <span id="2">
      <span id="b"><span>
    </span>
    <span id="3"></span>
  </div>

where all the nodes have XBL constructors. The order of operations will be (with the resulting list in the sForwardOrder case in parens, and the resulting values of mCurrentIsParent and mCurrentPendingBindingInsertionPoint in square brackets):

1)  AddPendingBinding A  (A)                [false, listhead]
2)  Push A               (A)                [true, A]
3)  AddPendingBinding 1  (1, A)             [true, 1]
4)  AddPendingBinding 2  (2, 1, A)          [true, 2]
5)  Push 2               (2, 1, A)          [true, 2]
6)  AddPendingBinding b  (b, 2, 1, A)       [true, b]
7)  Push b               (b, 2, 1, A)       [true, b]
8)  Pop b                (b, 2, 1, A)       [false, b]
9)  Pop 2                (b, 2, 1, A)       [false, 2]
10) AddPendingBinding 3  (b, 2, 3, 1, A)    [false, 2]
11) Push 3               (b, 2, 3, 1, A)    [true, 3]
12) Pop 3                (b, 2, 3, 1, A)    [false, 2]
13) Pop A                (b, 2, 3, 1, A)    [false, listhead]
14) Push 1               (b, 2, 3, 1, A)    [true, 1]
15) AddPendingBinding a  (b, 2, 3, a, 1, A) [true, a]
16) Push a               (b, 2, 3, a, 1, A) [true, a]
17) Pop a                (b, 2, 3, a, 1, A) [false, a]
18) Pop 1                (b, 2, 3, a, 1, A) [false, listhead]

Now we run the constructors in reverse-list order, so they run in the order:

  A, 1, a, 3, 2, b.

whereas I would think you're aiming for:

  A, 1, a, 2, b, 3.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The key part being that we process kids of inlines before moving on to their siblings, for purposes of this stuff, which is why the <span> bit is relevant.

Comment 10

[lekma]

(In reply to Boris Zbarsky (:bz) from comment #8)
> whereas I would think you're aiming for:
> 
>   A, 1, a, 2, b, 3.
Which is exactly what I get...

Would you consider the following bindings good enough for this test?:
<binding id="myDiv">
  <content>
    <xul:box style="display: block;" />
  </content>
  <implementation>
    <constructor>
    <![CDATA[
      dump("div: " + this.getAttribute("id") + "\n");
    ]]>
    </constructor>
  </implementation>
</binding>

<binding id="mySpan">
  <content>
    <xul:box style="display: inline;" />
  </content>
  <implementation>
    <constructor>
    <![CDATA[
      dump("span: " + this.getAttribute("id") + "\n");
    ]]>
    </constructor>
  </implementation>
</binding>

if yes, then when I use them like this:
<myDiv id="A">
  <myDiv id="1">
    <myDiv id="a"></myDiv>
  </myDiv>
  <mySpan id="2">
    <mySpan id="b"></mySpan>
  </mySpan>
  <mySpan id="3"></mySpan>
</myDiv>

I get the following dump:
div: A
div: 1
div: a
span: 2
span: b
span: 3

So unless I misunderstood/missed something it seems to be on target (I'll dig a little bit deeper for an actual explanation (if I ever find one :))).

Comment 11

[lekma]

All right, I did miss something. If I apply the style in the css instead of in the binding I get what you're describing...

Comment 12

[Boris Zbarsky [:bzbarsky]]

Right; the styles in the bindings were applied to random <xul:box> kids, not to the elements themselves.

Comment 13

[lekma]

Attachment: control bindings load order and preserve parent/child ctor order

(In reply to Boris Zbarsky (:bz) from comment #12)
> Right; the styles in the bindings were applied to random <xul:box> kids, not
> to the elements themselves.
Yes, I was being dumb, sorry :( (thank you for staying with me thus far).

Anyway, I have a fix for that case, but I'd like to ask you a few questions:
1) Do you see any other case, beside display, where reordering might be a problem? Also, I tested my fix with display=none/block/inline/inline-block, but assumed that all the other values would behave like either one of these, is that a safe assumption?
2) Do you mind me renaming mCurrentPendingBindingInsertionPoint to mCurrentPendingBinding?
3) Is there a global init func somewhere where I could initialize the pref as a global instead of using it as a static local?

Again, I'd like to thank you for your help and patience.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> 1) Do you see any other case, beside display, where reordering might be a problem?

Right this second, I think not, but in general the CSS specs can add something any day that would require that sort of behavior.  Furthermore, we might switch to that sort of behavior for other display types various reasons (e.g. for better parallelizability).  We've even been thinking about making that determination dynamically, depending on various heuristics.

So I would really rather not depend on this being an inline-specific behavior.

> 2) Do you mind me renaming mCurrentPendingBindingInsertionPoint to mCurrentPendingBinding?

I think so, yeah.  That member indicates where new bindings should be inserted; it's a current insertion point for pending bindings.  There are no pending bindings being current there...

> 3) Is there a global init func somewhere where I could initialize the pref as a global
> instead of using it as a static local?

Sure.  You could add an nsCSSFrameConstructor::Init static and call it from nsLayoutStatics::Initialize in layout/build/nsLayoutStatics.cpp.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Another option, of course, is to just accept the bug with inlines and document it...

Comment 16

[Boris Zbarsky [:bzbarsky]]

But that won't help if we start doing more depth-first stuff on boxes.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 628261 [details] [diff] [review]
control bindings load order and preserve parent/child ctor order

Per comment 14.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm really not convinced this is something that we want to do, even as a preference. This part of XBL and XUL is very fragile and I'm concerned that if we add a branched code-path here we'll end up having to add it in other places too. For example make some XUL bindings do things differently if the pref is set in order to not break.

I'd be more interested in doing this if XBL was a technology which we were planning on exposing to the web. However we've been going in the opposite direction and hopefully XBL will one day be replaced by WebComponents or something like it.

So adding more code here which we'll have to support for an unknown length of time doesn't seem worth it.

Comment 19

[lekma]

(In reply to Jonas Sicking (:sicking) from comment #18)
> I'm really not convinced this is something that we want to do, even as a
> preference. This part of XBL and XUL is very fragile and I'm concerned that
> if we add a branched code-path here we'll end up having to add it in other
> places too. For example make some XUL bindings do things differently if the
> pref is set in order to not break.
> 
> I'd be more interested in doing this if XBL was a technology which we were
> planning on exposing to the web. However we've been going in the opposite
> direction and hopefully XBL will one day be replaced by WebComponents or
> something like it.
> 
> So adding more code here which we'll have to support for an unknown length
> of time doesn't seem worth it.
Hmm, and here I was, thinking I could use Mozilla for something funny (i.e. not web related)...

Comment 20

[lekma]

Attachment: control bindings load order

(In reply to Boris Zbarsky (:bz) from comment #14)
> > 3) Is there a global init func somewhere where I could initialize the pref as a global
> > instead of using it as a static local?
> 
> Sure.  You could add an nsCSSFrameConstructor::Init static and call it from
> nsLayoutStatics::Initialize in layout/build/nsLayoutStatics.cpp.
Unfortunately this happens too early (and it would make the all thing much more complicated if we were to do it right).

Well, given the discouraging comment from Jonas, if you don't think it's worth pursuing, please close this bug (in this case I'll try and find another toolkit to play with, any recommendation?)

Thanks, again :), for your help and patience.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Couldn't you simply work around the admittedly weird binding order? That's what we've successfully done in the Firefox and thunderbird UI for quite a while now.

Comment 22

[Boris Zbarsky [:bzbarsky]]

That would honestly be my suggestion too, if at all possible.  I'm really not happy with the amount of complication we're ending up with here.  :(

Comment 23

[Boris Zbarsky [:bzbarsky]]

In fact, I'm just going to make that call....