752770 - re-enable 'arguments' portion of browser_dbg_propertyview-07 and 08

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Luke Wagner [:luke]]

Attachment: disabled tests

In bug 746601 I made a change to how 'arguments' is exposed to the debugger.  This apparently changed the enumeration order which broke browser_dbg_propertyview-07.js and 08.  These tests make assumptions about where 'arguments' will be in the list as well as where 'length' will be in the list of arguments's properties.  Instead of just poking around to use different magic constants, I think the tests need to be fixed to not depend on these internal details.  The attached patch shows the tests I had to comment out.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 621830 [details] [diff] [review]
disabled tests

In the meantime, it's not trivial to fix these tests, so could I temporarily disable them (assuming I patch in this bug # to the patch)?

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 621830 [details] [diff] [review]
disabled tests

Oops, didn't see bug 690135 comment 32.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

Luke, when I apply the patch from bug 746601 (fix arguments) on top of the patch from bug 690135 (combined for Panos) I get lots of conflicts. What is the proper way to get a working patch queue?

Comment 4

[Luke Wagner [:luke]]

Sorry for not being more clear: all you need is the (combined for Panos) patch.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch

This patch removes the workaround of using 'frame.arguments', now that the environment bindings always contain an 'arguments' property.

Luke, we just need to make sure that both this patch and yours land in the same push, to avoid test breakage.

Comment 6

[Luke Wagner [:luke]]

Fantastic, thanks!  If it sounds good to you, I'll push this with bug 746601.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Luke Wagner [:luke] from comment #6)
> Fantastic, thanks!  If it sounds good to you, I'll push this with bug 746601.

Sure. I assume you'll push to m-i, whereas we usually work off fx-team, and I wonder if we should coordinate a subsequent merge of m-c to fx-team, to avoid any merge conflicts. Of course Rob would know better.

Comment 8

[Rob Campbell [:rc] (:robcee)]

(In reply to Luke Wagner [:luke] from comment #6)
> Fantastic, thanks!  If it sounds good to you, I'll push this with bug 746601.

sure thing. This is ready-to-land. When you've done so, I'll keep an eye on m-c to watch for your merge and pull it over.

Thanks!

Comment 9

[Luke Wagner [:luke]]

Great; I'll land it and the m-i/m-c csets will be posted here.

Comment 10

[Victor Porof [:vporof][:vp]]

Since the Array hack for frame.arguments was removed, isn't it good to rewrite the monstrous if in SF__addExpander?

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch v2

(In reply to Victor Porof from comment #10)
> Since the Array hack for frame.arguments was removed, isn't it good to
> rewrite the monstrous if in SF__addExpander?

Yes, I forgot about that, but only the last isArray() check can go. The rest is necessary to avoid arrows in null, etc. Patch updated with the trivial change.

Comment 12

[Luke Wagner [:luke]]

So I tried out the patch in comment 11 (with bug 746601 applied) but even more tests seem to fail.  In the interest of time (this is blocking IonMonkey big time), I just landed the original patch which disables the 'arguments' portion of the tests:

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3395398d9f3

so that the tests can be fixed independently.  (Btw, debugger-only 'arguments' is being tested in a number of jsdbg shell tests.)

Comment 13

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Luke Wagner [:luke] from comment #12)
> So I tried out the patch in comment 11 (with bug 746601 applied) but even
> more tests seem to fail.  In the interest of time (this is blocking
> IonMonkey big time), I just landed the original patch which disables the
> 'arguments' portion of the tests:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f3395398d9f3
> 
> so that the tests can be fixed independently.  (Btw, debugger-only
> 'arguments' is being tested in a number of jsdbg shell tests.)

OK thanks, I'll take a look on Monday.

Comment 14

[Luke Wagner [:luke]]

Great; sorry for the trouble.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

(In reply to Luke Wagner [:luke] from comment #12)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f3395398d9f3

https://hg.mozilla.org/mozilla-central/rev/f3395398d9f3

Comment 16

[Panos Astithas (he/him) [:past] (please ni?)]

Attachment: Working patch v3

Apparently the tests didn't need any changes, besides uncommenting the commented-out parts. I sent it to try, just to be on the safe side, though.

Comment 17

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/integration/fx-team/rev/157fa37cea55

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/157fa37cea55