Closed Bug 847839 Opened 7 years ago Closed 7 years ago

Remove obsolete files for Android XUL Fennec: embedding/android/ and mobile/xul/

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22
Tracking Status
firefox21 --- wontfix
firefox22 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached patch remove-xul-android.patch (obsolete) — Splinter Review
I think there are other files in the embedding/ directory that are not used by any other products, but I'll let someone else weed those out. :) AFAICT, the only embedding/ files used by Metro are the embedding/components/webbrowserpresist/.

Here is try build that is green for all platforms except B2G:

https://tbpl.mozilla.org/?tree=Try&rev=4eb7a735e6c7

The B2G break do not look related to my changes. Here is a try build from the same mozilla-inbound revision without my patch. B2G is still red:

https://tbpl.mozilla.org/?tree=Try&rev=e69721092a54
Attachment #721143 - Flags: review?(mark.finkle)
Comment on attachment 721143 [details] [diff] [review]
remove-xul-android.patch

After a quick look, this seems fine. We do want to move the "VideoPlayer" parts to /mobile/android/base. We used that in XUL fennec to act as a fallback "youtube" video player for platformas without the real Youtube player: Amazon Kindle and Cyanogemod.

Can you make a patch to port it over to mobile/android ?
We have bug 844414 for that.
f? This patch moves the XUL VideoPlayer.java into mobile/android. Unfortunately, this code DOES NOT work as-is.

I'd like to land this code, but disable it. This code does not affect devices that have an official YouTube app and leaves devices like the Kindle Fire without a YouTube app no worse for the wear. If we decide to support the Kindle Fire, we can debug the YouTube URL problem then. <:)

When we ask YouTube for a manifest of a video's available formats, the manifest's video URLs return HTTP 403 Forbidden. Requesting the same manifest on the desktop browser returns different video URLs. If I substitute the desktop manifest's video URLs on Android, the videos play correctly in the XUL VideoPlayer. YouTube returns the same forbidden URLs on Android even if I use the desktop browser's User-Agent on Wi-Fi.
Attachment #722406 - Flags: feedback?(mark.finkle)
Comment on attachment 722406 [details] [diff] [review]
part-1-port-xul-videoplayer.patch

I agree with landing this and debugging the URL issue later.
Attachment #722406 - Flags: feedback?(mark.finkle) → feedback+
I need to determine whether we support H.264 or Flash on the Kindle Fire. If we do, then the YouTube desktop website should work and this YouTube app problem should only affect the m.youtube.com mobile website.
I found some more XUL code to remove and was too lazy to file a new bug so I'm throwing it here :)
Attachment #722722 - Flags: review?(cpeterson)
Comment on attachment 722722 [details] [diff] [review]
Remove some more XUL fennec entrails from widget/android

Review of attachment 722722 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! \o/
Attachment #722722 - Flags: review?(cpeterson) → review+
This patch ports VideoPlayer.java from embedding/android/ to mobile/android/. As noted in the above comments, IT DOES NOT WORK but it is not expected to be invoked. This patch is mostly to just keep hang onto this code until needed.
Attachment #722406 - Attachment is obsolete: true
Attachment #726364 - Flags: review?(mark.finkle)
Depends on: 844414
Whiteboard: [keep open]
Clean up VideoPlayer.java style nits.
Attachment #726367 - Flags: review?(mark.finkle)
rm -r embedding/android/
Attachment #721143 - Attachment is obsolete: true
Attachment #721143 - Flags: review?(mark.finkle)
Attachment #726367 - Flags: review?(mark.finkle) → review+
Attachment #726364 - Flags: review?(mark.finkle) → review+
Comment on attachment 726368 [details] [diff] [review]
part-2-remove-xul-fennec.patch

LGTM
Attachment #726368 - Flags: review+
I never actually landed attachment 722722 [details] [diff] [review] under the assumption that you would.. but I should have made that clearer. Landed now, marking bug reopened so it will get closed again (properly) once it is merged to central.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb0aff9c9129
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/cb0aff9c9129
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 869019
You need to log in before you can comment on or make changes to this bug.