Closed Bug 904346 (shumway) Opened 11 years ago Closed 11 years ago

[meta] add built-in SWF support to Firefox with Shumway

Categories

(Firefox Graveyard :: Shumway, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 28

People

(Reporter: yury, Assigned: yury)

References

(Depends on 1 open bug)

Details

(Keywords: feature)

Attachments

(3 files)

Meta bug to track the issues blocking m-c landing
Blocks: 905668
Depends on: 905679
Depends on: 911379
Depends on: 921152
Attachment #812355 - Flags: feedback?(mbebenita)
Attachment #812357 - Flags: feedback?(mbebenita)
Need info-ing gavin so he can id reviewers.
Flags: needinfo?(gavin.sharp)
Comment on attachment 812358 [details] [diff] [review]
Firefox integration with Shumway (3/3)

A build peer needs to review browser/extensions/Makefile.in.

Where is ShumwayUtils.jsm created? Seems to be omitted from this patch. The other bits looks OK.
Attachment #812358 - Flags: review?(gps)
Attachment #812358 - Flags: feedback+
Comment on attachment 812357 [details] [diff] [review]
Shumway extension abc files (2/3)

What are these files used for, and what's their licensing situation? Have Shumway licensing details in general been reviewed by a member of the licensing team (Gerv)?
Flags: needinfo?(gavin.sharp)
The .abc files are compiled actionscript. Because we don't expect Firefox builders to have the actionscript compiler toolchain, we're going to push them precompiled. They are compiled from src/avm2/generated/avm1lib in the shumway sources, and are all the Apache license. I am confident that licensing is not an issue for any of this code.
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> Comment on attachment 812358 [details] [diff] [review]
> Firefox integration with Shumway (3/3)

> Where is ShumwayUtils.jsm created? Seems to be omitted from this patch. The
> other bits looks OK.

ShumwayUtils.js is part of the "Shumway extension core files (1/3)" attachment (it is a re-write of the extension's bootstrap.js file).
We had a legal review, see 886678 for details.
Comment on attachment 812358 [details] [diff] [review]
Firefox integration with Shumway (3/3)

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

If this was a new Makefile.in, I'd probably r- this for doing things the non-preferred way. But since you are just piling on and I don't want to block Shumway's landing for something so trivial, it's acceptable. The build group will clean this up down the line. Don't worry about it.
Attachment #812358 - Flags: review?(gps) → review+
Er, sorry, but that r+ from gps was on the build bits specifically, and my f+ was not an r+ on the rest. My bad for poorly communicating that, I guess. I suppose I'll review the rest of the Shumway code after the fact.
Comment on attachment 812355 [details] [diff] [review]
Shumway extension core files (1/3)

Not critical issues, and I'm not sure how much of this was just copy/pasted from pdf.js. But here are the comments anyhow:

>diff --git a/browser/extensions/shumway/chrome.manifest b/browser/extensions/shumway/chrome.manifest

>+resource shumway content/
>+resource shumway.components components/

Why do you need two separate resource packages? "components" implies an XPCOM component, and that's not what FlashStreamConverter.js is.

>diff --git a/browser/extensions/shumway/components/FlashStreamConverter.js b/browser/extensions/shumway/components/FlashStreamConverter.js

>+var NSGetFactory1 = XPCOMUtils.generateNSGetFactory([FlashStreamConverter1]);
>+var NSGetFactory2 = XPCOMUtils.generateNSGetFactory([FlashStreamConverter2]);

This is non-sensical - NSGetFactory (no "1" or "2") is a magical symbol that needs to be defined in JS XPCOM components, and this file is not a JS XPCOM component. It should probably be named .jsm and this gunk can be removed.

>diff --git a/browser/extensions/shumway/content/ShumwayUtils.jsm b/browser/extensions/shumway/content/ShumwayUtils.jsm

>+// Register/unregister a constructor as a component.
>+function Factory() {}
>+
>+Factory.prototype = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIFactory]),

This object is only used to call register() and unregister() on, so it doesn't actually need to be a full implementation of nsIFactory (as used by JS XPCOM components). You can strip this down to just an object with register/unregister functions.

>+let ShumwayUtils = {

>+  /**
>+   * shumway is only enabled if it is both selected as the pdf viewer and if the 
>+   * global switch enabling it is true.

Too much copy/pasting in this comment :)
Attachment #812355 - Flags: feedback?(mbebenita)
Flags: needinfo?(ydelendik)
Depends on: 923527
Depends on: 923596
Attachment #812357 - Flags: feedback?(mbebenita)
(In reply to Michael Bebenita [:mbx] from comment #9)
> We had a legal review, see 886678 for details.

(Just got access to that bug.) That bug didn't seem to touch on the code licensing issue specifically.

bsmedberg's comment 7 seems to address the concern. I would still feel better if we had explicit sign-off from someone on the licensing team.
I filed bug 924571 on addressing my review comments.
Flags: needinfo?(ydelendik)
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #15)
> (In reply to Michael Bebenita [:mbx] from comment #9)
> > We had a legal review, see 886678 for details.
> 
> (Just got access to that bug.) That bug didn't seem to touch on the code
> licensing issue specifically.

I can't see it; can someone CC me?

> bsmedberg's comment 7 seems to address the concern. I would still feel
> better if we had explicit sign-off from someone on the licensing team.

If comment 7 is correct, then that sounds fine to me. (Questions of whether we check in compiled code are outside my remit :-).

Gerv
Depends on: 926988
Blocks: 924571
Blocks: 928969
Is this *the* high level meta bug for Shumway?  If so, can it be reopened for tracking purposes? (there are still several depends on bugs here) If this is not the high level bug for tracking Shumway, which bug is?  Thanks.
Keywords: feature
Target Milestone: Firefox 27 → Firefox 28
I don't think there necessarily needs to be a tracking bug, since most of the actual tracking is being done by program managers elsewhere.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> I don't think there necessarily needs to be a tracking bug, since most of
> the actual tracking is being done by program managers elsewhere.

Specifically, see the Shumway wiki page at https://wiki.mozilla.org/Shumway
Depends on: 958965
Depends on: metro-shumway
No longer depends on: 923596
Depends on: 1022388
Depends on: 1026009
No longer depends on: 870553
Depends on: 1123632
Depends on: 1133320
No longer depends on: 1135552
No longer depends on: 1123632
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.