1039028 - Provide info when "Show More Information" is selected for the OpenH264 plugin in the addon manager

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[David Benham]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 (Beta/Release)
Build ID: 20140715030207

Steps to reproduce:

Using 33.0a1 (2014-07-15), I looked in about:license for the OpenH264 binary license text.


Actual results:

Did not find the OpenH264 binary license text in about:license


Expected results:

License should include the http://www.openh264.org/BINARY_LICENSE.txt in its entirety.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Licenses for plugins typically are available via the plugin manager, not about:licenses.

Benjamin (or gerv): where do they need to add it, to the info file?

Comment 2

[Gervase Markham [:gerv]]

If there's a location specific to the plugin (I wasn't aware that this was the case), then that would probably be best. Looking at the Plugins tab of about:addons, I see a "more" link but none of the ones I have contain licensing information. However, I don't have any proprietary plugins, which would be most likely to have such text.

Otherwise, it'll have to go in about:license.

bsmedberg?

Gerv

Comment 3

[Benjamin Smedberg]

This is not part of Firefox, and its licensing info should *not* go in about:license. The license should be displayed when "Show More Information" is selected for the OpenH264 plugin in the addon manager. This will need to be a followup to bug 1009909.

Comment 4

[Maire Reavy [:mreavy]]

OpenH264 is getting ready to go to release.  What can we do to fulfill this?

Comment 5

[Benjamin Smedberg]

Is this bug still about the license, or about other info? I was certainly not tracking it as part of the FF33 requirements, and I don't know if my team will have time to work on this.

Blair, what magic do we need to show this as part of the more-information page? Can we "trick" something like options.xul to show the license?

Comment 6

[David Benham]

In the about:addons view, can you add a line above or below "homepage" that is titled "license" and has a link to http://www.openh264.org/BINARY_LICENSE.txt  ?    I think this might suffice and/or buy plenty of time to address further as needed.

Comment 7

[Maire Reavy [:mreavy]]

This(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Is this bug still about the license, or about other info? I was certainly
> not tracking it as part of the FF33 requirements, and I don't know if my
> team will have time to work on this.

I've largely been focused on technical requirements;  clearly this isn't a technical requirement.  I thought we would need this from a Legal perspective.  I would love to be wrong.  I'm not looking to add more work to anyone's plates, especially for Fx33.

I just looked at several of our plugins (Flash, Silverlight, iTunes and others), and the only one that even had a link to the homepage was QuickTime.  None of them had licensing info or copyright info in the add-ons manager -- so I could easily be wrong here.

However, the OpenH264 FAQ says:
Q. What license will be used for the binary?
A: Cisco provides the binary under the terms of a two-clause BSD license. Additionally, the binary is licensed under Cisco’s AVC/H.264 Patent Portfolio License from MPEG LA, at no cost to you, provided the requirements and conditions listed in the AVC/H.264 Patent Portfolio sections are met. Please see the full binary license text at http://www.openh264.org/BINARY_LICENSE.txt.

Q. My application doesn't have an end-user license agreement, so where must I reproduce all of the binary license text?
A: In the same location where any other licensing information is to be presented to the user. Some examples include in a "description," "about" or "licenses" section or file.


Can I propose that we update about:license with the OpenH264 license just to be on the safe side and then we can add "Show More Info" on the plugin at a later time?

Doing a needinfo to Gerv and bsmedberg to ask if they would be ok with this proposal.

I can provide a patch to update about:license and put it up to review to Gerv with relatively little effort.  Thoughts?

I'm also happy to do what David proposed in Comment 6 -- whichever is less work and still meets the requirement.

Comment 8

[Gervase Markham [:gerv]]

On consideration: the license for OpenH264 shouldn't go in about:license because it's a discrete component that not all Firefox installations will have, and there is a clear and obvious alternative place to put any necessary licensing information. In my gut, and reading BINARY_LICENSE.txt and the spirit of the patent licensing conditions, I also suspect that we need to maintain a clear distinction between this code and our code. Having their license shipped in our binary all the time does not really maintain that separation.

If we've messed up and not done this in time, we should do comment 6 and look at providing the full license text in the proper location where the plugin is enabled or disabled as soon as we can.

Gerv

Comment 9

[David Benham]

Your argument is rationale, Gerv.  The counter, I think, may be that in order for the OpenH264 license to be easy to find and thus a user be held to its terms, it should be in the same place other license info is.   If other not-distributed-in-FF add-ons all used one separate and common place or method for users get and read license info, that might the best intersection of both arguments.  If time is short, my comment 6 is reasonable approximation or step toward that goal, I think, but cannot warrant of course.

Comment 10

[Maire Reavy [:mreavy]]

(In reply to David Benham from comment #9)
> Your argument is rationale, Gerv.  The counter, I think, may be that in
> order for the OpenH264 license to be easy to find and thus a user be held to
> its terms, it should be in the same place other license info is.   If other
> not-distributed-in-FF add-ons all used one separate and common place or
> method for users get and read license info, that might the best intersection
> of both arguments.  

I appreciate the counter-argument you're making, but there is no common place for add-on licenses in Firefox.  (See Comment 7.) So I think we have choices where and how to display the license, especially in the short term.

Comment 11

[Blair McBride [:Unfocused] (UNAVAILABLE)]

So, we do have bug 624602 - which solves this as comment 7 describes. I have a patch for that that's almost complete (sans test) which I'd started working on for bug 1007336 (the requirements changed, so I de-prioritised working on it).

The downside of any of this is requiring localised strings. Which at this stage in 33, just isn't doable.

I think one other possible solution is to append the license to the plugin's description. The downside of that is we can't add links to anything, and the including the full license text will push down the UI to configure updates - effectively hiding that UI for most people. But text is selectable, so maybe simply including the URL for the license is enough for now?

Comment 12

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Hack v1

After discussing this a bit with jesup on IRC, here's a possible solution in the form of a hacky patch. It:
* Shows a link to the license in the details view of the OpenH264 plugin, just below the description
* When clicks, opens a new tab showing the license in full
* License is shipped with Firefox, but not in about:licenses
* Has no new strings

Comment 13

[Gervase Markham [:gerv]]

(In reply to Blair McBride [:Unfocused] from comment #12)
> * Shows a link to the license in the details view of the OpenH264 plugin,
> just below the description

Does the description contain the words "OpenH264 Video Codec provided by Cisco Systems, Inc."? I think we need to do that too.

> * When clicks, opens a new tab showing the license in full
> * License is shipped with Firefox, but not in about:licenses

I think that if it's not shown anywhere unless the plugin is actually installed, that will do for a temporary fix :-) (Ideally, in the long term, it would ship with the plugin package.)

So, sounds good to me :-)

Gerv

Comment 14

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Well, looks like I've taken this then.

I mentioned to Gerv that "OpenH264 Video Codec provided by Cisco Systems, Inc." is actually the add-on name, so we're all good there.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Screenshot of fix

For reference, this is what we end up seeing.

Comment 16

[Benjamin Smedberg]

Thank you Blair. Marco this should be in the 35.2 priority list.

Comment 17

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8489788 [details] [diff] [review]
Hack v1

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

Good enough under the circumstances.

::: toolkit/mozapps/extensions/content/OpenH264-license.txt
@@ +1,5 @@
> +-------------------------------------------------------
> +About The Cisco-Provided Binary of OpenH264 Video Codec
> +-------------------------------------------------------
> +
> +Cisco provides this program under the terms of the BSD license.  

Somebody needs to teach Intel about trailing white space ;->

::: toolkit/mozapps/extensions/content/extensions.js
@@ +2788,5 @@
>      desc.textContent = aAddon.description;
>  
>      var fullDesc = document.getElementById("detail-fulldesc");
>      if (aAddon.fullDescription) {
> +      // The following ios part of an awful hack to include the OpenH264 license

s/ios/is/

@@ +2789,5 @@
>  
>      var fullDesc = document.getElementById("detail-fulldesc");
>      if (aAddon.fullDescription) {
> +      // The following ios part of an awful hack to include the OpenH264 license
> +      // without having bug 624602 fixed yet, and intentinoally ignores

intentionally

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +40,5 @@
>  const OPENH264_OPTIONS_URL     = "chrome://mozapps/content/extensions/openH264Prefs.xul";
>  
>  const GMP_PREF_LASTCHECK       = "media.gmp-manager.lastCheck";
>  
> +// The following ios part of an awful hack to include the OpenH264 license

is

@@ +41,5 @@
>  
>  const GMP_PREF_LASTCHECK       = "media.gmp-manager.lastCheck";
>  
> +// The following ios part of an awful hack to include the OpenH264 license
> +// without having bug 624602 fixed yet, and intentinoally ignores localisation.

intentionally

@@ +95,5 @@
>    get name() { return pluginsBundle.GetStringFromName("openH264_name"); },
>    get creator() { return null; },
>    get homepageURL() { return OPENH264_HOMEPAGE_URL; },
>  
> +  get description() { 

trailing white space

@@ +98,5 @@
>  
> +  get description() { 
> +    return pluginsBundle.GetStringFromName("openH264_description");
> +  },
> +  

trailing white space

::: toolkit/mozapps/extensions/jar.mn
@@ +33,5 @@
>    content/mozapps/xpinstall/xpinstallConfirm.xul                (content/xpinstallConfirm.xul)
>    content/mozapps/xpinstall/xpinstallConfirm.js                 (content/xpinstallConfirm.js)
>    content/mozapps/xpinstall/xpinstallConfirm.css                (content/xpinstallConfirm.css)
>    content/mozapps/xpinstall/xpinstallItem.xml                   (content/xpinstallItem.xml)
> +

no blank line? there aren't any others in the file.

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Wow, typos galore :\ Can tell I did that in a rush.

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

https://hg.mozilla.org/integration/fx-team/rev/8768d4b9a003

Comment 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Totally legit patch v1.1

Approval Request Comment
[Feature/regressing bug #]: OpenH264 - bug 1009909, bug 948160
[User impact if declined]: Er, we do something illegal...? IANAL, but that seems bad.
[Describe test coverage new/current, TBPL]: YOLO
[Risks and why]: Minimal. Change is intentionally low-risk for quick uplift and only affects the OpenH264 plugin
[String/UUID change made/needed]: None

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

ni on Pike and Flod to comment on the hardcoded string "License information" that has been proposed to ship in 33 and 34.

(In reply to Blair McBride [:Unfocused] from comment #11)
> I think one other possible solution is to append the license to the plugin's
> description. The downside of that is we can't add links to anything, and the
> including the full license text will push down the UI to configure updates -
> effectively hiding that UI for most people. But text is selectable, so maybe
> simply including the URL for the license is enough for now?

Why did we rule out simply including a URL to the license? This seems like a viable and simple alternative that doesn't require hardcoding a string.

If we do need to ship the license text, what about including a scrollable text box with the license text in the plug-in page? This should prevent the user from needing to scroll to configure updates and I trust will meet our legal requirements.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Lawrence Mandel [:lmandel] from comment #21)
> ni on Pike and Flod to comment on the hardcoded string "License information"
> that has been proposed to ship in 33 and 34.
> 
> (In reply to Blair McBride [:Unfocused] from comment #11)
> > I think one other possible solution is to append the license to the plugin's
> > description. The downside of that is we can't add links to anything, and the
> > including the full license text will push down the UI to configure updates -
> > effectively hiding that UI for most people. But text is selectable, so maybe
> > simply including the URL for the license is enough for now?
> 
> Why did we rule out simply including a URL to the license? This seems like a
> viable and simple alternative that doesn't require hardcoding a string.

A link likely doesn't meet the requirements; see the OpenH264 license and their FAQ. 
 
> If we do need to ship the license text, what about including a scrollable
> text box with the license text in the plug-in page? This should prevent the
> user from needing to scroll to configure updates and I trust will meet our
> legal requirements.

My preference is to change "License Information" to "Licensing Infomation", which already appears in the Help->About Firefox/Beta/Aurora/Nightly popup.  We could just reuse the translation for that for now.

If for some odd reason we can't, lmandel's suggestion might be an be an alternative that avoids the hard-coded string; I don't believe Gerv would have a problem with it.  I know sticking it in the description was considered a problem due to lack of scrolling (per comment above).  Unfocused?

Comment 23

[Francesco Lodolo [:flod]]

In order of preference:
1) URL used as a link (if it meets the legal requirements)
2) Given the context in which the string is used, I think that reusing the string "Licensing Information" from aboutDialog.dtd should be safe. It's a link, more or less same context, no space constraints.
And then fix it properly on central with its own string (ignored on purpose the option 'English hard-coded string').

P.S. in the comment, "localisation" instead of localization is very British :-)

Comment 24

[Francesco Lodolo [:flod]]

(clearing pike's NI too)

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8768d4b9a003

Comment 26

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Lawrence Mandel [:lmandel] from comment #21)
> Why did we rule out simply including a URL to the license? This seems like a
> viable and simple alternative that doesn't require hardcoding a string.

It was my understanding that this wasn't adequate. Gerv should be the one to comment on this further.

> If we do need to ship the license text, what about including a scrollable
> text box with the license text in the plug-in page? This should prevent the
> user from needing to scroll to configure updates and I trust will meet our
> legal requirements.

We could, but it would also increase the complexity and risk of the patch. We've had issues with scrolling in the Add-ons Manager UI in the past, and unfortunately it's still fragile. I can put together a patch that implements this, and we can evaluate that.

Comment 27

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Francesco Lodolo [:flod] from comment #23)
> 2) Given the context in which the string is used, I think that reusing the
> string "Licensing Information" from aboutDialog.dtd should be safe. It's a
> link, more or less same context, no space constraints.
> And then fix it properly on central with its own string (ignored on purpose
> the option 'English hard-coded string').

Hmm.... we can't use a DTD in the method I've used in the current patch, but there is an even uglier low-risk hack we could do to use the DTD (and I've already won the award for ugliest hack of 2014).

> P.S. in the comment, "localisation" instead of localization is very British
> :-)

You mean not-American :P

Comment 28

[Gervase Markham [:gerv]]

Scrollable text box would have been fine, but it seems like we've gone with the current solution, which is also fine.

Gerv

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Alt patch (scrolling box) v1

Here's a patch showing the license text in a scrolling box. This could work. I don't see any regressions with scrolling, but we'd need QE verification on different OSes/window sizes, etc.

I've intentionally made this patch as simple as possible. The lines of the license text are *not* wrapped. Fixing that requires a different method that increases complexity, and therefore increases the risk of the patch.

Screenshot: http://d.pr/i/s2x6

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8199e1f796a5
https://hg.mozilla.org/releases/mozilla-beta/rev/01411f43df67

Comment 31

[tdinovo]

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Why did you change the status-firefox33 flag?

Comment 33

[tdinovo]

Ah, sorry about that was an accident on my part just meant to verify that it was fixed for all versions listed. Accidentally deselected it and was unable to set it back to fixed.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

No problem, thanks :)

Comment 35

[Ada [:adalucinet]]

Reproduced with Nightly 2014-07-19 on Mac OS X 10.9.5.
The text from http://www.openh264.org/BINARY_LICENSE.txt is present via Add-ons Manager/Plugins/OpenH264 plugin -> License Information link on Firefox 33 beta 6 (Build ID: 20140922173023), latest Nightly 35.0a1 (Build ID: 20140922030204) and latest Aurora 34.0a2 (Build ID: 20140922004004) on all platforms [1] in normal/private windows, but *NOT* with e10s enabled. Is this issue going to be fixed here or should I file another bug? Thanks in advance!

[1] Windows 7 x64, Mac OS X 10.9.5 and Linux 14.04 32-bit

Comment 36

[Francesco Lodolo [:flod]]

What happened to the 'reuse string' approach? We also need to fix this properly on m-c, without hard-coded strings.

Comment 37

[Gervase Markham [:gerv]]

I've found this on my Aurora - looks good. 

However, the plugin is set to "Always Activate", which seems strange given our commitments on active choice. Is that what I should expect to see? Is the UX treatment still being finalized? And is there anywhere I can test this out?

Gerv

Comment 38

[Georg Fritzsche [:gfritzsche]]

(In reply to Gervase Markham [:gerv] from comment #37)
> However, the plugin is set to "Always Activate", which seems strange given
> our commitments on active choice. Is that what I should expect to see?

Yes. There is no "ask to activate" for gecko media plugins (you should choose to activate features that use it instead like webrtc) and things should work out-of-the-box here.

> Is the UX treatment still being finalized?

What UX treatment do you mean? The license part could probably be improved, the rest of the addon manager integration should be done.
This ends up getting used when requiring h264 here: http://mozilla.github.io/webrtc-landing/pc_test.html

Comment 39

[Gervase Markham [:gerv]]

Sorry, brain fart. I was confusing H.264 from Cisco with something else. Yes, Always Activate is fine.

Gerv

Comment 40

[Randell Jesup [:jesup] (needinfo me)]

Blair wrote:
>Hmm.... we can't use a DTD in the method I've used in the current patch, but there is an even uglier >low-risk hack we could do to use the DTD (and I've already won the award for ugliest hack of 2014).

and:

(In reply to Blair McBride [:Unfocused] from comment #29)
> Created attachment 8491212 [details] [diff] [review]
> Alt patch (scrolling box) v1
> 
> Here's a patch showing the license text in a scrolling box. This could work.
> I don't see any regressions with scrolling, but we'd need QE verification on
> different OSes/window sizes, etc.
> 
> I've intentionally made this patch as simple as possible. The lines of the
> license text are *not* wrapped. Fixing that requires a different method that
> increases complexity, and therefore increases the risk of the patch.
> 
> Screenshot: http://d.pr/i/s2x6

We need to either switch to this patch (scrolling box), or ferret out the "Licensing information" localized string.  Blair - can you either put the new patch up for review/approval to replace the old one, or put up the uglier hack to share "licensing information"?

Thanks!

Comment 41

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8491212 [details] [diff] [review]
Alt patch (scrolling box) v1

I had assumed that sine Gerv was fine with the previous patch, it got a+, and no one spoke up about that vs this patch, that the first patch was deemed good enough.

Apparently that may not be the case!

Comment 42

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Francesco Lodolo [:flod] from comment #36)
> We also need to fix this
> properly on m-c, without hard-coded strings.

Proper fix is bug 624602. It's not a priority, but is on my todo list when I have some spare time.

Comment 43

[Francesco Lodolo [:flod]]

(In reply to Blair McBride [:Unfocused] from comment #41)
> I had assumed that sine Gerv was fine with the previous patch, it got a+,
> and no one spoke up about that vs this patch, that the first patch was
> deemed good enough.

My "(ignored on purpose the option 'English hard-coded string')" meant "hard-coding strings is not a acceptable solution'.

I'm honestly scared that removing this string depends on such a old bug. Can't we just add the string to the .properties file, load it in the code and then fix bug 624602 in due time?

Comment 44

[Francesco Lodolo [:flod]]

Comment on attachment 8491212 [details] [diff] [review]
Alt patch (scrolling box) v1

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

Sorry, missed the f?

It's fine for me, as long as it removes the hard-coded string (I don't see that part here, I assume because the patch became obsolete).

Comment 45

[Gervase Markham [:gerv]]

Comment on attachment 8491212 [details] [diff] [review]
Alt patch (scrolling box) v1

I'm fine with either what we have now or with this, from a licensing perspective. Which we choose is a UI/l10n/drivers etc. decision.

Gerv

Comment 46

[Blair McBride [:Unfocused] (UNAVAILABLE)]

flod: So, my take away from our discussion on IRC was that what we have landed right now is fine, on the assumption we fix bug 624602 before the next merge? That would by my preference, as it's the least risky option by far.

Comment 47

[Francesco Lodolo [:flod]]

(In reply to Blair McBride [:Unfocused] from comment #46)
> flod: So, my take away from our discussion on IRC was that what we have
> landed right now is fine, on the assumption we fix bug 624602 before the
> next merge? That would by my preference, as it's the least risky option by
> far.

Yes. We can live with an hard-coded string on Aurora and Beta for this cycle, also considering that it's not that visible, but we need a fix in m-c before we lose another cycle.

Comment 48

[Ada [:adalucinet]]

Blair, could you please give me an input based on comment 35? Besides the e10s issue, the license page is not displayed as RTL with l10n builds (E.g.: latest Nightly 35.0a1 AR build) on Windows 7 x64, Linux 14.04 x32 & Mac OS X 10.9.5.

Comment 49

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(Ugh, sorry, missed your earlier comment)


(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #35)
> but *NOT* with e10s enabled

We're not shipping e10s enabled, and it's too risky to make this work when e10s enabled on these branches. For Nightly, the solution is bug 624602.


(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #48)
> the license page is not displayed as RTL

The license is in an LTR language, so this is expected.

Comment 50

[Ada [:adalucinet]]

(In reply to Blair McBride [:Unfocused] from comment #49)
> (Ugh, sorry, missed your earlier comment)
> 
> 
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #35)
> > but *NOT* with e10s enabled
> 
> We're not shipping e10s enabled, and it's too risky to make this work when
> e10s enabled on these branches. For Nightly, the solution is bug 624602.
> 
> 
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #48)
> > the license page is not displayed as RTL
> 
> The license is in an LTR language, so this is expected.

Thanks for your reply. Marking accordingly.