[webkit-reviews] review denied: [Bug 3677] Safari fail to access a second time an element whose content was dynamically modified. : [Attachment 2610] Improve handling of duplicate ids

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Anders Carlsson <andersca at mac.com>'s
request for review:
Bug 3677: Safari fail to access a second time an element whose content was
dynamically modified.
http://bugzilla.opendarwin.org/show_bug.cgi?id=3677

Attachment 2610: Improve handling of duplicate ids
http://bugzilla.opendarwin.org/attachment.cgi?id=2610&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
The usual way to handle something where there's a cache is to make the member
mutable rather than making the method non-const. I suggest putting mutable in
from of the m_idCount declaration, and leaving the methods const.

In DocumentImpl::getElementById, you should use qId when calling find on
m_elementsById.

The code that checks the ID of an element should call hasID before calling
getAttribute(ATTR_ID); it's more efficient that way since most elements don't
have IDs.

I think it's a little strange to use a QDict<int>, but actually store the
integers as pointers. Maybe QDict<void> would be a better way to declare it for
now. And later we should change it to an actual integer dictionary.

This:

+	 int idCount;
+	 if ((idCount = (int)m_idCount.find(qId))) {

would be better written like this:

	if (int idCount = (int)m_idCount.find(qId)) {

But instead, I think the code should assert that an idCount is found. If it's
not, we have an internal inconsistency error and I'd like to drop into the
debugger. It's probably best to put the remove code in an "if (idCount <= 1)"
and then assert that the count is 1 in that branch. Or something similar and
equivalent.

Glad you used the strategy I suggested to fix the bug; I think this is simple
and will be nice and efficient.

One further idea about how to make m_idCount smaller that would make the code
only slightly more complex:

The m_idCount dictionary could hold counts that do not include the one in
m_elementsById. Thus we'd only bump the count by 1 if there was already an
element in m_elementsById, and we'd only decrement if it's an element other
than the one in m_elementsById being removed. Also, when an element was found
and put into m_elementsById, we would decrement the count.

The benefit here is that the m_idCount table would then be empty almost all the
time; we could even have the member variable be a pointer and allocate it only
when needed. Documents without duplicate IDs would have less overhead. Just an
idea -- the existing code is fine too.