6531: Make the flame chart view render labels on windows #41
Conversation
|
|
|
The comment in the upstream d3-flame-graph suggests that there is no solution and unless someone comes up with one, nothing will change. Your commits in the fork seem to be a solution. Could that be proposed upstream? Does it introduce issues outside of IE? |
I'll post back on the original issue to see what they think. It looks like they start using ES6 syntax later on, which would explain why versions
Outside of IE I didn't encounter any problems (but I didn't check much more than seeing what it looked like if used in Linux for JMC), although with regards to this patch and JMC the modified JS is only used in Windows environments. There is a bit more computation in the fork - the text on the labels need to be trimmed for text-overflow instead of having the css format it. From what I understand, elements within [0] https://www.w3.org/TR/SVG2/embedded.html#ForeignObjectElement |
|
To me it's a bit scary (as in possibly maintenance heavy) to make a fork of the d3-library, rather than investing in making the SWT browser component work with Edge or chromium. This could potentially be a stop gap measure though. @mirage22, would this complicate anything you were planning on doing with the flame view? |
|
While the guard to only use this on Windows does mitigate impact of this patch, I also am concerned with maintenance of a fork of the d3-flame-graph. I would rather see the upstream updated, or as Marcus stated, the SWT browser updated to use an alternative (newer) browser. |
That sounds fair, I replied to the original Windows-related issue and tagged the repo owner, will comment back when there's a response. In other news it looks like the latest version of Edge is based on Chromium [0], so getting Chromium support would be nice. Progress on that is tracked at https://bugs.eclipse.org/bugs/show_bug.cgi?id=549585 [0] https://support.microsoft.com/en-ca/help/4501095/download-the-new-microsoft-edge-based-on-chromium |
|
It doesn't feel right to me to maintain d3-flame-graph fork, as @thegreystone stated it's a bit scary. I'd propose the way @thegreystone has stated -> SWT update to use an alternative (newer) browser. we are planning to do the search/highlighting of nodes inside the flameview. This feature uses d3-lib functionality. I don't know whether this patch will not force us to maintain also the fork of d3 itself. |
|
How hard would it be to make SWT use Edge as the default embedded browser and fall back to IE if not available? |
|
Hi @jcompagner, thanks for making a comment in an OpenJDK project! All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user jcompagner for the summary. If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.
Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use. |
As for "maintaining the fork", the next version up from the fork (v2.0.3) is incompatible with IE, so the fork is basically a one-shot fix that can't be updated. The interesting thing here is that we're using v2.0.3 (Sept 2018) , so if we had used just one release prior then the flame labels would have rendered in JMC Windows (without text) prior to the latest commits for colouring.
Chromium support was initially slated for integration Q1 2017 [0], and the related patch looked to be on track to be posted by the end of 2019 [1], but it hasn't made it out yet. So when the patch does get posted, it'll need review, inclusion into SWT, and then shipped with a release .. we're looking at potentially quite a bit of time before this happens, but hopefully not. I'm conflicted both ways, so I'll go based on the consensus here. I'm not sure how popular JMC is for Windows, or how many people are wanting (or trying) to use the flame view, but it doesn't sit right with me that the view doesn't display anything. Maybe it can print an error screen mentioning the incompatibility instead? [0] https://projects.eclipse.org/development_effort/implement-swtchromium-integration |
|
I think I'm ok with this being a stop-gap measure. It does make third party-dependencies a bit funny - and there is a bit of trust involved in getting the javascript from a private fork of a project, and even a private push of the resulting build to a CDN. As soon as the Eclipse changes are in, in any form and shape, we should switch. |
aptmac
force-pushed
the
flamegraph-ie
branch
from
4258542
to
885913f
Compare
Agreed, it would be very limiting to continue on this way. This PR required some rebasing after the latest flamegraph commits to include colour highlighting. I had to include an additional commit here that allows for the colouring to work with IE, because some syntax and functions are too advanced for ES2015. About the Here's an example screenshot of it working in Windows with text highlighting: |
|
@aptmac this pull request can no longer be integrated into git checkout flamegraph-ie
git fetch https://git.openjdk.java.net/jmc master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
|
Dumb question, but would a shorter path be to run babel against d3 (Or have that as part of the build?) That would eliminate the need to maintain your own version and make updating a lot easier. |
|
Hi Alex! I think https://bugs.eclipse.org/bugs/show_bug.cgi?id=549585 is getting closer to release. Any chance you could see if that works as a drop-in replacement? |
Not a dumb question! I've given this a look, but I don't think Babel is able to help us out here. Specifically I'm finding a bunch of resources on the internet mixing up terminology.. Babel seems to be able to covert back to ECMA2015, which is ES6. Internet Explorer here requires ES5 (or "ECMA2009"), which is outside the realm of Babel's capabilities.
I've checked out the gerrit patch and managed to get it to build (it looks like there's still more work to be done to get tests to pass), but it doesn't seem to be working. I'm mostly sure I added the relevant parts in the relevant places, but the new property |
|
:'( |
|
Chromium support has been added to SWT, and the Flamegraph should be able to display once configured. https://bugs.eclipse.org/bugs/show_bug.cgi?id=549585 As such, there is no specific support required for Windows, and this PR can be dropped. |
|
Eclipse 2020-09-16 is now available which includes the upstream fix. @thegreystone Is upgrading just a matter of replicated the previous upgrade in 0a1c4b3 ? |
This PR addresses JMC-6531 [0], in which the flame chart view displays nothing in Windows.
tl;dr: the proposed solution uses a fork of d3-flame-graph [1] which substitutes the incompatible svg element with one that works with Internet Explorer, and modifies the flameviewColoring script to work with an older ECMA version.
Screenshot of Windows:
For reference, here's the same view in Linux:
When I had originally taken a look into the SWT Browser and compatibility with Windows, it looked as though we'd be able to use Edge as an embedded browser, which would be nice because it already plays nice with the d3-flame-graph library. After quite a bit of trial and error I was first able to get d3 to work (making simple line charts), and then found that d3-flame-graph version <= 2.0.2 will work. With this older version of d3-flame-graph the coloured labels will be drawn, but no text is shown - regardless of the SWT browser property being set to IE or Edge .. which makes it look like Edge can be used for visiting external webpages, but not local ones (internal browser stays IE). This is because the foreignObject element is incompatible with IE, and this has already been raised as an issue and closed as a "wontfix" by the maintainer on the upstream repo [2].
I made a fork of the d3-flame-graph repo [1], which accomplishes two things:
<foreignObject>for<text>[3]This forked version of d3-flame-graph is downloaded by the maven-download-plugin like the other jslibs, and is used if the Environment type is
WINDOWSas detected inFlameGraphView.java[5].Now that the flame chart view works at this point, I had to edit
flameviewColoring.javato be compatible with IE. From what I can find, Internet Explorer uses JScript, which in it's latest version supports ECMA 5 [6]. This means no maps, no named functions, etc.Lastly, and not completely related to this issue, I changed the name of
page.templatetotemplate.htmlbecause my IDE won't provide syntax highlighting unless the file ends with -.html, and it was a big help here. If it's deemed unecessary I can just drop the associated commit.Let me know what you think!
[0] https://bugs.openjdk.java.net/browse/JMC-6531
[1] https://github.com/aptmac/d3-flame-graph
[2] spiermar/d3-flame-graph#84
[3] aptmac/d3-flame-graph@c508e2a#diff-e866318288d23357a14da878e2435b49L5211
[4] aptmac/d3-flame-graph@c508e2a#diff-e866318288d23357a14da878e2435b49R5238
[5] aptmac@b64b434#diff-9cb007d54dc422cf345941df647ef24cR102
[6] https://en.wikipedia.org/wiki/JScript#JScript
Progress
Issue
Download
$ git fetch https://git.openjdk.java.net/jmc pull/41/head:pull/41$ git checkout pull/41