Bug 4925 – HTMLMapElementImpl::mapMouseEvent can be implemented without a stack

• Component: DOM
• Status: RESOLVED FIXED

[Darin Adler]

[Darin Adler]

Created attachment 3852 [details]
clean up of html_imageimpl.cpp, including eliminating that unnecessary stack

[Maciej Stachowiak]

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.

[Dave Hyatt]

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.

[Dave Hyatt]

I prefer 0 over NULL, and that is the current convention (well-established for some time) in KHTML.

[Darin Adler]

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.

[Darin Adler]

Meant to say "gives you an error if you accidentally use it as an int", but I'm not really sure about that 
anyway.

[Darin Adler]

Created attachment 4311 [details]
patch with Maciej's requested changes

[Maciej Stachowiak]

Comment on attachment 4311 [details]
patch with Maciej's requested changes

r=me

[Eric Seidel (no email)]

No longer merges cleanly... I didn't bother to fight with it long given that darin is a commiter anyway.