Closed Bug 905668 Opened 6 years ago Closed 6 years ago

[Meta] Shumway for Firefox for Android

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
fennec Nightly+ ---

People

(Reporter: elan, Assigned: yury)

References

(Depends on 1 open bug)

Details

(Keywords: feature)

Attachments

(1 file, 5 obsolete files)

This is the metabug for work related to shipping Shumway for Firefox for Android
Depends on: shumway
Assignee: nobody → ydelendik
Attachment #8334560 - Attachment is obsolete: true
Is this ready for review?
Flags: needinfo?(ydelendik)
Attached patch Pilot UI to enable Shumway (obsolete) — Splinter Review
Attachment #8334562 - Flags: review?(gps)
(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+
Depends on: 941778
Depends on: 941785
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+
(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)
Attached patch Pilot UI to enable Shumway (obsolete) — Splinter Review
Attachment #8334802 - Attachment is obsolete: true
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)
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
(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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1029848aea9a
Status: NEW → RESOLVED
Closed: 6 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.