1399087 - Reformat dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with clang-format

Status: RESOLVED WONTFIX

(Core :: Audio/Video: Playback, enhancement)

Description

[Kilik Kuo [:kikuo] (inactive)]

I'm gonna to land some patches which are reformatted with command "./mach clang-format", then I found lot of irrelevant codes were reformatted ... 

So I'd like to initiate the new style by reformatting all codes under dom/media/SOME_FILES with clang-format.

So that people won't suffer if they're try to land patches with clang-formatted but found lot of differences.

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

The reason I'm not going to reformat all files under dom/media is that I'm not sure if there are any on-going huge works. I don't want to bother them.

Comment 2

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Bug 1399087 - Reformat dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with clang-format

Review commit: https://reviewboard.mozilla.org/r/178764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/178764/

Comment 3

[Kilik Kuo [:kikuo] (inactive)]

Comment on attachment 8907042 [details]
Bug 1399087 - Reformat dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with clang-format

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178764/diff/1-2/

Comment 4

[Jean-Yves Avenard [:jya]]

Please don't.

After 58, we're going to have a code-wide reformatting.

And clang-format as it is today is still not 100% per coding style and this is being worked in.

Comment 5

[Jean-Yves Avenard [:jya]]

Comment on attachment 8907042 [details]
Bug 1399087 - Reformat dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with clang-format

https://reviewboard.mozilla.org/r/178764/#review184036

::: dom/media/MediaFormatReader.h:43
(Diff revision 2)
>    };
>  
>    WaitForDataRejectValue(MediaData::Type aType, Reason aReason)
>      : mType(aType)
>      , mReason(aReason)
> -  {
> +  {}

that itself is not per coding style.

the style is the constructor definition takes more than one line, to have the braces on two lines.

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

Comment 6

[Jean-Yves Avenard [:jya]]

Comment on attachment 8907042 [details]
Bug 1399087 - Reformat dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with clang-format

https://reviewboard.mozilla.org/r/178764/#review184038

Comment 7

[Jean-Yves Avenard [:jya]]

Make sure you update your version of clang-format to at least the current working 5.0

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8907042 [details]
Bug 1399087 - Reformat dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with clang-format

https://reviewboard.mozilla.org/r/178764/#review184036

> that itself is not per coding style.
> 
> the style is the constructor definition takes more than one line, to have the braces on two lines.
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes

That would be a pain if you need to check formatting manually every time after running `mach clang-format`.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Jean-Yves Avenard [:jya] from comment #4)
> Please don't.
> 
> After 58, we're going to have a code-wide reformatting.
> 
> And clang-format as it is today is still not 100% per coding style and this
> is being worked in.

Do you mean we will also have a new version (after 58) of clang-format which will produce a format 100% compatible with our coding style?

Comment 10

[Kilik Kuo [:kikuo] (inactive)]

(In reply to Jean-Yves Avenard [:jya] from comment #7)
> Make sure you update your version of clang-format to at least the current
> working 5.0

IIRC, last night, I found it automatically downloaded/upgraded the clang-format version to 5.0 when I ran ./mach clang-format --path XXX

According to my test, 
1) running |./mach clang-format --path PATH_TO_FILES| will reformat that file anyway (not only the diff) 
2) running |./mach clang-format| will reformat the changed files of the last commit. (And also the local diff files)

So I think the best way for me now is to run |./mach clang-format| after I commit everything I've changed.

Thanks for the information.
I'll leave those files as they are.

Comment 11

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8)
> Comment on attachment 8907042 [details]
> Bug 1399087 - Reformat
> dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with
> clang-format
> 
> https://reviewboard.mozilla.org/r/178764/#review184036
> 
> > that itself is not per coding style.
> > 
> > the style is the constructor definition takes more than one line, to have the braces on two lines.
> > 
> > https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes
> 
> That would be a pain if you need to check formatting manually every time
> after running `mach clang-format`.

Indeed.
I've lodged several bugs against clang-format. Though the current thought is to modify our coding style so that it matches the output of clang-format. This is being discussed in mozilla-dev
But in the mean time, I manually modify every single change made by clang-format.

Comment 12

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8907042 [details]
Bug 1399087 - Reformat dom/media/{MediaDecoder.*,MediaFormatReader.*,MediaDecoderOwner.h} with clang-format

https://reviewboard.mozilla.org/r/178764/#review184208

Comment 13

[Jean-Yves Avenard [:jya]]

I've lodged bug 1340083