545066 - Clean up nsClipboard.cpp/h

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P5)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

There's some cruft living in nsClipboard that can be removed (DisplayErr, odd commenting style, etc).

Comment 1

[Brian R. Bondy [:bbondy]]

> DisplayErr

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

Comment 2

[Aman Alam]

I wan to work on this bug

Comment 3

[Aman Alam]

I wan to work on this bug

Comment 4

[Aman Alam]

I wan to work on this bug

Comment 5

[Aman Alam]

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

Comment 6

[Aman Alam]

Could you guide on how to go about it. Thanks

Comment 7

[Jim Mathies [:jimm]]

(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.

Comment 8

[Rohan Jaswal]

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

Comment 9

[Rohan Jaswal]

Attachment: 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.

Comment 10

[Jim Mathies [:jimm]]

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(...)) {
}

Comment 11

[Rohan Jaswal]

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

[neilkakkar]

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

[neilkakkar]

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

[Rohan Jaswal]

Hey Jim
How to merge changes from a patch into an local mozilla repository?

Comment 15

[Makoto Kato [:m_kato]]

(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.

Comment 16

[harsh shah]

I would like to work on this bug its my first bug can you give some tips

Comment 17

[jbonnafo]

Attachment: Amendinf if else statements to include braces

Change to add braces for if / else statement in nsClipboard.cpp

Comment 18

[jbonnafo]

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 19

[Masatoshi Kimura [:emk]]

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.

Comment 20

[jbonnafo]

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,

Comment 21

[jbonnafo]

Attachment: patch for using braces with if else and avoiding tabs

removed use of tabs

Comment 22

[jbonnafo]

Attachment: adding braces for if else

Comment 23

[jbonnafo]

Apologies, i have submitted the same patch twice.

Comment 24

[jbonnafo]

Hello,

Could you please assign this issue to me ?

Thanks,

Comment 25

[Masatoshi Kimura [:emk]]

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.)

Comment 26

[jbonnafo]

Attachment: braces and coding style

Comment 27

[jbonnafo]

Hello,
i have made old patches proposal obsolete as required.
I have attached a new patch proposal for enforcing 2 spaces indent.
Thanks,

Comment 28

[jbonnafo]

Hello,

Is the latest patch proposal Ok ?

Thanks,

Comment 29

[Jim Mathies [:jimm]]

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!

Comment 30

[jbonnafo]

Sure, thanks will do.

Comment 31

[jbonnafo]

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

[Jim Mathies [:jimm]]

(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!

Comment 33

[jbonnafo]

Attachment: 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.

Comment 34

[jbonnafo]

Hello,

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

Thanks,

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca01cd5d229
Clean up nsClipboard.cpp - braces added for if..else, no tabs, ident fix. r=jimm

Comment 36

[jbonnafo]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ca01cd5d229