Remove unnecessary nsresult return value from nsGenericHTMLElement::GetLayoutHistoryAndKey

RESOLVED FIXED in mozilla12

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: jhk)

Tracking

Trunk
mozilla12
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=Ms2ger][lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
OS: Linux → All
Hardware: x86_64 → All
(Reporter)

Comment 1

6 years ago
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.
Wouldn't be better to return an already_Addrefed object insead of having an out param?
(Reporter)

Comment 3

6 years ago
Sure, that sounds reasonable to me.

Updated

6 years ago
Whiteboard: [good first bug][mentor=Ms2ger][lang=c++]
(Assignee)

Comment 4

6 years ago
Created attachment 588386 [details] [diff] [review]
Patch
Assignee: nobody → jigneshhk1992
Attachment #588386 - Flags: review?(Ms2ger)
(Assignee)

Comment 5

6 years ago
Created attachment 588387 [details] [diff] [review]
Patch
Attachment #588386 - Attachment is obsolete: true
Attachment #588386 - Flags: review?(Ms2ger)
Attachment #588387 - Flags: review?(Ms2ger)
Why not returning an already_Addrefed object?
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?
(Assignee)

Comment 8

6 years ago
> Want to take a shot at that?

Definitely:)
(Assignee)

Comment 9

6 years ago
Created attachment 588402 [details] [diff] [review]
Patch
Attachment #588387 - Attachment is obsolete: true
Attachment #588387 - Flags: review?(Ms2ger)
Attachment #588402 - Flags: review?(Ms2ger)
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.
Attachment #588402 - Flags: review?(Ms2ger) → review+
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(...);
Attachment #588402 - Flags: review+ → review?(Ms2ger)
Comment on attachment 588402 [details] [diff] [review]
Patch

Yes, thanks Mounir
Attachment #588402 - Flags: review?(Ms2ger) → review+
(Assignee)

Comment 13

6 years ago
Created attachment 588407 [details] [diff] [review]
Patch
Attachment #588402 - Attachment is obsolete: true
Attachment #588407 - Flags: review?(Ms2ger)
Jignesh, you don't have to re-ask a review if someone r+ a patch and ask you to do a few minor changes.
Attachment #588407 - Flags: review?(Ms2ger)
(Reporter)

Updated

6 years ago
Summary: Make nsGenericHTMLElement::GetLayoutHistoryAndKey return void → Remove unnecessary nsresult return value from nsGenericHTMLElement::GetLayoutHistoryAndKey
(Assignee)

Comment 15

6 years ago
Checkin-needed.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8f3d849c74a

Thanks for the patch!
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/f8f3d849c74a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.