1334986 - ActorsParent.cpp : Parameters are named differently in declaration vs implementation in TransactionBase::VerifyRequestParams

Status: RESOLVED FIXED

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

Description

[Andi [:andi]]

A good first bug for a beginner:
here: 
https://dxr.mozilla.org/mozilla-central/source/dom/indexedDB/ActorsParent.cpp?q=%2Bfunction%3A%22mozilla%3A%3Adom%3A%3AindexedDB%3A%3A%28anonymous+namespace%29%3A%3ATransactionBase%3A%3AVerifyRequestParams%28const+mozilla%3A%3Adom%3A%3AindexedDB%3A%3ASerializedKeyRange+%26%29+const%22&redirect_type=single#15215

The parameter name from the declaration differs from the parameter name from the definition.

Comment 1

[Aman Kumar]

can i work on this bug?

Comment 2

[Andi [:andi]]

Sure go ahead!
Thx

Comment 3

[Aman Kumar]

Attachment: renamed the parameter in definition as it was in declaration

Comment 4

[Andi [:andi]]

I'm not a reviewer for this module, please see the module owners: 
https://wiki.mozilla.org/Modules/All

Comment 5

[Andi [:andi]]

Comment on attachment 8832166 [details] [diff] [review]
renamed the parameter in definition as it was in declaration

We are getting closer, the patch looks OK, thank you, now you just need to ask for a review from a module owner: 
https://wiki.mozilla.org/Modules/All

As this is part of dom, here are the eligible reviewers:
https://wiki.mozilla.org/Modules/All#Document_Object_Model

Comment 6

[Aman Kumar]

sorry i didn't catch you early and yeah i will ask for review from module owner. Thanx :)

Comment 7

[Aman Kumar]

(In reply to Andi-Bogdan Postelnicu from comment #5)
> Comment on attachment 8832166 [details] [diff] [review]
> renamed the parameter in definition as it was in declaration
> 
> We are getting closer, the patch looks OK, thank you, now you just need to
> ask for a review from a module owner: 
> https://wiki.mozilla.org/Modules/All
> 
> As this is part of dom, here are the eligible reviewers:
> https://wiki.mozilla.org/Modules/All#Document_Object_Model

Now i have asked the module owner for the review.

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 8832166 [details] [diff] [review]
renamed the parameter in definition as it was in declaration

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

Shouldn't you do the same change to the VerifyRequestParams that takes a const OptionalKeyRange&?

::: dom/indexedDB/ActorsParent.cpp
@@ +15210,4 @@
>  
>    return true;
>  }
> +// renamed the parameter as defined earlier

Don't add this comment.

Comment 9

[Aman Kumar]

Sure. I will make the change.

Comment 10

[Aman Kumar]

Attachment: renamedParameter1.patch

Made changes for the VerifyRequestParams that takes a const
OptionalKeyRange& and also removed the comment as suggested.

Comment 11

[dwivediaakar]

Attachment: Bug 1334986 - Rectified function definition of VerifyRequestParams for SerializedKeyRange and OptionalKeyRange

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8833960 [details] [diff] [review]
Bug 1334986 - Rectified function definition of VerifyRequestParams for SerializedKeyRange and OptionalKeyRange

There is already someone working on this patch. Sorry

Comment 13

[Jan Varga [:janv]]

Comment on attachment 8833960 [details] [diff] [review]
Bug 1334986 - Rectified function definition of VerifyRequestParams for SerializedKeyRange and OptionalKeyRange

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

This looks good.

Comment 14

[Peter Van der Beken [:peterv]]

Can you please attach a combined patch of all the changes?

Comment 15

[Aman Kumar]

Attachment: new_patch.patch

I have attached the combined patch of all the changes.

Comment 16

[Shashwat Mishra]

Can someone help me out? I am a new developer and I wanted to try my hands on this bug.How do I get it's source code to?

Comment 17

[Andi [:andi]]

(In reply to Aman Kumar from comment #15)
> Created attachment 8835603 [details] [diff] [review]
> new_patch.patch
> 
> I have attached the combined patch of all the changes.

Great job, it seems that the patch is now reviewed and can be committed. But before that there is still one more step to do. Can you please change the name of the patch from:

>>Bug 1334986 - renamed the parameter in definition as it was in declaration of >>TransactionBase::VerifyRequestParams

to 

>>Bug 1334986 - renamed the parameter in definition as it was in declaration of TransactionBase::VerifyRequestParams. r=peterv

In this way we also include the reviewers nick to the commit message. After you do this you only need to update the patch. 

After the patch is updated it must be marked as checkin-needed in the keywords section.

If you need help just let me know!

Comment 18

[Andi [:andi]]

(In reply to Shashwat Mishra from comment #16)
> Can someone help me out? I am a new developer and I wanted to try my hands
> on this bug.How do I get it's source code to?

Hello, this bug is almost finished, but you can search for other bugs having keywords like: good-first-bug

Comment 19

[Shashwat Mishra]

(In reply to Andi-Bogdan Postelnicu from comment #18)
> (In reply to Shashwat Mishra from comment #16)
> > Can someone help me out? I am a new developer and I wanted to try my hands
> > on this bug.How do I get it's source code to?
> 
> Hello, this bug is almost finished, but you can search for other bugs having
> keywords like: good-first-bug
Well can you allot me an easy bug to start with?

Comment 20

[Andi [:andi]]

(In reply to Shashwat Mishra from comment #19)
> (In reply to Andi-Bogdan Postelnicu from comment #18)
> > (In reply to Shashwat Mishra from comment #16)
> > > Can someone help me out? I am a new developer and I wanted to try my hands
> > > on this bug.How do I get it's source code to?
> > 
> > Hello, this bug is almost finished, but you can search for other bugs having
> > keywords like: good-first-bug
> Well can you allot me an easy bug to start with?

What do you say about this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1337294
If you are planning on working on this you could simply assign it on your name.

Comment 21

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 23

[Sachin]

Since it is reopened, I want to work on this bug.

Comment 24

[Jens Stutte [:jstutte]]

:andi, are you still available as mentor here? Thank you!

Comment 25

[Andi [:andi]]

Sure I can mentor this.
Sachin if you want to work on this please assign it to you.

Comment 26

[Sachin]

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #25)

Sure I can mentor this.
Sachin if you want to work on this please assign it to you.

Hi, I am actually confused about earlier patches. They were looking fine. What was wrong there ?? Thanks

Comment 27

[Andi [:andi]]

(In reply to Sachin from comment #26)

(In reply to Andi-Bogdan Postelnicu [:andi] from comment #25)

Sure I can mentor this.
Sachin if you want to work on this please assign it to you.

Hi, I am actually confused about earlier patches. They were looking fine. What was wrong there ?? Thanks

The patch was OK, it wasn't landed. I think the patch needs to be rebased and ask for review one more time.

Comment 28

[Sneha k]

Attachment: Bug 1334986 - changed the parameters in declarations and definitions of Transaction Base::VerifyRequestParams. r?peterv

Depends on D106257

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/615d4abfd69e
changed the parameters in declarations and definitions of Transaction Base::VerifyRequestParams. r=sg

Comment 30

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/615d4abfd69e