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

[Jie Kang]

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
> >
> >
>