Last Comment Bug 717066 - Remove unnecessary nsresult return value from nsGenericHTMLElement::GetLayoutHistoryAndKey
: Remove unnecessary nsresult return value from nsGenericHTMLElement::GetLayout...
Status: RESOLVED FIXED
[good first bug][mentor=Ms2ger][lang=...
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jignesh Kakadiya [:jhk]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-10 15:17 PST by Daniel Holbert [:dholbert]
Modified: 2012-01-19 02:50 PST (History)
3 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.28 KB, patch)
2012-01-13 05:19 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (3.27 KB, patch)
2012-01-13 05:21 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch (4.71 KB, patch)
2012-01-13 06:32 PST, Jignesh Kakadiya [:jhk]
Ms2ger: review+
Details | Diff | Splinter Review
Patch (5.14 KB, patch)
2012-01-13 07:01 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-01-10 15:17:13 PST
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] 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

Note You need to log in before you can comment on or make changes to this bug.