DevTools: [Sources] Improve editor load time by optimizing setMimeType

DevTools: [Sources] Improve editor load time by optimizing setMimeType

Now setMimeType is called only once per mimeType.
It runs synchronously if it can to avoid re-highlighting the document.

BUG=664324
Committed: https://crrev.com/1f11bb313b15f3805841fa940a9543232417fa78
Cr-Commit-Position: refs/heads/master@{#432744}

[einbinder, 2016-11-12 02:59:23]

einbinder@chromium.org changed reviewers:
+ lushnikov@chromium.org

[einbinder, 2016-11-12 02:59:24]

ptal

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:628:
if (this._mimeType === mimeType)
We have to do this check after the mimeType is rewritten.

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:1110:
if (this._hasLongLines())
This code was previously in setMimeType, but it makes more sense in setText.

[einbinder, 2016-11-12 02:59:42]

The CQ bit was checked by einbinder@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-12 02:59:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-12 06:21:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-12 06:21:05]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[lushnikov, 2016-11-14 19:32:56]

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:268:
* @return {?Promise}
Let's keep this "!"

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:613:
* @return {!Promise}
can we drop !Promise from here?

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:616:
var promise = WebInspector.CodeMirrorTextEditor._loadMimeTypeModes(mimeType);
Let's come up with a clearer API.

A few suggestions:
1. Split this method into two: _modesToInstall(mimeType) and
_installModes(Array<Mode>)
2. Introduce an explicit method _isMissingMode(mimeType)

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:630:
this._mimeType = mimeType;
I'm not sure it's a good idea to expose the whitespace-specific mimeType to the
outer world. Seems to be an implementation detail of the SourcesTextEditor.

Another concern is that mimeType like "text/javascripit+whitespace" will get as
an argument in the SourcesTextEditor.rewriteMimeType, and will end up being
"text/javascript+whitespace+whitespace".

[einbinder, 2016-11-15 02:33:15]

ptal

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:613:
* @return {!Promise}
On 2016/11/14 at 19:32:55, lushnikov wrote:
> can we drop !Promise from here?

Yes.

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:616:
var promise = WebInspector.CodeMirrorTextEditor._loadMimeTypeModes(mimeType);
On 2016/11/14 at 19:32:55, lushnikov wrote:
> Let's come up with a clearer API.
> 
> A few suggestions:
> 1. Split this method into two: _modesToInstall(mimeType) and
_installModes(Array<Mode>)
> 2. Introduce an explicit method _isMissingMode(mimeType)

I am instead having _loadMimeTypeModes take a callback like it use to.

https://codereview.chromium.org/2498783002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:630:
this._mimeType = mimeType;
On 2016/11/14 at 19:32:55, lushnikov wrote:
> I'm not sure it's a good idea to expose the whitespace-specific mimeType to
the outer world. Seems to be an implementation detail of the SourcesTextEditor.
> 
> Another concern is that mimeType like "text/javascripit+whitespace" will get
as an argument in the SourcesTextEditor.rewriteMimeType, and will end up being
> "text/javascript+whitespace+whitespace".

Done.

https://codereview.chromium.org/2498783002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js
(left):

https://codereview.chromium.org/2498783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/source_frame/SourcesTextEditor.js:517:
if (this._mimeType)
This if was not needed.

[lushnikov, 2016-11-16 05:23:36]

https://codereview.chromium.org/2498783002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2498783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:270:
static _loadMimeTypeModes(mimeType, callback) {
let's rather split this into two functions. It would be easier to read and
understand

[einbinder, 2016-11-16 23:17:54]

https://codereview.chromium.org/2498783002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2498783002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:270:
static _loadMimeTypeModes(mimeType, callback) {
On 2016/11/16 at 05:23:35, lushnikov wrote:
> let's rather split this into two functions. It would be easier to read and
understand

Done.

[lushnikov, 2016-11-17 00:00:24]

thanks, lgtm

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:270:
static _gatherModesForMimeType(mimeType) {
_collectUninstalledModes

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:299:
static _installMimeTypeMode(extension) {
_installMimeTypeModes

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:300:
return extension.instance().then(installMode);
var promises = extensions.map(extension =>
extension.instance().then(installMode));

return Promise.all(promises);

[einbinder, 2016-11-17 00:57:03]

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js
(right):

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:270:
static _gatherModesForMimeType(mimeType) {
On 2016/11/17 at 00:00:24, lushnikov wrote:
> _collectUninstalledModes

Done.

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:299:
static _installMimeTypeMode(extension) {
On 2016/11/17 at 00:00:24, lushnikov wrote:
> _installMimeTypeModes

Done.

https://codereview.chromium.org/2498783002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/text_editor/CodeMirrorTextEditor.js:300:
return extension.instance().then(installMode);
On 2016/11/17 at 00:00:24, lushnikov wrote:
> var promises = extensions.map(extension =>
extension.instance().then(installMode));
> 
> return Promise.all(promises);

Done.

[einbinder, 2016-11-17 00:57:06]

The CQ bit was checked by einbinder@chromium.org

[einbinder, 2016-11-17 00:57:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
lushnikov@chromium.org
Link to the patchset: https://codereview.chromium.org/2498783002/#ps100001
(title: "installMimeTypeModes")

[commit-bot: I haz the power, 2016-11-17 00:57:55]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 03:00:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-17 03:00:51]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_TIMED_OUT,
no build URL)
  android_compile_dbg on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)
  android_cronet on master.tryserver.chromium.android (JOB_TIMED_OUT, no build
URL)
  cast_shell_android on master.tryserver.chromium.android (JOB_TIMED_OUT, no
build URL)

[einbinder, 2016-11-17 03:25:13]

The CQ bit was checked by einbinder@chromium.org

[commit-bot: I haz the power, 2016-11-17 03:26:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 04:45:20]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-11-17 04:48:07]

Description was changed from

==========
DevTools: [Sources] Improve editor load time by optimizing setMimeType

Now setMimeType is called only once per mimeType.
It runs synchronously if it can to avoid re-highlighting the document.

BUG=664324
==========

to

==========
DevTools: [Sources] Improve editor load time by optimizing setMimeType

Now setMimeType is called only once per mimeType.
It runs synchronously if it can to avoid re-highlighting the document.

BUG=664324

Committed: https://crrev.com/1f11bb313b15f3805841fa940a9543232417fa78
Cr-Commit-Position: refs/heads/master@{#432744}
==========

[commit-bot: I haz the power, 2016-11-17 04:48:08]

Patchset 6 (id:??) landed as
https://crrev.com/1f11bb313b15f3805841fa940a9543232417fa78
Cr-Commit-Position: refs/heads/master@{#432744}