Clean up nsClipboard.cpp/h

RESOLVED FIXED in Firefox 53

Status

()

Core
Widget: Win32
P5
normal
RESOLVED FIXED
7 years ago
5 months ago

People

(Reporter: khuey, Assigned: jbonnafo, Mentored)

Tracking

unspecified
mozilla53
x86
Windows 7
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

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

Attachments

(1 attachment, 5 obsolete attachments)

There's some cruft living in nsClipboard that can be removed (DisplayErr, odd commenting style, etc).
Assignee: khuey → nobody
Status: ASSIGNED → NEW
> DisplayErr

For this do you mean to move the "static void DisplayErrCode(HRESULT hres)" into some debug file or...?
Assignee: nobody → netzen
Assignee: netzen → nobody

Updated

3 years ago
Mentor: jmathies@mozilla.com, tabraldes@mozilla.com, netzen@gmail.com
Whiteboard: [good first bug][lang=c++]

Comment 2

3 years ago
I wan to work on this bug

Comment 3

3 years ago
I wan to work on this bug

Comment 4

3 years ago
I wan to work on this bug

Comment 5

3 years ago
:jimm Yeah sorry for that. I'd like to work on this bug if you could assign it to me.

Comment 6

3 years ago
Could you guide on how to go about it. Thanks

Comment 7

3 years ago
(In reply to Aman Alam from comment #6)
> Could you guide on how to go about it. Thanks

Sure, we have coding standards outlined here - 

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Generally if you look around at other c++ source files you'll find some obvious differences in the clipboard code.
Assignee: nobody → aman_alam

Comment 8

3 years ago
Hi, I would like to work on this bug. Please tell me how to proceed.
I am a newbie to Mozilla.

Comment 9

3 years ago
Created attachment 8519467 [details] [diff] [review]
Changed the styles of comparing conditions in if()

I have changed the comparison condition in if blocks as per the given coding style of Mozilla. I could not get any idea about DisplayErr.
Please review my patch and also tell me how to proceed with "DisplayErr" thing.
Attachment #8519467 - Flags: review?(jmathies)

Comment 10

3 years ago
Comment on attachment 8519467 [details] [diff] [review]
Changed the styles of comparing conditions in if()

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

There are a bunch of file changes here that look unrelated - please remove them.

generally this is a step in the right direction, lets try and finish up the rest of these nits.

::: widget/windows/nsClipboard.cpp
@@ +92,4 @@
>  {
>    UINT format;
>  
> +  if (!strcmp(aMimeStr, kTextMime))

nit - on all of these can we add parens for the blocks please.

@@ +183,5 @@
>          FORMATETC textFE;
>          SET_FORMATETC(textFE, CF_TEXT, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL);
>          dObj->AddDataFlavor(kTextMime, &textFE);
>        }
> +      else if ( !strcmp(flavorStr, kHTMLMime) ) {      

nit - white space, you can check for this by posting your patch and looking at it under Review. There's also random white space in this file unrelated to your changes, can you seek those out as well and clean them up?

@@ +219,4 @@
>          SET_FORMATETC(imageFE, CF_DIB, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL)
>          dObj->AddDataFlavor(flavorStr, &imageFE);
>        }
> +      else if ( !strcmp(flavorStr, kFilePromiseMime) ) {

nit - no spaces here and below.

if (!strcmp(...)) {
}
Attachment #8519467 - Flags: review?(jmathies) → review-

Comment 11

2 years ago
Thanks for your review Jim Mathies.
I had been busy with my exams, projects for past few weeks.
Now, I will work on this.

Regarding the nit on the line #92,
Do you want me to add parens to if-else blocks?

Comment 12

2 years ago
Hello Jim!
I'm Neil, and still looking for my first bug to solve!
If Rohan isn't working on it anymore, could I

Comment 13

2 years ago
Sorry about that,
Hello Jim!
I'm Neil, and still looking for my first bug to solve!
If Rohan isn't working on it anymore, could I start working on this?

I need to work on the paren blocks as well as white space, right?

Comment 14

2 years ago
Hey Jim
How to merge changes from a patch into an local mozilla repository?
(In reply to Rohan Jaswal from comment #11)
> Thanks for your review Jim Mathies.
> I had been busy with my exams, projects for past few weeks.
> Now, I will work on this.
> 
> Regarding the nit on the line #92,
> Do you want me to add parens to if-else blocks?

Mozilla coding style must use { } for 'if' event if one line.  So you must write the fix like the following.

 if (!strcmp(aMimeStr, kTextMime)) {
  ...
 } else if (!strcmp(aMimeStr, kUnicodeMime)) {
  ...
 }

After fixing comment #10, please attach new fix.

Updated

9 months ago
Priority: -- → P5
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][tpi:+]

Comment 16

8 months ago
I would like to work on this bug its my first bug can you give some tips
(Assignee)

Comment 17

5 months ago
Created attachment 8816873 [details] [diff] [review]
Amendinf if else statements to include braces

Change to add braces for if / else statement in nsClipboard.cpp
Attachment #8816873 - Flags: review?(jmathies)
(Assignee)

Comment 18

5 months ago
Hello,

I have proposed a patched attached to this bugzilla case.
This is my first contribution tentative.
Happy to discuss about the proposed changes.

Thanks,
Comment on attachment 8816873 [details] [diff] [review]
Amendinf if else statements to include braces

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

Please do not use tabs.
(Assignee)

Comment 20

5 months ago
Hello,

Thanks for reviewing the submitted patch. 
I will amend the code to avoid the use of tabs and attach a new patch to this bugzilla case.

Thanks,
(Assignee)

Comment 21

5 months ago
Created attachment 8817012 [details] [diff] [review]
patch for using braces with if else and avoiding tabs

removed use of tabs
Attachment #8817012 - Flags: review?(jmathies)
(Assignee)

Comment 22

5 months ago
Created attachment 8817235 [details] [diff] [review]
adding braces for if else
(Assignee)

Comment 23

5 months ago
Apologies, i have submitted the same patch twice.
(Assignee)

Comment 24

5 months ago
Hello,

Could you please assign this issue to me ?

Thanks,
Assignee: aman_alam → jeanluc.bonnafoux
(Assignee)

Updated

5 months ago
Attachment #8817235 - Flags: review?(jmathies)
Comment on attachment 8817235 [details] [diff] [review]
adding braces for if else

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

Please obsolete old patches when you attach a new patch (now you should be possible because you are assignee).

::: widget/windows/nsClipboard.cpp
@@ +64,5 @@
>   // to the OS clipboard.
>    nsCOMPtr<nsIObserverService> observerService =
>      do_GetService("@mozilla.org/observer-service;1");
> +  if (observerService) {
> +      observerService->AddObserver(this, NS_XPCOM_SHUTDOWN_OBSERVER_ID, PR_FALSE);

Our coding style uses 2-space indent, not 4.
(Same for all your changes.)
(Assignee)

Comment 26

5 months ago
Created attachment 8817865 [details] [diff] [review]
braces and coding style
Attachment #8816873 - Attachment is obsolete: true
Attachment #8817012 - Attachment is obsolete: true
Attachment #8817235 - Attachment is obsolete: true
Attachment #8816873 - Flags: review?(jmathies)
Attachment #8817012 - Flags: review?(jmathies)
Attachment #8817235 - Flags: review?(jmathies)
(Assignee)

Updated

5 months ago
Attachment #8817865 - Flags: review?(VYV03354)
(Assignee)

Updated

5 months ago
Attachment #8817865 - Flags: review?(VYV03354) → review?(jmathies)
(Assignee)

Comment 27

5 months ago
Hello,
i have made old patches proposal obsolete as required.
I have attached a new patch proposal for enforcing 2 spaces indent.
Thanks,
(Assignee)

Comment 28

5 months ago
Hello,

Is the latest patch proposal Ok ?

Thanks,

Updated

5 months ago
Attachment #8817865 - Flags: review?(jmathies) → review+

Updated

5 months ago
Attachment #8519467 - Attachment is obsolete: true

Comment 29

5 months ago
To land this:

1) update the check-in comment to include reviewer info, something like:

"Bug 545066 - Code cleanup in widget/windows/nsClipboard: add braces to if else, removed tabs, ident fix. r=jimm"

and post it to this bug. Obsolete the old patches.

2) add 'checkin-needed' to the keywords so drivers will land it. If you don't have bugzilla perms to add thit, needinfo me.

Thanks for the contribution!
(Assignee)

Comment 30

5 months ago
Sure, thanks will do.
Keywords: checkin-needed
(Assignee)

Comment 31

5 months ago
Hello,

Quick question on item 1, should i submit a new patch (with same code changes) but only with a different comment ? 
My first guess was to update the comment of the current attachment but this does not look possible in bugzilla.

Thanks for your help,

Comment 32

5 months ago
(In reply to jbonnafo from comment #31)
> Hello,
> 
> Quick question on item 1, should i submit a new patch (with same code
> changes) but only with a different comment ? 

Yes, thanks!
(Assignee)

Comment 33

5 months ago
Created attachment 8818610 [details] [diff] [review]
Bug 545066 - Code cleanup in widget/windows/nsClipboard: add braces to if else, removed tabs, ident fix. r=jimm

Bug 545066 - Code cleanup in widget/windows/nsClipboard: add braces to if else, removed tabs, ident fix.
Attachment #8817865 - Attachment is obsolete: true
Attachment #8818610 - Flags: review+
(Assignee)

Comment 34

5 months ago
Hello,

I have added new patch with comment as requested by :jimm.
I have set the flag to 'checkin-needed'.

Thanks,
Target Milestone: mozilla1.9.3a2 → ---

Comment 35

5 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca01cd5d229
Clean up nsClipboard.cpp - braces added for if..else, no tabs, ident fix. r=jimm
Keywords: checkin-needed
(Assignee)

Comment 36

5 months ago
Hello,

There are some other files in widget/windows directory that would require some code clean-up.
Should i raise a new bugzilla ticket for this ?

Thanks,

Comment 37

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7ca01cd5d229
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.