Bug 904346 (shumway)

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

RESOLVED FIXED in Firefox 28

Status

Firefox Graveyard
Shumway
RESOLVED FIXED
4 years ago
a year ago

People

(Reporter: yury, Assigned: yury)

Tracking

(Depends on: 2 bugs, {feature})

Trunk
Firefox 28
feature
Dependency tree / graph

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Meta bug to track the issues blocking m-c landing

Updated

4 years ago
Blocks: 905668

Updated

4 years ago
Depends on: 905679
Depends on: 911379

Updated

4 years ago
Depends on: 921152
(Assignee)

Comment 1

4 years ago
Created attachment 812355 [details] [diff] [review]
Shumway extension core files (1/3)
Attachment #812355 - Flags: feedback?(mbebenita)
(Assignee)

Comment 2

4 years ago
Created attachment 812357 [details] [diff] [review]
Shumway extension abc files (2/3)
Attachment #812357 - Flags: feedback?(mbebenita)
(Assignee)

Comment 3

4 years ago
Created attachment 812358 [details] [diff] [review]
Firefox integration with Shumway (3/3)
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.
(Assignee)

Comment 8

4 years ago
(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.
Depends on: 886678
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+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d1799802f5c1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/de3bc3201329
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/da0afdb234f5

Exceedingly green try run here:
https://tbpl.mozilla.org/?tree=Try&rev=94efa935a348
Assignee: nobody → ydelendik
Status: NEW → ASSIGNED
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.
https://hg.mozilla.org/mozilla-central/rev/d1799802f5c1
https://hg.mozilla.org/mozilla-central/rev/de3bc3201329
https://hg.mozilla.org/mozilla-central/rev/da0afdb234f5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
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)
(Assignee)

Updated

4 years ago
Flags: needinfo?(ydelendik)

Updated

4 years ago
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
(Assignee)

Updated

4 years ago
Depends on: 926988
(Assignee)

Updated

4 years ago
Blocks: 924571
(Assignee)

Updated

4 years ago
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

Updated

4 years ago
Depends on: 958965
Depends on: 967306
No longer depends on: 923596
Depends on: 1022388
Depends on: 1026009
No longer depends on: 870553
Depends on: 1129378
Depends on: 1129248
Depends on: 1129603
Depends on: 1130107
Depends on: 1132871

Updated

2 years ago
Depends on: 1123632

Updated

2 years ago
Depends on: 1133320
Depends on: 1133324
No longer depends on: 1129378
Depends on: 1135552
(Assignee)

Updated

2 years ago
No longer depends on: 1135552
Depends on: 1136470
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.