1313765 - Make Reps work with Proxy

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect)

Description

[Tim Nguyen :ntim]

The old output does that without any specific rep for Proxy, so it might be worth investigating why the default rep currently does not work for Proxy.

var p = new Proxy({}, () => {}); p;

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1313765 - Make Rep handles Proxy. ;

In the GripRep, we were using the root property `ownPropertyLength` to
retrieve additionnal uninterresting props on the Grip. For Proxy
objects, we don't have this property, but a `preview.ownPropertiesLength`,
which also exists on the other grips.
Since we are already checking that `grip.preview` exist in `supportObject`,
we can use this directly.
This adds a test case similar to the one for Proxy in browser_webconsole_output_05.js.

Review commit: https://reviewboard.mozilla.org/r/89644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/89644/

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8806149 [details]
Bug 1313765 - Make Rep handles Proxy. ;

https://reviewboard.mozilla.org/r/89644/#review89290

I am seeing a test failure on Try:

TEST-UNEXPECTED-FAIL | devtools/client/shared/components/test/mochitest/test_reps_event.html | Event rep has expected text content for a message event - got "MessageEvent { isTrusted: false, data: \"test data\", 7 more… }", expected "MessageEvent { isTrusted: false, data: \"test data\", origin: \"null\", 7 more… }"

Why the 'origin' field is now missing?


Honza

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8806149 [details]
Bug 1313765 - Make Rep handles Proxy. ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/89644/diff/1-2/

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

I made some false assumptions and it's why this test fails. I fixed it, and ran the whole reps' test suite, which now pass.

One thing bothers me, aren't the Reps' tests running on TRY ? I have a run where I can't see any error on the test, which indeed was failing locally https://treeherder.mozilla.org/#/jobs?repo=try&revision=cffdbeccf2fc .

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8806149 [details]
Bug 1313765 - Make Rep handles Proxy. ;

https://reviewboard.mozilla.org/r/89644/#review89590

R+ assuming try is green.

Thanks!
Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)
> One thing bothers me, aren't the Reps' tests running on TRY ? I have a run
> where I can't see any error on the test, which indeed was failing locally
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cffdbeccf2fc .

This is weird they should be running on Try.
Here is a test push with an error, let's see if it's visible.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eaba1987502d4630e93c29e48f6859a78f55790

Honza

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Jan Honza Odvarko [:Honza] from comment #6)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #4)
> > One thing bothers me, aren't the Reps' tests running on TRY ? I have a run
> > where I can't see any error on the test, which indeed was failing locally
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cffdbeccf2fc .
> 
> This is weird they should be running on Try.
> Here is a test push with an error, let's see if it's visible.
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=7eaba1987502d4630e93c29e48f6859a78f55790
> 
> Honza

The TRY run you point too contains the fixed version of the patch ( https://hg.mozilla.org/try/diff/4241815ac1ad/devtools/client/shared/components/reps/grip.js ) and not the faulty one ( https://hg.mozilla.org/try/diff/006f54eaa79c/devtools/client/shared/components/reps/grip.js ), so, if I did my job right, we should not see jobs failing. Anyway, I'll check in the raw logs of dt-* tests to check if reps tests where executed, which does not seem the case in https://treeherder.mozilla.org/#/jobs?repo=try&revision=cffdbeccf2fc .

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Ah, I did an explicit change in testProxy() method (modified & wrong defaultOutput), but it didn't propagate to Try.

Here is a new push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78525cf44f50b5529e4fcf2b1a1312b279770a74

I am curious to see what happens.
Honza

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

From IRC:

linclark Honza: nchevobbe I think they are in the oth suite
linclark Honza: mochitest-dt only runs browser mochitests IIRC


Honza

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

TRY is looking fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1b200770eb1

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d897730b70c1
Make Rep handles Proxy. r=Honza;

Comment 12

[Phil Ringnalda (:philor)]

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