Last Comment Bug 904346 - (shumway) [meta] add built-in SWF support to Firefox with Shumway
(shumway)
: [meta] add built-in SWF support to Firefox with Shumway
Status: RESOLVED FIXED
: feature
Product: Firefox Graveyard
Classification: Graveyard
Component: Shumway (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 28
Assigned To: Yury Delendik (:yury)
:
Mentors:
Depends on: 885526 1026009 886196 886678 900566 905679 911379 921152 923527 926988 958965 metro-shumway 1022388 1129248 1129603 1130107 1132871 1133320 1133324 1136470
Blocks: 905668 924571 928969
  Show dependency treegraph
 
Reported: 2013-08-12 20:24 PDT by Yury Delendik (:yury)
Modified: 2016-02-05 06:09 PST (History)
36 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Shumway extension core files (1/3) (1.84 MB, patch)
2013-09-30 16:59 PDT, Yury Delendik (:yury)
no flags Details | Diff | Review
Shumway extension abc files (2/3) (428.43 KB, patch)
2013-09-30 17:00 PDT, Yury Delendik (:yury)
no flags Details | Diff | Review
Firefox integration with Shumway (3/3) (10.26 KB, patch)
2013-09-30 17:02 PDT, Yury Delendik (:yury)
gps: review+
gavin.sharp: feedback+
Details | Diff | Review

Description Yury Delendik (:yury) 2013-08-12 20:24:44 PDT
Meta bug to track the issues blocking m-c landing
Comment 1 Yury Delendik (:yury) 2013-09-30 16:59:53 PDT
Created attachment 812355 [details] [diff] [review]
Shumway extension core files (1/3)
Comment 2 Yury Delendik (:yury) 2013-09-30 17:00:42 PDT
Created attachment 812357 [details] [diff] [review]
Shumway extension abc files (2/3)
Comment 3 Yury Delendik (:yury) 2013-09-30 17:02:51 PDT
Created attachment 812358 [details] [diff] [review]
Firefox integration with Shumway (3/3)
Comment 4 Erin Lancaster [:elan] 2013-09-30 19:22:10 PDT
Need info-ing gavin so he can id reviewers.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-10-01 07:34:58 PDT
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.
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-10-01 07:36:06 PDT
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)?
Comment 7 Benjamin Smedberg [:bsmedberg] 2013-10-01 07:43:24 PDT
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.
Comment 8 Yury Delendik (:yury) 2013-10-01 08:20:06 PDT
(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).
Comment 9 Michael Bebenita [:mbx] 2013-10-01 09:22:37 PDT
We had a legal review, see 886678 for details.
Comment 10 Gregory Szorc [:gps] 2013-10-01 10:02:37 PDT
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.
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-10-01 15:49:18 PDT
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 14 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-10-02 07:56:14 PDT
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 :)
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-10-08 12:25:37 PDT
(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.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-10-08 12:27:32 PDT
I filed bug 924571 on addressing my review comments.
Comment 17 Gervase Markham [:gerv] 2013-10-08 12:30:02 PDT
(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
Comment 18 Tracy Walker [:tracy] 2013-11-06 07:20:03 PST
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.
Comment 19 Benjamin Smedberg [:bsmedberg] 2013-11-06 08:51:05 PST
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.
Comment 20 Till Schneidereit [:till] 2013-11-06 10:10:38 PST
(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

Note You need to log in before you can comment on or make changes to this bug.