Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(5)

Issue 11414094: Make Object.observe on the global object functional (Closed)

Created:
8 years, 1 month ago by adamk
Modified:
8 years ago
CC:
v8-dev, rafaelw, arv (Not doing code reviews), abarth-chromium
Visibility:
Public.

Description

Make Object.observe on the global object functional The approach in this change is to handle the unwrapping/wrapping of the global object transparently with respect to the JS implementation of Object.observe. An alternate approach would be to add a runtime method like %IsJSGlobalProxy and %UnwrapJSGlobalProxy, but it seems ugly to give JS (even implementation JS) access to the unwrapped global. BUG=v8:2409 Committed: https://code.google.com/p/v8/source/detail?r=13142

Patch Set 1 #

Total comments: 11

Patch Set 2 : Review comments addressed #

Patch Set 3 : Added API test #

Patch Set 4 : Test with reattaching to a different context #

Total comments: 4

Patch Set 5 : Add test for attaching via Context::New #

Unified diffs Side-by-side diffs Delta from patch set Stats (+96 lines, -18 lines) Patch
M src/object-observe.js View 1 1 chunk +1 line, -1 line 0 comments Download
M src/objects.cc View 1 2 3 4 1 chunk +3 lines, -0 lines 0 comments Download
M src/runtime.h View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M src/runtime.cc View 1 2 3 4 4 chunks +20 lines, -10 lines 0 comments Download
M test/cctest/test-object-observe.cc View 1 2 3 4 1 chunk +60 lines, -0 lines 0 comments Download
M test/mjsunit/harmony/object-observe.js View 1 2 3 4 5 chunks +12 lines, -6 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
adamk
8 years, 1 month ago (2012-11-20 23:07:51 UTC) #1
rossberg
I think this is the right approach. However, it'd be good to have some API-side ...
8 years, 1 month ago (2012-11-21 12:43:17 UTC) #2
adamk
Will work on API tests for this today. And will try to simulate the way ...
8 years ago (2012-11-21 17:02:57 UTC) #3
adamk
Added an API test with a call to DetachGlobal. Demonstrates the behavior, but I'm not ...
8 years ago (2012-11-21 23:27:28 UTC) #4
rossberg
I think the most important case to test for would be one that reuses a ...
8 years ago (2012-11-22 12:17:05 UTC) #5
adamk
On 2012/11/22 12:17:05, rossberg wrote: > I think the most important case to test for ...
8 years ago (2012-11-26 18:43:53 UTC) #6
abarth-chromium
https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-observe.cc File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-observe.cc#newcode244 test/cctest/test-object-observe.cc:244: CHECK(inner_global->StrictEquals(CompileRun("records[1].object"))); So, records[1].object no longer points to the global_proxy? ...
8 years ago (2012-11-26 22:53:07 UTC) #7
abarth-chromium
adamk and I talked through this topic and his understanding of how things work on ...
8 years ago (2012-11-26 22:54:02 UTC) #8
rossberg
Great, LGTM. https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-observe.cc File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-observe.cc#newcode258 test/cctest/test-object-observe.cc:258: context2->ReattachGlobal(global_proxy); Is this equivalent to calling Context::New ...
8 years ago (2012-12-03 13:26:18 UTC) #9
adamk
https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-observe.cc File test/cctest/test-object-observe.cc (right): https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-observe.cc#newcode258 test/cctest/test-object-observe.cc:258: context2->ReattachGlobal(global_proxy); On 2012/12/03 13:26:18, rossberg wrote: > Is this ...
8 years ago (2012-12-03 19:23:20 UTC) #10
rossberg
8 years ago (2012-12-05 11:52:26 UTC) #11
On 2012/12/03 19:23:20, adamk wrote:
>
https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
> File test/cctest/test-object-observe.cc (right):
> 
>
https://codereview.chromium.org/11414094/diff/6/test/cctest/test-object-obser...
> test/cctest/test-object-observe.cc:258:
context2->ReattachGlobal(global_proxy);
> On 2012/12/03 13:26:18, rossberg wrote:
> > Is this equivalent to calling Context::New with a given global object?
(That's
> > what the WebKit bindings do, I believe.)
> 
> Added an explicit test for that codepath as it's a bit different.

Excellent, thanks!

Powered by Google App Engine
This is Rietveld 408576698