Closed Bug 1368907 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Firefox for Android Graveyard
Component:
Audio/Video
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox56 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(6 files)

Bug 1368907 - Part1 - Rename GeckoHlsDemuxerWrapper and HlsDemuxerCallbacks for naming consistency.
59 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1368907 - Part2 - Rename GeckoHlsSample to GeckoHLSSample.
59 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1368907 - Part3 - Rename GeckoHlsResourceWrapper to GeckoHLSResourceWrapper for naming consistency.
59 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1368907 - Part4 - Update the moz.build after renaming the files.
59 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1368907 - Part5 - Regenerate the JNI glue codes after renaming the java files.
59 bytes, text/x-review-board-request
jhlin
: review+
Details
Bug 1368907 - Part6 - Modify the corresponding native code callers.
59 bytes, text/x-review-board-request
jhlin
: review+
Details
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: