Clear mutable inline style when it is empty.

Clear mutable inline style when it is empty.

As part of this, add a new InlineVariablesIterator class. This implements a
CSSVariablesIterator interface, but in a way that is compatible with how inline
styles as represented in a MutableStylePropertySet are accessed through
ElementRareData rather than a ref pointer. This allows us to decouple the
lifetimes of a CSSVariablesIterator and the MutableStylePropertySet that it is a
helper/iterator for.

BUG=326664
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164307

[chrishtr, 2013-12-23 20:34:11]

@Eric: this is a new version of the CL I sent to you last week. The other one
turned out to be incorrect, since it did not manage object lifetimes properly.
This time it does so, and also fixes the issue you raised about clearing at
mutation time and not access time.

[eseidel, 2013-12-23 21:22:43]

Made one pass.  Need to make another to understand what I'm reading. :)

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
File Source/core/css/InlineVariablesIterator.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
Source/core/css/InlineVariablesIterator.cpp:35: const StylePropertySet*
propertySet = element->inlineStyle();
Shouldn't this be in the constructor itself instead of the ::create wrapper?

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Propert...
File Source/core/css/PropertySetCSSStyleDeclaration.h (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Propert...
Source/core/css/PropertySetCSSStyleDeclaration.h:141:
PassRefPtr<CSSVariablesIterator> variablesIterator() const OVERRIDE;
Please mark it virtual as well as override (I know that's redundant in C++ but
it helps prevent confusion).

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
File Source/core/css/VariablesIterator.h (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
Source/core/css/VariablesIterator.h:40: const StylePropertySet*,
AbstractVariablesIterator*);
We don't wrap to 80c in blink (yet).

https://codereview.chromium.org/119533003/diff/180001/Source/core/editing/Rem...
File Source/core/editing/RemoveCSSPropertyCommand.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/editing/Rem...
Source/core/editing/RemoveCSSPropertyCommand.cpp:53: if (!style) {
Although (I think?) we allow {} around one-line ifs these days, historically
they were verbotten and most places in blink still avoid them.

[eseidel, 2013-12-23 21:30:22]

I think I just need a bit more context/comments.  Happy to do another round or
discuss over irc.

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
File Source/core/css/InlineVariablesIterator.h (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
Source/core/css/InlineVariablesIterator.h:29: class InlineVariablesIterator :
public AbstractVariablesIterator {
WebKit was historically very anti-comments.  I (and others) ahve been trying to
change that in blink.  These days when I add a new class I tend to add comments
above it to explain what its' for how it's used, etc.  I might encourage you to
do similar here, since I'm at least not immediately familar with an "inline
variables iterator".  Presumably it iterates over CSS variables in an inline
style block?  (I'm sure I'll understand better after further reading.)

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
File Source/core/css/VariablesIterator.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
Source/core/css/VariablesIterator.cpp:30: void
AbstractVariablesIterator::initRemainingNames(
Could this just be part of the AbstractVariablesIterator constructor?  If not,
is there a way we can ASSERT when implementors fail to call this method?

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
Source/core/css/VariablesIterator.cpp:51:
AbstractVariablesIterator::initRemainingNames(propertySet, iterator.get());
As I commented in a previous pass, it's uncommon to have ::create() do anything
besides wrap the new calls in an adoptRef.  That said, if this needs to be done
this way it's totally fine.  I would add a comment in the header next to
initRemainingNames() that all subclasses must call it, etc.

https://codereview.chromium.org/119533003/diff/180001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3446:
const_cast<Element*>(this)->removeSynchronizedLazyAttribute(styleAttr);
I remember being in this code for the hasAttribute/style-resolve patch (which
you were kind enough to run with) and I can't remember if setAtribute('foo',
null) will actually execute a remove?  Is removeSynchronizedLazyAttribute
actually necessary?

(I remember thsi because I was confused as to if synchronizing lazy attributes
could ever cause them to be removed.  It seemed liek the current implementation
made that impossible by accident iirc.)

[chrishtr, 2013-12-23 22:06:46]

Going to ping you on IM for any further questions, and address the comments.

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
File Source/core/css/InlineVariablesIterator.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
Source/core/css/InlineVariablesIterator.cpp:35: const StylePropertySet*
propertySet = element->inlineStyle();
On 2013/12/23 21:22:43, eseidel wrote:
> Shouldn't this be in the constructor itself instead of the ::create wrapper?

Good point. I was following the existing structure without thinking much about
it.

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
File Source/core/css/InlineVariablesIterator.h (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/InlineV...
Source/core/css/InlineVariablesIterator.h:29: class InlineVariablesIterator :
public AbstractVariablesIterator {
On 2013/12/23 21:30:23, eseidel wrote:
> WebKit was historically very anti-comments.  I (and others) ahve been trying
to
> change that in blink.  These days when I add a new class I tend to add
comments
> above it to explain what its' for how it's used, etc.  I might encourage you
to
> do similar here, since I'm at least not immediately familar with an "inline
> variables iterator".  Presumably it iterates over CSS variables in an inline
> style block?  (I'm sure I'll understand better after further reading.)

Done.

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Propert...
File Source/core/css/PropertySetCSSStyleDeclaration.h (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Propert...
Source/core/css/PropertySetCSSStyleDeclaration.h:141:
PassRefPtr<CSSVariablesIterator> variablesIterator() const OVERRIDE;
On 2013/12/23 21:22:43, eseidel wrote:
> Please mark it virtual as well as override (I know that's redundant in C++ but
> it helps prevent confusion).

Done.

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
File Source/core/css/VariablesIterator.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
Source/core/css/VariablesIterator.cpp:30: void
AbstractVariablesIterator::initRemainingNames(
On 2013/12/23 21:30:23, eseidel wrote:
> Could this just be part of the AbstractVariablesIterator constructor?  If not,
> is there a way we can ASSERT when implementors fail to call this method?

I think it can, per your other comment. Will fix.

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
File Source/core/css/VariablesIterator.h (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
Source/core/css/VariablesIterator.h:40: const StylePropertySet*,
AbstractVariablesIterator*);
On 2013/12/23 21:22:43, eseidel wrote:
> We don't wrap to 80c in blink (yet).

Done.

https://codereview.chromium.org/119533003/diff/180001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/dom/Element...
Source/core/dom/Element.cpp:3446:
const_cast<Element*>(this)->removeSynchronizedLazyAttribute(styleAttr);
On 2013/12/23 21:30:23, eseidel wrote:
> I remember being in this code for the hasAttribute/style-resolve patch (which
> you were kind enough to run with) and I can't remember if setAtribute('foo',
> null) will actually execute a remove?  Is removeSynchronizedLazyAttribute
> actually necessary?
> 
> (I remember thsi because I was confused as to if synchronizing lazy attributes
> could ever cause them to be removed.  It seemed liek the current
implementation
> made that impossible by accident iirc.)

Will adjust this method to use setSynchronizedLazyAttribute always.

https://codereview.chromium.org/119533003/diff/180001/Source/core/editing/Rem...
File Source/core/editing/RemoveCSSPropertyCommand.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/editing/Rem...
Source/core/editing/RemoveCSSPropertyCommand.cpp:53: if (!style) {
On 2013/12/23 21:22:43, eseidel wrote:
> Although (I think?) we allow {} around one-line ifs these days, historically
> they were verbotten and most places in blink still avoid them.

Done.

[chrishtr, 2013-12-23 22:34:20]

Changed to constructor instead of create() pattern. Everything looks like it
works fine. The only reason I can think of for the create() pattern being better
is that its return value is a PassRefPtr, which ensures reference counting. But
overkill & outweighed by clearer code?

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
File Source/core/css/VariablesIterator.cpp (right):

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
Source/core/css/VariablesIterator.cpp:30: void
AbstractVariablesIterator::initRemainingNames(
On 2013/12/23 22:06:46, Chris Harrelson wrote:
> On 2013/12/23 21:30:23, eseidel wrote:
> > Could this just be part of the AbstractVariablesIterator constructor?  If
not,
> > is there a way we can ASSERT when implementors fail to call this method?
> 
> I think it can, per your other comment. Will fix.

Done.

https://codereview.chromium.org/119533003/diff/180001/Source/core/css/Variabl...
Source/core/css/VariablesIterator.cpp:51:
AbstractVariablesIterator::initRemainingNames(propertySet, iterator.get());
On 2013/12/23 21:30:23, eseidel wrote:
> As I commented in a previous pass, it's uncommon to have ::create() do
anything
> besides wrap the new calls in an adoptRef.  That said, if this needs to be
done
> this way it's totally fine.  I would add a comment in the header next to
> initRemainingNames() that all subclasses must call it, etc.

Done.

[eseidel, 2013-12-23 23:26:28]

I think this looks right.  Except for the removal of ::create().

Remind me why you need to add InlineVariablesIterator?  (Or explain so in the CL
description).

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/InlineV...
File Source/core/css/InlineVariablesIterator.h (right):

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/InlineV...
Source/core/css/InlineVariablesIterator.h:29: // A CSSVariablesIterator
implementation for the inline style of an element.
Odd.  So the only thing this provides is mapping from an element to its property
set?  Strange that we don't have a more generic way to do that that we just
provide to a generic iterator.

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/StylePr...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/StylePr...
Source/core/css/StylePropertySet.cpp:121: return new VariablesIterator(this);
Oh. The ::create() pattern is still perfered (since we're not correctly calling
adoptRef here!)  It's just create methods are generally mostly empty:

static PassRefPtr<Foo> Foo::create(Bar*)
{
   return adoptRef(new Foo(bar));
}

the adoptRef is important, because RefCounted subclasses start with a single
ref, which is adopted by adoptRef() and placed into a PassRefPtr.

[eseidel, 2013-12-23 23:29:38]

On 2013/12/23 22:34:20, Chris Harrelson wrote:
> Changed to constructor instead of create() pattern. Everything looks like it
> works fine. The only reason I can think of for the create() pattern being
better
> is that its return value is a PassRefPtr, which ensures reference counting.
But
> overkill & outweighed by clearer code?

Sorry.  I was unclear in my previous review.  Foo::create() *is* the right
pattern.  It's just that we normally end up putting all the logic in the
(private) constructor and the ::create() call is just a dumb wrapper.

::create() calls will probably completely go away once we have Oilpan and don't
have refcounting.  ::create() was added in part to avoid leaks (due to lack of
PassRefPtr) as well as to allow RefCounted to start with a ref-count of 1 (as a
minor perf optimization from years ago).

[chrishtr, 2013-12-23 23:54:50]

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/InlineV...
File Source/core/css/InlineVariablesIterator.h (right):

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/InlineV...
Source/core/css/InlineVariablesIterator.h:29: // A CSSVariablesIterator
implementation for the inline style of an element.
On 2013/12/23 23:26:28, eseidel wrote:
> Odd.  So the only thing this provides is mapping from an element to its
property
> set?  Strange that we don't have a more generic way to do that that we just
> provide to a generic iterator.

I think it's maybe because of non-standard semantics. In particular, if you add
stuff while iterating it will include the stuff you added at the end. (I noticed
that part while debugging a failing test last week.)

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/StylePr...
File Source/core/css/StylePropertySet.cpp (right):

https://codereview.chromium.org/119533003/diff/280001/Source/core/css/StylePr...
Source/core/css/StylePropertySet.cpp:121: return new VariablesIterator(this);
On 2013/12/23 23:26:28, eseidel wrote:
> Oh. The ::create() pattern is still perfered (since we're not correctly
calling
> adoptRef here!)  It's just create methods are generally mostly empty:
> 
> static PassRefPtr<Foo> Foo::create(Bar*)
> {
>    return adoptRef(new Foo(bar));
> }
> 
> the adoptRef is important, because RefCounted subclasses start with a single
> ref, which is adopted by adoptRef() and placed into a PassRefPtr.

Done.

[eseidel, 2013-12-24 00:04:51]

lgtm

I still don't quite understand the addition of the iterator.  Please explain
that in the CL description before landing.  (In general is nice to make CL
descriptions very descriptive, since they're part of the permanent record of the
svn repo and tend to be one of the first things people check when seeing your
patch in the log/annotate.)

[chrishtr, 2013-12-24 00:14:12]

I updated the description. It's a subtle issue regarding object lifetimes.

[commit-bot: I haz the power, 2013-12-24 00:14:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/119533003/360001

[commit-bot: I haz the power, 2013-12-24 00:45:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/119533003/300002

[commit-bot: I haz the power, 2013-12-24 01:39:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/chrishtr@chromium.org/119533003/300002

[commit-bot: I haz the power, 2013-12-24 02:29:04]

Change committed as 164307