ActorsParent.cpp : Parameters are named differently in declaration vs implementation in AutoSavepoint::Start

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: dwivediaakar, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

Trunk
mozilla54
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [lang=C++])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Comment 1

2 years ago
(In reply to Andi-Bogdan Postelnicu from comment #0)
> 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+name
> space%29%3A%3ADatabaseConnection%3A%3AAutoSavepoint%3A%3AStart%28const+mozill
> a%3A%3Adom%3A%3AindexedDB%3A%3A%28anonymous+namespace%29%3A%3ATransactionBase
> +%2A%29%22&redirect_type=single#11348
> 
> The parameter name from the declaration differs from the parameter name from
> the definition.

Hi,
I modified the code, and the build was successful on my machine.
Is this much enough to check the code.
If yes, then How do I commit the changes to mozilla code base?
If no, then kindly tell me the next steps.
Flags: needinfo?(bpostelnicu)
(Reporter)

Comment 2

2 years ago
Hello,
Great to hear this. You need to attach your patch to this bug and ask for a review from the module owner. A list with the module owners can be found here:

https://wiki.mozilla.org/Modules/All

If the patch is accepted then you need to mark it as checkin-needed.
Flags: needinfo?(bpostelnicu)
(Assignee)

Comment 3

2 years ago
Created attachment 8831736 [details] [diff] [review]
ActorsParent.cpp

I have changed the definition of the function 
``` nsresult
DatabaseConnection::
AutoSavepoint::Start(const TransactionBase* aTransaction) ```
to
```nsresult
DatabaseConnection::
AutoSavepoint::Start(const TransactionBase* aConnection)```

and changed the name inside the function definition block too.
Flags: needinfo?(jvarga)
Attachment #8831736 - Flags: review?(jvarga)
Attachment #8831736 - Flags: review?(btseng)
Attachment #8831736 - Flags: checkin+
Comment on attachment 8831736 [details] [diff] [review]
ActorsParent.cpp

Several things:
* please provide a diff
* please ask only to a single person, don't need more people for a simple patch
* checkin is not needed
* don't need to needinfo on top of review
Flags: needinfo?(jvarga)
Attachment #8831736 - Flags: review?(jvarga)
Attachment #8831736 - Flags: review?(btseng)
Attachment #8831736 - Flags: checkin+
(Assignee)

Comment 5

2 years ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #4)
> Comment on attachment 8831736 [details] [diff] [review]
> ActorsParent.cpp
> 
> Several things:
> * please provide a diff
> * please ask only to a single person, don't need more people for a simple
> patch
> * checkin is not needed
> * don't need to needinfo on top of review

Thank you for the suggestions.

Do i need to submit the patch again?
and
How do i provide a diff?
(Assignee)

Comment 6

2 years ago
Created attachment 8831751 [details] [diff] [review]
my.diff

Function definition changed.
Attachment #8831736 - Attachment is obsolete: true
Attachment #8831751 - Flags: review?(jvarga)

Comment 7

2 years ago
Comment on attachment 8831751 [details] [diff] [review]
my.diff

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

I'm sorry, but I think we shouldn't change the definition like this. aTransaction makes perfect sense, so change aConnection to aTransaction in the declaration instead.
Thanks.
Attachment #8831751 - Flags: review?(jvarga)
(Assignee)

Comment 8

2 years ago
Created attachment 8831990 [details] [diff] [review]
my.diff

Function declaration changed.
Attachment #8831751 - Attachment is obsolete: true
Attachment #8831990 - Flags: review?(jvarga)

Comment 9

2 years ago
Comment on attachment 8831990 [details] [diff] [review]
my.diff

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

Thanks!
Attachment #8831990 - Flags: review?(jvarga) → review+
(Assignee)

Comment 10

2 years ago
Comment on attachment 8831990 [details] [diff] [review]
my.diff

Check-in needed.
Attachment #8831990 - Flags: checkin?(jvarga)
(Reporter)

Updated

2 years ago
Keywords: checkin-needed
(Reporter)

Updated

2 years ago
Attachment #8831990 - Flags: checkin?(jvarga) → checkin-
(Reporter)

Comment 11

2 years ago
checkin-needed Must be added to keywords field.
(Assignee)

Comment 12

2 years ago
(In reply to Andi-Bogdan Postelnicu from comment #11)
> checkin-needed Must be added to keywords field.

ohkk
So, only mentors and Module owners can change the keywords right?
(Reporter)

Comment 13

2 years ago
I think you can change keywords as well.
Assignee: nobody → dwivediaakar
Attachment #8831990 - Attachment is obsolete: true
Attachment #8831990 - Attachment is obsolete: false
Attachment #8831990 - Flags: checkin-
Please attach a patch that includes proper commit information when requesting checkin.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Keywords: checkin-needed
(Assignee)

Comment 15

2 years ago
Created attachment 8833604 [details] [diff] [review]
Parameter in the declaration of the function made same as the parameter in definition
Attachment #8831990 - Attachment is obsolete: true
Attachment #8833604 - Flags: review?(jvarga)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 16

2 years ago
Comment on attachment 8833604 [details] [diff] [review]
Parameter in the declaration of the function made same as the parameter in definition

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

You need to add reviewer's name or nick, to the commit message, "; r=janv" in this case.
So it should be:
Bug 1334989 - Parameter in the declaration of the function made same as the one in definition of the function; r=janv
Attachment #8833604 - Flags: review?(jvarga) → review+
(Assignee)

Comment 17

2 years ago
Created attachment 8833605 [details] [diff] [review]
Parameter in the declaration of the function made same as the parameter in definition

Reviewer added in the commit message.
Attachment #8833604 - Attachment is obsolete: true
Attachment #8833605 - Flags: review?(jvarga)

Comment 18

2 years ago
Comment on attachment 8833605 [details] [diff] [review]
Parameter in the declaration of the function made same as the parameter in definition

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

Thanks!
Attachment #8833605 - Flags: review?(jvarga) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
(Assignee)

Comment 19

2 years ago
Comment on attachment 8833605 [details] [diff] [review]
Parameter in the declaration of the function made same as the parameter in definition

checkin-needed 
keyword added
Attachment #8833605 - Flags: checkin?(jvarga)

Comment 20

2 years ago
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f39d89313f10
Parameter in the declaration of the function made same as the one in definition of the function; r=janv
Keywords: checkin-needed

Updated

2 years ago
Attachment #8833605 - Flags: checkin?(jvarga) → checkin+

Comment 21

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f39d89313f10
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.