Bug 102845 – Make it possible for elements with different tag names to share attribute data.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Andreas Kling]

We currently prevent e.g <div style="display: none"> and <span style="display: none"> from sharing attribute data.
The reason we do this is because presentation attributes map to different CSS properties depending on the name of the tag they're on.

A cool optimization would be to force elements with presentation attributes to always use mutable attribute data.
This way we could share equivalent immutable attribute data between all element types, and shrink ImmutableElementAttributeData by one pointer.

[Andreas Kling]

Created attachment 175295 [details]
Patch

[WebKit Review Bot]

Attachment 175295 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/Element.cpp:2567:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/dom/Element.cpp:2568:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 175295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175295&action=review

> Source/WebCore/ChangeLog:25
> +        On the upside, excluding elements with presentation attributes from the data cache means that
> +        we can move one pointer (m_presentationAttributeStyle) from ImmutableElementAttributeData
> +        to MutableElementAttributeData.

I suppose we could also add 3rd state, ImmutableElementAttributeDataWithPresentationAttributeStyle. Might not be worth it.

[Andreas Kling]

Comment on attachment 175295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175295&action=review

>> Source/WebCore/ChangeLog:25
>> +        to MutableElementAttributeData.
> 
> I suppose we could also add 3rd state, ImmutableElementAttributeDataWithPresentationAttributeStyle. Might not be worth it.

That would be a reasonable way to go if this causes a monstrous regression on that Y2K content chromium tracks.

[Andreas Kling]

Committed r135421: <http://trac.webkit.org/changeset/135421>

[Ojan Vafai]

(In reply to comment #4)
> (From update of attachment 175295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175295&action=review
> 
> >> Source/WebCore/ChangeLog:25
> >> +        to MutableElementAttributeData.
> > 
> > I suppose we could also add 3rd state, ImmutableElementAttributeDataWithPresentationAttributeStyle. Might not be worth it.
> 
> That would be a reasonable way to go if this causes a monstrous regression on that Y2K content chromium tracks.

llol. It's funny cuz it's true. We talk about updating it once in a while, but noone's made it a priority. So far, our performance work has mostly gone into doing better about noticing regressions. I expect we'll get around to this eventually. I actually think tracking both old and new context is worthwhile.

Anyways, I agree. Lets see if we need it. I think it's unlikely that this will cause a regression in any real content.

[Jussi Kukkonen (jku)]

This seems to assert editing/undo/replace-by-span-then-remove.html:

11:40:02.722 9037   ASSERTION FAILED: !other.m_inlineStyle->isMutable()
11:40:02.722 9037   /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/dom/ElementAttributeData.cpp(76) : WebCore::ImmutableElementAttributeData::ImmutableElementAttributeData(const WebCore::MutableElementAttributeData&)
11:40:02.722 9037   1   0x7f4b3da4ada6 WebCore::ImmutableElementAttributeData::ImmutableElementAttributeData(WebCore::MutableElementAttributeData const&)
11:40:02.722 9037   2   0x7f4b3da4b2ff WebCore::ElementAttributeData::makeImmutableCopy() const
11:40:02.722 9037   3   0x7f4b3da41e5b WebCore::Element::cloneAttributesFromElement(WebCore::Element const&)
11:40:02.722 9037   4   0x7f4b3da41fc1 WebCore::Element::cloneDataFromElement(WebCore::Element const&)
11:40:02.722 9037   5   0x7f4b3db62171
11:40:02.722 9037   6   0x7f4b3db622c5 WebCore::ReplaceNodeWithSpanCommand::doApply()
11:40:02.722 9037   7   0x7f4b3db1d569 WebCore::SimpleEditCommand::doReapply()
11:40:02.722 9037   8   0x7f4b3db03c42 WebCore::EditCommandComposition::reapply()
11:40:02.722 9037   9   0x7f4b41f20a67 WebKit::WebPage::reapplyEditCommand(unsigned long)
11:40:02.722 9037   10  0x7f4b41fe5cc0 void CoreIPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long), unsigned long>(CoreIPC::Arguments1<unsigned long> const&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long))
11:40:02.722 9037   11  0x7f4b41fe44d3 void CoreIPC::handleMessage<Messages::WebPage::ReapplyEditCommand, WebKit::WebPage, void (WebKit::WebPage::*)(unsigned long)>(CoreIPC::MessageDecoder&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned long))
11:40:02.722 9037   12  0x7f4b41fe04fe WebKit::WebPage::didReceiveWebPageMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
11:40:02.722 9037   13  0x7f4b41f2171a WebKit::WebPage::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
11:40:02.722 9037   14  0x7f4b41cecedd CoreIPC::MessageReceiverMap::dispatchMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
11:40:02.722 9037   15  0x7f4b41e4e208 WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)
11:40:02.722 9037   16  0x7f4b41ce1e64 CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&)
11:40:02.722 9037   17  0x7f4b41ce1fd0 CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&)
11:40:02.722 9037   18  0x7f4b41cdfd07 CoreIPC::Connection::SyncMessageState::dispatchMessages()
11:40:02.722 9037   19  0x7f4b41ce1149 CoreIPC::Connection::waitForSyncReply(unsigned long, double, unsigned int)
11:40:02.722 9037   20  0x7f4b41ce0fb0 CoreIPC::Connection::sendSyncMessage(CoreIPC::MessageID, unsigned long, WTF::PassOwnPtr<CoreIPC::MessageEncoder>, double, unsigned int)
11:40:02.723 9037   21  0x7f4b41ee3780 bool CoreIPC::Connection::sendSync<Messages::WebPageProxy::ExecuteUndoRedo>(Messages::WebPageProxy::ExecuteUndoRedo const&, Messages::WebPageProxy::ExecuteUndoRedo::Reply const&, unsigned long, double, unsigned int)
11:40:02.723 9037   22  0x7f4b41ee2b10 bool CoreIPC::MessageSender<WebKit::WebPage>::sendSync<Messages::WebPageProxy::ExecuteUndoRedo>(Messages::WebPageProxy::ExecuteUndoRedo const&, Messages::WebPageProxy::ExecuteUndoRedo::Reply const&, unsigned long, double)
11:40:02.723 9037   23  0x7f4b41ee2307 bool CoreIPC::MessageSender<WebKit::WebPage>::sendSync<Messages::WebPageProxy::ExecuteUndoRedo>(Messages::WebPageProxy::ExecuteUndoRedo const&, Messages::WebPageProxy::ExecuteUndoRedo::Reply const&, double)
11:40:02.723 9037   24  0x7f4b41ee0c23 WebKit::WebEditorClient::redo()
11:40:02.723 9037   25  0x7f4b3db2cbce WebCore::Editor::redo()
11:40:02.723 9037   26  0x7f4b3db3d77e
11:40:02.723 9037   27  0x7f4b3db3f389 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
11:40:02.723 9037   28  0x7f4b3d9dd9da WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
11:40:02.723 9037   29  0x7f4b3e69cf7d WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)
11:40:02.723 9037   30  0x7f4aed3ed265