717066 - Remove unnecessary nsresult return value from nsGenericHTMLElement::GetLayoutHistoryAndKey

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Holbert [:dholbert]]

Ever since it was first checked in (in bug 108309 back in 2002), the method nsGenericHTMLElement::GetLayoutHistoryAndKey has had a nsresult return value that we ignore everywhere it's called.  (See bug 717004 comment 0).

From the callers' perspectives, GetLayoutHistoryAndKey indicates failure by leaving its |aHistory| argument untouched (e.g. as nsnull).  Since it's already got that de-facto method of indicating failure, we should just drop the nsresult return value.

Comment 1

[Daniel Holbert [:dholbert]]

There are only two callers of GetLayoutHistoryAndKey.

One is effectively:
 GetLayoutHistoryAndKey(..., getter_AddRefs(history));
 if (history) {
   [bulk of the method]
 }
 return;

and the other is effectively:
 GetLayoutHistoryAndKey(..., getter_AddRefs(history));
 if (!history) {
   return false;
 }

So in both cases, we only care about what GetLayoutHistoryAndKey did if it returns a non-null |history| pointer.  Volkmar pointed out in bug 717004 comment 10 that a null |history| pointer doesn't necessarily indicate failure, but I don't think that really matters from the clients' perspectives.

Comment 2

[Mounir Lamouri (:mounir)]

Wouldn't be better to return an already_Addrefed object insead of having an out param?

Comment 3

[Daniel Holbert [:dholbert]]

Sure, that sounds reasonable to me.

Comment 4

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 5

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 6

[Mounir Lamouri (:mounir)]

Why not returning an already_Addrefed object?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

That works, but I'd prefer changing the signature to

already_AddRefed<nsILayoutHistoryState>
GetLayoutHistoryAndKey(nsGenericHTMLElement* aContent,
                       bool aRead,
                       nsACString& aKey);

so you would start the function with

nsCOMPtr<nsILayoutHistoryState> history = doc->GetLayoutHistoryState();

change the references to |*aHistory| to |history|, get rid of the NS_RELEASEs, and return either |nsnull| (in case of failure), or |history.forget()| (in case of success).

Want to take a shot at that?

Comment 8

[Jignesh Kakadiya [:jhk]]

> Want to take a shot at that?

Definitely:)

Comment 9

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 588402 [details] [diff] [review]
Patch

Review of attachment 588402 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a few small nits to change:

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1551,5 @@
>    nsresult result = NS_OK;
>  
>    nsCOMPtr<nsILayoutHistoryState> history;
>    nsCAutoString key;
> +  history = GetLayoutHistoryAndKey(aContent, false, key);

Make this

nsCAutoString key;
nsCOMPtr<nsILayoutHistoryState> history = GetLayoutHistoryAndKey(aContent, false, key);

@@ +1589,3 @@
>    }
>  
> +  if (aRead && !(history)->HasStates()) {

The ()s around history can go now.

@@ +1619,5 @@
>                                                nsIFormControl* aControl)
>  {
>    nsCOMPtr<nsILayoutHistoryState> history;
>    nsCAutoString key;
> +  history = GetLayoutHistoryAndKey(aContent, true, key);

Same here

::: content/html/content/src/nsGenericHTMLElement.h
@@ +472,5 @@
>     * @param aKey the key (out param)
>     */
> +  static already_AddRefed<nsILayoutHistoryState> GetLayoutHistoryAndKey(nsGenericHTMLElement* aContent,
> +                                                                        bool aRead,
> +                                                                        nsACString& aKey);

These lines are getting too long now, please move |static already_AddRefed<nsILayoutHistoryState>| onto a line by itself.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 588402 [details] [diff] [review]
Patch

Review of attachment 588402 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +1551,5 @@
>    nsresult result = NS_OK;
>  
>    nsCOMPtr<nsILayoutHistoryState> history;
>    nsCAutoString key;
> +  history = GetLayoutHistoryAndKey(aContent, false, key);

I would write:
nsCOMPtr<...> history = GetLayoutHistoryAndKey(...);

@@ +1619,5 @@
>                                                nsIFormControl* aControl)
>  {
>    nsCOMPtr<nsILayoutHistoryState> history;
>    nsCAutoString key;
> +  history = GetLayoutHistoryAndKey(aContent, true, key);

I would write:
nsCOMPtr<...> history = GetLayoutHistoryAndKey(...);

Comment 12

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 588402 [details] [diff] [review]
Patch

Yes, thanks Mounir

Comment 13

[Jignesh Kakadiya [:jhk]]

Attachment: Patch

Comment 14

[Mounir Lamouri (:mounir)]

Jignesh, you don't have to re-ask a review if someone r+ a patch and ask you to do a few minor changes.

Comment 15

[Jignesh Kakadiya [:jhk]]

Checkin-needed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f3d849c74a

Thanks for the patch!

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/f8f3d849c74a