[webkit-reviews] review granted: [Bug 13602] Amazon product pages keep repainting over and over again : [Attachment 14398] new patch, now with passing layout tests (oops!)

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue May 29 01:39:43 PDT 2007


Maciej Stachowiak <mjs at apple.com> has granted Darin Adler <darin at apple.com>'s
request for review:
Bug 13602: Amazon product pages keep repainting over and over again
http://bugs.webkit.org/show_bug.cgi?id=13602

Attachment 14398: new patch, now with passing layout tests (oops!)
http://bugs.webkit.org/attachment.cgi?id=14398&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>

In +static bool shallowEqual(Node* a, Node* b) you have a comment:

+	     // Don't bother comparing these node types for now; they don't
typically occur.
+	     // Since these functions are used only for optimization, it's OK
to have a false negative.

Some of these node types literally can't ever appear inside a document tree (at
least document, document fragment, and xpath namespace nodes), not sure if it
is worth putting a different comment for those.

replaceChildrenWithFragment() codes the algorithm to compare whether children
are identical. Would it not be better to factor that out into a
contentsAreEqual() function?

The optimizations for replacing a single child and for replacing a sole text
child with different text are a bit hard to follow at first. (a) are these
really helpful in real cases? (b) could the if statements be coded to use
functions that have clear names for what they are checking? Perhaps similar for
replaceChildrenWithText.

setOuterHTML also has a deep identical contents check, perhaps refactoring
would allow this to be shared with setInnerHTML more.

r=me notwithstanding the style comments.



More information about the webkit-reviews mailing list