[Enhancement] JMC-6589 Bundle the d3 library for flame charts with JMC

Tue Oct 29 20:44:12 UTC 2019

corrected:
webrev: http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.06/ <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.06/>

/M

> On Oct 29, 2019, at 7:38 PM, Marcus Hirt <marcus.hirt at datadoghq.com> wrote:
> 
> Small typo - jsIeLibraies should be jsIeLibraries.
> 
> /M
> 
> On Tue, Oct 29, 2019 at 7:30 PM Miro Wengner <miro.wengner at gmail.com> wrote:
>> 
>> Hi Everyone,
>> I’ve moved all html/js stuff to the src/main/resources folder
>> 
>> webrev: http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.05/
>> 
>> Kind Regards,
>> Miro
>> 
>> On Oct 29, 2019, at 12:24 AM, Marcus Hirt <marcus.hirt at datadoghq.com> wrote:
>> 
>> Might be nicer to download the javascript files to a subfolder under
>> src/main/resources instead, and also include that folder in the
>> .hgignore.
>> 
>> Kind regards,
>> Marcus
>> 
>> On Mon, Oct 28, 2019 at 11:50 PM Miro Wengner <miro.wengner at gmail.com> wrote:
>> 
>> 
>> Me neither, I had a small ‘ksh’ issue.
>> Here is corrected webrev, hopefully now all good.
>> 
>> webrev: http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.04/
>> 
>> Kind Regards,
>> Miro
>> 
>> On Oct 28, 2019, at 11:03 PM, Marcus Hirt <marcus.hirt at datadoghq.com> wrote:
>> 
>> Hi Miro,
>> 
>> Can't seem to find the jmc.patch file (expected
>> http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.03/jmc.patch).
>> 
>> Kind regards,
>> Marcus
>> 
>> On Mon, Oct 28, 2019 at 8:49 PM Miro Wengner <miro.wengner at gmail.com> wrote:
>> 
>> 
>> Hi Everyone,
>> 
>> thank you for discussions and feedback! Here is my patch.
>> All JS related content is downloaded through the plugin.
>> 
>> I hope that now all is acceptable,
>> 
>> issue: https://bugs.openjdk.java.net/browse/JMC-6589
>> webrev: http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.03/
>> 
>> Kind Regards,
>> Miro
>> 
>> 
>> On Oct 25, 2019, at 4:09 PM, Mario Torre <neugens at redhat.com> wrote:
>> 
>> Thanks :)
>> 
>> Cheers,
>> Mario
>> 
>> On Fri, Oct 25, 2019 at 3:58 PM Miro Wengner <miro.wengner at gmail.com> wrote:
>> 
>> 
>> So decision has been taken :)
>> We go the plugin way.
>> 
>> Ps: i agree with all of you
>> Kind Regards,
>> Miro
>> 
>> 
>> On 25. Oct 2019, at 15:19, Mario Torre <neugens at redhat.com> wrote:
>> 
>> On Thu, Oct 24, 2019 at 8:54 PM Miro Wengner <miro.wengner at gmail.com> wrote:
>> 
>> [OFF LIST]
>> 
>> Hi Jie,
>> I’d normally agree with you, but:
>> 1. In case we’d love to have it pulled out of the remote sources then we'd need to use npm or another front-end package build system.
>> 
>> 
>> I think there's an issue with licensing and the OpenJDK repositories
>> where we can only have content that is specifically coming from
>> OpenJDK, everything else must be downloaded during build time.
>> 
>> So we should take care of this, and I also don't like the idea of
>> embedding 3rd party, this makes it very difficult to track security
>> issues and also do packaging.
>> 
>> Cheers,
>> Mario
>> 
>> 2. What I also realized, during my work with “d3 libraries”, is that not all modifications are compatible with each other, could be an issue.
>> 
>> maybe this solution is the lowest pain and effective.
>> 
>> What do you think?
>> 
>> Thank you for looking into it !
>> 
>> Kind Regards,
>> Miro
>> 
>> 
>> 
>> On Oct 24, 2019, at 7:40 PM, Jie Kang <jkang at redhat.com> wrote:
>> 
>> 
>> Hi Miro, Marcus,
>> 
>> I think the contents are technically sound. I'm surprised to see the
>> minified js assets be added to version control of the jmc project
>> though. I see them as binary results of another project's build, so
>> this is akin to adding say a copy of the HdrHistogram jars to the vcs.
>> As dependencies, could it be made to have these files pulled from the
>> remote sources during the build process of the flame chart? What do
>> you think?
>> 
>> 
>> Regards,
>> Jie Kang
>> 
>> On Tue, Oct 22, 2019 at 3:52 AM Miro Wengner <miro.wengner at gmail.com> wrote:
>> 
>> 
>> Hi Everyone,
>> here is my updated version, please review my changes,
>> 
>> issue: https://bugs.openjdk.java.net/browse/JMC-6589 <https://bugs.openjdk.java.net/browse/JMC-6589>
>> webrev: http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.02/ <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev.02/>
>> 
>> Kind Regards,
>> Miro
>> 
>> On Oct 21, 2019, at 1:42 PM, Marcus Hirt <marcus at hirt.se> wrote:
>> 
>> Let's discuss #3 in the Slack a bit.
>> 
>> Kind regards,
>> Marcus
>> 
>> -----Ursprungligt meddelande-----
>> Från: jmc-dev <jmc-dev-bounces at openjdk.java.net> För Miro Wengner
>> Skickat: den 21 oktober 2019 13:28
>> Till: Marcus Hirt <marcus.hirt at datadoghq.com>
>> Kopia: jmc-dev at openjdk.java.net
>> Ämne: Re: [Enhancement] JMC-6589 Bundle the d3 library for flame charts with JMC
>> 
>> Hi Marcus,
>> thank you for taking a look!
>> 1) “page.html.o <http://page.html.org/>rig” : should not be there, I’ve it overlooked -> removed, formatting too!
>> 2) I’d personally also prefer renaming -> page.html -> page.template
>> 3) I’d also go with placeholders in following case
>> 
>> I’ll post a new update version
>> Kind Regards,
>> Miro
>> 
>> 
>> On Oct 20, 2019, at 2:42 PM, Marcus Hirt <marcus.hirt at datadoghq.com> wrote:
>> 
>> Might be better to just rename the page.html to page.template, and add placeholders for the place(s) you want to inject stuff.
>> 
>> /M
>> 
>> On Sun, Oct 20, 2019 at 12:14 PM Marcus Hirt <marcus.hirt at datadoghq.com <mailto:marcus.hirt at datadoghq.com>> wrote:
>> Hi Miro,
>> 
>> Thank you for looking into this. A few nits, after a very superficial look:
>> * Why keep page.html.orig around? Is it still being used somewhere?
>> * The comments you have for where you fetched the javascript bundles differ in that only the last one has a space between the beginning of the comment and the text. All of them should be in the same format (personally I like the space).
>> 
>> And then a general comment:
>> I guess inlining the Javascript into the HTML is one of the few options we have, but wouldn't you want to have a more "unique" string to go look for? Many of the keys you use are short and look like they, if we're unlucky, could perhaps be used in one of the libraries we include in the future. I'd suggest namespacing them with something like jmc.inline just to reduce the likelihood of any future collisions.
>> 
>> Kind regards,
>> Marcus
>> 
>> 
>> 
>> On Fri, Oct 18, 2019 at 10:52 PM Miro Wengner <miro.wengner at gmail.com <mailto:miro.wengner at gmail.com>> wrote:
>> Hi Everyone
>> please review my enhancement for flamechart
>> 
>> webrev:http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/ <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/> <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/ <http://cr.openjdk.java.net/~mwengner/JMC-6589/webrev/>>
>> bug:https://bugs.openjdk.java.net/browse/JMC-6589 <https://bugs.openjdk.java.net/browse/JMC-6589> <https://bugs.openjdk.java.net/browse/JMC-6589 <https://bugs.openjdk.java.net/browse/JMC-6589>>
>> 
>> Kind Regards,
>> Miro
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> 
>> --
>> Mario Torre
>> Associate Manager, Software Engineering
>> Red Hat GmbH <https://www.redhat.com>
>> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>> 
>> 
>> 
>> 
>> --
>> Mario Torre
>> Associate Manager, Software Engineering
>> Red Hat GmbH <https://www.redhat.com>
>> 9704 A60C B4BE A8B8 0F30  9205 5D7E 4952 3F65 7898
>> 
>> 
>> 
>>