Comment 1 Daniel Holbert [:dholbert] 2012-01-10 15:24:09 PST

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) 2012-01-10 15:27:54 PST

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

Comment 3 Daniel Holbert [:dholbert] 2012-01-10 15:28:33 PST

Sure, that sounds reasonable to me.

Comment 4 Jignesh Kakadiya [:jhk] 2012-01-13 05:19:16 PST

Created attachment 588386 [details] [diff] [review]
Patch

Comment 5 Jignesh Kakadiya [:jhk] 2012-01-13 05:21:10 PST

Created attachment 588387 [details] [diff] [review]
Patch

Comment 6 Mounir Lamouri (:mounir) 2012-01-13 05:23:30 PST

Why not returning an already_Addrefed object?

Comment 7 :Ms2ger (⌚ UTC+1/+2) 2012-01-13 05:26:27 PST

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] 2012-01-13 05:35:12 PST

> Want to take a shot at that?

Definitely:)

Comment 9 Jignesh Kakadiya [:jhk] 2012-01-13 06:32:41 PST

Created attachment 588402 [details] [diff] [review]
Patch

Comment 10 :Ms2ger (⌚ UTC+1/+2) 2012-01-13 06:37:13 PST

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) 2012-01-13 06:37:46 PST

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 (⌚ UTC+1/+2) 2012-01-13 06:38:46 PST

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

Yes, thanks Mounir

Comment 13 Jignesh Kakadiya [:jhk] 2012-01-13 07:01:42 PST

Created attachment 588407 [details] [diff] [review]
Patch

Comment 14 Mounir Lamouri (:mounir) 2012-01-13 07:03:38 PST

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] 2012-01-17 06:53:59 PST

Checkin-needed.

Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-01-18 13:15:17 PST

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

Thanks for the patch!

Comment 17 Marco Bonardo [::mak] 2012-01-19 02:50:15 PST

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