Last Comment Bug 752770 - re-enable 'arguments' portion of browser_dbg_propertyview-07 and 08
: re-enable 'arguments' portion of browser_dbg_propertyview-07 and 08
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Debugger (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: Firefox 15
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on:
Blocks: 746601
  Show dependency treegraph
 
Reported: 2012-05-07 18:19 PDT by Luke Wagner [:luke]
Modified: 2012-05-22 06:24 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
disabled tests (3.88 KB, patch)
2012-05-07 18:19 PDT, Luke Wagner [:luke]
no flags Details | Diff | Review
Working patch (9.03 KB, patch)
2012-05-10 02:42 PDT, Panos Astithas [:past]
rcampbell: review+
Details | Diff | Review
Working patch v2 (9.48 KB, patch)
2012-05-11 05:45 PDT, Panos Astithas [:past]
no flags Details | Diff | Review
Working patch v3 (8.88 KB, patch)
2012-05-21 05:47 PDT, Panos Astithas [:past]
no flags Details | Diff | Review

Description Luke Wagner [:luke] 2012-05-07 18:19:13 PDT
Created attachment 621830 [details] [diff] [review]
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] 2012-05-09 09:31:17 PDT
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] 2012-05-09 09:40:26 PDT
Comment on attachment 621830 [details] [diff] [review]
disabled tests

Oops, didn't see bug 690135 comment 32.
Comment 3 Panos Astithas [:past] 2012-05-10 00:35:46 PDT
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] 2012-05-10 00:40:10 PDT
Sorry for not being more clear: all you need is the (combined for Panos) patch.
Comment 5 Panos Astithas [:past] 2012-05-10 02:42:37 PDT
Created attachment 622662 [details] [diff] [review]
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] 2012-05-10 08:10:17 PDT
Fantastic, thanks!  If it sounds good to you, I'll push this with bug 746601.
Comment 7 Panos Astithas [:past] 2012-05-10 08:32:05 PDT
(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) 2012-05-10 10:43:08 PDT
(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] 2012-05-10 11:01:54 PDT
Great; I'll land it and the m-i/m-c csets will be posted here.
Comment 10 Victor Porof [:vporof][:vp] 2012-05-11 04:05:03 PDT
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 [:past] 2012-05-11 05:45:42 PDT
Created attachment 623111 [details] [diff] [review]
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] 2012-05-18 12:23:15 PDT
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 [:past] 2012-05-18 12:59:58 PDT
(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] 2012-05-18 13:39:58 PDT
Great; sorry for the trouble.
Comment 15 Ryan VanderMeulen [:RyanVM] 2012-05-18 18:42:13 PDT
(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 [:past] 2012-05-21 05:47:02 PDT
Created attachment 625618 [details] [diff] [review]
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 [:past] 2012-05-21 23:35:00 PDT
https://hg.mozilla.org/integration/fx-team/rev/157fa37cea55
Comment 18 Tim Taubert [:ttaubert] 2012-05-22 06:24:53 PDT
https://hg.mozilla.org/mozilla-central/rev/157fa37cea55

Note You need to log in before you can comment on or make changes to this bug.