Replace DOMWindowProperty in core/ with ContextClient

Replace DOMWindowProperty in core/ with ContextClient

This is a preparation for deprecating DOMWindowProperty.

BUG=610176
Committed: https://crrev.com/e2eaec7e72e92484106db0546183c7921a370530
Cr-Commit-Position: refs/heads/master@{#440692}

[haraken, 2016-12-21 10:01:09]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-12-21 10:01:09]

PTAL

https://codereview.chromium.org/2593983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/StyleMedia.h (right):

https://codereview.chromium.org/2593983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/StyleMedia.h:41: public ContextClient {

ScriptWrappable must be the left-most class to make the wrapper tracing work.
This was not a problem because StyleMedia was not traced by TRACE_WRAPPERS.

[sof, 2016-12-21 10:10:48]

lgtm

https://codereview.chromium.org/2593983002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/StyleMedia.h (right):

https://codereview.chromium.org/2593983002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/StyleMedia.h:41: public ContextClient {
On 2016/12/21 10:01:09, haraken wrote:
> 
> ScriptWrappable must be the left-most class to make the wrapper tracing work.
> This was not a problem because StyleMedia was not traced by TRACE_WRAPPERS.
> 

leftmost (GC) mixin? Got a pointer to where this is covered/discussed?

[haraken, 2016-12-21 11:51:39]

haraken@chromium.org changed reviewers:
+ mlippautz@chromium.org

[haraken, 2016-12-21 11:51:39]

mlippautz@: I was thinking that at some point the wrapper tracing  was relying
on the fact that beginningAddress(ScriptWrappable) == beginningAddress(the
object). So we had to put ScriptWrappable at the left most class.

Was the restriction removed already?

[haraken, 2016-12-21 11:51:49]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2016-12-21 11:52:06]

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

[Michael Lippautz, 2016-12-21 12:14:06]

On 2016/12/21 11:51:39, haraken wrote:
> mlippautz@: I was thinking that at some point the wrapper tracing  was relying
> on the fact that beginningAddress(ScriptWrappable) == beginningAddress(the
> object). So we had to put ScriptWrappable at the left most class.
> 
> Was the restriction removed already?

I just checked the code and, yes, wrapper tracing assumes that ScriptWrappable
is the left most "mixin". 

This restriction was introduced/discussed before I joined the project.
Presumably, we have it because we'd like to avoid the indirection of adjusting
the header for performance reasons. We should have an assertion of some sort
though.

(Removing it in the current infrastructure would require us to add infra like
DEFINE_GARBAGE_COLLECTED_MIXIN_WRAPPER_METHODS.)

[haraken, 2016-12-21 12:37:58]

On 2016/12/21 12:14:06, Michael Lippautz wrote:
> On 2016/12/21 11:51:39, haraken wrote:
> > mlippautz@: I was thinking that at some point the wrapper tracing  was
relying
> > on the fact that beginningAddress(ScriptWrappable) == beginningAddress(the
> > object). So we had to put ScriptWrappable at the left most class.
> > 
> > Was the restriction removed already?
> 
> I just checked the code and, yes, wrapper tracing assumes that ScriptWrappable
> is the left most "mixin". 
> 
> This restriction was introduced/discussed before I joined the project.
> Presumably, we have it because we'd like to avoid the indirection of adjusting
> the header for performance reasons. We should have an assertion of some sort
> though.
> 
> (Removing it in the current infrastructure would require us to add infra like
> DEFINE_GARBAGE_COLLECTED_MIXIN_WRAPPER_METHODS.)

We have a runtime verification somewhere to check that ScriptWrappable (or
TraceWrapperBase in general) is a left-most class, right?

I guess we can have something like DCHECK(traceWrappable->checkHeader()).

[commit-bot: I haz the power, 2016-12-21 12:48:46]

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

[commit-bot: I haz the power, 2016-12-21 12:48:47]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Michael Lippautz, 2016-12-21 12:57:36]

On 2016/12/21 12:37:58, haraken wrote:
> On 2016/12/21 12:14:06, Michael Lippautz wrote:
> > On 2016/12/21 11:51:39, haraken wrote:
> > > mlippautz@: I was thinking that at some point the wrapper tracing  was
> relying
> > > on the fact that beginningAddress(ScriptWrappable) == beginningAddress(the
> > > object). So we had to put ScriptWrappable at the left most class.
> > > 
> > > Was the restriction removed already?
> > 
> > I just checked the code and, yes, wrapper tracing assumes that
ScriptWrappable
> > is the left most "mixin". 
> > 
> > This restriction was introduced/discussed before I joined the project.
> > Presumably, we have it because we'd like to avoid the indirection of
adjusting
> > the header for performance reasons. We should have an assertion of some sort
> > though.
> > 
> > (Removing it in the current infrastructure would require us to add infra
like
> > DEFINE_GARBAGE_COLLECTED_MIXIN_WRAPPER_METHODS.)
> 
> We have a runtime verification somewhere to check that ScriptWrappable (or
> TraceWrapperBase in general) is a left-most class, right?
> 
> I guess we can have something like DCHECK(traceWrappable->checkHeader()).

The checkHeader() call is in HeapObject::fromPayload() which will be used for
ScriptWrappable, so we should always have it already.

[sof, 2016-12-21 13:31:27]

Some interesting failures coming through here; is it required/expected that
DOMWindowProperty keeps a strong frame reference? (which ContextClient doesn't)

[haraken, 2016-12-21 13:34:57]

On 2016/12/21 13:31:27, sof wrote:
> Some interesting failures coming through here; is it required/expected that
> DOMWindowProperty keeps a strong frame reference? (which ContextClient
doesn't)

It looks like yes in some cases (but then I need to understand why).

[haraken, 2016-12-26 04:28:19]

On 2016/12/21 13:34:57, haraken wrote:
> On 2016/12/21 13:31:27, sof wrote:
> > Some interesting failures coming through here; is it required/expected that
> > DOMWindowProperty keeps a strong frame reference? (which ContextClient
> doesn't)
> 
> It looks like yes in some cases (but then I need to understand why).

The problem comes from the change in Navigator.h. I'll drop the change from this
CL and address it in a separate CL.

[haraken, 2016-12-26 04:28:23]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-12-26 04:28:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2593983002/#ps20001
(title: "temp")

[commit-bot: I haz the power, 2016-12-26 04:28:34]

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-12-26 05:42:28]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482726503663010,
"parent_rev": "e9874ecc0b871300c23c59d8daf7fc2cb7e9a2e3", "commit_rev":
"af7c9bce1cee0958c0f7490c834dd8454bbd23d2"}

[commit-bot: I haz the power, 2016-12-26 05:43:58]

Description was changed from

==========
Replace DOMWindowProperty in core/ with ContextClient

This is a preparation for deprecating DOMWindowProperty.

BUG=610176
==========

to

==========
Replace DOMWindowProperty in core/ with ContextClient

This is a preparation for deprecating DOMWindowProperty.

BUG=610176

Review-Url: https://codereview.chromium.org/2593983002
==========

[commit-bot: I haz the power, 2016-12-26 05:43:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-12-26 05:46:52]

Description was changed from

==========
Replace DOMWindowProperty in core/ with ContextClient

This is a preparation for deprecating DOMWindowProperty.

BUG=610176

Review-Url: https://codereview.chromium.org/2593983002
==========

to

==========
Replace DOMWindowProperty in core/ with ContextClient

This is a preparation for deprecating DOMWindowProperty.

BUG=610176

Committed: https://crrev.com/e2eaec7e72e92484106db0546183c7921a370530
Cr-Commit-Position: refs/heads/master@{#440692}
==========

[commit-bot: I haz the power, 2016-12-26 05:46:53]

Patchset 2 (id:??) landed as
https://crrev.com/e2eaec7e72e92484106db0546183c7921a370530
Cr-Commit-Position: refs/heads/master@{#440692}