[CachedAccessor] for window.document.

[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.

Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/

Depends on V8 CL:
https://codereview.chromium.org/2405213002/

BUG=chromium:634276
Committed: https://crrev.com/460ec9e9068eb8bf57ce3fae68bb277ca4106f9a
Cr-Commit-Position: refs/heads/master@{#430551}

[vogelheim, 2016-10-19 13:45:57]

Patchset #2 (id:20001) has been deleted

[vogelheim, 2016-10-19 13:46:03]

Patchset #1 (id:1) has been deleted

[vogelheim, 2016-10-19 13:58:38]

Description was changed from

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.


Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/


BUG=chromium.634276
==========

to

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.


Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/


BUG=chromium:634276
==========

[vogelheim, 2016-10-19 14:01:23]

Patchset #1 (id:40001) has been deleted

[vogelheim, 2016-10-19 14:01:33]

Patchset #1 (id:60001) has been deleted

[vogelheim, 2016-10-19 14:02:03]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 14:02:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vogelheim, 2016-10-19 14:03:00]

Description was changed from

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.


Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/


BUG=chromium:634276
==========

to

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.


Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/

Depends on V8 CL:
https://codereview.chromium.org/2405213002/


BUG=chromium:634276
==========

[commit-bot: I haz the power, 2016-10-19 14:16:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 14:16:01]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[vogelheim, 2016-10-20 08:09:54]

vogelheim@chromium.org changed reviewers:
+ haraken@chromium.org, jochen@chromium.org

[vogelheim, 2016-10-20 08:09:55]

ptal.

This is the follow-up to crrev.com/2335203006. For easier review, I split up the
patch sets into the original change; updated test expectations; and fix(es)
relative to the original change.

Try-bots are presently red because compilation will fail until the V8 companion
CL crrev.com/2405213002 is in, which should be shortly.

[haraken, 2016-10-20 08:22:23]

LGTM

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:246: isolate,
getterCallback, nullptr, data, v8::Local<v8::Signature>(),

nullptr => cachedAccessorCallback?

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:66: typedef
v8::experimental::FastAccessorBuilder* (*FastAccessorBuilderCallback)(

Is this typedef needed?

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:83: //
TODO(peterssen|vogelheim): This has to be removed too since it's only

Remove peterssen (unless he has a plan to work on it)?

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:85:
CachedAccessorCallback cachedAccessorCallback;  // The accessor's 'result'

Move up the comment.

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:126:
CachedAccessorCallback cachedAccessorCallback;  // The accessor's 'result'

Ditto.

[jochen (gone - plz use gerrit), 2016-10-20 08:44:42]

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:423:
CHECK(V8PrivateProperty::getWindowDocumentCachedAccessor(m_isolate).set(
how does this work for isolated worlds? This method (updateDocumentProperty)
bails out for non-main-worlds

[vogelheim, 2016-10-20 13:27:59]

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:246: isolate,
getterCallback, nullptr, data, v8::Local<v8::Signature>(),
On 2016/10/20 08:22:23, haraken wrote:
> 
> nullptr => cachedAccessorCallback?

Well... not sure. :-/

Since this is case is presently not used (and thus also untested), I'd prefer to
not add the 'cache' here until we actually have a use for. There's no 'hard'
reason, I'm just trying to be careful since there've been several somewhat
subtle bugs with this functionality so far.

Does this make sense?

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:66: typedef
v8::experimental::FastAccessorBuilder* (*FastAccessorBuilderCallback)(
On 2016/10/20 08:22:23, haraken wrote:
> 
> Is this typedef needed?

Removed.

(That was a leftover from dis-entagling the original mega-CL for several pieces
of functionality.)

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:83: //
TODO(peterssen|vogelheim): This has to be removed too since it's only
On 2016/10/20 08:22:23, haraken wrote:
> 
> Remove peterssen (unless he has a plan to work on it)?

Done.

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:85:
CachedAccessorCallback cachedAccessorCallback;  // The accessor's 'result'
On 2016/10/20 08:22:23, haraken wrote:
> 
> Move up the comment.

Done.

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.h:126:
CachedAccessorCallback cachedAccessorCallback;  // The accessor's 'result'
On 2016/10/20 08:22:23, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp (right):

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/WindowProxy.cpp:423:
CHECK(V8PrivateProperty::getWindowDocumentCachedAccessor(m_isolate).set(
On 2016/10/20 08:44:42, jochen wrote:
> how does this work for isolated worlds? This method (updateDocumentProperty)
> bails out for non-main-worlds

Yes.

main world:
- accessor gets created with hidden, 'cached' accessor
- value gets set here.
- the real & slow accessor still exists. any code paths that don't know about
cached accessors use the real accessor (with a speed penalty)

non-main worlds:
- the first two steps don't happen.
- all accessors use the 'true' accessor.

We could change that, if we think this penalizes non-main-worlds too much. But
that's how it works in this CL.

[haraken, 2016-10-20 13:40:33]

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/2429343004/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8DOMConfiguration.cpp:246: isolate,
getterCallback, nullptr, data, v8::Local<v8::Signature>(),
On 2016/10/20 13:27:59, vogelheim wrote:
> On 2016/10/20 08:22:23, haraken wrote:
> > 
> > nullptr => cachedAccessorCallback?
> 
> Well... not sure. :-/
> 
> Since this is case is presently not used (and thus also untested), I'd prefer
to
> not add the 'cache' here until we actually have a use for. There's no 'hard'
> reason, I'm just trying to be careful since there've been several somewhat
> subtle bugs with this functionality so far.
> 
> Does this make sense?

OK, let's keep nullptr here.

[jochen (gone - plz use gerrit), 2016-10-20 13:51:27]

lgtm

[haraken, 2016-10-24 12:55:43]

https://codereview.chromium.org/2429343004/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl (right):

https://codereview.chromium.org/2429343004/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:240: static
v8::Local<v8::Private> {{attribute.name}}CachedAccessorCallback(v8::Isolate*
isolate)

I'm just curious, but when is the CachedAccessorCallback called? Is it called
back only at the first time the accessor is accessed? Or is it called back every
time the accessor is accessed?

[haraken, 2016-10-24 13:07:47]

One more comment:

Can we implement a logic in Debug builds to fall back to a normal C++ getter and
confirm that the cached result is equal to the result returned by the normal C++
getter?

[haraken, 2016-11-02 02:37:58]

Do you have any timeline for landing this CL?

We want to move all Window's attributes to accessor-type properties by the end
of Q4.

[chromium-reviews, 2016-11-03 15:56:08]

Hi Kentaro,

The timeline is basically: 1) 2429343004 (v8) -> 2) 2405213002 (v8) -> 3)
2429343004 (blink)

1) landed on Monday, and seems to finally have stuck.
I'm currently working on 2). With some luck tomorrow, but more likely next
week.
3) would be next. I think 3) is largely uncontroversial, so it'll hopefully
be in next week as well.

This set of changes has had quite a few regressions & reverts,
unfrotunately, so there's a certain amount of risk and it's quite
conceivable we'll have another derail along the way. I'm hopeful, though...

Daniel

On Wed, Nov 2, 2016 at 3:37 AM, <haraken@chromium.org> wrote:

> Do you have any timeline for landing this CL?
>
> We want to move all Window's attributes to accessor-type properties by the
> end
> of Q4.
>
>
> https://codereview.chromium.org/2429343004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[blink-reviews, 2016-11-03 15:56:09]

Hi Kentaro,

The timeline is basically: 1) 2429343004 (v8) -> 2) 2405213002 (v8) -> 3)
2429343004 (blink)

1) landed on Monday, and seems to finally have stuck.
I'm currently working on 2). With some luck tomorrow, but more likely next
week.
3) would be next. I think 3) is largely uncontroversial, so it'll hopefully
be in next week as well.

This set of changes has had quite a few regressions & reverts,
unfrotunately, so there's a certain amount of risk and it's quite
conceivable we'll have another derail along the way. I'm hopeful, though...

Daniel

On Wed, Nov 2, 2016 at 3:37 AM, <haraken@chromium.org> wrote:

> Do you have any timeline for landing this CL?
>
> We want to move all Window's attributes to accessor-type properties by the
> end
> of Q4.
>
>
> https://codereview.chromium.org/2429343004/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[vogelheim, 2016-11-07 13:36:05]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 13:36:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vogelheim, 2016-11-07 14:59:39]

ptal (or let me know if the previous lgtms still hold)

The dependent V8 CLs are now in (and seem to have stuck for 3 days).

There's no substantial changes; only rebase + updated test expectations.
Unfortunately, the test expectations make the CL a bit hard to review; sorry.
Let me know if I can do anything to make it easier.

[haraken, 2016-11-07 15:02:48]

Still LGTM

[commit-bot: I haz the power, 2016-11-07 17:07:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 17:07:53]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jochen (gone - plz use gerrit), 2016-11-07 18:13:30]

lgtm

[vogelheim, 2016-11-07 18:15:22]

The CQ bit was checked by vogelheim@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 18:15:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-07 20:13:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-07 20:13:19]

Dry run: This issue passed the CQ dry run.

[vogelheim, 2016-11-08 09:03:36]

The CQ bit was checked by vogelheim@chromium.org

[vogelheim, 2016-11-08 09:03:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2429343004/#ps210001
(title: "Fix expectation for global-interface-listing-expected, windows.")

[commit-bot: I haz the power, 2016-11-08 09:03:58]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-08 09:09:13]

Description was changed from

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.


Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/

Depends on V8 CL:
https://codereview.chromium.org/2405213002/


BUG=chromium:634276
==========

to

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.


Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/

Depends on V8 CL:
https://codereview.chromium.org/2405213002/


BUG=chromium:634276
==========

[commit-bot: I haz the power, 2016-11-08 09:09:14]

Committed patchset #8 (id:210001)

[commit-bot: I haz the power, 2016-11-08 09:19:09]

Description was changed from

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.


Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/

Depends on V8 CL:
https://codereview.chromium.org/2405213002/


BUG=chromium:634276
==========

to

==========
[CachedAccessor] for window.document.

- Implemend [CachedAccessor] attribute to enable cached accessors.
- Implement a cached accessor for window.document.

This is based on crrev.com/2335203006 by peterssen@google.com, with
only a minor fix added.

Intent to implement & ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OhIFnre7ytQ

Previous (reviewed) CL:
https://codereview.chromium.org/2335203006/

Depends on V8 CL:
https://codereview.chromium.org/2405213002/

BUG=chromium:634276

Committed: https://crrev.com/460ec9e9068eb8bf57ce3fae68bb277ca4106f9a
Cr-Commit-Position: refs/heads/master@{#430551}
==========

[commit-bot: I haz the power, 2016-11-08 09:19:10]

Patchset 8 (id:??) landed as
https://crrev.com/460ec9e9068eb8bf57ce3fae68bb277ca4106f9a
Cr-Commit-Position: refs/heads/master@{#430551}