Bug 716338 turned on warnings-as-errors in content/html/content/src/Makefile.in This breaks my local build (with --enable-warnings-as-errors), due to this build warning: { nsGenericHTMLElement.cpp:1627:12: error: variable ‘rv’ set but not used [-Werror=unused-but-set-variable] } which points to these line of code: > 1627 nsresult rv = GetLayoutHistoryAndKey(aContent, true, > 1628 getter_AddRefs(history), key); [...] > 1635 rv = history->GetState(key, &state); where we capture GetLayoutHistoryAndKey's return value and ignore it. That issue actually dates back to a patch for bug 108309 that landed in 2002: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/content/html/content/src&command=DIFF_FRAMESET&file=nsGenericHTMLElement.cpp&rev2=1.343&rev1=1.342 That patch added one other call to GetLayoutHistoryAndKey, whose return-value is *not* checked (and still isn't checked to this day). Given that we've always ignored the return-value in both places (explicitly in one place, implicitly in the other), seems like we might as well make both places explicitly ignore it.

FWIW, the only way that GetLayoutHistoryAndKey can fail is if nsContentUtils::GenerateStateKey fails. GenerateStateKey, in turn, has "return NS_OK;" as its only explicit return statements. It also has 2 NS_ENSURE_ statements that could return failure codes, if (a) a null 'aContent' pointer is passed in, or (b) if we're out of memory. We know the former isn't true, because we dereference the passed-in aContent pointer in GetLayoutHistoryAndKey before we call GenerateStateKey. And if the latter is true, we're kind of screwed anyway. So I think it's safe to just drop the "nsresult rv =" and assume that GetLayoutHistoryAndKey succeeds (as we already do elsewhere).

Comment on attachment 587448 [details] [diff] [review] fix Review of attachment 587448 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/html/content/src/nsGenericHTMLElement.cpp @@ +1631,5 @@ > } > > nsPresState *state; > // Get the pres state for this key > + nsresult rv = history->GetState(key, &state); You don't seem to be using |rv| again... Seems like you could just remove |rv| from the patch. BTW, I would change: if (state) { // blah } return false; to: if (!state) { return false; } // blah

Oh -- I didn't notice it, until now, but that part is addressed in bug 717015. :) (with an rv check) I'll post a merged patch here.

Here's the patch with bug 717015's fix merged in. So when this "GetState()" call originally landed, it was passing in a nsCOMPtr, which was guaranteed to be left at its default value (null) if GetState failed. So the null-check was basically sufficient/correct at that time. However, now we're passing in a raw uninitialized pointer, and if GetState were to fail, we absolutely should not be relying on that raw pointer being null or containing anything useful. So, this patch adds a NS_SUCCEEDED() check before we test that pointer's value. (As it turns out, we actually know that the underlying impl for this interface method will always succeed; still, best not to hardcode that assumption in if we don't have to.)

Comment on attachment 587491 [details] [diff] [review] fix v2 Review of attachment 587491 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the follow-up opened. ::: content/html/content/src/nsGenericHTMLElement.cpp @@ +1624,5 @@ > { > nsCOMPtr<nsILayoutHistoryState> history; > nsCAutoString key; > + GetLayoutHistoryAndKey(aContent, true, > + getter_AddRefs(history), key); Could you open a follow-up to make GetLayoutHistoryAndKey infallible? Seems like the only reason to not return NS_OK is when GenerateStateKey doesn't return NS_OK but it happens that this method *always* return NS_OK. Let's remove the |nsresult| return value.

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #7) > Seems > like the only reason to not return NS_OK is when GenerateStateKey doesn't > return NS_OK but it happens that this method *always* return NS_OK. Let's > remove the |nsresult| return value. Not quite -- GenerateStateKey can fail. (via NS_ENSURE_* statments) See comment 1.

I'm still happy to file the followup, though; GetLayoutHistoryAndKey could still change to indicate failure by returning a null |history| pointer. (which is basically what we depend on right now anyway.)

(In reply to Daniel Holbert [:dholbert] from comment #8) > Not quite -- GenerateStateKey can fail. (via NS_ENSURE_* statments) See > comment 1. Oups... Sorry :( (In reply to Daniel Holbert [:dholbert] from comment #9) > I'm still happy to file the followup, though; GetLayoutHistoryAndKey could > still change to indicate failure by returning a null |history| pointer. > (which is basically what we depend on right now anyway.) Seems like |GetLayoutHistoryAndKey| returns NS_OK but has a null |history| pointer in one case. Maybe some callers are using that?

Filed bug 717066 as the followup requested in comment 7. (In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #10) > Seems like |GetLayoutHistoryAndKey| returns NS_OK but has a null |history| > pointer in one case. Maybe some callers are using that? I don't think so. In any case, we can figure that out in the followup. (bug 717066)