[Rev 01] RFR: 6689: Export (and print?) the flame graph

[Kangcheng Xu]

On Mon, 2 Mar 2020 18:18:56 GMT, Alex Macdonald <aptmac at openjdk.org> wrote:

>> The pull request has been updated with 1 additional commit.
>
> Neat! I've been trying this out and the functionality looks good to me. I do have a few nits that I've added as comments.
> 
> When the chart is exported as a jpg, the background gets filled in black. IMO it should be white like how it looks in JMC, but maybe that's just my preference. However, because the opacity for the rectangles is `< 1`, and the background alters the colours of the labels. I left a suggestion in-line that the canvas background could be filled in white using `fillRect`, and here's a quick comparison of the resulting jpgs.
> 
> ![black-vs-white](https://user-images.githubusercontent.com/10425301/75704350-394e6200-5c87-11ea-8947-f1ced349e285.jpg)

Commit 87198ac24f30e467f911dabc8d82c97239e511f6 fixed issues mentioned in @aptmac 's review. Additionally, the second parameter is added to the `HTMLCanvasElement.toDataURL()` for a higher image quality.

> application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/page.template line 116:
> 
>> 115: 					"	white-space: nowrap;" +
>> 116: 					"   text-overflow: ellipsis;" +
>> 117: 					"	overflow: hidden;" +
> 
> the space before the `text-overflow` looks to be indented with spaces, rather than a tab like the other lines

Nice catch! Fixed in 87198ac24f30e467f911dabc8d82c97239e511f6

> application/org.openjdk.jmc.flightrecorder.flameview/src/main/resources/page.template line 140:
> 
>> 139: 					canvas.setAttribute("height", height);
>> 140: 
>> 141: 					canvas.getContext('2d').drawImage(image, 0, 0);
> 
> Just a suggestion, but if the canvas is filled in here then the resulting jpg image will have a white background. But it means the .png will also have a background instead of containing just the graph on a transparent background.. so if you're going this route then maybe the background should only fill if the type is jpeg.
> 
> Something like:
> canvas.getContext('2d').fillStyle = 'white';
> canvas.getContext('2d').fillRect(0, 0, canvas.width, canvas.height);

Fixed in 87198ac24f30e467f911dabc8d82c97239e511f6

> application/org.openjdk.jmc.flightrecorder.flameview/src/main/java/org/openjdk/jmc/flightrecorder/flameview/views/FlameGraphView.java line 391:
> 
>> 390: 			// FIXME: FileDialog filterIndex returns -1 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=546256)
>> 391: 			if (fd.getFileName().endsWith(".jpg") || fd.getFileName().endsWith(".jpeg")) { //$NON-NLS-1$ //$NON-NLS-2$
>> 392: 				type = "image/jpeg"; //$NON-NLS-1$
> 
> Small nit, (and the users would have to change it manually themselves), but this would make extensions written in capitals to throw an exception. Something like `image.JPG`.

Fixed in 87198ac24f30e467f911dabc8d82c97239e511f6

-------------

PR: https://git.openjdk.java.net/jmc/pull/56