Closed Bug 1368907 Opened 3 years ago Closed 3 years ago

[Fennec][HLS] Unify the naming from Hls into HLS for our Gecko HLS implementation

Categories

(Firefox for Android :: Audio/Video, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox56 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(6 files)

jya mentioned that we have inconsistent naming for HLS / Hls in Bug 1350246.

We've decided to unify the naming in this bug.

Since 'Hls' is used by Google's Exoplayer, we decided to respect to the naming convention from Exoplayer.

Therefore, we will unify the naming into 'HLS' in demuxer/ resource wrapper.java and their corresponding cpp implementation.
Assignee: nobody → jacheng
Hi John,

Our goals as comment 0 said, 

We want to rename the Hls into HLS only focusing on the classes which ares used by the native implementation.

Since Exoplayer's nameing conventional is 'Hls', so we leave this convention in our gecko java codes which did not be used by native.

Please help to review this changes, thanks.
Comment on attachment 8876719 [details]
Bug 1368907 - Part1 - Rename GeckoHlsDemuxerWrapper and HlsDemuxerCallbacks for naming consistency.

https://reviewboard.mozilla.org/r/148050/#review152664

::: commit-message-52721:1
(Diff revision 1)
> +Bug 1368907 - Part1 - GeckoHlsDemuxerWrapper to GeckoHLSDemuxerWrapper HLSDelsDemuxerCallbacksacks to HLSDemuxerCallbacks and the its java file name. r?jolin

`HLSDelsDemuxerCallbacksacks` is a typo.
Suggestion: "rename GeckoHlsDemuxerWrapper for naming consistency." should be sufficient.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHLSDemuxerWrapper.java:40
(Diff revision 1)
> +        }
> +    }
> +
> +    private GeckoHlsPlayer mPlayer = null;
> +
> +    public static class HLSDemuxerCallbacks extends JNIObject

Nit: `GeckoHLSDemuxerWrapper.HLSDemuxerCallbacks` seems redundant, just name it `Callbacks`?
Attachment #8876719 - Flags: review?(jolin) → review+
Comment on attachment 8876720 [details]
Bug 1368907 - Part2 - Rename GeckoHlsSample to GeckoHLSSample.

https://reviewboard.mozilla.org/r/148052/#review152668

::: commit-message-276c8:1
(Diff revision 1)
> +Bug 1368907 - Part2 - Rename GeckoHlsSample to GeckoHLSSample and its related callers. r?jolin

Nit: just "Rename GeckoHlsSample to GeckoHLSSample." (Callers were not renamed.)
Attachment #8876720 - Flags: review?(jolin) → review+
Comment on attachment 8876721 [details]
Bug 1368907 - Part3 - Rename GeckoHlsResourceWrapper to GeckoHLSResourceWrapper for naming consistency.

https://reviewboard.mozilla.org/r/148054/#review152672

::: commit-message-0fb98:1
(Diff revision 1)
> +Bug 1368907 - Part3 - Rename GeckoHlsResourceWrapper to GeckoHLSResourceWrapper, HLlsesourceCallbacksH HLSResourceCallbacks and its java file name. r?jolin

typo? `HLlsesourceCallbacksH`
And I think "Rename GeckoHlsResourceWrapper to GeckoHLSResourceWrapper for naming consistency." should be enough.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/media/GeckoHLSResourceWrapper.java:18
(Diff revision 1)
> +    private static final String LOGTAG = "GeckoHLSResourceWrapper";
> +    private static final boolean DEBUG = false;
> +    private GeckoHlsPlayer mPlayer = null;
> +    private boolean mDestroy = false;
> +
> +    public static class HLSResourceCallbacks extends JNIObject

Nit: just `Callbacks`
Attachment #8876721 - Flags: review?(jolin) → review+
Comment on attachment 8876722 [details]
Bug 1368907 - Part4 - Update the moz.build after renaming the files.

https://reviewboard.mozilla.org/r/148056/#review152676
Attachment #8876722 - Flags: review?(jolin) → review+
Comment on attachment 8876723 [details]
Bug 1368907 - Part5 - Regenerate the JNI glue codes after renaming the java files.

https://reviewboard.mozilla.org/r/148058/#review152678
Attachment #8876723 - Flags: review?(jolin) → review+
Comment on attachment 8876724 [details]
Bug 1368907 - Part6 - Modify the corresponding native code callers.

https://reviewboard.mozilla.org/r/148060/#review152680
Attachment #8876724 - Flags: review?(jolin) → review+
Thanks for the quick review, addressed all the comments.
Wait for bug 1350246 to be landed.
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7170473b1fb
Part1 - Rename GeckoHlsDemuxerWrapper and HlsDemuxerCallbacks for naming consistency. r=jolin
https://hg.mozilla.org/integration/autoland/rev/67d66a88e833
Part2 - Rename GeckoHlsSample to GeckoHLSSample. r=jolin
https://hg.mozilla.org/integration/autoland/rev/a4e8de872bd0
Part3 - Rename GeckoHlsResourceWrapper to GeckoHLSResourceWrapper for naming consistency. r=jolin
https://hg.mozilla.org/integration/autoland/rev/7562c34ed064
Part4 - Update the moz.build after renaming the files. r=jolin
https://hg.mozilla.org/integration/autoland/rev/9f8e9e56f886
Part5 - Regenerate the JNI glue codes after renaming the java files. r=jolin
https://hg.mozilla.org/integration/autoland/rev/955bb796fd65
Part6 - Modify the corresponding native code callers. r=jolin
In the future, please do not do rename of files by simply changing the case. It will break things for people using case insensitive file system (like on windows)

We have hooks to disable renaming in pushes. However here you didn't rename, you deleted and created a new one (which is even worse as you lose the history)
Flags: needinfo?(jacheng)
I apologize that I make the case-insensitive file system bump into some problem. I am not pretty sure the behavior when using HG.

I simply use 'git mv' to rename the file but it seems the actual behavior in the patch is 'delete file then create new one'.

If I wanna simply change the case of the file name, what should I do?

Change file 'A' to file 'B' then change to file 'a'? or just using HG?

Thank you for your reminding.
Flags: needinfo?(jacheng)
git doesn't have a concept of renaming, only mercurial does.

You can trick git log / diff to make it look the file was moved, but that only works if no modifications were made on the file after the rename.

Ideally, this shouldn't have been spawned into a new bug, the name of the file should have been changed in the bug the comment was made.

as mentioned, renaming with just a case change would normally be blocked from being committed.

I'll talk to the mercurial folks so that they add a hook to prevent delete + rename with just case change.
You need to log in before you can comment on or make changes to this bug.