[webkit-reviews] review denied: [Bug 4925] HTMLMapElementImpl::mapMouseEvent can be implemented without a stack : [Attachment 3852] clean up of html_imageimpl.cpp, including eliminating that unnecessary stack

[bugzilla-request-daemon at opendarwin.org]

Maciej Stachowiak <mjs at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 4925: HTMLMapElementImpl::mapMouseEvent can be implemented without a stack
http://bugzilla.opendarwin.org/show_bug.cgi?id=4925

Attachment 3852: clean up of html_imageimpl.cpp, including eliminating that
unnecessary stack
http://bugzilla.opendarwin.org/attachment.cgi?id=3852&action=edit

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
Since this patch is all about code cleanup, I'm going to be more nitpicky about
style than I would be ordinarily:

-    if (m_image)
-	 m_image->deref(this);
+    if (CachedImage *image = m_image)
+	 image->deref(this);

Is this type of change really a cleanup? It looks less legible and I don't
think it will help performance in this case, since there's nothing between one
access to m_image and the next that would force the compiler to reload it. I'd
prefer not to see this type of change unless profiling tools show it matters
(assuming performance was the motivation).

I'm not going to complain about the similar changes to updateFromElement since
they don't render the code less legible; but they too smack of premature
optimization. I don't think we should go crazy putting things into local
variables except on code paths where it's hot enough to matter.


+    CachedImage *newImage = NULL;

Is NULL instead of 0 our official style for null pointers in C++ code? We used
to change stuff in the other direction, from NULL to 0. I'm happy to go with
either style and stick with it, if we codify it in our coding style guidelines.



-    if (attrName == widthAttr ||
-	 attrName == heightAttr ||
-	 attrName == vspaceAttr ||
-	 attrName == hspaceAttr ||
-	 attrName == valignAttr) {
+    if (attrName == widthAttr || attrName == heightAttr || attrName ==
vspaceAttr || attrName == hspaceAttr || attrName == valignAttr) {

Highly debatable that this change (and the similar ones below) is a cleanup,
the former is easier to read for me.


-    if (attr->name() == altAttr) {
+    const QualifiedName &attrName = attr->name();
+    if (attrName == altAttr) {

Again probably a premature optimization, but it doesn't make the code less
readable and makes it a bit more compact, so no objection from me.


in  void HTMLImageElementImpl::parseMappedAttribute(MappedAttributeImpl *attr),
WebKit coding style calls for no braces around the single-line if clauses; I
know this isn't really something you're changing, but perhaps worth taking care
of? I admit I doubt the appropriateness of skipping the braces in long if
chains like this where some cases have braces...


+	int width = getAttribute(widthAttr).qstring().toInt(&ok);

Looks like there are tabs in the patch around this line.


I love the replacement of the nodeStack code with traversNextNode. *kisses
fingers into the air* c'est magnifique!

I like all the other code changes besides the ones I complained about.

r- for the tabs, the ImageLoader destructor changes, and combining attr name
tests in ifs in single blocks. But make it an r+ if you fix those or convince
me they are actually good.