[Meta] Shumway for Firefox for Android

RESOLVED FIXED in Firefox 29

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: elan, Assigned: yury)

Tracking

(Depends on: 1 bug, {feature})

unspecified
Firefox 29
ARM
Android
feature
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennecNightly+)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

4 years ago
This is the metabug for work related to shipping Shumway for Firefox for Android
(Reporter)

Updated

4 years ago
Depends on: 904346
(Assignee)

Updated

4 years ago
Assignee: nobody → ydelendik
(Assignee)

Comment 1

4 years ago
Created attachment 8334560 [details] [diff] [review]
Adds browser's Shumway extension to Android Nightly
(Assignee)

Comment 2

4 years ago
Created attachment 8334562 [details] [diff] [review]
Adds browser's Shumway extension to Android Nightly
Attachment #8334560 - Attachment is obsolete: true
Is this ready for review?
Flags: needinfo?(ydelendik)
(Assignee)

Comment 4

4 years ago
Created attachment 8334802 [details] [diff] [review]
Pilot UI to enable Shumway
(Assignee)

Updated

4 years ago
Attachment #8334562 - Flags: review?(gps)
(Assignee)

Comment 5

4 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3)
> Is this ready for review?

Integration patch is ready.

UI patch is not ready yet due to "Disable Shumway" title of the option. We need to rename "shumway.disabled" to "shumway.enabled" on the browsers extension.
Flags: needinfo?(ydelendik)
Comment on attachment 8334562 [details] [diff] [review]
Adds browser's Shumway extension to Android Nightly

The non-build parts look fine
Attachment #8334562 - Flags: review+
(Reporter)

Updated

4 years ago
Depends on: 941778
(Reporter)

Updated

4 years ago
Depends on: 941785

Comment 7

4 years ago
Comment on attachment 8334562 [details] [diff] [review]
Adds browser's Shumway extension to Android Nightly

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

Not getting r+ because not using build system best practices.

::: mobile/android/extensions/Makefile.in
@@ +11,5 @@
> +  $(NULL)
> +
> +ifdef NIGHTLY_BUILD
> +$(FINAL_TARGET)/chrome/shumway.manifest: $(GLOBAL_DEPS)
> +	printf "manifest shumway/chrome.manifest" > $@

This should be using $(call py_action,buildlist). See browser/extensions/Makefile.in or later in this file.

@@ +18,5 @@
> +	$(PYTHON) $(topsrcdir)/config/nsinstall.py \
> +	  $(SHUMWAY_BROWSER_EXTENSION) \
> +          $(foreach exclude,$(exclude_files), -X $(SHUMWAY_BROWSER_EXTENSION)/$(exclude)) \
> +          $(FINAL_TARGET)/chrome
> +	$(call py_action,buildlist,$(FINAL_TARGET)/chrome.manifest "manifest chrome/shumway.manifest")

This doesn't need to be in the same rule. I'd prefer it be in a separate libs:: rule.
Attachment #8334562 - Flags: review?(gps) → feedback+
(Assignee)

Comment 8

4 years ago
Created attachment 8340836 [details] [diff] [review]
Adds browser's Shumway extension to Android Nightly

(In reply to Gregory Szorc [:gps] from comment #7)
> Comment on attachment 8334562 [details] [diff] [review]
> Adds browser's Shumway extension to Android Nightly
> 
> Review of attachment 8334562 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not getting r+ because not using build system best practices.
> 
> ::: mobile/android/extensions/Makefile.in
> @@ +11,5 @@
> > +  $(NULL)
> > +
> > +ifdef NIGHTLY_BUILD
> > +$(FINAL_TARGET)/chrome/shumway.manifest: $(GLOBAL_DEPS)
> > +	printf "manifest shumway/chrome.manifest" > $@
> 
> This should be using $(call py_action,buildlist). See
> browser/extensions/Makefile.in or later in this file.
> 

Fixed.

> @@ +18,5 @@
> > +	$(PYTHON) $(topsrcdir)/config/nsinstall.py \
> > +	  $(SHUMWAY_BROWSER_EXTENSION) \
> > +          $(foreach exclude,$(exclude_files), -X $(SHUMWAY_BROWSER_EXTENSION)/$(exclude)) \
> > +          $(FINAL_TARGET)/chrome
> > +	$(call py_action,buildlist,$(FINAL_TARGET)/chrome.manifest "manifest chrome/shumway.manifest")
> 
> This doesn't need to be in the same rule. I'd prefer it be in a separate
> libs:: rule.

New libs:: rule added for nsinstall.py action
Attachment #8334562 - Attachment is obsolete: true
Attachment #8340836 - Flags: review?(gps)
(Assignee)

Comment 9

4 years ago
Created attachment 8340837 [details] [diff] [review]
Pilot UI to enable Shumway
Attachment #8334802 - Attachment is obsolete: true

Updated

4 years ago
Attachment #8340836 - Flags: review?(gps) → review+
Yuri, do you want to get review on the UI patch?
Flags: needinfo?(ydelendik)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> Yuri, do you want to get review on the UI patch?

Yury and I chatted on IRC and decided we need to think about the Settings UI a bit more. We need UX to get involved too. Personally, I'd like to integrate the UI into the existing "Plugins" UI and *not* create a separate checkbox UI for enabling Shumway. Also, we probably need a different way of describing Shumway, since no one will know what it means.
Flags: needinfo?(ibarlow)
(Assignee)

Comment 12

4 years ago
Created attachment 8345911 [details] [diff] [review]
patch for checkin

fixes a conflict in mobile/android/chrome/content/browser.js

(https://tbpl.mozilla.org/?tree=Try&rev=4303854568a4)
Attachment #8340836 - Attachment is obsolete: true
Attachment #8340837 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #10)
> Yuri, do you want to get review on the UI patch?

There is bug 941778 to fix that. For now, as in Desktop case, about:config can be used to enable the Shumway.
Flags: needinfo?(ydelendik)
Mark and I were talking yesterday about this, and landed on creating a section for Plugins where you can edit your general preferences, as well as whether you want to try our experimental flash player. We could still put it under display, maybe before the "advanced" section. What's also nice about this is that down the road we could build this section out to provide prefs for other built in players like the PDF reader we're working on. 

So it would look something like this:


-------------------------------------
PLUGINS
-------------------------------------
# Installed plugins
Tap to play
-------------------------------------
# Flash content
Use built-in player (experimental)
-------------------------------------


Tapping the items would show our little popup choosers:

# Installed plugins
Enabled
Disabled
Tap to play

# Flash content
Use built-in player (experimental)
Disabled
Use <plugin x>

Note: regarding the last item in the Flash menu, I'm not sure if we can detect if the user already has a plugin installed that runs flash, but if we can it would be nice to include that in the list of options.
Flags: needinfo?(ibarlow)
(In reply to Ian Barlow (:ibarlow) from comment #14)

> Note: regarding the last item in the Flash menu, I'm not sure if we can
> detect if the user already has a plugin installed that runs flash, but if we
> can it would be nice to include that in the list of options.

we can. see the code for about:plugins
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1029848aea9a
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1029848aea9a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: Firefox 28 → Firefox 29
Depends on: 952844
No longer depends on: 941785
tracking-fennec: --- → Nightly+
Keywords: feature
You need to log in before you can comment on or make changes to this bug.