Upgrade in-document custom elements when an element is defined.

Upgrade in-document custom elements when an element is defined.

BUG=594918
Committed: https://crrev.com/c561a337b93cf622ea0bd4f066e7e74f30ca7c3d
Cr-Commit-Position: refs/heads/master@{#397660}

[dominicc (has gone to gerrit), 2016-06-01 04:43:06]

dominicc@chromium.org changed reviewers:
+ kojii@chromium.org, yosin@chromium.org

[dominicc (has gone to gerrit), 2016-06-01 04:49:23]

PTAL

This is just the core/dom side of in-document upgrade. I have the script side in
a local patch; it will come next. It's basically just filling in the virtual on
ScriptCustomElementDefinition::runConstructor and some layout tests.

This wires up the custom element reaction stack you reviewed earlier. It's on
FrameHost because that's the closest thing we have to the set of similar origin
browsing contexts. It's OK to mix everything together on FrameHost because
dis-similar origin browsing contexts can't really view the causal relationship
anyway.

It is not clear to me if we should always have a frame on the reaction queue
stack, and what to do if something tries to enqueue a reaction with no
CEReactionsScope on the stack. For now this will dereference null and crash, but
you can't trigger this because define is the only thing that triggers reaction
and it puts one on the stack.

I have integration of CEReactions in IDL and CEReactionsScope in a local patch.
I have decided we probably should *not* just make Reflect mean implicit
CEReactions like we did in custom elements v1. The reason is: Reflect does not
mean "this reflects attributes"; it means "bindings generator, reflect this
attribute!" It's a subtle difference but I see places IDL where people removed
Reflect and started doing the reflection manually (because they wanted to
trigger something else I guess, so they just did it in code) but did not add
CustomElementCallbacks. So let's put CEReactions *everywhere* like the specs do.

[yosin_UTC9, 2016-06-01 06:15:45]

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:23:
WTF_MAKE_NONCOPYABLE(CEReactionsScope);
nit: Better to use DISALLOW_COPY_AND_ASSIGN(CEReactionScope)

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:30:
CEReactionsScope()
Do we really need to them inline?
To reduce compilation time, they should be in .cpp file
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:28:
m_constructionStack.append(element);
We should have DCHECK(!m_constructionStack.isEmpty());

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:33:
DCHECK(m_constructionStack.last() == element
Better to use DCHECK_EQ
if (!m_consturctionStack.last())
  DCHECK_EQ(m_constructionStack.last(), element)

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:36:
DCHECK(m_constructionStack.size() == depth); // It's a *stack*.
nit: DCHECK_EQ

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h:39: virtual
bool runConstructor(Element*) { return true; }
virtual function should be in .cpp file.
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.cpp
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.cpp:19: {
s/{ }/=default;/

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h:24:
void invoke(Element*) override;
nit: Should be private?

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h (right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h:35: static
CustomElementsRegistry* create(Document*);
(^_^)b

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:28:
public:
No need to have public accessibility for Test class.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:41:
public:
No need to have public accessibility for Test class.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:69:
public:
No need to have public accessibility for Test class.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:102:
OwnPtr<DummyPageHolder> m_page;
nit: Let's use std::unique_ptr<DummyPageHolder>

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:117:
EXPECT_EQ(0u, elements.size())
nit: isEmpty() is more readable.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:153:
Element* a = CreateElement("hello-world")
Please avoid to use one letter variable name.

[dominicc (has gone to gerrit), 2016-06-01 23:30:57]

Thank you for the review, PTAL.

No comment = Done!

Some replies inline.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:30:
CEReactionsScope()
On 2016/06/01 at 06:15:44, Yosi_UTC9 wrote:
> Do we really need to them inline?
> To reduce compilation time, they should be in .cpp file
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

Probably yes; these will get added to every CEReactions binding.

Note that enqueue and invokeReactions are in the .cpp file; this split is
deliberate.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:28:
m_constructionStack.append(element);
On 2016/06/01 at 06:15:44, Yosi_UTC9 wrote:
> We should have DCHECK(!m_constructionStack.isEmpty());

Why? How would anything get upgraded if there's an assertion that the stack is
not-empty before pushing onto the stack--how would the first element get on
there?

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:33:
DCHECK(m_constructionStack.last() == element
On 2016/06/01 at 06:15:44, Yosi_UTC9 wrote:
> Better to use DCHECK_EQ
> if (!m_consturctionStack.last())
>   DCHECK_EQ(m_constructionStack.last(), element)

I don't think that's applicable because of the test on line 34 which also allows
the TOS to be nullptr.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h (right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.h:35: static
CustomElementsRegistry* create(Document*);
On 2016/06/01 at 06:15:44, Yosi_UTC9 wrote:
> (^_^)b

Yeah, I feel a bit nervous about adding document, window, etc. here but we do
need to retrieve the document to do upgrade.

I think windows may change documents (document.open), so we may need something
more dynamic here.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:28:
public:
On 2016/06/01 at 06:15:45, Yosi_UTC9 wrote:
> No need to have public accessibility for Test class.

I suspect you're wrong about this; fixtures using CustomElementRegistryTest need
to cast it to ::testing::Test. Making ::testing::Test non-public only works in
the case CustomElementsRegistryTestBase has no subclasses.

Even for test fixtures with no subclasses, I do not see any particular harm in
making it public. The only benefit I can see of not doing that is if I was
worried about someone adding subclasses. It will be easy to spot the
dependencies because these are defined in .cpp.

Look at the examples in the primer; they're all public ::testing::Test:

https://github.com/google/googletest/blob/master/googletest/docs/Primer.md

Also look at the codebase; in Blink there are 160 occurrences of
'::testing::Test' and 157 of them are 'public ?::testing::Test' The remaining
three are a comment and two aliases.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:117:
EXPECT_EQ(0u, elements.size())
On 2016/06/01 at 06:15:45, Yosi_UTC9 wrote:
> nit: isEmpty() is more readable.

True; this produces a more useful failure. With isEmpty you know it is not
empty; with size you know it should be empty but there were 7 elements.

[kojii, 2016-06-02 01:30:44]

lgtm

[yosin_UTC9, 2016-06-02 02:02:18]

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:28:
m_constructionStack.append(element);
On 2016/06/01 at 23:30:56, dominicc wrote:
> On 2016/06/01 at 06:15:44, Yosi_UTC9 wrote:
> > We should have DCHECK(!m_constructionStack.isEmpty());
> 
> Why? How would anything get upgraded if there's an assertion that the stack is
not-empty before pushing onto the stack--how would the first element get on
there?

Sorry, my comment is wrong.

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:33:
DCHECK(m_constructionStack.last() == element
On 2016/06/01 at 23:30:57, dominicc wrote:
> On 2016/06/01 at 06:15:44, Yosi_UTC9 wrote:
> > Better to use DCHECK_EQ
> > if (!m_consturctionStack.last())
> >   DCHECK_EQ(m_constructionStack.last(), element)
> 
> I don't think that's applicable because of the test on line 34 which also
allows the TOS to be nullptr.

Oh, typo

if (m_consturctionStack.last())
   DCHECK_EQ(m_constructionStack.last(), element)

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp
(right):

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:28:
public:
On 2016/06/01 at 23:30:57, dominicc wrote:
> On 2016/06/01 at 06:15:45, Yosi_UTC9 wrote:
> > No need to have public accessibility for Test class.
> 
> I suspect you're wrong about this; fixtures using CustomElementRegistryTest
need to cast it to ::testing::Test. Making ::testing::Test non-public only works
in the case CustomElementsRegistryTestBase has no subclasses.
> 
> Even for test fixtures with no subclasses, I do not see any particular harm in
making it public. The only benefit I can see of not doing that is if I was
worried about someone adding subclasses. It will be easy to spot the
dependencies because these are defined in .cpp.
> 
> Look at the examples in the primer; they're all public ::testing::Test:
> 
> https://github.com/google/googletest/blob/master/googletest/docs/Primer.md
> 
> Also look at the codebase; in Blink there are 160 occurrences of
'::testing::Test' and 157 of them are 'public ?::testing::Test' The remaining
three are a comment and two aliases.

Sorry, my explanation is bad. I mean all members of derived class of
::testing::Test should be protected or private; == no public in derived class.

Note: Google coding style says all inheritance should be "public".

https://codereview.chromium.org/2023093003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp:117:
EXPECT_EQ(0u, elements.size())
On 2016/06/01 at 23:30:57, dominicc wrote:
> On 2016/06/01 at 06:15:45, Yosi_UTC9 wrote:
> > nit: isEmpty() is more readable.
> 
> True; this produces a more useful failure. With isEmpty you know it is not
empty; with size you know it should be empty but there were 7 elements.

How about:
EXPECT_TRUE(elements.isEmpty()) << "no candidates should been found, but we have
" << elements.size();

Anyway, this is personal preference.

https://codereview.chromium.org/2023093003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementReaction.h (right):

https://codereview.chromium.org/2023093003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementReaction.h:17:
DISALLOW_COPY_AND_ASSIGN(CustomElementReaction);
Sorry for confusion.

Oops, usually DISALLOW_COPY_AND_ASSING() is at bottom of class. I'm not sure
whether PRESUBMIT checks this for Chromium or not.
Since, discussion of DISALLOW_XXX() is still on going, whether remove or keep.
It is better to keep WTF_MAKE_NONCOPYABLE() for existing files.

[dominicc (has gone to gerrit), 2016-06-02 02:50:48]

PTAL

Thank you for all of the feedback. I really like your assertion wording. Thanks
for keeping an eye on evolving Blink -> Chromium -> Google C++ style, I'm a bit
rusty.

[yosin_UTC9, 2016-06-02 04:06:38]

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:49: static
CEReactionsScope* top = nullptr;
It is better to have |static CReactionScopt* top| in .cpp file as file level
static to avoid thread lock in this function to initialize |top|,
since we have not way to tell compiler that this function is called by only one
thread.

See "Static local variables" in
http://en.cppreference.com/w/cpp/language/storage_duration

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:36:
DCHECK_EQ(m_constructionStack.size(), depth); // It's a *stack*.
Better way to catch weird situation is:

m_constructionStack.append(element);
size_t depth = m_constructionStack.size();
bool succeeded = runConstructor(element);
DCHECK_EQ(m_constructionStack.size(), depth);

Above sequence tells compiler that removing |RELEASE_ASSERT(index < size.())| in
|at()| via |last()| is safe.

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h
(right):

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h:18:
class CORE_EXPORT CustomElementUpgradeReaction : public CustomElementReaction {
Q: final?

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h:22:
virtual ~CustomElementUpgradeReaction();
~CustomElementUpgradeReaction() override;

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:175: if
(!element)
nit: It is better to use early-continue:

if (!element || !desc.matches(*element)
    continue
sorter.add(element);

[yosin_UTC9, 2016-06-02 04:13:51]

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):

https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:49: static
CEReactionsScope* top = nullptr;
On 2016/06/02 at 04:06:38, Yosi_UTC9 wrote:
> It is better to have |static CReactionScopt* top| in .cpp file as file level
static to avoid thread lock in this function to initialize |top|,
> since we have not way to tell compiler that this function is called by only
one thread.
> 
> See "Static local variables" in
http://en.cppreference.com/w/cpp/language/storage_duration

If we use file level variable, this class can be simpler or avoid
pointer-to-pointer:

static CEReactionScope* topOfStack;

CEReactionScope::CEReactionScope() : m_prev(topOfStack) { topOfStack = this; }
CEReactionScope::~CEReactionScope() { DCHECK(topOfStack); topOfStack =
topOfStack->m_prev; }

[dominicc (has gone to gerrit), 2016-06-02 05:40:19]

On 2016/06/02 at 04:06:38, yosin wrote:
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):
> 
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:49: static
CEReactionsScope* top = nullptr;
> It is better to have |static CReactionScopt* top| in .cpp file as file level
static to avoid thread lock in this function to initialize |top|,
> since we have not way to tell compiler that this function is called by only
one thread.
> 
> See "Static local variables" in
http://en.cppreference.com/w/cpp/language/storage_duration
> 
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp
(right):
> 
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.cpp:36:
DCHECK_EQ(m_constructionStack.size(), depth); // It's a *stack*.
> Better way to catch weird situation is:
> 
> m_constructionStack.append(element);
> size_t depth = m_constructionStack.size();
> bool succeeded = runConstructor(element);
> DCHECK_EQ(m_constructionStack.size(), depth);
> 
> Above sequence tells compiler that removing |RELEASE_ASSERT(index < size.())|
in |at()| via |last()| is safe.

True, but it is a different assertion.

I want to assert that the construction stack depth on entry is the same as the
depth on exit.

Your assertion is that runConstructor does not change the depth of the stack.

This would miss things such as upgrade failing to push or failing to pop the
stack.

>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h
(right):
> 
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h:18:
class CORE_EXPORT CustomElementUpgradeReaction : public CustomElementReaction {
> Q: final?
> 
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElementUpgradeReaction.h:22:
virtual ~CustomElementUpgradeReaction();
> ~CustomElementUpgradeReaction() override;
> 
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):
> 
>
https://codereview.chromium.org/2023093003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:175: if
(!element)
> nit: It is better to use early-continue:
> 
> if (!element || !desc.matches(*element)
>     continue
> sorter.add(element);

[yosin_UTC9, 2016-06-02 07:16:02]

lgtm

My comments are mostly personal preferences.

https://codereview.chromium.org/2023093003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):

https://codereview.chromium.org/2023093003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:25: static
CEReactionsScope* current()
So, you think this class is performance critical and it is worth to expose
implementation, s_topOfStack, detail in .h file.
My preference is all of them in .cpp file and make s_topOfStack is file static
variable instead of static class member.
Anyway, this is personal preference and performance consideration.

[dominicc (has gone to gerrit), 2016-06-02 07:25:29]

On 2016/06/02 at 07:16:02, yosin wrote:
> lgtm
> 
> My comments are mostly personal preferences.
> 
>
https://codereview.chromium.org/2023093003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):
> 
>
https://codereview.chromium.org/2023093003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:25: static
CEReactionsScope* current()
> So, you think this class is performance critical and it is worth to expose
implementation, s_topOfStack, detail in .h file.
> My preference is all of them in .cpp file and make s_topOfStack is file static
variable instead of static class member.
> Anyway, this is personal preference and performance consideration.

I would like to encapsulate it if I could. Let me land it--benchmarks may reject
this anyway--and we can measure the difference with perf trybots next.

[dominicc (has gone to gerrit), 2016-06-02 07:26:50]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-06-02 07:26:50]

The patchset sent to the CQ was uploaded after l-g-t-m from kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2023093003/#ps60001
(title: "More feedback.")

[commit-bot: I haz the power, 2016-06-02 07:27:06]

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

[dominicc (has gone to gerrit), 2016-06-02 07:27:56]

On 2016/06/02 at 07:25:29, dominicc wrote:
> On 2016/06/02 at 07:16:02, yosin wrote:
> > lgtm
> > 
> > My comments are mostly personal preferences.
> > 
> >
https://codereview.chromium.org/2023093003/diff/60001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h (right):
> > 
> >
https://codereview.chromium.org/2023093003/diff/60001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/dom/custom/CEReactionsScope.h:25: static
CEReactionsScope* current()
> > So, you think this class is performance critical and it is worth to expose
implementation, s_topOfStack, detail in .h file.
> > My preference is all of them in .cpp file and make s_topOfStack is file
static variable instead of static class member.
> > Anyway, this is personal preference and performance consideration.
> 
> I would like to encapsulate it if I could. Let me land it--benchmarks may
reject this anyway--and we can measure the difference with perf trybots next.

Ah, perf bots won't reject this because it's not used. We'll need to add this to
the bindings, I have a WIP patch if you want to experiment with it, and then do
the experiment.

[commit-bot: I haz the power, 2016-06-02 09:01:41]

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

[commit-bot: I haz the power, 2016-06-02 09:01:43]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominicc (has gone to gerrit), 2016-06-03 04:28:42]

The CQ bit was checked by dominicc@chromium.org

[commit-bot: I haz the power, 2016-06-03 04:29:05]

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

[commit-bot: I haz the power, 2016-06-03 04:36:13]

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

[commit-bot: I haz the power, 2016-06-03 04:36:14]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dominicc (has gone to gerrit), 2016-06-03 06:19:07]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-06-03 06:19:07]

The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org,
kojii@chromium.org
Link to the patchset: https://codereview.chromium.org/2023093003/#ps80001
(title: "Fix ASAN failure in test helper.")

[commit-bot: I haz the power, 2016-06-03 06:19:14]

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

[commit-bot: I haz the power, 2016-06-03 08:11:46]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-03 08:12:54]

Description was changed from

==========
Upgrade in-document custom elements when an element is defined.

BUG=594918
==========

to

==========
Upgrade in-document custom elements when an element is defined.

BUG=594918

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

[commit-bot: I haz the power, 2016-06-03 08:12:56]

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