First version of PerformanceObserver

First version of PerformanceObserver

 http://w3c.github.io/performance-timeline/#the-performance-observer-interface

BUG=485613
Committed: https://crrev.com/4be99b78ed497a76b14ab6c45da229dd6e4d03bf
git-svn-id: svn://svn.chromium.org/blink/trunk@202144 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[igrigorik, 2015-06-23 13:28:08]

igrigorik@chromium.org changed reviewers:
+ igrigorik@chromium.org

[igrigorik, 2015-06-23 13:28:09]

https://codereview.chromium.org/1198863006/diff/1/Source/core/timing/Performa...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/1/Source/core/timing/Performa...
Source/core/timing/PerformanceObserver.cpp:86: // TODO(mpb)
FWIW, "If the options' typeFilter attribute is an empty sequence, throw a
JavaScript TypeError." - we want to discourage "subscribe to everything" usage
pattern, you have to be explicit in what you want to subscribe to.

[MikeB, 2015-06-24 17:19:23]

https://codereview.chromium.org/1198863006/diff/1/Source/core/timing/Performa...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/1/Source/core/timing/Performa...
Source/core/timing/PerformanceObserver.cpp:86: // TODO(mpb)
On 2015/06/23 13:28:09, igrigorik wrote:
> FWIW, "If the options' typeFilter attribute is an empty sequence, throw a
> JavaScript TypeError." - we want to discourage "subscribe to everything" usage
> pattern, you have to be explicit in what you want to subscribe to.

Done.

[caseq, 2015-06-24 17:23:19]

caseq@chromium.org changed reviewers:
+ pfeldman@chromium.org

[MikeB, 2015-07-01 19:59:49]

Patchset #7 (id:120001) has been deleted

[MikeB, 2015-07-09 17:43:39]

mpb@chromium.org changed reviewers:
+ esprehn@chromium.org

[MikeB, 2015-07-17 22:45:04]

Patchset #9 (id:180001) has been deleted

[MikeB, 2015-07-17 22:45:57]

Patchset #6 (id:100001) has been deleted

[esprehn, 2015-07-18 22:24:17]

What's up with this create* method? Please don't add more of those, this should
use a constructor.

https://codereview.chromium.org/1198863006/diff/1/Source/core/timing/Performa...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/1/Source/core/timing/Performa...
Source/core/timing/PerformanceObserver.cpp:1: /*
We use the short 4 line copyright now.

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
File Source/bindings/core/v8/V8PerformanceObserverCallback.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/V8PerformanceObserverCallback.cpp:61:
ScriptController::callFunction(m_scriptState->executionContext(),
m_callback.newLocal(m_scriptState->isolate()),
m_scriptState->context()->Global(), 2, argv, m_scriptState->isolate());
ditto about the code generator, we should really be able to generate this stuff
for you, especially since we have two of them now.

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
File Source/bindings/core/v8/V8PerformanceObserverCallback.h (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/V8PerformanceObserverCallback.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
15

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/V8PerformanceObserverCallback.h:5: // This file has been
auto-generated by code_generator_v8.py. DO NOT MODIFY!
??? I don't think this is supposed to be here.

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8PerformanceCustom.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceCustom.cpp:2: * Copyright (C) 2015
Google Inc. All rights reserved.
Please use the short copyright in all your files.

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceCustom.cpp:45: void
V8Performance::createPerformanceObserverMethodCustom(const
v8::FunctionCallbackInfo<v8::Value>& info)
hmm, does this really need to be custom? The code generator has come a very long
way since we made MutationObservers. If this still needs to be custom you should
file a bug and assign to haraken@/Cr-Blink-Bindings so we can get it fixed.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/inspector/...
File Source/core/inspector/AsyncCallTracker.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/inspector/...
Source/core/inspector/AsyncCallTracker.cpp:345:
data->m_performanceObserverCallChains.set(observer, operationId);
The web inspector should really auto generate this or create some helpers so we
don't need this much boilerplate

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:54: "_dummy_",
What is _dummy_/ we don't generally do this. maybe you want an Unknown or
Invalid entry at the start of the enum and the same here?

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:332: m_userTiming->measure(measureName,
startMark, endMark, callback, exceptionState);
Hmm, this seems like you're going to notify multiple times one per type since
you bind with Mark and Measure. I don't think you want to do that, or use this
bind.

What's with the ExceptionState here btw, is script running down there?

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:344: for (FilterTypeIndex filterIndex =
1; filterIndex < (sizeof(kFilterTypes) / sizeof(char*)); ++filterIndex) {
I think you want WTF_ARRAY_LENGTH here.

1 is a thing, maybe assign Composite

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:347: registration->addReference();
What is this addReference() ? That means each thing you listen for adds another
reference?

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:352: PerformanceObserver*
PerformanceBase::createPerformanceObserver(PerformanceObserverCallback*
callback)
What is this? it should be new PerformanceObserver() not
performance.createPerformanceObserver

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:360: size_t index =
registry.value.find(registration);
I think you want to make this simpler, have a single LinkedHashSet of observers
and a combined unsigned of listened for types.

When adding a record you do

if (!(m_listenedTypes & type))
  return;

then adding a new observer you do;

m_observers.add(observer);
m_listenedTypes &= observer->filterTypes();

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:105: Composite = 1,
UnknownFilter, or maybe InvalidFilter
CompositeFilter,
...

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:16: * this software without specific
prior written permission.
short copyright

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:53: {
If you just use a ListHashSet you don't need this

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:67: ,
m_priority(s_observerPriority++)
or this

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverEntryList.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.cpp:66: if (entryType.isNull()
|| equalIgnoringCase(entryType, entry->entryType())) {
This method shouldn't accept null at all, doing getEntriesByType(null) returning
all entries is very weird.

i think you want to convert the entrytype argument to an enum and then compare
that here as well, not compare strings for every entry

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.cpp:80: if (entry->name() ==
name && (entryType.isNull() || equalIgnoringCase(entryType,
entry->entryType()))) {
ditto, you and to use AtomicStrings here as well. the name argument should be an
AtomicString and name() on the entries should be as well

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverEntryList.idl (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.idl:41:
sequence<PerformanceEntry> getEntriesByName (DOMString name, optional DOMString
entryType = null);
no space after the method name

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverRegistration.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverRegistration.cpp:60: if
(!--m_referenceCount) {
don't manually reference count, you want to use WeakHandles or something

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverRegistration.h (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverRegistration.h:46: static
PerformanceObserverRegistration* create(PerformanceObserver&, PerformanceBase*,
const HashSet<AtomicString>& typeFilter);
Why the PerformanceBase argument? The observer should know what performance it
belongs to

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverRegistration.h:54: PerformanceBase*
performance() const { return m_performance.get(); }
Can this be null? i don't think it can, you want *m_observer

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverRegistration.h:60: void addReference() {
++m_referenceCount; }
This manual reference count is very weird, what are you trying to do here? If
you do need this you want this to be RefCounted, but the GC thing should remove
the need for that. maybe you want Weak references to this somewhere?

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverRegistration.h:68:
WeakMember<PerformanceBase> m_performance;
the observer should know the performance.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverRegistration.h:70: HashSet<AtomicString>
m_typeFilter;
This should be an unsigned bitfield where each bit is a different type.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceUserTiming.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceUserTiming.cpp:86: (*callback)(entry);
This is very unusual for blink code, can you return an iterator instead?

[esprehn, 2015-07-18 22:32:08]

Sigh I see the create* method discussion here:
https://github.com/w3c/performance-timeline/pull/29

Okay we can do that then, I think it's bad for the platform but there's worse
things that could happen.

I do think we should call this .createObserver() though, you know what it is
because you write performance.createObserver,
performance.createPerformanceObserver seems needlessly verbose

[blink-reviews, 2015-07-20 17:55:03]

On Sat, Jul 18, 2015 at 3:32 PM, <esprehn@chromium.org> wrote:

> Sigh I see the create* method discussion here:
> https://github.com/w3c/performance-timeline/pull/29
>
> Okay we can do that then, I think it's bad for the platform but there's
> worse
> things that could happen.
>

Opened: https://github.com/w3c/performance-timeline/issues/40

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[MikeB, 2015-07-20 23:06:50]

I still have to adjust what we're doing with the UserTiming Mark/Measures. I
think returning a pointer to the PerformanceEntry that was just added would be
ok, but I have to verify that. Other than that, I think I addressed all the
review comments.
PTAL, thanks!

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
File Source/bindings/core/v8/V8PerformanceObserverCallback.h (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/V8PerformanceObserverCallback.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2015/07/18 22:24:15, esprehn wrote:
> 15

I updated the template for code_generator_v8.py

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/V8PerformanceObserverCallback.h:5: // This file has been
auto-generated by code_generator_v8.py. DO NOT MODIFY!
On 2015/07/18 22:24:15, esprehn wrote:
> ??? I don't think this is supposed to be here.

I actually did generate this file. I had to add line 38, the acecssor for
executionContext, so that I could pass it to InspectorFramework. If there was a
way to do that using the generator, that would make me much happier!
Who might know the answer to that one?

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8PerformanceCustom.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceCustom.cpp:2: * Copyright (C) 2015
Google Inc. All rights reserved.
On 2015/07/18 22:24:15, esprehn wrote:
> Please use the short copyright in all your files.

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceCustom.cpp:45: void
V8Performance::createPerformanceObserverMethodCustom(const
v8::FunctionCallbackInfo<v8::Value>& info)
On 2015/07/18 22:24:15, esprehn wrote:
> hmm, does this really need to be custom? The code generator has come a very
long
> way since we made MutationObservers. If this still needs to be custom you
should
> file a bug and assign to haraken@/Cr-Blink-Bindings so we can get it fixed.

I couldn't get code generator to correctly do line 58. It would either create a
blink::ScriptValue, which couldn't cast to PerformanceObserverCallback* or try
and call PerformanceObserverCallback::toImplWithTypeCheck which doesn't exist. 
The problem goes away if I can skip the custom V8PerformanceObserverCallback.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/inspector/...
File Source/core/inspector/AsyncCallTracker.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/inspector/...
Source/core/inspector/AsyncCallTracker.cpp:345:
data->m_performanceObserverCallChains.set(observer, operationId);
On 2015/07/18 22:24:15, esprehn wrote:
> The web inspector should really auto generate this or create some helpers so
we
> don't need this much boilerplate

You mean I should add a feature request for that, or it should do it today and I
can use that instead?

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:54: "_dummy_",
On 2015/07/18 22:24:15, esprehn wrote:
> What is _dummy_/ we don't generally do this. maybe you want an Unknown or
> Invalid entry at the start of the enum and the same here?

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:332: m_userTiming->measure(measureName,
startMark, endMark, callback, exceptionState);
On 2015/07/18 22:24:16, esprehn wrote:
> Hmm, this seems like you're going to notify multiple times one per type since
> you bind with Mark and Measure. I don't think you want to do that, or use this
> bind.
> 
> What's with the ExceptionState here btw, is script running down there?

That's how it's supposed to work, no? You can create marks without ever
measuring. If you are monitoring the performance timeline for marks, it should
trigger. And vice versa, if you are observing for measures, then you want to get
triggered when a measure happens. 

Yes, the UserTiming::mark and UserTiming::measure will throw exceptions if you
use an invlid mark name or try and measure without actually having the mark
defined. 

I use the bind because I need to trigger when a new PerformanceEntry is created,
and I want to use the enum for entryType rather than string comparisons all the
time. Following your other suggestion though, I think I'll try and make
PerformanceEntry know/own the enum and then I can drop the bind call.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:347: registration->addReference();
On 2015/07/18 22:24:15, esprehn wrote:
> What is this addReference() ? That means each thing you listen for adds
another
> reference?

Got rid of it (and PerformanceObserverRegistration entirely)

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:352: PerformanceObserver*
PerformanceBase::createPerformanceObserver(PerformanceObserverCallback*
callback)
On 2015/07/18 22:24:16, esprehn wrote:
> What is this? it should be new PerformanceObserver() not
> performance.createPerformanceObserver

See issue tracker - switching back to new PerformanceObserver.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:360: size_t index =
registry.value.find(registration);
On 2015/07/18 22:24:15, esprehn wrote:
> I think you want to make this simpler, have a single LinkedHashSet of
observers
> and a combined unsigned of listened for types.
> 
> When adding a record you do
> 
> if (!(m_listenedTypes & type))
>   return;
> 
> then adding a new observer you do;
> 
> m_observers.add(observer);
> m_listenedTypes &= observer->filterTypes();

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:105: Composite = 1,
On 2015/07/18 22:24:16, esprehn wrote:
> UnknownFilter, or maybe InvalidFilter
> CompositeFilter,
> ...

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:16: * this software without specific
prior written permission.
On 2015/07/18 22:24:16, esprehn wrote:
> short copyright

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:53: {
On 2015/07/18 22:24:16, esprehn wrote:
> If you just use a ListHashSet you don't need this

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:67: ,
m_priority(s_observerPriority++)
On 2015/07/18 22:24:16, esprehn wrote:
> or this

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverEntryList.cpp (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.cpp:66: if (entryType.isNull()
|| equalIgnoringCase(entryType, entry->entryType())) {
On 2015/07/18 22:24:16, esprehn wrote:
> This method shouldn't accept null at all, doing getEntriesByType(null)
returning
> all entries is very weird.
> 
> i think you want to convert the entrytype argument to an enum and then compare
> that here as well, not compare strings for every entry

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.cpp:80: if (entry->name() ==
name && (entryType.isNull() || equalIgnoringCase(entryType,
entry->entryType()))) {
On 2015/07/18 22:24:16, esprehn wrote:
> ditto, you and to use AtomicStrings here as well. the name argument should be
an
> AtomicString and name() on the entries should be as well

Performance and PerformanceEntry use String at the moment, should I update them
to AtomicString too?

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverEntryList.idl (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.idl:41:
sequence<PerformanceEntry> getEntriesByName (DOMString name, optional DOMString
entryType = null);
On 2015/07/18 22:24:16, esprehn wrote:
> no space after the method name

Done.

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverRegistration.h (right):

https://codereview.chromium.org/1198863006/diff/200001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverRegistration.h:46: static
PerformanceObserverRegistration* create(PerformanceObserver&, PerformanceBase*,
const HashSet<AtomicString>& typeFilter);
On 2015/07/18 22:24:16, esprehn wrote:
> Why the PerformanceBase argument? The observer should know what performance it
> belongs to

Got rid of this class entirely. Simplifies things a bit. PerformanceObserver now
has no reason to track multiple Performance targets, so we can drop it.

[MikeB, 2015-07-21 21:12:56]

Patchset #5 (id:80001) has been deleted

[esprehn, 2015-07-24 08:23:11]

esprehn@chromium.org changed reviewers:
+ adamk@chromium.org

[esprehn, 2015-07-24 08:23:13]

this looks pretty good, please fix comments.

I'm out for the next 2 weeks on vacation, perhaps adamk@ would be willing to
finish your review (even though this isn't v8 :) because this is similar to
MutationObservers.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:93: PerformanceEntry::EntryType
entryTypeEnum = PerformanceEntry::toEntryTypeEnum(entryType);
Maybe just |type| ?

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:348: for (auto& observer : m_observers) {
const auto&

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:93: void
unregisterPerformanceObserver(PerformanceObserver*);
These should all take references.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:107: void
notifyObserversOfEntry(PerformanceEntry*);
ditto

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:118: PerformanceEntryType_t
m_observerFilterOptions;
Remove _t, we don't use that naming in blink.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceEntry.cpp (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceEntry.cpp:83: return Resource;
This is fine for now, but we'd probably use a sorted array and std::lower_bound
to be faster in the future.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:56: {
No static, PerformanceBase should own this. Each window.performance should have
it's own timer.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:63:
DEFINE_STATIC_LOCAL(Timer<PerformanceObserverTimerCoordinator>*, timer, (new
Timer<PerformanceObserverTimerCoordinator>(&timerCoordinator(),
&PerformanceObserverTimerCoordinator::fired)));
static Timer is usually bad, I think you want PerformanceBase to own this.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceUserTiming.cpp (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceUserTiming.cpp:86: (*callback)(entry);
This still feels very weird, but okay.

[MikeB, 2015-07-24 19:30:38]

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:93: PerformanceEntry::EntryType
entryTypeEnum = PerformanceEntry::toEntryTypeEnum(entryType);
On 2015/07/24 08:23:12, esprehn (ooo-until-aug-10) wrote:
> Maybe just |type| ?

Done.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:348: for (auto& observer : m_observers) {
On 2015/07/24 08:23:12, esprehn (ooo-until-aug-10) wrote:
> const auto&

Done.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.h (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:93: void
unregisterPerformanceObserver(PerformanceObserver*);
On 2015/07/24 08:23:12, esprehn (ooo-until-aug-10) wrote:
> These should all take references.

Done.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:107: void
notifyObserversOfEntry(PerformanceEntry*);
On 2015/07/24 08:23:12, esprehn (ooo-until-aug-10) wrote:
> ditto

Done.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.h:118: PerformanceEntryType_t
m_observerFilterOptions;
On 2015/07/24 08:23:12, esprehn (ooo-until-aug-10) wrote:
> Remove _t, we don't use that naming in blink.

Done.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:56: {
On 2015/07/24 08:23:12, esprehn (ooo-until-aug-10) wrote:
> No static, PerformanceBase should own this. Each window.performance should
have
> it's own timer.

Done.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:63:
DEFINE_STATIC_LOCAL(Timer<PerformanceObserverTimerCoordinator>*, timer, (new
Timer<PerformanceObserverTimerCoordinator>(&timerCoordinator(),
&PerformanceObserverTimerCoordinator::fired)));
On 2015/07/24 08:23:12, esprehn (ooo-until-aug-10) wrote:
> static Timer is usually bad, I think you want PerformanceBase to own this.

Done.

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
File Source/core/timing/PerformanceUserTiming.cpp (right):

https://codereview.chromium.org/1198863006/diff/260001/Source/core/timing/Per...
Source/core/timing/PerformanceUserTiming.cpp:86: (*callback)(entry);
On 2015/07/24 08:23:13, esprehn (ooo-until-aug-10) wrote:
> This still feels very weird, but okay.

ok, I switched it around to return a pointer to the new PerformanceEntry it
created.

[MikeB, 2015-07-24 21:04:26]

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

[adamk, 2015-07-24 21:26:30]

Still working on grokking this, but figured I'd get a set of comments out.

The biggest issue I see is the duplication of code with MutationObserver:
V8PerformanceObserverCallback is one place this happens, the inspector code is
another.

Also, this needs tests (easiest would be W3C test harness tests).

https://codereview.chromium.org/1198863006/diff/280001/Source/core/inspector/...
File Source/core/inspector/AsyncCallTracker.cpp (right):

https://codereview.chromium.org/1198863006/diff/280001/Source/core/inspector/...
Source/core/inspector/AsyncCallTracker.cpp:337: void
AsyncCallTracker::didEnqueuePerformanceObserverEntries(ExecutionContext*
context, PerformanceObserver* observer)
Seems sad to have to duplicate all these calls. Wonder if pfeldman has ideas
about reducing copy-pasting here.

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverEntryList.idl (right):

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.idl:12: // TODO(philipj):
getEntries() should take an optional FilterOptions argument.
What's the TODO about?

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverInit.idl (right):

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverInit.idl:7:
RuntimeEnabled=PerformanceObserverSupport,  // Experimental flag
This 'Experimental flag' comment and the others like it in this patch should be
dropped; they'll become stale as soon as the flag changes state.

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverInit.idl:9: required sequence<DOMString>
entryType;
The spec says "typeFilter" here; should this say "typeFilter" too?

https://codereview.chromium.org/1198863006/diff/280001/Source/platform/Runtim...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1198863006/diff/280001/Source/platform/Runtim...
Source/platform/RuntimeEnabledFeatures.in:111: PerformanceObserverSupport
status=experimental
Naming nit: why not just "PerformanceObserver"? The "Support" suffix seems
redundant.

[MikeB, 2015-07-24 23:07:15]

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverEntryList.idl (right):

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverEntryList.idl:12: // TODO(philipj):
getEntries() should take an optional FilterOptions argument.
On 2015/07/24 21:26:30, adamk wrote:
> What's the TODO about?

Same comment as the one in Performance.idl - different flavor of the same
routine.

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserverInit.idl (right):

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverInit.idl:7:
RuntimeEnabled=PerformanceObserverSupport,  // Experimental flag
On 2015/07/24 21:26:30, adamk wrote:
> This 'Experimental flag' comment and the others like it in this patch should
be
> dropped; they'll become stale as soon as the flag changes state.

Done.

https://codereview.chromium.org/1198863006/diff/280001/Source/core/timing/Per...
Source/core/timing/PerformanceObserverInit.idl:9: required sequence<DOMString>
entryType;
On 2015/07/24 21:26:30, adamk wrote:
> The spec says "typeFilter" here; should this say "typeFilter" too?

Done.

https://codereview.chromium.org/1198863006/diff/280001/Source/platform/Runtim...
File Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/1198863006/diff/280001/Source/platform/Runtim...
Source/platform/RuntimeEnabledFeatures.in:111: PerformanceObserverSupport
status=experimental
On 2015/07/24 21:26:30, adamk wrote:
> Naming nit: why not just "PerformanceObserver"? The "Support" suffix seems
> redundant.

Done.

[esprehn, 2015-08-20 08:33:48]

This needs tests, once it's has those I think it's good to go.

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:335: PerformanceEntry* entry =
m_userTiming->measure(measureName, startMark, endMark, exceptionState);
We usually combine the assignment into the if.

if (Type* thing = fn())
  use(thing)

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:52: for (unsigned i = 0; i <
sequence.size(); ++i)
You can use range loops.

for (const auto& item : items)

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:60:
m_performance->unregisterPerformanceObserver(*this);
Why unregister? I think you just want to only register if you aren't already? I
guess you're trying to update the observation types? You might just add a
function for that explicitly instead.

[haraken, 2015-08-20 08:45:38]

BTW, I'm ok with the V8 custom bindings at the moment. I wrote up a proposal to
control DOM wrapper lifetime in a more easy-to-understand manner
(https://docs.google.com/document/d/1gu7NwxuQ9fLWRRBjbH1MGKr7RZEx9ESRCZ3P4KwvP...),
but it will take time to implement the infrastructure.

[yurys, 2015-08-20 15:28:38]

yurys@chromium.org changed reviewers:
+ yurys@chromium.org

[yurys, 2015-08-20 15:28:39]

https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
File Source/core/inspector/AsyncCallTracker.h (right):

https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
Source/core/inspector/AsyncCallTracker.h:91: void
didEnqueuePerformanceObserverEntries(ExecutionContext*, PerformanceObserver*);
This will need a layout test. See LayoutTests/inspector/sources/debugger-async/
for examples.

[MikeB, 2015-08-20 16:24:46]

On 2015/08/20 15:28:39, yurys wrote:
>
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> File Source/core/inspector/AsyncCallTracker.h (right):
> 
>
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> Source/core/inspector/AsyncCallTracker.h:91: void
> didEnqueuePerformanceObserverEntries(ExecutionContext*, PerformanceObserver*);
> This will need a layout test. See
LayoutTests/inspector/sources/debugger-async/
> for examples.

Thanks. Besides a layout test, @adamk mentioned a w3c web-platform-test as an
end-to-end. Is there any other test that would be relevant?

[esprehn, 2015-08-20 20:32:15]

On 2015/08/20 at 16:24:46, mpb wrote:
> On 2015/08/20 15:28:39, yurys wrote:
> >
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> > File Source/core/inspector/AsyncCallTracker.h (right):
> > 
> >
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> > Source/core/inspector/AsyncCallTracker.h:91: void
> > didEnqueuePerformanceObserverEntries(ExecutionContext*,
PerformanceObserver*);
> > This will need a layout test. See
LayoutTests/inspector/sources/debugger-async/
> > for examples.
> 
> Thanks. Besides a layout test, @adamk mentioned a w3c web-platform-test as an
end-to-end. Is there any other test that would be relevant?

That's probably good. We don't need to block landing your code on a
web-platform-test though.

[adamk, 2015-08-20 20:45:55]

On 2015/08/20 20:32:15, esprehn wrote:
> On 2015/08/20 at 16:24:46, mpb wrote:
> > On 2015/08/20 15:28:39, yurys wrote:
> > >
>
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> > > File Source/core/inspector/AsyncCallTracker.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> > > Source/core/inspector/AsyncCallTracker.h:91: void
> > > didEnqueuePerformanceObserverEntries(ExecutionContext*,
> PerformanceObserver*);
> > > This will need a layout test. See
> LayoutTests/inspector/sources/debugger-async/
> > > for examples.
> > 
> > Thanks. Besides a layout test, @adamk mentioned a w3c web-platform-test as
an
> end-to-end. Is there any other test that would be relevant?
> 
> That's probably good. We don't need to block landing your code on a
> web-platform-test though.

To be clear, some form of LayoutTest exercising the API should be part of this
patch though, right? I suggested a w3c testharness-style test as that's likely
to be easiest for someone touching Blink for the first time.

[esprehn, 2015-08-20 21:02:18]

On 2015/08/20 at 20:45:55, adamk wrote:
> On 2015/08/20 20:32:15, esprehn wrote:
> > ...
> > 
> > That's probably good. We don't need to block landing your code on a
> > web-platform-test though.
> 
> To be clear, some form of LayoutTest exercising the API should be part of this
patch though, right? I suggested a w3c testharness-style test as that's likely
to be easiest for someone touching Blink for the first time.

I actually find js-test.js tests much easier to understand, but yes some kind of
tests are required to land this. The web-platform-tests/ directory is imported,
you can't make changes here, so you'd have to upstream them which shouldn't
block this.

[MikeB, 2015-08-25 18:25:48]

On 2015/08/20 15:28:39, yurys wrote:
>
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> File Source/core/inspector/AsyncCallTracker.h (right):
> 
>
https://codereview.chromium.org/1198863006/diff/340001/Source/core/inspector/...
> Source/core/inspector/AsyncCallTracker.h:91: void
> didEnqueuePerformanceObserverEntries(ExecutionContext*, PerformanceObserver*);
> This will need a layout test. See
LayoutTests/inspector/sources/debugger-async/
> for examples.

Added layout test that verifies callback is invoked as expected.

[MikeB, 2015-08-25 19:42:42]

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:335: PerformanceEntry* entry =
m_userTiming->measure(measureName, startMark, endMark, exceptionState);
On 2015/08/20 08:33:48, esprehn wrote:
> We usually combine the assignment into the if.
> 
> if (Type* thing = fn())
>   use(thing)

Done.

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.cpp (right):

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:52: for (unsigned i = 0; i <
sequence.size(); ++i)
On 2015/08/20 08:33:48, esprehn wrote:
> You can use range loops.
> 
> for (const auto& item : items)

Done.

https://codereview.chromium.org/1198863006/diff/340001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.cpp:60:
m_performance->unregisterPerformanceObserver(*this);
On 2015/08/20 08:33:48, esprehn wrote:
> Why unregister? I think you just want to only register if you aren't already?
I
> guess you're trying to update the observation types? You might just add a
> function for that explicitly instead.

Done.

[MikeB, 2015-08-26 17:31:02]

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

[commit-bot: I haz the power, 2015-08-26 17:31:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198863006/360001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1198863006/360001

[commit-bot: I haz the power, 2015-08-26 17:31:13]

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

[commit-bot: I haz the power, 2015-08-26 17:31:14]

Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are
accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[yurys, 2015-08-26 19:17:05]

https://codereview.chromium.org/1198863006/diff/360001/LayoutTests/inspector/...
File
LayoutTests/inspector/sources/debugger-async/async-callstack-performance-observer.html
(right):

https://codereview.chromium.org/1198863006/diff/360001/LayoutTests/inspector/...
LayoutTests/inspector/sources/debugger-async/async-callstack-performance-observer.html:33:
function timeout3()
what's the value for the user of async call stacks for PeformanceObserver
callback? In this particular scenario we already have async stacks for
setTimeout which should be enough for debugging purposes. Also it seems unusual
to have debugger instrumentation on the profiling stuff. I'd rather not add
async call stack support for PerformanceObserver at all unless there is a strong
case that we need it.

https://codereview.chromium.org/1198863006/diff/360001/Source/core/inspector/...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://codereview.chromium.org/1198863006/diff/360001/Source/core/inspector/...
Source/core/inspector/InspectorInstrumentation.idl:171: void
didEnqueuePerformanceObserverEntries([Keep] ExecutionContext*,
PerformanceObserver*);
You don't need all these changes. Use generic methods
InspectorInstrumentation::traceAsync* [1] instead and store asyncOperationId on
PerformanceObserver instance. See for example how it is done in
FileReader.cpp[2]. We only create separate map from domain object to
asyncOperationId when it is impossible to store the operation id on the object
itself (e.g. for performance reason).


[1] 
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[2]
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

[MikeB, 2015-08-27 18:02:49]

https://codereview.chromium.org/1198863006/diff/360001/LayoutTests/inspector/...
File
LayoutTests/inspector/sources/debugger-async/async-callstack-performance-observer.html
(right):

https://codereview.chromium.org/1198863006/diff/360001/LayoutTests/inspector/...
LayoutTests/inspector/sources/debugger-async/async-callstack-performance-observer.html:33:
function timeout3()
On 2015/08/26 19:17:05, yurys wrote:
> what's the value for the user of async call stacks for PeformanceObserver
> callback? In this particular scenario we already have async stacks for
> setTimeout which should be enough for debugging purposes. Also it seems
unusual
> to have debugger instrumentation on the profiling stuff. I'd rather not add
> async call stack support for PerformanceObserver at all unless there is a
strong
> case that we need it.

Done.

https://codereview.chromium.org/1198863006/diff/360001/Source/core/inspector/...
File Source/core/inspector/InspectorInstrumentation.idl (right):

https://codereview.chromium.org/1198863006/diff/360001/Source/core/inspector/...
Source/core/inspector/InspectorInstrumentation.idl:171: void
didEnqueuePerformanceObserverEntries([Keep] ExecutionContext*,
PerformanceObserver*);
On 2015/08/26 19:17:05, yurys wrote:
> You don't need all these changes. Use generic methods
> InspectorInstrumentation::traceAsync* [1] instead and store asyncOperationId
on
> PerformanceObserver instance. See for example how it is done in
> FileReader.cpp[2]. We only create separate map from domain object to
> asyncOperationId when it is impossible to store the operation id on the object
> itself (e.g. for performance reason).
> 
> 
> [1] 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> [2]
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...

Done.

[yurys, 2015-08-27 18:19:30]

Thanks. Let's add async stack tracking later if we feel it would be useful. The
new layout tests implemented without devtools instrumentation look good to me.

On 2015/08/27 18:02:49, MikeB wrote:
>
https://codereview.chromium.org/1198863006/diff/360001/LayoutTests/inspector/...
> File
>
LayoutTests/inspector/sources/debugger-async/async-callstack-performance-observer.html
> (right):
> 
>
https://codereview.chromium.org/1198863006/diff/360001/LayoutTests/inspector/...
>
LayoutTests/inspector/sources/debugger-async/async-callstack-performance-observer.html:33:
> function timeout3()
> On 2015/08/26 19:17:05, yurys wrote:
> > what's the value for the user of async call stacks for PeformanceObserver
> > callback? In this particular scenario we already have async stacks for
> > setTimeout which should be enough for debugging purposes. Also it seems
> unusual
> > to have debugger instrumentation on the profiling stuff. I'd rather not add
> > async call stack support for PerformanceObserver at all unless there is a
> strong
> > case that we need it.
> 
> Done.
> 
>
https://codereview.chromium.org/1198863006/diff/360001/Source/core/inspector/...
> File Source/core/inspector/InspectorInstrumentation.idl (right):
> 
>
https://codereview.chromium.org/1198863006/diff/360001/Source/core/inspector/...
> Source/core/inspector/InspectorInstrumentation.idl:171: void
> didEnqueuePerformanceObserverEntries([Keep] ExecutionContext*,
> PerformanceObserver*);
> On 2015/08/26 19:17:05, yurys wrote:
> > You don't need all these changes. Use generic methods
> > InspectorInstrumentation::traceAsync* [1] instead and store asyncOperationId
> on
> > PerformanceObserver instance. See for example how it is done in
> > FileReader.cpp[2]. We only create separate map from domain object to
> > asyncOperationId when it is impossible to store the operation id on the
object
> > itself (e.g. for performance reason).
> > 
> > 
> > [1] 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > [2]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/blin...
> 
> Done.

[esprehn, 2015-09-09 22:41:40]

So close!

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
File LayoutTests/fast/performance/performance-observer.html (right):

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
LayoutTests/fast/performance/performance-observer.html:13: function
testPerformanceObserver()
Just inline this code below.

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
LayoutTests/fast/performance/performance-observer.html:28:
testRunner.notifyDone();
finishJSTest(), don't call testRunner directly.

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
LayoutTests/fast/performance/performance-observer.html:46:
testRunner.waitUntilDone();
jsTestIsAsync should do this for you, remove this.

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
File Source/bindings/core/v8/V8PerformanceObserverCallback.h (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
Source/bindings/core/v8/V8PerformanceObserverCallback.h:5: // This file has been
auto-generated by code_generator_v8.py. DO NOT MODIFY!
?? We don't usually check in generated code. Please remove this comment.

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp:31: if (window)
Throw if there's no window? There should always be one.

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp:32: performance =
DOMWindowPerformance::performance(*window);
ASSERT(performance) ?

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:98: if (type ==
PerformanceEntry::Invalid)
This should be a switch() over the type.

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.h (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.h:40: PerformanceEntryTypeMask
filterOptions() { return m_filterOptions; }
const

[MikeB, 2015-09-10 19:53:01]

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
File LayoutTests/fast/performance/performance-observer.html (right):

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
LayoutTests/fast/performance/performance-observer.html:13: function
testPerformanceObserver()
On 2015/09/09 22:41:40, esprehn wrote:
> Just inline this code below.

Done.

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
LayoutTests/fast/performance/performance-observer.html:28:
testRunner.notifyDone();
On 2015/09/09 22:41:40, esprehn wrote:
> finishJSTest(), don't call testRunner directly.

Done.

https://codereview.chromium.org/1198863006/diff/380001/LayoutTests/fast/perfo...
LayoutTests/fast/performance/performance-observer.html:46:
testRunner.waitUntilDone();
On 2015/09/09 22:41:40, esprehn wrote:
> jsTestIsAsync should do this for you, remove this.

Done.

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
File Source/bindings/core/v8/V8PerformanceObserverCallback.h (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
Source/bindings/core/v8/V8PerformanceObserverCallback.h:5: // This file has been
auto-generated by code_generator_v8.py. DO NOT MODIFY!
On 2015/09/09 22:41:40, esprehn wrote:
> ?? We don't usually check in generated code. Please remove this comment.

Done.

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
File Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp:31: if (window)
On 2015/09/09 22:41:40, esprehn wrote:
> Throw if there's no window? There should always be one.

Done.

https://codereview.chromium.org/1198863006/diff/380001/Source/bindings/core/v...
Source/bindings/core/v8/custom/V8PerformanceObserverCustom.cpp:32: performance =
DOMWindowPerformance::performance(*window);
On 2015/09/09 22:41:40, esprehn wrote:
> ASSERT(performance) ?

Done.

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
File Source/core/timing/PerformanceBase.cpp (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
Source/core/timing/PerformanceBase.cpp:98: if (type ==
PerformanceEntry::Invalid)
On 2015/09/09 22:41:40, esprehn wrote:
> This should be a switch() over the type.

Done.

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
File Source/core/timing/PerformanceObserver.h (right):

https://codereview.chromium.org/1198863006/diff/380001/Source/core/timing/Per...
Source/core/timing/PerformanceObserver.h:40: PerformanceEntryTypeMask
filterOptions() { return m_filterOptions; }
On 2015/09/09 22:41:40, esprehn wrote:
> const

Done.

[esprehn, 2015-09-10 19:58:55]

lgtm

[MikeB, 2015-09-10 21:04:48]

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

[commit-bot: I haz the power, 2015-09-10 21:05:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198863006/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1198863006/400001

[commit-bot: I haz the power, 2015-09-10 22:32:44]

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

[commit-bot: I haz the power, 2015-09-10 22:32:45]

Dry run: This issue passed the CQ dry run.

[MikeB, 2015-09-11 16:31:26]

The CQ bit was checked by mpb@chromium.org

[commit-bot: I haz the power, 2015-09-11 16:31:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198863006/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1198863006/400001

[commit-bot: I haz the power, 2015-09-11 16:36:18]

Committed patchset #17 (id:400001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=202144

[commit-bot: I haz the power, 2015-09-23 12:22:57]

Patchset 17 (id:??) landed as
https://crrev.com/4be99b78ed497a76b14ab6c45da229dd6e4d03bf