Created attachment 3852 [details] clean up of html_imageimpl.cpp, including eliminating that unnecessary stack
Comment on attachment 3852 [details] clean up of html_imageimpl.cpp, including eliminating that unnecessary stack 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.
I should say that I find this style: - if (m_image) - m_image->deref(this); + if (CachedImage *image = m_image) + image->deref(this); to be exceptionally ugly. I don't think we should do this unless it really matters for performance.
I prefer 0 over NULL, and that is the current convention (well-established for some time) in KHTML.
OK. Re: NULL vs. 0 -- I've always used 0 in my own C++ programs. I was actually using NULL because I thought Maciej preferred it (seemed to recall it in a patch) and because it gives you an error if you accidentally mix it with 0. I'd be happy to go the other way and just use 0 all the time. Re: variables declared in an if statement: If you guys don't like it, I'd be happy to forgo it. I think it looks great and it's cool how it scopes the variable so you can't accidentally use it if it's null. But this is very unimportant to me and it seems we have consensus against it! Point taken from you two about local variables. While I do think we should go crazy putting things into local variables, I don't feel strongly enough about it to try to convince the two of you; I can't think of any compelling arguments either way. Re: the long multi-line if statements with || on the end -- I find them hard to read but I see now that it's only my personal preference with no consensus. Since my editor will never let me type a tab, the tabs must come from copy and paste. I'll fix it all and put up a new patch for review some day.
Meant to say "gives you an error if you accidentally use it as an int", but I'm not really sure about that anyway.
Created attachment 4311 [details] patch with Maciej's requested changes
Comment on attachment 4311 [details] patch with Maciej's requested changes r=me
No longer merges cleanly... I didn't bother to fight with it long given that darin is a commiter anyway.