Make Element::attributes() less error-prone and simplify call sites

Make Element::attributes() less error-prone and simplify call sites

Make Element::attributes() less error-prone by:
- Having it return an empty AttributeCollection if ElementData() is null
  so that the callers no longer have to call hasAttributes() to know if
  they can call Element::attributes().
- Call synchronizeAllAttributes() in attributes() as callers no longer
  have to call hasAttributes() before calling attributes().

This CL also adds an Element::attributesWithoutUpdate() getter for call
sites where we do not want to synchronize all attributes (e.g. no attribute
synchronization or single attribute synchronization).

This CL also drops the Element::hasAttributesWithoutUpdate() as there are no
remaining callers.

This simplifies the call sites and makes Element::attributes() less error
prone.

R=esprehn@chromium.org, adamk@chromium.org

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

[adamk, 2014-08-04 18:22:32]

From the CL description, I'm wondering how you figured out where to replace
calls to attributes() with calls to attributesWithoutUpdate(). It seems like it
might be hard to tell which callers explicitly do not want an update...

[Inactive, 2014-08-04 18:28:15]

On 2014/08/04 18:22:32, adamk wrote:
> From the CL description, I'm wondering how you figured out where to replace
> calls to attributes() with calls to attributesWithoutUpdate(). It seems like
it
> might be hard to tell which callers explicitly do not want an update...

Basically, if callers were calling hasAttributes() (or
synchronizeAllAttributes()) before calling attributes() then they did
synchronization and they now merely call attributes().
If callers were calling hasAttributesWithoutUpdate() before calling
attributes(), then they did not do synchronization and they now call
attributesWithoutUpdate() instead of attributes().

Summary of changes (before -> after):
- hasAttributes() + attributes() -> attributes()
- synchronizeAllAttributes() + attributes() -> attributes()
- hasAttributesWithoutUpdate() + attributes() -> attributesWithoutUpdate()

[Inactive, 2014-08-04 18:38:11]

Basically, the biggest risk is if I replace hasAttributes()+attributes() or
synchronizeAllAttributes()+attributes() by attributesWithoutUpdate() because
this would mean that we used to synchronize the attributes and now we don't
(potentially giving wrong results). However, there are few callers to
attributesWithoutUpdate() in the patch and it should be obvious they did not do
synchronization before (no call to hasAttributes() or
synchronizeAllAttributes()).

[adamk, 2014-08-04 18:54:35]

I like getting rid of the hasAttributes() calls everywhere, but I'm a little
worried about attributes() vs elementData()->attributes() inside Element (see
below).

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

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2490: if (!attrNodes || attrNodes->isEmpty())
Why did this change?

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2963: AttributeCollection attributes =
elementData()->attributes();
I've seen enough of these that it worries me someone will get it wrong in the
future. Perhaps even Element.cpp should use attributesWithoutUpdate() instead of
elementData()->attributes() to make it clearer what the callers are trying to
do? Or do you think the branch there is too expensive?

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:710: if (!elementData())
Is it really safe to check this before calling synchronize? I guess it's safe
for style, because that's stored in element data, but what about SVG stuff?
Couldn't this break if that state was elsewhere?

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:726: return elementData() &&
!elementData()->attributes().isEmpty();
Can't this just be:

return !attributes().isEmpty();

https://codereview.chromium.org/436603003/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/436603003/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:451: AttributeCollection attributes =
this->attributesWithoutUpdate();
Nit: you can drop |this| here.

[Inactive, 2014-08-04 19:09:47]

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

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2490: if (!attrNodes || attrNodes->isEmpty())
On 2014/08/04 18:54:34, adamk wrote:
> Why did this change?

I wanted to avoid the ElementData() null check in hasAttributes(), but it could
stay as is without any perf regression (I did not change hasAttributes()).

I'll revert this part of the change to reduce the diff.

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2963: AttributeCollection attributes =
elementData()->attributes();
On 2014/08/04 18:54:34, adamk wrote:
> I've seen enough of these that it worries me someone will get it wrong in the
> future. Perhaps even Element.cpp should use attributesWithoutUpdate() instead
of
> elementData()->attributes() to make it clearer what the callers are trying to
> do? Or do you think the branch there is too expensive?

I did not want do cause more work than before my patch to avoid any chance of
perf regression. If we call attributesWithoutUpdate() here, then we do an extra
null-check on ElementData(). I haven't measured if it matters, I just did not
want to chance it.

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:695: return elementData() &&
elementData()->attributes().findIndex(name) != kNotFound;
I could call "return attributesWithoutUpdate().findIndex(name) != kNotFound;"

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:702: if (const Attribute* attribute =
elementData()->attributes().find(name))
I could call "return attributesWithoutUpdate().findIndex(name) != kNotFound;"

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:710: if (!elementData())
On 2014/08/04 18:54:34, adamk wrote:
> Is it really safe to check this before calling synchronize? I guess it's safe
> for style, because that's stored in element data, but what about SVG stuff?
> Couldn't this break if that state was elsewhere?

synchronizeAllAttributes() returns early if elementData() is null so it is safe.
I put the calls in the order to avoid incurring the call of
synchronizeAllAttributes() as it is not inline.

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:726: return elementData() &&
!elementData()->attributes().isEmpty();
On 2014/08/04 18:54:34, adamk wrote:
> Can't this just be:
> 
> return !attributes().isEmpty();

Good point, yes. My original version of this patch didn't synchronize in
hasAttributes() so I couldn't do this. However, I noticed after this could cause
trouble for the style attribute so I put the synchronizeAllAttributes() call
back. I'll refactor as suggested.

[adamk, 2014-08-04 19:29:52]

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

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2963: AttributeCollection attributes =
elementData()->attributes();
The more I think about this the more it's a reasonable compromise to maintain
existing performance...

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:695: return elementData() &&
elementData()->attributes().findIndex(name) != kNotFound;
For these cases I definitely see the advantage of avoiding unnecessary logic,
I'd suggest leaving them as-is.

[Inactive, 2014-08-04 19:39:52]

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

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2490: if (!attrNodes || attrNodes->isEmpty())
On 2014/08/04 19:09:46, Chris Dumez wrote:
> On 2014/08/04 18:54:34, adamk wrote:
> > Why did this change?
> 
> I wanted to avoid the ElementData() null check in hasAttributes(), but it
could
> stay as is without any perf regression (I did not change hasAttributes()).
> 
> I'll revert this part of the change to reduce the diff.

Done.

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.cpp:2963: AttributeCollection attributes =
elementData()->attributes();
On 2014/08/04 19:29:51, adamk wrote:
> The more I think about this the more it's a reasonable compromise to maintain
> existing performance...

Ok, I kept it as is.

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:695: return elementData() &&
elementData()->attributes().findIndex(name) != kNotFound;
On 2014/08/04 19:29:51, adamk wrote:
> For these cases I definitely see the advantage of avoiding unnecessary logic,
> I'd suggest leaving them as-is.

Ok, I kept them as is.

https://codereview.chromium.org/436603003/diff/40001/Source/core/dom/Element....
Source/core/dom/Element.h:726: return elementData() &&
!elementData()->attributes().isEmpty();
On 2014/08/04 18:54:34, adamk wrote:
> Can't this just be:
> 
> return !attributes().isEmpty();

Done.

https://codereview.chromium.org/436603003/diff/40001/Source/core/html/HTMLInp...
File Source/core/html/HTMLInputElement.cpp (right):

https://codereview.chromium.org/436603003/diff/40001/Source/core/html/HTMLInp...
Source/core/html/HTMLInputElement.cpp:451: AttributeCollection attributes =
this->attributesWithoutUpdate();
On 2014/08/04 18:54:34, adamk wrote:
> Nit: you can drop |this| here.

Done.

[adamk, 2014-08-04 19:47:44]

lgtm

[Inactive, 2014-08-04 20:00:43]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-08-04 20:01:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/436603003/60001

[commit-bot: I haz the power, 2014-08-04 20:44:31]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20605)

[commit-bot: I haz the power, 2014-08-04 21:32:35]

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

[commit-bot: I haz the power, 2014-08-04 21:32:37]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20627)

[Inactive, 2014-08-04 23:30:40]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-08-04 23:31:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/436603003/60001

[commit-bot: I haz the power, 2014-08-05 00:13:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20673)

[commit-bot: I haz the power, 2014-08-05 00:58:17]

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

[commit-bot: I haz the power, 2014-08-05 00:58:18]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20685)

[Inactive, 2014-08-05 13:04:37]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-08-05 13:09:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/436603003/60001

[commit-bot: I haz the power, 2014-08-05 14:32:03]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20926)

[commit-bot: I haz the power, 2014-08-05 15:57:03]

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

[commit-bot: I haz the power, 2014-08-05 15:57:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20964)

[Inactive, 2014-08-05 19:07:32]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-08-05 19:09:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/436603003/60001

[commit-bot: I haz the power, 2014-08-05 21:10:02]

Change committed as 179526