1426908 - CodeMirror styling does not apply for some javascript files in Response tab

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[magicp]

Attachment: netmonitor-codemirror-styling.png

Steps to reproduce:
1. Launch Nightly
2. Open Netmonitor (Ctrl+Shift+E)
3. Set "JS" requests list filter
4. Go to "https://bugzilla.mozilla.org"
5. Select "jquery-min.js"
6. Switch sidebar tab to "Response"

Actual results:
CodeMirror styling does not apply.
It sometimes works after select other javascript file or html. But, it does not work again if switch to other sidebar tab and switch back.

Expected results:
CodeMirror styling should apply.

Regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=e16b6746b48f5a673888b4958ed3cb9192f7b1ab&tochange=cbf0cc9cb0b5ea2724e1b0586523ce6a275a991e

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks for the report I can reproduce the problem.

@Ricky, could this regression also be caused by Bug 1350226?

Honza

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Syntax highlight will be disabled if file size > 50kb.

See https://searchfox.org/mozilla-central/source/devtools/client/netmonitor/src/constants.js#320

It's by design for mitigating perf issue since applying syntax highlight could slow down editor loading. I hope that makes sense :)

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Ricky Chien [:rickychien] from comment #2)
> Syntax highlight will be disabled if file size > 50kb.
It doesn't sound like the issue happens due to the limit, the STR says: "It sometimes works".

Honza

Comment 4

[magicp]

Attachment: steps-to-reproduce_20180105_163039.mp4

Could you check the attached video that it is intentional or not?

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Oh, okay I overlooked the STR. This looks like an issue. Let me take a look. Thanks!

Comment 6

[Ricky Chien [:rickychien] (inactive)]

Attachment: Bug 1426908 - Asynchronously enable CodeMirror syntax highlight for large content

Review commit: https://reviewboard.mozilla.org/r/210380/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210380/

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8940099 [details]
Bug 1426908 - Asynchronously enable CodeMirror syntax highlight for large content

https://reviewboard.mozilla.org/r/210380/#review216130

Thanks for working on this Ricky!

Pleas see my inline comments.

Honza

::: devtools/client/netmonitor/src/components/SourceEditor.js:20
(Diff revision 1)
>   * CodeMirror editor as a React component
>   */
>  class SourceEditor extends Component {
>    static get propTypes() {
>      return {
>        // Source editor syntax hightligh mode, which is a mime type defined in CodeMirror

typo: hightligh -> highlight

::: devtools/client/netmonitor/src/components/SourceEditor.js:33
(Diff revision 1)
>  
>      this.editor = new Editor({
>        lineNumbers: true,
>        lineWrapping: false,
> -      mode: text.length < SOURCE_EDITOR_SYNTAX_HIGHLIGHT_MAX_SIZE ? mode : null,
>        readOnly: true,

Why did you remove the limit? We might want to make the limit bigger, but I would keep it there. I know it's async, but having limit in this case is good thing.

Also, the mode is not set when the component mounts, so the initial opening of the response panel doesn't show colorized code.

::: devtools/client/netmonitor/src/components/SourceEditor.js:38
(Diff revision 1)
>        readOnly: true,
>        theme: "mozilla",
>        value: text,
>      });
>  
>      // Delay to CodeMirror initialization content to prevent UI freezed

typo: to prevent UI freezed -> to prevent UI freezing

Comment 8

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8940099 [details]
Bug 1426908 - Asynchronously enable CodeMirror syntax highlight for large content

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210380/diff/1-2/

Comment 9

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8940099 [details]
Bug 1426908 - Asynchronously enable CodeMirror syntax highlight for large content

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210380/diff/2-3/

Comment 10

[Ricky Chien [:rickychien] (inactive)]

After investigation, I found out that the original constrain of disabling syntax highlight when processing large content can be addressed by enabling syntax highlight (calling setMode) asynchronously.

This patch basically renders source code first, and the enable syntax highlight later within setTimeout.

I've tested on several large JS files like jquery-ui.min.js (247.72 kb) in https://bugzilla.mozilla.org/, and it performs pretty fast.

What's different between current approach & this patch

Without patch: render source code + drawing syntax highlight in the same time (in sync way & if file size < 50kb)

If we're going to draw syntax highlight in this case, the source code will be rendered with syntax highlight enabled in the same time, so the rendering time of source code would be slower. But we can separate them into two steps and make syntax highlight async. 

With this patch: render source code -> drawing syntax highlight (in async way)

As a result, user can always see source code appears immediately without syntax color, gain more perceived performance.

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8940099 [details]
Bug 1426908 - Asynchronously enable CodeMirror syntax highlight for large content

https://reviewboard.mozilla.org/r/210380/#review216140

Ok, looks good, thanks Ricky!

(I tested with ~4M js file and it's still quite fast)

R+ assuming try is green.

Honza

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63d8a697c151
Asynchronously enable CodeMirror syntax highlight for large content r=Honza

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/63d8a697c151

Comment 14

[magicp]

This bug fix has verified in the latest Nightly build (20180105220204). Thanks!

Comment 15

[Gerry Chang [:gchang]]

It's too late for 58, mark 58 won't fix.