This will be useful for multiple tools, including web console (Bug 1308216) and net monitor (Bug 1308441)

Gerv, could you please give a license review for this external library that we're adding? react-addons-shallow-compare is part of React, so we already have the license for that. React Virtualized is the one that we need to add a license for.

According to elvin, dcamp is currently thinking about React usage within Mozilla. Before I r= this I'd like him to comment. Gerv

Hi Gerv, My understanding - this is complex and has been under discussion for 6+ months already, and React Virtualized isn't material to the React discussion I'm keen for us to not hold up important work on devtools because of a side issue to a long running debate. Even if the answer to react is 'no' then we won't regret the decision to go ahead with React Virtualized.

Joe: AIUI, React Virtualized is additional components for React. If you want them in the codebase, my assumption is that you want to use them, and that this would increase our use of React. I don't feel I can give an r= to that without dcamp's approval - which he is of course at liberty to give while discussions about React continue, if he feels it's appropriate. Gerv

I don't think it would increase our usage of React, perhaps the opposite! The alternative to React Virtualized isn't "write that component without using React", it's "write our own version of React Virtualized".

Like I said, just get Dave to give it the nod, and all will be well :-) If he doesn't, you can ask him what to use instead :-) Gerv

a+

Comment on attachment 8804093 [details] [diff] [review] Bug1312481-part1.patch Review of attachment 8804093 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Lin for doing this! R+ assuming my comment is resolved. Honza ::: devtools/client/shared/vendor/react-addons-shallow-compare.js @@ +1,1 @@ > +module.exports = require("devtools/client/shared/vendor/react").addons.shallowCompare; \ No newline at end of file Please put Mozilla license headers at the top.

Comment on attachment 8804094 [details] [diff] [review] Bug1312481-part2.patch Review of attachment 8804094 [details] [diff] [review]: ----------------------------------------------------------------- We have an extra license files for react libs in the directory (e.g. REDUX_LICENSE and REACT_REDUX_LICENSE). I think we should have an extra file for react-virtualized too: REACT_VIRTUALIZED_LICENSE Honza

One more thing, I think that the REACT_VIRTUALIZED_UPGRADING file should also have Mozilla license header at the top (I know that the other *_UPGRADING files are missing that). Honza

Comment on attachment 8804093 [details] [diff] [review] Bug1312481-part1.patch Review of attachment 8804093 [details] [diff] [review]: ----------------------------------------------------------------- r=gerv. Gerv

I added the Mozilla license to the shallow compare file. On the other comments, it seemed like more files in the vendor directory did not do it that way. If we want to change that, I think it would make sense to file an issue and do them all together to make them consistent.

(In reply to Lin Clark [:linclark] from comment #13) > Created attachment 8806166 [details] [diff] [review] > Bug1312481-part2.patch > > I added the Mozilla license to the shallow compare file. On the other > comments, it seemed like more files in the vendor directory did not do it > that way. If we want to change that, I think it would make sense to file an > issue and do them all together to make them consistent. Done, bug 1314539 Honza

needs rebasing applying Bug1312481-part1.patch patching file devtools/client/shared/vendor/moz.build Hunk #1 FAILED at 1 1 out of 1 hunks FAILED -- saving rejects to file devtools/client/shared/vendor/moz.build.rej patching file toolkit/content/license.html Hunk #1 FAILED at 129 1 out of 2 hunks FAILED -- saving rejects to file toolkit/content/license.html.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh Bug1312481-part1.patch

Since this has been in review for a while, I'm going to update it to a more recent version

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/da601059fdb2 Part 1: Add react-virtualized. r=Honza https://hg.mozilla.org/integration/mozilla-inbound/rev/f12283aa3114 Part 2: Make it possible to load React Virtualized in devtools (including XUL-based tools). r=Honza https://hg.mozilla.org/integration/mozilla-inbound/rev/2f53c8537285 Update react-virtualized to 8.3.1. r=bgrins

https://hg.mozilla.org/mozilla-central/rev/da601059fdb2 https://hg.mozilla.org/mozilla-central/rev/f12283aa3114 https://hg.mozilla.org/mozilla-central/rev/2f53c8537285