1396493 - [EME] Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

Status: RESOLVED FIXED

(Core :: Audio/Video: GMP, enhancement, P2)

Description

[James Cheng[:JamesCheng]]

My next step is to make TestGMPCrossOrigin.cpp get rid of eme-decrypt-v9 (GMPDecryptor interface) then I can remove PGMPDecryptor.ipdl entirely.

Therefore, I think changing gmp-fake into chromium interface is necessary.

Comment 1

[Bulk Bug Changes for mreavy's org]

Mass change P1->P2 to align with new Mozilla triage process

Comment 2

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

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

Comment 3

[James Cheng[:JamesCheng]]

Add my WIP, still need to refine it and test...

Comment 4

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part2 - Modify the gtest to adapt to the interface change.

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

Comment 5

[James Cheng[:JamesCheng]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

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

Comment 6

[James Cheng[:JamesCheng]]

Hi cpearce, 

please review my change of gmp-fake,

Currently, I cannot create a thread to run DoTestStorage[1] due to [2](I've write it in the code comment).

Could I remove the assertion since I don't know why we limited to run on mainthread?

The "mPersistentStateAllowed" was set when calling RecvInit, so I think there won't be a race condition.

Thank you.


[1]
http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/gmp-plugin/gmp-test-decryptor.cpp#400-410
[2]
http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/gmp/ChromiumCDMChild.cpp#404

Comment 7

[James Cheng[:JamesCheng]]

In addition,
I don't use lambda to create the continuation object, I use functor instead to reduce the redundant code.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

https://reviewboard.mozilla.org/r/182154/#review188344

For the most part, this is fine, but I'm r-ing it until the thread issue is sorted out.

::: dom/media/gmp-plugin/gmp-test-decryptor.cpp:71
(Diff revision 2)
>  
>    static void Error(const string& msg) {
>      FakeDecryptor::Message(msg);
>    }
>  
> -  static void Finish() {
> +  static void Finish() {;

Typo: unnecessary semicolon at the end of this line.

::: dom/media/gmp-plugin/gmp-test-decryptor.cpp:365
(Diff revision 2)
>    DoTestStorage("mt2-", testManager);
>  
> +  // ChromiumCDMChild::CreateFileIO only allows running on message loop thread.
> +  // Currently, I cannot create a thread to tun the test.
>    // Off-main-thread tests.
> -  if (GMP_SUCCEEDED(g_platform_api->createthread(&thread1))) {
> +  // std::thread thread1(DoTestStorage, "mt1-", testManager);

Given that the FileIO API is asynchronous, can we re-write this test to not create new threads, and instead do the work asynchronuosly on the main thread?

When I created this test, I probably didn't think it through enough, and created threads when I didn't need them.

The other thing we could do is make our FileIO work on non-message loop threads. I think we probably don't need to have it work off the message loop thread, at least at this stage. So I think it would be best to just make this test rely on the FileIO's asynchronous nature if possible.

::: dom/media/gmp-plugin/moz.build:14
(Diff revision 2)
> -    'fake.info',
> +    'manifest.json',
> -    'fake.voucher',
>  ]
>  
>  SOURCES += [
>          'gmp-fake.cpp',

Can we rename this from gmp-fake to cdm-fake so that it's clearer that it does not implement the GMP API, but in fact the CDM API? I think we should do that in a separate commit.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8911718 [details]
Bug 1396493 - Part2 - Modify the gtest to adapt to the interface change.

https://reviewboard.mozilla.org/r/183124/#review188350

r+, but please address the comments in new commits.

::: dom/media/gtest/TestGMPCrossOrigin.cpp:667
(Diff revision 1)
> -    EXPECT_TRUE(!mNodeId.IsEmpty());
> -
>      nsTArray<nsCString> tags;
>      tags.AppendElement(NS_LITERAL_CSTRING("fake"));
>  
> -    UniquePtr<GetGMPDecryptorCallback> callback(
> +    RefPtr<GMPStorageTest> self = this;

Can we (in a subsequent commit) rename GMPStorageTest to CDMStorageTest (or something like that). This makes it clear we're testing CDM storage.

It would be good if we could split out the stuff testing the CDM API from the stuff testing the GMP API, and make it clear which is associated with which. So can we please move the CDM related GTests out into a new file which contains the CDM specific stuff? There are still tests for the GMPVideoDecoder that should stay here.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to James Cheng[:JamesCheng] from comment #6)
> Hi cpearce, 
> 
> please review my change of gmp-fake,
> 
> Currently, I cannot create a thread to run DoTestStorage[1] due to [2](I've
> write it in the code comment).
> 
> Could I remove the assertion since I don't know why we limited to run on
> mainthread?

I think we shouldn't actually need to use threads here, as the FileIO API is already async?

If the CDM calls the WidevineFileIO's functions, it will do IPC, which needs to be done on the message loop thread. So we could make WidevineFileIO work off the message loop thread if we re-routed all IPC through the main thread, similar to the work you did elsewhere recently. I think we could get away without using threads here however.

Comment 11

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

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

Comment 12

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage.

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

Comment 13

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part6 - Fix unified build error due to adding a new gtest unit.

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

Comment 14

[James Cheng[:JamesCheng]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

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

Comment 15

[James Cheng[:JamesCheng]]

Comment on attachment 8911718 [details]
Bug 1396493 - Part2 - Modify the gtest to adapt to the interface change.

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

Comment 16

[James Cheng[:JamesCheng]]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Comment on attachment 8910692 [details]
> Bug 1396493 - Part1 - Convert gmp-fake to use Chromium

> > +  // ChromiumCDMChild::CreateFileIO only allows running on message loop thread.
> > +  // Currently, I cannot create a thread to tun the test.
> >    // Off-main-thread tests.
> > -  if (GMP_SUCCEEDED(g_platform_api->createthread(&thread1))) {
> > +  // std::thread thread1(DoTestStorage, "mt1-", testManager);
> 
> Given that the FileIO API is asynchronous, can we re-write this test to not
> create new threads, and instead do the work asynchronuosly on the main
> thread?
> 
I've updated the patches according to the review feedback.

I manually set r? for part1 since it showed r cancel but I don't know why.

IIUC, I just need to delete the creating thread code, only need to remain the main thread test.

Comment 17

[James Cheng[:JamesCheng]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182154/diff/3-4/

Comment 18

[James Cheng[:JamesCheng]]

Comment on attachment 8911718 [details]
Bug 1396493 - Part2 - Modify the gtest to adapt to the interface change.

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

Comment 19

[James Cheng[:JamesCheng]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

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

Comment 20

[James Cheng[:JamesCheng]]

Comment on attachment 8912153 [details]
Bug 1396493 - Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage.

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

Comment 21

[James Cheng[:JamesCheng]]

Comment on attachment 8912154 [details]
Bug 1396493 - Part6 - Fix unified build error due to adding a new gtest unit.

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

Comment 22

[Jan Keromnes [:janx]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

https://reviewboard.mozilla.org/r/182154/#review188688

Quick drive-by comments because our static analysis bot found 2 issues in this patch:

::: dom/media/gmp-plugin/gmp-test-storage.cpp:118
(Diff revision 3)
>  }
>  
> -class ReadRecordClient : public GMPRecordClient {
> +class ReadRecordClient : public FileIOClient
> +{
>  public:
> -  GMPErr Init(GMPRecord* aRecord,
> +  ReadRecordClient(function<void(bool, const uint8_t*, uint32_t)>&& aOnReadComplete)

Our static analysis bot found the following issue on this line:

Error: Bad implicit conversion constructor for 'ReadRecordClient' [clang-tidy: mozilla-implicit-constructor]

Note: Consider adding the explicit keyword to the constructor

::: dom/media/gmp-plugin/gmp-test-storage.cpp:191
(Diff revision 3)
> -  return g_platform_api->runonmainthread(aTask);
> -}
> -
> -class OpenRecordClient : public GMPRecordClient {
>  public:
> -  /*
> +  OpenRecordClient(function<void(bool)>&& aOpenComplete)

Same problem as above:

Error: Bad implicit conversion constructor for 'OpenRecordClient' [clang-tidy: mozilla-implicit-constructor]

Note: Consider adding the explicit keyword to the constructor

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

https://reviewboard.mozilla.org/r/182154/#review188692

Comment 24

[James Cheng[:JamesCheng]]

(In reply to Jan Keromnes [:janx] from comment #22)
> Comment on attachment 8910692 [details]
> Bug 1396493 - Part1 - Convert gmp-fake to use Chromium
> ContentDecryptionModule8 interface.
> 
> https://reviewboard.mozilla.org/r/182154/#review188688
> 
> Quick drive-by comments because our static analysis bot found 2 issues in
> this patch:
> 
> ::: dom/media/gmp-plugin/gmp-test-storage.cpp:118
> (Diff revision 3)
> >  }
> >  
> > -class ReadRecordClient : public GMPRecordClient {
> > +class ReadRecordClient : public FileIOClient
> > +{
> >  public:
> > -  GMPErr Init(GMPRecord* aRecord,
> > +  ReadRecordClient(function<void(bool, const uint8_t*, uint32_t)>&& aOnReadComplete)
> 
> Our static analysis bot found the following issue on this line:
> 
> Error: Bad implicit conversion constructor for 'ReadRecordClient'
> [clang-tidy: mozilla-implicit-constructor]
> 
> Note: Consider adding the explicit keyword to the constructor
> 
> ::: dom/media/gmp-plugin/gmp-test-storage.cpp:191
> (Diff revision 3)
> > -  return g_platform_api->runonmainthread(aTask);
> > -}
> > -
> > -class OpenRecordClient : public GMPRecordClient {
> >  public:
> > -  /*
> > +  OpenRecordClient(function<void(bool)>&& aOpenComplete)
> 
> Same problem as above:
> 
> Error: Bad implicit conversion constructor for 'OpenRecordClient'
> [clang-tidy: mozilla-implicit-constructor]
> 
> Note: Consider adding the explicit keyword to the constructor

Thanks, I've add it to the latest patch,

but I still got some weird error from Mac OSX and Windows,

I need to see what happened.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e07c265ad749797c34ccae05a4750e789cc8a555&selectedJob=133296213

Comment 25

[James Cheng[:JamesCheng]]

For windows,
I need to add 
EFINES['CDM_IMPLEMENTATION'] = True in the corresponding moz.build to make 

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/gmp/widevine-adapter/content_decryption_module_export.h#12

be true.

For MacOS,

I should make it be a reference, just like 

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/media/gmp-clearkey/0.1/ClearKeyStorage.cpp#219

did...

I guess they bumped into this error before so it used rvalue ref to avoid this error.

Comment 26

[James Cheng[:JamesCheng]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182154/diff/4-5/

Comment 27

[James Cheng[:JamesCheng]]

Comment on attachment 8911718 [details]
Bug 1396493 - Part2 - Modify the gtest to adapt to the interface change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183124/diff/3-4/

Comment 28

[James Cheng[:JamesCheng]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

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

Comment 29

[James Cheng[:JamesCheng]]

Comment on attachment 8912153 [details]
Bug 1396493 - Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage.

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

Comment 30

[James Cheng[:JamesCheng]]

Comment on attachment 8912154 [details]
Bug 1396493 - Part6 - Fix unified build error due to adding a new gtest unit.

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

Comment 31

[James Cheng[:JamesCheng]]

Try with revised patches

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99dcd4413157e522b7c6dece95b8c7949a69a26e

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

https://reviewboard.mozilla.org/r/183534/#review188718

::: dom/media/gmp-plugin/moz.build:15
(Diff revision 2)
>      'manifest.json',
>  ]
>  
>  SOURCES += [
> -        'gmp-fake.cpp',
> +        'cdm-fake.cpp',
>          'gmp-test-decryptor.cpp',

gmp-test-decryptor.cpp and gmp-test-storage.cpp should be renamed to cdm-*.

This directory should also be renamed to fake-cdm or somesuch, so that it's clear that thus is not a GMP.

Can you please checkc that everything going into the fake CDM is labeled as a CDM, and not as a GMP?

::: dom/media/gmp-plugin/moz.build:19
(Diff revision 2)
> -        'gmp-fake.cpp',
> +        'cdm-fake.cpp',
>          'gmp-test-decryptor.cpp',
>          'gmp-test-storage.cpp',
>  ]
>  
>  DEFINES['GMP_FAKE_SUPPORT_DECRYPT'] = True

We can remove the GMP_FAKE_SUPPORT_DECRYPT blocks from the fake openh264 plugin, i.e.:

https://searchfox.org/mozilla-central/source/dom/media/gmp-plugin-openh264/gmp-fake-openh264.cpp#53

Then we can remove the need for the GMP_FAKE_SUPPORT_DECRYPT from our codebase too.

We can remove all uses of gmp-decryption from Gecko, and the file fakeopenh264.voucher.

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912153 [details]
Bug 1396493 - Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage.

https://reviewboard.mozilla.org/r/183536/#review188752

Thanks!

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912154 [details]
Bug 1396493 - Part6 - Fix unified build error due to adding a new gtest unit.

https://reviewboard.mozilla.org/r/183538/#review188754

Comment 35

[James Cheng[:JamesCheng]]

Comment on attachment 8910692 [details]
Bug 1396493 - Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182154/diff/5-6/

Comment 36

[James Cheng[:JamesCheng]]

Comment on attachment 8911718 [details]
Bug 1396493 - Part2 - Modify the gtest to adapt to the interface change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183124/diff/4-5/

Comment 37

[James Cheng[:JamesCheng]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183534/diff/3-4/

Comment 38

[James Cheng[:JamesCheng]]

Comment on attachment 8912153 [details]
Bug 1396493 - Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183536/diff/3-4/

Comment 39

[James Cheng[:JamesCheng]]

Comment on attachment 8912154 [details]
Bug 1396493 - Part6 - Fix unified build error due to adding a new gtest unit.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183538/diff/3-4/

Comment 40

[James Cheng[:JamesCheng]]

(In reply to Chris Pearce (:cpearce) from comment #32)
> Comment on attachment 8912152 [details]
> Bug 1396493 - Part3 - Rename gmp-* to cdm-* and their file names.
> 
> https://reviewboard.mozilla.org/r/183534/#review188718
> 
> ::: dom/media/gmp-plugin/moz.build:15
> (Diff revision 2)
> >      'manifest.json',
> >  ]
> >  
> >  SOURCES += [
> > -        'gmp-fake.cpp',
> > +        'cdm-fake.cpp',
> >          'gmp-test-decryptor.cpp',
> 
> gmp-test-decryptor.cpp and gmp-test-storage.cpp should be renamed to cdm-*.
> 
Done.
> This directory should also be renamed to fake-cdm or somesuch, so that it's
> clear that thus is not a GMP.
I'm not sure what you mean, I guess you mean

http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/gmp-plugin/moz.build#7

make it as FINAL_TARGET = 'dist/bin/cdm-fake/1.0'? 
If so,
I met this constrain(It cost me a lot of time debugging since I replace all "gmp-fake" with "cdm-fake" in our code base, but test always return failure....)
http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/dom/media/gmp/GMPParent.cpp#976

or you mean changing "gmp-plugin" to "fake-cdm"?
If this one, I will do it in the Part6(I have never do the folder rename, I don't know what happened with git).
> 
> Can you please checkc that everything going into the fake CDM is labeled as
> a CDM, and not as a GMP?
You mean replace all gmp* with cdm? If yes, I've done.
> 
> ::: dom/media/gmp-plugin/moz.build:19
> (Diff revision 2)
> > -        'gmp-fake.cpp',
> > +        'cdm-fake.cpp',
> >          'gmp-test-decryptor.cpp',
> >          'gmp-test-storage.cpp',
> >  ]
> >  
> >  DEFINES['GMP_FAKE_SUPPORT_DECRYPT'] = True
> 
> We can remove the GMP_FAKE_SUPPORT_DECRYPT blocks from the fake openh264
> plugin, i.e.:
> 
> https://searchfox.org/mozilla-central/source/dom/media/gmp-plugin-openh264/
> gmp-fake-openh264.cpp#53
> 
> Then we can remove the need for the GMP_FAKE_SUPPORT_DECRYPT from our
> codebase too.
> 
> We can remove all uses of gmp-decryption from Gecko, and the file
> fakeopenh264.voucher.

I don't know the usage of the voucher file, should I just remove it?
If so I will do it in Part7.

And I found Windows needs dxva2.dll so I add "mLibs = NS_LITERAL_CSTRING("dxva2.dll");" in Part1.

Thanks.

Comment 41

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part7 - Rename dom/media/gmp-plugin to dom/media/fake-cdm

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

Comment 42

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part8 - Delete fakeopenh264.voucher.

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

Comment 43

[James Cheng[:JamesCheng]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

accidentally r cancel again by mozreview....

Attach treeherder with above changes.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b26e1620f5660922890e482ac09fe45aaecad92

Comment 44

[James Cheng[:JamesCheng]]

Try looks good with my change

Comment 45

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

https://reviewboard.mozilla.org/r/183534/#review189038

Moving the files from gmp\* to cdm\* (what you've done in this patch) is what I meant. But can you please *move* them rather than deleting and re-creating them, so that the history of the file is preserved when somone goes `hg log --follow`? I don't know how to do that with Git, but it should be possible.

Comment 46

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce (:cpearce) from comment #45)
> Comment on attachment 8912152 [details]
> Bug 1396493 - Part3 - Rename gmp-* to cdm-* and their file names.
> 
> https://reviewboard.mozilla.org/r/183534/#review189038
> 
> Moving the files from gmp\* to cdm\* (what you've done in this patch) is
> what I meant. But can you please *move* them rather than deleting and
> re-creating them, so that the history of the file is preserved when somone
> goes `hg log --follow`? I don't know how to do that with Git, but it should
> be possible.

You should be able to use `git mv` here to rename? ReviewBoard is showing the patch as deleting and re-adding the files you're renaming, which will lose history.

Comment 47

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912360 [details]
Bug 1396493 - Part7 - Rename dom/media/gmp-plugin to dom/media/fake-cdm

https://reviewboard.mozilla.org/r/183690/#review189048

Yes, this is what I wanted. Here you're renaming rather than creating a copy and deleting the old file.

Comment 48

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912361 [details]
Bug 1396493 - Part8 - Delete fakeopenh264.voucher.

https://reviewboard.mozilla.org/r/183692/#review189050

Thanks, this file was left over from supporting Adobe EME, and is now unused.

Comment 49

[James Cheng[:JamesCheng]]

I will study how to use hg, I will redo all the stuff and push again.

I'm not sure if the new patch may link to the old one as a new revision or it will be a new patch instead.

Rename is very painful for git with cinnabar.

I should perceive this at the beginning and use hg to deal with it.

Comment 50

[James Cheng[:JamesCheng]]

Attachment: Bug 1396493 - Part4 - Fix the include header name after renaming by Part3.

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

Comment 51

[James Cheng[:JamesCheng]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183534/diff/4-5/

Comment 52

[James Cheng[:JamesCheng]]

Comment on attachment 8912153 [details]
Bug 1396493 - Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183536/diff/4-5/

Comment 53

[James Cheng[:JamesCheng]]

Comment on attachment 8912154 [details]
Bug 1396493 - Part6 - Fix unified build error due to adding a new gtest unit.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183538/diff/4-5/

Comment 54

[James Cheng[:JamesCheng]]

Comment on attachment 8912360 [details]
Bug 1396493 - Part7 - Rename dom/media/gmp-plugin to dom/media/fake-cdm

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

Comment 55

[James Cheng[:JamesCheng]]

Comment on attachment 8912361 [details]
Bug 1396493 - Part8 - Delete fakeopenh264.voucher.

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

Comment 56

[James Cheng[:JamesCheng]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183534/diff/5-6/

Comment 57

[James Cheng[:JamesCheng]]

Comment on attachment 8912671 [details]
Bug 1396493 - Part4 - Fix the include header name after renaming by Part3.

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

Comment 58

[James Cheng[:JamesCheng]]

Comment on attachment 8912153 [details]
Bug 1396493 - Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183536/diff/5-6/

Comment 59

[James Cheng[:JamesCheng]]

Comment on attachment 8912154 [details]
Bug 1396493 - Part6 - Fix unified build error due to adding a new gtest unit.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183538/diff/5-6/

Comment 60

[James Cheng[:JamesCheng]]

Comment on attachment 8912360 [details]
Bug 1396493 - Part7 - Rename dom/media/gmp-plugin to dom/media/fake-cdm

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

Comment 61

[James Cheng[:JamesCheng]]

Comment on attachment 8912361 [details]
Bug 1396493 - Part8 - Delete fakeopenh264.voucher.

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

Comment 62

[James Cheng[:JamesCheng]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

I found that if I just git mv A.cpp B.cpp but I don't modified the content.

Even git treat it as "delete then create", but reviewboard or git-cinnabar will treat it as a "move".

If I modified the content of B.cpp , then I will always got "delete then create" instead of "rename".

Can HG do the "rename and modify" in the same patch?

I split it into two patch to avoid this symptom.

Thanks.

Comment 63

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912152 [details]
Bug 1396493 - Part3 - Rename only the file names from gmp-* to cdm-* a.

https://reviewboard.mozilla.org/r/183534/#review189640

Thanks for updating the patch to preserve history.

Comment 64

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8912671 [details]
Bug 1396493 - Part4 - Fix the include header name after renaming by Part3.

https://reviewboard.mozilla.org/r/184000/#review189642

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dcf9f51015d
Part1 - Convert gmp-fake to use Chromium ContentDecryptionModule8 interface. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/2dcc72594553
Part2 - Modify the gtest to adapt to the interface change. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8e98418e6c1b
Part3 - Rename only the file names from gmp-* to cdm-* a. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8faa7ec9f5d3
Part4 - Fix the include header name after renaming by Part3. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/c41c0cd06016
Part5 - Split out the CDM testing from TestGMPCrossOrigin.cpp to TestCDMStorage.cpp and rename GMPStorage into CDMStorage. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/d5f0f1ba4b5c
Part6 - Fix unified build error due to adding a new gtest unit. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/8454cca82c4d
Part7 - Rename dom/media/gmp-plugin to dom/media/fake-cdm r=cpearce
https://hg.mozilla.org/integration/autoland/rev/e2174dbb6efc
Part8 - Delete fakeopenh264.voucher. r=cpearce

Comment 66

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4dcf9f51015d
https://hg.mozilla.org/mozilla-central/rev/2dcc72594553
https://hg.mozilla.org/mozilla-central/rev/8e98418e6c1b
https://hg.mozilla.org/mozilla-central/rev/8faa7ec9f5d3
https://hg.mozilla.org/mozilla-central/rev/c41c0cd06016
https://hg.mozilla.org/mozilla-central/rev/d5f0f1ba4b5c
https://hg.mozilla.org/mozilla-central/rev/8454cca82c4d
https://hg.mozilla.org/mozilla-central/rev/e2174dbb6efc