1181646 - Add React as shared library

Status: RESOLVED FIXED

(DevTools :: Debugger, defect)

Description

[James Long (:jlongster)]

Let's go ahead and add React in a single place that anything related to devtools can use. bug 1132203 already needs it for the JSON response viewer (an experimental feature that is going to land).

Comment 1

[James Long (:jlongster)]

Attachment: 1181646.patch

This adds the latest React (0.13.3) to a shared directory that we can all use. Honza already needs this for bug 1132203, which is going to land soon but be only be pref-ed on in Dev Edition. And I'm going to need it pretty soon for the debugger after the first pass of my refactoring lands.

This adds both the prod and dev versions, but only builds in the dev version if you are building it locally (detected with MOZ_BRANDING_DIRECTORY="browser/branding/unofficial"). 

Right now it's up to the user to include the right one, but the BrowserLoader that I made for my debugger refactoring will automatically handle this correctly. Honza is using require.js so he can configure it accordingly, and I'm hoping eventually we'll all just use BrowserLoader (a perfect use-case for it because React needs to load in the window scope. 0.14 might make it easier to pass a window in)

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Please request build peer review for this change, especially since this alters the main moz.build for toolkit.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Also, why not put the files at browser/devtools/shared instead of browser/devtools/shared/browser, which is a new directory here?

shared already contains things like d3.js, so I'm not sure what the extra "browser" is meant to suggest.

Comment 4

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Also, why not put the files at browser/devtools/shared instead of
> browser/devtools/shared/browser, which is a new directory here?
> 
> shared already contains things like d3.js, so I'm not sure what the extra
> "browser" is meant to suggest.

Things in "browser" are meant to be included in a browser environment. They require a window. Looks like d3 is this way, so it could be moved here. You could never load d3 as a module, you could only load it as a script tag. Separating this into a separate folder makes that clear.

The other big benefit is for my BrowserLoader. We found an awesome solution for loading browser files as modules: create a loader that *only* loads files needed for my window instance, and delegate everything else to the devtools loader. You have modules now, but in a normal browser environment, and all the system modules are still loaded and cached with the devtools loader. Here's the code: https://gist.github.com/jlongster/0fc1aaa1f7325ac35dc0

With an explicit path to the shared browser modules, I can also easily check it and load anything under there with the browser loader as well, which will load it within the window instance: https://gist.github.com/jlongster/0fc1aaa1f7325ac35dc0#file-browser-loader-js-L51

Otherwise I'd have to whitelist specific modules... which I could do.

Comment 5

[James Long (:jlongster)]

It's not a huge deal if you want me to just whitelist modules that should be loaded in the browser. Would be easy to change in the future if we change our minds.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Please don't forget to request build review (as I mentioned in comment 2), that's more important to me than my nits below.

(In reply to James Long (:jlongster) from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > Also, why not put the files at browser/devtools/shared instead of
> > browser/devtools/shared/browser, which is a new directory here?
> > 
> > shared already contains things like d3.js, so I'm not sure what the extra
> > "browser" is meant to suggest.
> 
> Things in "browser" are meant to be included in a browser environment. They
> require a window. Looks like d3 is this way, so it could be moved here. You
> could never load d3 as a module, you could only load it as a script tag.
> Separating this into a separate folder makes that clear.

Okay, I follow what you are trying to convey, and I think the concept is good. :)

My main nit here I guess is that the word "browser" is too overloaded so as to be almost devoid of meaning, so it doesn't immediately suggest what you are going for.  Are they files meant to be shipped with the browser?  (Yes.) One assumes so, since they are already under /browser.  Are they files about browsing?  (No.)  Are they files for managing child browsers?  (No.)

Could we use "content" as the directory name (browser/devtools/shared/content)?  This is a common name in the tree for this purpose.  While DevTools does not follow the convention very well, /browser/devtools/webide is one example: there is "content" for HTML and JS loaded in window environment / via script tags, and there is "modules" for things loaded via a CommonJS (non-window) loader.

There are many other examples of the "content" name outside devtools, like /browser/base/content, /b2g/chrome/content, etc.

> The other big benefit is for my BrowserLoader. We found an awesome solution
> for loading browser files as modules: create a loader that *only* loads
> files needed for my window instance, and delegate everything else to the
> devtools loader. You have modules now, but in a normal browser environment,
> and all the system modules are still loaded and cached with the devtools
> loader. Here's the code:
> https://gist.github.com/jlongster/0fc1aaa1f7325ac35dc0

Again, I like this concept, it seems nice.

But as far as the name, could we call this thing ContentLoader?  It doesn't load the whole browser, so BrowserLoader is unclear to me.  Also, it would be great if the loader's ID said "devtools" somewhere for clarity, so maybe "devtools-content-loader".

Comment 7

[James Long (:jlongster)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Please don't forget to request build review (as I mentioned in comment 2),
> that's more important to me than my nits below.

How do I do that? I don't see a flag for it. I will definitely make sure to do that.

> 
> My main nit here I guess is that the word "browser" is too overloaded so as
> to be almost devoid of meaning, so it doesn't immediately suggest what you
> are going for.  Are they files meant to be shipped with the browser?  (Yes.)
> One assumes so, since they are already under /browser.  Are they files about
> browsing?  (No.)  Are they files for managing child browsers?  (No.)

> 
> Could we use "content" as the directory name
> (browser/devtools/shared/content)? 

Sure! Yeah browser is way too overloaded. I'm fine calling this anything, as long as it's available. I'd be fine with `ContentWithBrowserAPIsLoader`. But just `Content` works too.

> Also, it would
> be great if the loader's ID said "devtools" somewhere for clarity, so maybe
> "devtools-content-loader".

Will do! Thanks.

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to James Long (:jlongster) from comment #7)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> > Please don't forget to request build review (as I mentioned in comment 2),
> > that's more important to me than my nits below.
> 
> How do I do that? I don't see a flag for it. I will definitely make sure to
> do that.

No special flag, just request an additional review from a peer of the Build Config module[1].

[1]: https://wiki.mozilla.org/Modules/All#Build_Config

Comment 9

[James Long (:jlongster)]

Attachment: 1181646.patch

renamed "browser" folder to "content"

Comment 10

[James Long (:jlongster)]

Comment on attachment 8631726 [details] [diff] [review]
1181646.patch

Hey Ted, do you mind taking a look at this patch? I had to expose the MOZ_BRANDING_DIRECTORY to AppConstants, and needed to change toolkit/modules/moz.build for that, and jryans asked to get an additional review from someone on the build team.

Comment 11

[James Long (:jlongster)]

Attachment: 1181646.patch

needed to fix a path issue

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8631769 [details] [diff] [review]
1181646.patch

Redirecting to :gps, Ted's queue seems quite long.

Comment 13

[Gregory Szorc [:gps]]

We need a license review on this. I can already tell you that the PATENTS file referenced by the license boilerplate will almost certainly need reviewed and included in the tree.

Also, React historically had a very poorly worded license. Some very large companies were refusing to use it until Facebook changed the language. I /think/ the current version is acceptable to others now. But we should still look at it.

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Gregory Szorc [:gps] from comment #13)
> We need a license review on this. I can already tell you that the PATENTS
> file referenced by the license boilerplate will almost certainly need
> reviewed and included in the tree.
> 
> Also, React historically had a very poorly worded license. Some very large
> companies were refusing to use it until Facebook changed the language. I
> /think/ the current version is acceptable to others now. But we should still
> look at it.

Note that react is already included in the tree for hello/loop.

Comment 15

[James Long (:jlongster)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #14)
> (In reply to Gregory Szorc [:gps] from comment #13)
> > We need a license review on this. I can already tell you that the PATENTS
> > file referenced by the license boilerplate will almost certainly need
> > reviewed and included in the tree.
> > 
> > Also, React historically had a very poorly worded license. Some very large
> > companies were refusing to use it until Facebook changed the language. I
> > /think/ the current version is acceptable to others now. But we should still
> > look at it.
> 
> Note that react is already included in the tree for hello/loop.

Yep, I believe we've already gone through this, the license is fine. The patents thing was overblown, but they changed it anyway. I'll see if one of the Hello people can comment on this.

Comment 16

[James Long (:jlongster)]

Also note that the license is just a BSD license, without any mention of patents: https://github.com/facebook/react/blob/master/LICENSE

Comment 17

[James Long (:jlongster)]

I need to land this asap, can we resolve this?

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8631769 [details] [diff] [review]
1181646.patch

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

I'm not keen on React files being checked in to browser/devtools/shared. Surely there will be a non-devtools use of React in the near future (or perhaps Loop today already qualifies) and they'll have to do the work of moving React elsewhere. Let's just put it somewhere shared from day 0.

If I had a time machine, I'd put all third party code in a third_party/ directory at the root of the source tree. But I don't and creating a new root directory requires approval from Brendan and feels like it would draw unwanted debate. So how about toolkit/third_party/react?

::: browser/devtools/shared/content/react-dev.js
@@ +7,5 @@
> + * All rights reserved.
> + *
> + * This source code is licensed under the BSD-style license found in the
> + * LICENSE file in the root directory of this source tree. An additional grant
> + * of patent rights can be found in the PATENTS file in the same directory.

I don't feel comfortable granting r+ without this PATENTS file checked in to version control. I insist on being overridden by Gerv or someone else who understands licenses better than me.

Comment 19

[James Long (:jlongster)]

(In reply to Gregory Szorc [:gps] from comment #18)
> Comment on attachment 8631769 [details] [diff] [review]
> 1181646.patch
> 
> Review of attachment 8631769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not keen on React files being checked in to browser/devtools/shared.
> Surely there will be a non-devtools use of React in the near future (or
> perhaps Loop today already qualifies) and they'll have to do the work of
> moving React elsewhere. Let's just put it somewhere shared from day 0.

The problem is that this creates a really strong coupling between projects about React versions. If we actually do have multiple large projects depend on this file, I can't imagine the work it would take to coordinate an upgrade of React, which would require changes across all projects. I do not want devtools to be held back (or potentially even stuck) on old versions of React. That's not acceptable.

The only benefit is saving space, right? The production version of React is about 100KB, about the size of an image. I don't think it's work this huge coupling of projects to save a tiny bit of space. Hello is already stuck on 0.12 of React until they have time to upgrade.

It may that various smaller services like Hello could share a React library, if smaller things are going to be built over time, but I'd really like devtools to have its own version of React. It's already going to be complicated enough maintaining our own code.

> ::: browser/devtools/shared/content/react-dev.js
> @@ +7,5 @@
> > + * All rights reserved.
> > + *
> > + * This source code is licensed under the BSD-style license found in the
> > + * LICENSE file in the root directory of this source tree. An additional grant
> > + * of patent rights can be found in the PATENTS file in the same directory.
> 
> I don't feel comfortable granting r+ without this PATENTS file checked in to
> version control. I insist on being overridden by Gerv or someone else who
> understands licenses better than me.

Sure, although remember React is already in our tree, and if we actually do find a problem we're going to have to remove Hello. I'm pretty sure this has already been looked at.

Comment 20

[Gregory Szorc [:gps]]

`hg log -f browser/components/loop/content/shared/libs/react-0.12.2.js` says 56ba52f28300 / bug 1033841 (checked in July 2014) introduced React into the tree. Looking at p1(56ba52f28300) (3f60457cbb7b), there appear to be no React files in the tree before that commit.

Looking at that bug, comment #1 makes a reference to licensing. However, there was no subsequent discussion on the subject. I can only assume React was checked in without proper licensing review. This is, uh, slightly problematic because React's contentious patents clause wasn't removed until April 10, 2015 (https://github.com/facebook/react/commit/b8ba8c83f318b84e42933f6928f231dc0918f864, which appears to fall after the 0.13.1 release).

While it's possible React's licensing was looked at post landing, I see no evidence of it. While React does appear to be using a BSD license, the boilerplate in the file requested for review makes reference to a PATENTS file. In my mind, this makes the license a variation of a standard OSS license and needs to be looked at by someone who understands these things.

I hate to put my foot down on this. And I'm confident Gerv or someone will say things are OK in the end since React's updated patents clause is apparently acceptable now. But I'm not comfortable giving a rubber stamp on licensing foo since I'm not formally versed in the subject and don't fully understand the legal requirements and consequences. I know enough about licensing that I know it is important to ask someone who understands these things for a formal sign-off. So, until there is evidence this has licensing approval, I'm forced to put my foot down and withhold granting review. Sorry :/

Comment 21

[Gervase Markham [:gerv]]

On PTO until Wednesday - can't look at it until then. Sorry :-(

Gerv

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:gps, while we're waiting on the licensing front, I was curious what you thought of the build changes in the patch which make use of MOZ_BRANDING_DIRECTORY to discriminate between local development vs. all other builds.

Does this seem okay?

Comment 23

[James Long (:jlongster)]

(In reply to Gregory Szorc [:gps] from comment #20)
> 
> I hate to put my foot down on this. And I'm confident Gerv or someone will
> say things are OK in the end since React's updated patents clause is
> apparently acceptable now. But I'm not comfortable giving a rubber stamp on
> licensing foo since I'm not formally versed in the subject and don't fully
> understand the legal requirements and consequences. I know enough about
> licensing that I know it is important to ask someone who understands these
> things for a formal sign-off. So, until there is evidence this has licensing
> approval, I'm forced to put my foot down and withhold granting review. Sorry
> :/

Alright, thanks for trying to track down if it had been looked into already. Sorry if I applied undue pressure, these things are important so it's ok. I was hoping to land a bunch of small bugs like this before I went on vacation, but I can wait. I respect wanting to do things right :)


(In reply to Gervase Markham [:gerv] from comment #21)
> On PTO until Wednesday - can't look at it until then. Sorry :-(
> 
> Gerv

No worries, sorry for the ping! I'm on vacation next week anyway but I'll be checking bugmail.

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8631769 [details] [diff] [review]
1181646.patch

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

::: browser/devtools/shared/moz.build
@@ +53,5 @@
> +
> +if CONFIG['MOZ_BRANDING_DIRECTORY'] == 'browser/branding/unofficial':
> +    EXTRA_JS_MODULES.devtools.shared.content += [
> +        'content/react-dev.js'
> +    ]

I assume you are adding MOZ_BRANDING_DIRECTORY to AppConstants.jsm so you can do some kind of conditional import from JavaScript? e.g.

  if (MOZ_BRANDING_DIRECTORY == 'browser/branding/unofficial') {
     import "react-dev.js"
  } else {
     import "react.js"
  }

If so, I'm not too keen on this solution. I would rather there be a single react.js in the package and the version is whatever the build configuration is. Of course, if you actually need to load both variations, then we'll need both in the package. Do you?

I'm also not keen on using MOZ_BRANDING_DIRECTORY as the thing we key off of. It works, but it isn't the proper abstraction layer. We really want a DEBUGGABLE_JS_MODULES flag or something. It's not that difficult to add one to configure.in. AC_SUBST makes a variable available to moz.build. Before you do that, it might be a good idea to see if we have an existing variable/flag we can repurpose.

Comment 25

[Mark Banner (:standard8)]

Regarding the licensing issues. We landed React in Hello with version 0.10.0, as bug 1033841 shows (the hg history is based on the filename, but it doesn't show the renames). This also had licensing approval (bug 1033784).

We then missed the change of license in the 0.11.2 -> 0.12.2 upgrade.

Rather than try to go through the re-approval process here, I've split this off to bug 1185704 with a fuller explanation and we'll deal with anything related to licensing that we need to there.

Comment 26

[Mark Banner (:standard8)]

(In reply to Gregory Szorc [:gps] from comment #24)
> If so, I'm not too keen on this solution. I would rather there be a single
> react.js in the package and the version is whatever the build configuration
> is. Of course, if you actually need to load both variations, then we'll need
> both in the package. Do you?

Hello does this in jar.mn and keys off MOZ_DEBUG:

http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/jar.mn

Since the dev version of react is slower & does more checks, this seemed like a reasonable thing to do for us.

Comment 27

[James Long (:jlongster)]

(In reply to Gregory Szorc [:gps] from comment #24)
> Comment on attachment 8631769 [details] [diff] [review]
> 1181646.patch
> 
> Review of attachment 8631769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/moz.build
> @@ +53,5 @@
> > +
> > +if CONFIG['MOZ_BRANDING_DIRECTORY'] == 'browser/branding/unofficial':
> > +    EXTRA_JS_MODULES.devtools.shared.content += [
> > +        'content/react-dev.js'
> > +    ]
> 
> I assume you are adding MOZ_BRANDING_DIRECTORY to AppConstants.jsm so you
> can do some kind of conditional import from JavaScript? e.g.
> 
>   if (MOZ_BRANDING_DIRECTORY == 'browser/branding/unofficial') {
>      import "react-dev.js"
>   } else {
>      import "react.js"
>   }
> 
> If so, I'm not too keen on this solution. I would rather there be a single
> react.js in the package and the version is whatever the build configuration
> is. Of course, if you actually need to load both variations, then we'll need
> both in the package. Do you?

No. I would like it to work that way too. Unfortunately, we can't do that with EXTRA_JS_MODULES. We would need the ability to "rename" a file. If you can show me how to add react.js at "devtools/shared/content/react.js" in a production build, but in a development instead put react-dev.js at that same place, let me know. I even asked in #build and they said you can't do that.

> 
> I'm also not keen on using MOZ_BRANDING_DIRECTORY as the thing we key off
> of. It works, but it isn't the proper abstraction layer. We really want a
> DEBUGGABLE_JS_MODULES flag or something. It's not that difficult to add one
> to configure.in. AC_SUBST makes a variable available to moz.build. Before
> you do that, it might be a good idea to see if we have an existing
> variable/flag we can repurpose.

I'm pretty far down a rabbit trail already. I don't have time to figure all that out in our build system, so either I need help doing it or we should just use MOZ_BRANDING_DIRECTORY for now.

Comment 28

[James Long (:jlongster)]

(In reply to Mark Banner (:standard8) from comment #26)
> (In reply to Gregory Szorc [:gps] from comment #24)
> > If so, I'm not too keen on this solution. I would rather there be a single
> > react.js in the package and the version is whatever the build configuration
> > is. Of course, if you actually need to load both variations, then we'll need
> > both in the package. Do you?
> 
> Hello does this in jar.mn and keys off MOZ_DEBUG:
> 
> http://mxr.mozilla.org/mozilla-central/source/browser/components/loop/jar.mn
> 
> Since the dev version of react is slower & does more checks, this seemed
> like a reasonable thing to do for us.

We thought of that but we really don't want to force devs to compile in debug mode, as it's so much slower. Frontend debug is way different than platform debug.

Comment 29

[James Long (:jlongster)]

ni? gps again because I'd really like to know if you can figure out how to build in different dev/prod versions of a file into the exact same path

Comment 30

[Gregory Szorc [:gps]]

(In reply to James Long (:jlongster) from comment #29)
> ni? gps again because I'd really like to know if you can figure out how to
> build in different dev/prod versions of a file into the exact same path

See comment #26.

Comment 31

[James Long (:jlongster)]

(In reply to Gregory Szorc [:gps] from comment #30)
> (In reply to James Long (:jlongster) from comment #29)
> > ni? gps again because I'd really like to know if you can figure out how to
> > build in different dev/prod versions of a file into the exact same path
> 
> See comment #26.

We really, really don't want to use jar.mn. We are moving away from chrome URLs and mapping everything to a sane path on the filesystem. Using resource URLs consistently for everything is really nice, and easy to discover where things are loaded from (we are currently working on moving everything from devtools into a single directory).

What is your concern about exposing MOZ_BRANDING_DIRECTORY in AppConstants?

Comment 32

[Gregory Szorc [:gps]]

Oh, I see.

moz.build files don't yet expose a way to install files under a different filename via EXTRA_JS_MODULES. They could, if needed. But this seems like a lot of work for what appears to be a one-off.

How about having the moz.build add the appropriate file to EXTRA_JS_MODULES depending on MOZ_DEBUG or some such and then have consumers try to import one, catch the exception, then fall back to another? You could always write a shim js file that imports the appropriate file (or possibly #includes it via the preprocessor) and re-exports its symbols.

MOZ_BRANDING_DIRECTORY is a build configuration that says which *source* directory to grab branding files from. From the perspective of a built application, the branding files all exist in the same path (more or less). Using MOZ_BRANDING_DIRECTORY for this is a hack and a leaky abstraction.

Comment 33

[James Long (:jlongster)]

(In reply to Gregory Szorc [:gps] from comment #32)
> Oh, I see.
> 
> moz.build files don't yet expose a way to install files under a different
> filename via EXTRA_JS_MODULES. They could, if needed. But this seems like a
> lot of work for what appears to be a one-off.

This may seem like a one-off, but we're really trying to reduce weirdness when it comes to loading JS files. To me, it seems really straight-forward to just include them in moz.build (potentially ignoring the dev version in prod if you want to save space). These one-offs tend to build up and create a lot of papercuts.

> How about having the moz.build add the appropriate file to EXTRA_JS_MODULES
> depending on MOZ_DEBUG or some such and then have consumers try to import
> one, catch the exception, then fall back to another? You could always write
> a shim js file that imports the appropriate file (or possibly #includes it
> via the preprocessor) and re-exports its symbols.

When would you do that exception/fall-back? Every single time you load React? Definitely don't want to do that. We could maybe do that in a custom loader but that seems like a ton of work, when all we just need is a runtime flag.

> 
> MOZ_BRANDING_DIRECTORY is a build configuration that says which *source*
> directory to grab branding files from. From the perspective of a built
> application, the branding files all exist in the same path (more or less).
> Using MOZ_BRANDING_DIRECTORY for this is a hack and a leaky abstraction.

Ok, I'm fine if we don't use that, but we also can't use MOZ_DEBUG. No way we want to run the full debug build just to get nice frontend errors.

I guess we really need to introduce a new JS variable like you mentioned before then?

Comment 34

[Gregory Szorc [:gps]]

Yeah, a new variable in configure.in is the way to go. It's pretty easy to add a variable. Search for MOZ_ARG_ENABLE_BOOL in configure.in. Use AC_SUBST to make the variable available to moz.build files.

Comment 35

[James Long (:jlongster)]

Cool, thanks! What should the new variable be? You mentioned DEBUGGABLE_JS_MODULES. Other options: DEBUG_JS_MODULES, JS_DEBUG, FRONTEND_JS_DEBUG, FRONTEND_DEBUG, etc etc

Comment 36

[Gregory Szorc [:gps]]

I like DEBUG_JS_MODULES.

Comment 37

[James Long (:jlongster)]

Attachment: 1181646.patch

This patch introduces the DEBUG_JS_MODULES config flag, that can be enabled with `ac_add_options --enable-debug-js-modules`. Eventually we should include that in the list of build options on whatever "getting started" page for devtools.

When that is enabled, the dev version of React will be bundled and the browser loader will automatically require it (without needing to change any `require`s)

Comment 38

[James Long (:jlongster)]

I guess this still deserves a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ff149d539f6

Comment 39

[James Long (:jlongster)]

None of those failures should be related to this (this only adds some files and adds an option to the build process)

Comment 40

[Pulsebot]

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

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/eb43469cf706