Closed Bug 1039028 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox :: General, defect)

33 Branch
x86_64
Windows 7
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
firefox33 --- verified
firefox34 --- verified
firefox35 --- verified

People

(Reporter: dbenham, Assigned: Unfocused)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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.
Component: Untriaged → General
Blocks: OpenH264
Depends on: 985249
Blocks: 1009909
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?
Flags: needinfo?(benjamin)
Summary: Add OpenH264 binary license to about:license → Add OpenH264 binary license so it's visible in the plugin manager
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
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.
Points: --- → 3
Flags: needinfo?(benjamin) → firefox-backlog+
No longer blocks: 1009909
Depends on: 1009909
OpenH264 is getting ready to go to release.  What can we do to fulfill this?
Flags: needinfo?(georg.fritzsche)
Flags: needinfo?(benjamin)
Summary: Add OpenH264 binary license so it's visible in the plugin manager → Provide info when "Show More Information" is selected for the OpenH264 plugin in the addon manager
Whiteboard: [openh264-uplift]
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?
Flags: needinfo?(georg.fritzsche)
Flags: needinfo?(bmcbride)
Flags: needinfo?(benjamin)
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.
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.
Flags: needinfo?(gerv)
Flags: needinfo?(benjamin)
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
Flags: needinfo?(gerv)
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.
(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.
Flags: needinfo?(benjamin)
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?
Flags: needinfo?(bmcbride)
Attached patch Hack v1 (obsolete) — Splinter Review
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
(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
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.
Assignee: nobody → bmcbride
Status: NEW → ASSIGNED
Flags: qe-verify+
Flags: needinfo?(mmucci)
Attached image Screenshot of fix
For reference, this is what we end up seeing.
Attachment #8489788 - Flags: review?(irving)
Flags: needinfo?(mmucci) → needinfo?(gavin.sharp)
Thank you Blair. Marco this should be in the 35.2 priority list.
Flags: needinfo?(gavin.sharp)
Iteration: --- → 35.2
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.
Attachment #8489788 - Flags: review?(irving) → review+
Wow, typos galore :\ Can tell I did that in a rush.
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
Attachment #8489788 - Attachment is obsolete: true
Attachment #8490497 - Flags: review+
Attachment #8490497 - Flags: approval-mozilla-beta?
Attachment #8490497 - Flags: approval-mozilla-aurora?
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.
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(bmcbride)
(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?
Flags: needinfo?(gerv)
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 :-)
Flags: needinfo?(francesco.lodolo)
(clearing pike's NI too)
Flags: needinfo?(l10n)
https://hg.mozilla.org/mozilla-central/rev/8768d4b9a003
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
(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.
Flags: needinfo?(bmcbride)
(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
Scrollable text box would have been fine, but it seems like we've gone with the current solution, which is also fine.

Gerv
Flags: needinfo?(gerv)
Attached patch Alt patch (scrolling box) v1 (obsolete) — Splinter Review
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
Attachment #8490497 - Flags: approval-mozilla-beta?
Attachment #8490497 - Flags: approval-mozilla-beta+
Attachment #8490497 - Flags: approval-mozilla-aurora?
Attachment #8490497 - Flags: approval-mozilla-aurora+
QA Contact: alexandra.lucinet
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
Why did you change the status-firefox33 flag?
Flags: needinfo?(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.
Flags: needinfo?(tdinovo)
No problem, thanks :)
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
Flags: needinfo?(bmcbride)
What happened to the 'reuse string' approach? We also need to fix this properly on m-c, without hard-coded strings.
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
(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
Sorry, brain fart. I was confusing H.264 from Cisco with something else. Yes, Always Activate is fine.

Gerv
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 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!
Attachment #8491212 - Flags: review?(irving)
Attachment #8491212 - Flags: feedback?(gerv)
Attachment #8491212 - Flags: feedback?(francesco.lodolo)
Flags: needinfo?(bmcbride)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
(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 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).
Attachment #8491212 - Flags: feedback?(francesco.lodolo) → feedback+
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
Attachment #8491212 - Flags: feedback?(gerv) → feedback+
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.
Flags: needinfo?(francesco.lodolo)
(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.
Flags: needinfo?(francesco.lodolo)
Attachment #8491212 - Attachment is obsolete: true
Attachment #8491212 - Flags: review?(irving)
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
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.
Flags: needinfo?(bmcbride)
(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.
Flags: needinfo?(bmcbride)
(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.
Whiteboard: [openh264-uplift]
You need to log in before you can comment on or make changes to this bug.