Devtools: [WIP] Implement enhanced devtools extension language APIs

Devtools: [WIP] Implement enhanced devtools extension language APIs

I've been hoping to get some feedback on the APIs I've been prototyping here:
https://gist.github.com/weswigham/5508b3532325d86c14bb
These extend the devtools to add support for a number of
IDE-esque features, the specific use cases I'm looking to
fulfill with this are "Go to definition" and "Find all
references" context menu actions, completions based on a
language-specific service, live feedback on code
correctness (a la red squiggles), and support for using
the console and watch expressions in the source language
which is being edited/debugged. (And, because it's on
theme, specifying a new syntax highlighter for a
language. I actually have a simple example of what I've 
been enabling in that regard here:
https://gist.github.com/weswigham/722ae531865fe29e5e3b)

I haven't gotten much feedback on the exact shape of the
APIs yet, but what I have at present I've prototyped
(most) out so we know what's needed and what's possible,
and hope that seeing it will get people interested:
http://ham.io/files/dt.mp4

(I've been writing a TypeScript extension in parallel
with the API.)

Context menu actions, completions, live code feedback,
transpilation and completion support for the console and
watch window, and custom syntax highlighters should all
be possible with these changes. All if it could still use
some work/cleaning/testing but it should do what it says
on the tin.

Notably: I have a _TON_ of TODOs/FIXMEs related to
displaying search results (and a few others) - the
internal API of the search pane was not meant for this
use case and I've thus far shoehorned it into its new
multipurpose role. I also need to write tests. :3

Still, it would be _really_ nice to get feedback on both
the shape of the public API and what I should/shouldn't
be mucking with internally.

BUG=484261

[wes, 2015-08-12 19:23:16]

wwigham@gmail.com changed reviewers:
+ paulirish@chromium.org, pavel.feldman@gmail.com

[wes, 2015-08-12 19:29:41]

I would love to get some feedback on what's acceptable here. I'd really like to
see this possible in chrome!

[paulirish, 2015-08-12 21:01:39]

This is great, Wes. Thank you.

Pavel likes the shape of this so far. On the whole, it's definitely going in the
right direction. 
In fact, I think we're a bit impressed with your work understanding the
codebase.

I'm going to set up some time so you can walk us through these changes.
Also, we'll want to land this incrementally, so perhaps you could consider how
you'd like to split this work apart.

Expect a message from me booking some time for us to talk more.

[paulirish, 2015-08-12 21:01:55]

paulirish@chromium.org changed reviewers:
+ pfeldman@chromium.org
- pavel.feldman@gmail.com

[pfeldman, 2015-08-13 21:15:47]

This looks like a great start. Please find random comments below!

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/Runtime.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/Runtime.js:399: return this._modulesMap[moduleName] ?
this._modulesMap[moduleName]._loaded : false;
You should never care whether module is loaded.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js:212:
this._fileSystemBaseURL = "filesystem:" + this._fileSystem.normalizedPath() +
"/";
We use file:/// for origin urls so that we could quickly navigate to them / open
them in sibling tabs.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/bindings/NetworkMapping.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/bindings/NetworkMapping.js:99: return
this.uiSourceCodeForURLForAnyTarget(url) ||
this._workspace.uiSourceCodeForFilePath(url);
When is it used?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/common/ResourceType.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:143:
WebInspector.ResourceType.mimeFromUrl = function(url) {
mimeFromURL would be all caps
we place { on the next line everywhere

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:157:
WebInspector.ResourceType.registerMimeRecognizer = function(match, mime,
matchName) {
This signature and the method logic are very confusing.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:241: return
WebInspector.ResourceType.mimeFromUrl(WebInspector.SourcesPanel.instance().sourcesView().currentUISourceCode().contentURL());
You should never reach out to the module you don't depend on - this code won't
compile. You should runtime-based DI for that.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/common/TextUtils.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/TextUtils.js:171: fileName: function(text) {
{ goes the next line. Aslo seem like these two have a lot in commont. Extract
private helper?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/components/ExecutionContextSelector.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/components/ExecutionContextSelector.js:157:
WebInspector.ExecutionContextSelector.completionsForTextPromptInCurrentContext =
function(proxyElement, text, cursorOffset, wordRange, force,
completionsReadyCallback)
What is |text|?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/components/module.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/components/module.json:16: "languages",
Is there actually anything in components that depends on the languages?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/extensions/ExtensionAPI.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/extensions/ExtensionAPI.js:201: transpile:
function(text, location){ return Promise.reject(); },
We are using callbacks to be consistent with chrome extensions API style.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/inspector.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/inspector.json:4: { "name": "languages", "type":
"autostart" },
This should not be autostart - extensions module would wake it up.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/languages/LanguageService.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:13: get name() {},
We don't use getters for the new code.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:19: handlesCapability:
function(capability) { return false; }
capabilities: function getter instead?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:28: Transpile:
"transpile",
4 space indent everywhere.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/main/Main.js:847: // TODO: Expose the 'sources' panel
features more cleanly; files are
This needs to use runtime DI

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/main/Main.js:863:
self.runtime.loadModulePromise("sources").then(function() { //sources isn't
autoloaded, but it tracks line messages - load it
For example check out UISourceCode revealers (Revealer.reveal)

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/resources/ResourcesPanel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/resources/ResourcesPanel.js:2338: return "filesystem:"
+ this._fileSystem.name;
This is only used to restore last used item.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:198: var mime =
WebInspector.ResourceType.fromActivePanel();
Use DI for that. We should teach language support to respect runtime
extension-provided transpilers as well as API-added transpilers.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/RuntimeModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:44: this._locationResolver = raw
=> {
SDK can't depend on the bindings, source mappings are external to SDK.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:363: var location =
this.pauseLocation();
You should transpile expressionString prior to getting here, this is just and
SDK, language support layer needs to be on top of it.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/module.json:5: "languages",
So you don't really want this.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:961: getMimeType:
function() {
no get prefixes in Blink please!

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/SourceFrame.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/SourceFrame.js:1077:
populateLineGutterContextMenu: function(contextMenu, lineNumber)
This is worth a separate patch.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js
(right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js:129:
var oldPrefixRange = this._prefixRange;
Language support needs to be added outside of this layer as well.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/ui/SuggestBox.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/ui/SuggestBox.js:56: WebInspector.SuggestBox =
function(suggestBoxDelegate, maxItemsHeight)
This is worth a separate patch.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/ui/TextPrompt.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/ui/TextPrompt.js:34: * @param {function(!Element,
string, number, !Range, boolean, function(!Array.<string>, number=))}
completions
This is worth a separate patch.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/workspace/Workspace.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/workspace/Workspace.js:637: uiSourceCodeForFilePath:
function(url) {
You should not distinguish resources from the filesystem project from the
network-provided ones.

[caseq, 2015-08-13 21:28:10]

caseq@chromium.org changed reviewers:
+ caseq@chromium.org

[wes, 2015-08-14 00:55:06]

I think I've commented regarding my questions/feelings/action items on most of
your comments. I've also added a couple more on my own code where I'd like some
info, if possible.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js:212:
this._fileSystemBaseURL = "filesystem:" + this._fileSystem.normalizedPath() +
"/";
On 2015/08/13 21:15:46, pfeldman wrote:
> We use file:/// for origin urls so that we could quickly navigate to them /
open
> them in sibling tabs.

Some APIs returned file:/// URLs, some returned filesystem: ones (for the same
file!). There was a time during development when I unified them all to file:///,
but after reading some of the fetch spec reverted my changes and unified to
filesystem: instead. Which one is considered canonical?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/common/ResourceType.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:143:
WebInspector.ResourceType.mimeFromUrl = function(url) {
On 2015/08/13 21:15:46, pfeldman wrote:
> mimeFromURL would be all caps
> we place { on the next line everywhere

Acknowledged.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:157:
WebInspector.ResourceType.registerMimeRecognizer = function(match, mime,
matchName) {
On 2015/08/13 21:15:46, pfeldman wrote:
> This signature and the method logic are very confusing.

Does it need better argument names? Ie:

+/**
+ * @param {string} fileExtensionOrNamePattern
+ * @param {string} mime
+ * @param {boolean=} matchName
+ */

Or should it be broken out into two methods (one for by-extension, one for
by-name) across the entire stack?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:241: return
WebInspector.ResourceType.mimeFromUrl(WebInspector.SourcesPanel.instance().sourcesView().currentUISourceCode().contentURL());
On 2015/08/13 21:15:46, pfeldman wrote:
> You should never reach out to the module you don't depend on - this code won't
> compile. You should runtime-based DI for that.

I mean, it does compile (and run) thanks to the 
* @suppress {missingProperties}
annotation but it's more like a band-aid, since the 'correct' way to do it
is rather expensive for what amounts to a utility function. 

ANYWAYS! So, what would be the correct way to inject this information into a
scope accessible to
locations which call out to the language service manager? We want to reach into
the source module for information if its available, but without depending on
it...
I suppose I could define an interface for a delegate which provides this
function
and load it as an extension here?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/common/TextUtils.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/TextUtils.js:171: fileName: function(text) {
On 2015/08/13 21:15:46, pfeldman wrote:
> { goes the next line. Aslo seem like these two have a lot in commont. Extract
> private helper?

Probably. They're both just helpers for extracting the filename/extension from a
URL without parsing the URL... wait, why aren't I just parsing the URL with the
URL class? (I know why - it's because I extracted this code from elsewhere so I
could reuse it and left it unmodified) Is there a good (...perf?) reason I
shouldn't replace these with:

_pathMatcher: function(text, pattern)
{
  var match = new URL(text).pathname.match(pattern);
  if (match) {
    return match[1];
  }
},

extension: function(text) {
  return WebInspector.TextUtils._pathMatcher(/.*\.(.*)$/) || '';
},

fileName: function(text) {
  return WebInspector.TextUtils._pathMatcher(text, /.*\/(.*)$/) || '';
}

_Wait_ I think I know why already. filesystem: URLs are invalid URLs and cause
the class to throw. :(

I'll extract this into a common helper.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/components/ExecutionContextSelector.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/components/ExecutionContextSelector.js:157:
WebInspector.ExecutionContextSelector.completionsForTextPromptInCurrentContext =
function(proxyElement, text, cursorOffset, wordRange, force,
completionsReadyCallback)
On 2015/08/13 21:15:46, pfeldman wrote:
> What is |text|?

The actual, whole text of the prompt within which we're finding completions.
(cursorOffset being where the cursor is within that text.)

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/components/module.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/components/module.json:16: "languages",
On 2015/08/13 21:15:46, pfeldman wrote:
> Is there actually anything in components that depends on the languages?

Uhhhhh... there might not be anymore. I moved things around a lot. Good catch, I
think this can just be removed.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/extensions/ExtensionAPI.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/extensions/ExtensionAPI.js:201: transpile:
function(text, location){ return Promise.reject(); },
On 2015/08/13 21:15:46, pfeldman wrote:
> We are using callbacks to be consistent with chrome extensions API style.

Are we? Before this patch there were absolutely no extension call chains 
which started within chrome and resulted in a call out to an extension 
(and return a value to chrome). This is first of its kind.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/languages/LanguageService.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:13: get name() {},
On 2015/08/13 21:15:46, pfeldman wrote:
> We don't use getters for the new code.

Oh, okay. I had seen them around elsewhere and was unsure what the preference
was.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:19: handlesCapability:
function(capability) { return false; }
On 2015/08/13 21:15:46, pfeldman wrote:
> capabilities: function getter instead?

Possible. I can't really think of a use-case where a provider would
want to dynamically alter its capabilities like this would imply it
could, anyway.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:28: Transpile:
"transpile",
On 2015/08/13 21:15:46, pfeldman wrote:
> 4 space indent everywhere.

Ach. At least I was internally consistent to the file. Out of curiosity, 
is there a linter anywhere in the toolchain? I know there's the CC, but 
it doesn't check for style like this.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/main/Main.js:847: // TODO: Expose the 'sources' panel
features more cleanly; files are
On 2015/08/13 21:15:46, pfeldman wrote:
> This needs to use runtime DI 

It can also probably be moved out of Main and back into extension server.
It doesn't really need to be a delegate.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/main/Main.js:863:
self.runtime.loadModulePromise("sources").then(function() { //sources isn't
autoloaded, but it tracks line messages - load it
On 2015/08/13 21:15:46, pfeldman wrote:
> For example check out UISourceCode revealers (Revealer.reveal)

I actually mentioned that class in my TODO above. ;3
So this would be like a revealer but rather than opening the view, it should
simply
call setMessagesForSource on it.

So would I create a construct which simply functions like a Revealer; 
or hijack the existing revealer construct and re-purpose it to 'reveal'
resource line messages?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/main/Main.js:883: getResourceLineMessages:
function(code) {
We mentioned removing the `get` line messages (and the change event) at our
meeting. 
I just want to record that - this will likely be gone in the next patch and
that'll be why.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/resources/ResourcesPanel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/resources/ResourcesPanel.js:2338: return "filesystem:"
+ this._fileSystem.name;
On 2015/08/13 21:15:46, pfeldman wrote:
> This is only used to restore last used item.

This may be pedantic, but it's the result of going through (literally grep) and
unifying every 
single url returning function which dealt with files (file:// or filesystem: or
filesystem://) 
to all uniformly return `filesystem:` URLs rather than the menagerie of URL
prefixes each API 
returned before. The lack of consistency made things very confusing. Though this
one wasn't directly
used, it still got caught up in the mass change. One of my other comments asks
which should be
considered the 'canonical' URL for a file.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:198: var mime =
WebInspector.ResourceType.fromActivePanel();
On 2015/08/13 21:15:46, pfeldman wrote:
> Use DI for that. We should teach language support to respect runtime
> extension-provided transpilers as well as API-added transpilers.

So, I would declare an 'ActiveMimeExtensionDelegate' interface in the language
service module,
which has an implementation in the 'sources' module, which the language service
module can 
attempt to load each time this function is called (once it's moved into the
language service module)?

Is that the correct place for each of the parts to go?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:204: return; //preempted by a
following entry - drop ths one.
I don't know if this behavior is desirable, but I figure it's better than
getting results in a random order. Would like feedback.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:209: //TODO: decide how to handle
errors?
I would like some feedback on how to fulfill this TODO - errors from the
transpiler should probably be reported like a 'ReferenceError' in the js context
would (ie, immediately reported in the console). Right now it's just swallowed.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/RuntimeModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:44: this._locationResolver = raw
=> {
On 2015/08/13 21:15:46, pfeldman wrote:
> SDK can't depend on the bindings, source mappings are external to SDK.

This was literally a hack I threw in the other day after fiddling about. :D

So I added the transpile call as close to the sdk API as I could (knowing I'd
want 
some debugger info and not knowing the design
goals of the system very well) to reduce code duplication. Given what I've been
told now, 
it should be lifted from within ExecutionContext (which shouldn't know about
languages 
since its in sdk) and out to where `evaluate` gets called. This means lifting it
into 
JavaScriptSourceFrame _and_ WatchExpressionsSidebarPane in the sources module -
where 
the mapping can be done without this hack. This implies that in order to reuse
code 
I'll end up needing a TranspilingExecutionContextWrapper in sources for both to
use.

I also now realize I was creating awkward multiple transpiling code paths in
WatchExpressionsSidebarPane.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:363: var location =
this.pauseLocation();
On 2015/08/13 21:15:46, pfeldman wrote:
> You should transpile expressionString prior to getting here, this is just and
> SDK, language support layer needs to be on top of it.

Is expressionString supposed to be an identifier for member completions or
something? I don't think the still-being-built
string-in-the-console-or-watch-window should be transpiled by this point... so
it's not that... In fact, on inspection of my code at present, when a language
service is queried for completions at this point, expressionString isn't even
used in that process! Though to be fair, the debuggerCompletions codepath also
doesn't query the debugger for completion members (since the service is supposed
to know those). This is one of those things which has more use the more dynamic
the completions are supposed to be, I think?
 
So! This function is called in exactly one place (in the components module), but
that static delegate in components gets reused in a few places (in components &
console). Moving this into that delegate is pretty simple (since... it doesn't
rely on anything private here), but it means components will take a dependency
on language now. Is that okay (it gets it out of SDK, which is the goal, so I
think so)?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/module.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/module.json:5: "languages",
On 2015/08/13 21:15:47, pfeldman wrote:
> So you don't really want this.

I can see why now - yeah.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:961: getMimeType:
function() {
On 2015/08/13 21:15:47, pfeldman wrote:
> no get prefixes in Blink please!

Understood! Is there a lint rule for this? :P

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/CodeMirrorTextEditor.js:1078:
prom.then(showAsync, showAsync);
By the way - showing the context menu async seems flaky on the c++ side.

Random assertion violations seem to happen if one context menu attempts to get
drawn while another is, or if the JS tries to draw the menu twice. I hunted down
assertion violations many a time before I added the `event.consume(true);`
above.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/SourceFrame.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/SourceFrame.js:606:
getMessagesFromSource: function()
This is marked for removal.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/SourceFrame.js:1077:
populateLineGutterContextMenu: function(contextMenu, lineNumber)
On 2015/08/13 21:15:47, pfeldman wrote:
> This is worth a separate patch.

Making the context menu flow async in general, or the gutter menu? Both
originate from the same function in the CM wrapper, so the expectations on both
seem the same.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js
(right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js:129:
var oldPrefixRange = this._prefixRange;
On 2015/08/13 21:15:47, pfeldman wrote:
> Language support needs to be added outside of this layer as well.

So if not in source_frame then in sources? (I suppose UISourceCodeFrame?)

Right now UISourceCodeFrame uses WebInspector.SimpleAutocompleteDelegate (from
source_frame)
which means I'd end up subclassing WebInspector.TextEditorAutocompleteDelegate
and... ugh, the
TextEditorAutocompleteDelegate interface will need changing. wordsWithPrefix
needs to be async 
_and_ it needs to be able to return both the completions list and the list of
thunks for the details
of those completions. :welp:

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sources/SourcesView.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sources/SourcesView.js:100: var mimeType =
WebInspector.ResourceType.mimeFromUrl(uiSourceCode.uri());
I've been wavering about this for awhile - is .uri() or .contentURL() the
canonical URL for a file?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/ui/TextPrompt.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/ui/TextPrompt.js:34: * @param {function(!Element,
string, number, !Range, boolean, function(!Array.<string>, number=))}
completions
On 2015/08/13 21:15:47, pfeldman wrote:
> This is worth a separate patch.

Adding extra parameters to the completions delegate function call (refer back to
the answer to your 'what is |text|' comment)?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/ui/module.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/ui/module.json:6: "languages"
Looking at the final diff, I don't think this is required anymore, either.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/workspace/Workspace.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/workspace/Workspace.js:637: uiSourceCodeForFilePath:
function(url) {
On 2015/08/13 21:15:47, pfeldman wrote:
> You should not distinguish resources from the filesystem project from the
> network-provided ones.

I'll double check something but at some point I added this because OpenResource
wasn't working on the URLs of files from workspaces. If that's not the case when
I remove this I'm 100% sure it's 100% unnecessary. (I thought I had to make
other changes, too, but they're not in the diff... which means either I'm
crazy/forgetful or someone else integrated them into master while I was working
and I'm really fortunate that it got done already. Doesn't really matter.) I'll
remove it and double check.

[wes, 2015-08-14 01:13:32]

more assorted comments

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/bindings/NetworkMapping.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/bindings/NetworkMapping.js:99: return
this.uiSourceCodeForURLForAnyTarget(url) ||
this._workspace.uiSourceCodeForFilePath(url);
On 2015/08/13 21:15:46, pfeldman wrote:
> When is it used?

Solely in the extension server (get/set line messages, get resource content) and
related bits (ie, displaySearchResults delegate).

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/extensions/ExtensionServer.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/extensions/ExtensionServer.js:590: var uiSourceCode =
WebInspector.networkMapping.uiSourceCodeForURLForAnyTarget(message.url) ||
WebInspector.workspace.uiSourceCodeForFilePath(message.url);
Upon review, I see that this line is an alias to the
NetworkMapping.uiSourceCodeFromURL function I added, and should (likely?) be
replaced with it.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/inspector.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/inspector.json:4: { "name": "languages", "type":
"autostart" },
On 2015/08/13 21:15:46, pfeldman wrote:
> This should not be autostart - extensions module would wake it up.

If "components" or "extensions" module has a real dependency on it, doesn't that
mean it needs to be "autostart", too? Am I wrong? (I know the CC script throws a
fit if you claim an autostart module depends on a lazy one, so I don't think I
am?) Or are you proposing that everything it does should be behind lazy
interfaces and facades, and those modules should become dependent on those
interfaces, allowing it to be lazily loaded?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/workspace/Workspace.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/workspace/Workspace.js:637: uiSourceCodeForFilePath:
function(url) {
On 2015/08/14 00:55:06, wes wrote:
> On 2015/08/13 21:15:47, pfeldman wrote:
> > You should not distinguish resources from the filesystem project from the
> > network-provided ones.
> 
> I'll double check something but at some point I added this because
OpenResource
> wasn't working on the URLs of files from workspaces. If that's not the case
when
> I remove this I'm 100% sure it's 100% unnecessary. (I thought I had to make
> other changes, too, but they're not in the diff... which means either I'm
> crazy/forgetful or someone else integrated them into master while I was
working
> and I'm really fortunate that it got done already. Doesn't really matter.)
I'll
> remove it and double check.

Right! I found the other bit I added (the answer to my self questioning above if
forgetfulness!) - ExtensionServer._onGetPageResources had a line added to also
pickup the files from filesystem (aka workspace) projects. Previously extensions
could get events and stuff about them from some channels (events), but couldn't
get a list of extant ones (they were excluded from getPageResources - which I
suppose is true, they're not _technically_ part of the page). After adding that
and immersing myself in using workspaces I realized that there were a small few
other places where the extension API was incapable of dealing with FileSystem
(aka workspace) project file urls, and the addition of this helper was the
result.
It's also why I was hellbent on unifying the canonical URLs used to represent
filesystem files.

[wes, 2015-08-14 01:25:51]

misc comments

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/Runtime.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/Runtime.js:399: return this._modulesMap[moduleName] ?
this._modulesMap[moduleName]._loaded : false;
On 2015/08/13 21:15:45, pfeldman wrote:
> You should never care whether module is loaded.

Acknowledged.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/common/ResourceType.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:241: return
WebInspector.ResourceType.mimeFromUrl(WebInspector.SourcesPanel.instance().sourcesView().currentUISourceCode().contentURL());
On 2015/08/14 00:55:04, wes wrote:
> On 2015/08/13 21:15:46, pfeldman wrote:
> > You should never reach out to the module you don't depend on - this code
won't
> > compile. You should runtime-based DI for that.
> 
> I mean, it does compile (and run) thanks to the 
> * @suppress {missingProperties}
> annotation but it's more like a band-aid, since the 'correct' way to do it
> is rather expensive for what amounts to a utility function. 
> 
> ANYWAYS! So, what would be the correct way to inject this information into a
> scope accessible to
> locations which call out to the language service manager? We want to reach
into
> the source module for information if its available, but without depending on
> it...
> I suppose I could define an interface for a delegate which provides this
> function
> and load it as an extension here?

But if I did that then the module the extension was in would get loaded. 
Which isn't a thing we want. (Since we don't want any info from the module if it
doesn't exist) :(
Suggestions?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/ui/SuggestBox.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/ui/SuggestBox.js:56: WebInspector.SuggestBox =
function(suggestBoxDelegate, maxItemsHeight)
On 2015/08/13 21:15:47, pfeldman wrote:
> This is worth a separate patch.

Adding the details popover flow? Alright, I'll note it as one of the bits that
should get broken out.

[pfeldman, 2015-08-17 21:15:52]

Made a round of reviews. Since the change is large, lets try to extract the
pieces that make sense on their own. Things like suggest box, UISourceCode
markers, etc.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/bindings/FileSystemWorkspaceBinding.js:212:
this._fileSystemBaseURL = "filesystem:" + this._fileSystem.normalizedPath() +
"/";
They serve different purposes. file:/// is to point to the original file on the
disk, filesystem: is the withing workspace project prefix.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/bindings/NetworkMapping.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/bindings/NetworkMapping.js:99: return
this.uiSourceCodeForURLForAnyTarget(url) ||
this._workspace.uiSourceCodeForFilePath(url);
Then it should be at its call site since it discards the target and should be
caller's last resort. Hence does not belong to the mapping API.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/common/ResourceType.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:157:
WebInspector.ResourceType.registerMimeRecognizer = function(match, mime,
matchName) {
Well, registerRecognizer suggests that it'll be a callback registered here.
Could fileExtensionOrNamePattern be unified into one fileNamePattern? It is not
clear what mime is doing and why it is important, etc. What do you intend to do
here?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:241: return
WebInspector.ResourceType.mimeFromUrl(WebInspector.SourcesPanel.instance().sourcesView().currentUISourceCode().contentURL());
We have plenty of DI and context to handle this. You need a current
UISourceCode, right? That would be
WebInspector.context.flavor(WebInspector.UISourceCode). Make sure SourceViewer
maintains the right UISourceCode instance in the context though. Is probably
worth a separate change. You can even listen to the change to the flavor from
your module.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/extensions/ExtensionAPI.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/extensions/ExtensionAPI.js:201: transpile:
function(text, location){ return Promise.reject(); },
On 2015/08/14 00:55:05, wes wrote:
> On 2015/08/13 21:15:46, pfeldman wrote:
> > We are using callbacks to be consistent with chrome extensions API style.
> 
> Are we? Before this patch there were absolutely no extension call chains 
> which started within chrome and resulted in a call out to an extension 
> (and return a value to chrome). This is first of its kind.

Ok, I misunderstood its purpose then.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/extensions/ExtensionServer.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/extensions/ExtensionServer.js:590: var uiSourceCode =
WebInspector.networkMapping.uiSourceCodeForURLForAnyTarget(message.url) ||
WebInspector.workspace.uiSourceCodeForFilePath(message.url);
Your message has a target, so you should get resource from the project built on
that target first, then callback to any project.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/inspector.json (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/inspector.json:4: { "name": "languages", "type":
"autostart" },
If languages has API and compact implementation, it is bind for it to be
autostart (it would be very similar to bindings).

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/languages/LanguageService.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:13: get name() {},
On 2015/08/14 00:55:05, wes wrote:
> On 2015/08/13 21:15:46, pfeldman wrote:
> > We don't use getters for the new code.
> 
> Oh, okay. I had seen them around elsewhere and was unsure what the preference
> was.

I know, we are slowly getting rid of them wherever we change associated code.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/languages/LanguageService.js:28: Transpile:
"transpile",
On 2015/08/14 00:55:05, wes wrote:
> On 2015/08/13 21:15:46, pfeldman wrote:
> > 4 space indent everywhere.
> 
> Ach. At least I was internally consistent to the file. Out of curiosity, 
> is there a linter anywhere in the toolchain? I know there's the CC, but 
> it doesn't check for style like this.

Nope, we don't have a linter :( We use the Blink code style for JS which is
matching the C++ one.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/main/Main.js:863:
self.runtime.loadModulePromise("sources").then(function() { //sources isn't
autoloaded, but it tracks line messages - load it
I think we need to introduce proper markers the UISourceCodes themselves. The
ones that would bind messages to lines and allow listening to them.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/resources/ResourcesPanel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/resources/ResourcesPanel.js:2338: return "filesystem:"
+ this._fileSystem.name;
On 2015/08/14 00:55:05, wes wrote:
> On 2015/08/13 21:15:46, pfeldman wrote:
> > This is only used to restore last used item.
> 
> This may be pedantic, but it's the result of going through (literally grep)
and
> unifying every 
> single url returning function which dealt with files (file:// or filesystem:
or
> filesystem://) 
> to all uniformly return `filesystem:` URLs rather than the menagerie of URL
> prefixes each API 
> returned before. The lack of consistency made things very confusing. Though
this
> one wasn't directly
> used, it still got caught up in the mass change. One of my other comments asks
> which should be
> considered the 'canonical' URL for a file.

Right, except for these are used for different purposes and are different on
purpose.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:198: var mime =
WebInspector.ResourceType.fromActivePanel();
As discussed during our meeting, we should take this out of SDK and place it on
the level higher. I'd take this entire
WebInspector.ConsoleModel.evaluateCommandInConsole method and move it into your
language module. Call sites are UI and can depend on your module just fine.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:209: //TODO: decide how to handle
errors?
- please don't use arrow functions just yet
- that depends on the kind of error this is. When does the code above throw?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/RuntimeModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:44: this._locationResolver = raw
=> {
Your _locationResolver looks very similar to the source mapping that is baked
into both SDK and workspace. Are you doing the same thing here?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:363: var location =
this.pauseLocation();
This would also go into your module

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js
(right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js:129:
var oldPrefixRange = this._prefixRange;
Let me talk to lushnikov@ about the way we should refactor it.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sources/SourcesView.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sources/SourcesView.js:100: var mimeType =
WebInspector.ResourceType.mimeFromUrl(uiSourceCode.uri());
uri is the URI within workspace, contentURL is where it came from (fs or
network). You should take the mime from the name, i.e. uri.

[wes, 2015-08-25 18:13:18]

Was out for a week, came back with more comments. Noticed nobody's looked at the
most recent changeset yet.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/common/ResourceType.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/common/ResourceType.js:241: return
WebInspector.ResourceType.mimeFromUrl(WebInspector.SourcesPanel.instance().sourcesView().currentUISourceCode().contentURL());
On 2015/08/17 21:15:51, pfeldman wrote:
> We have plenty of DI and context to handle this. You need a current
> UISourceCode, right? That would be
> WebInspector.context.flavor(WebInspector.UISourceCode). Make sure SourceViewer
> maintains the right UISourceCode instance in the context though. Is probably
> worth a separate change. You can even listen to the change to the flavor from
> your module.

But... the contexts in which we'd like to know this information (within the
languages module) we don't have access to the UISourceCode type at _all_ (since
it's a class from the lazily loaded sources module)... I suppose if languages is
changed to be lazy and depend on sources then it works? Hm... The issue with
that is that the extensions depends on languages, and extensions is eagerly
loaded... And the registration and management part of the languages module
really does need to be eagerly loaded with it, so it's there to handle extension
requests...

I could break it apart into two modules, languages and languages_lazy -
languages being the management portion that gets eagerly loaded, and
languages_lazy being the implementations? But even then there's still the issue
that sources would need to depend on languages_lazy and languages_lazy would
need to depend on sources... Unless languages_lazy's components in sources get
loaded as extensions, then it works.

So... break languages down into a lazy part and an eager part, make the lazy
part depend on sources, then inject it where required?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/extensions/ExtensionServer.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/extensions/ExtensionServer.js:590: var uiSourceCode =
WebInspector.networkMapping.uiSourceCodeForURLForAnyTarget(message.url) ||
WebInspector.workspace.uiSourceCodeForFilePath(message.url);
On 2015/08/17 21:15:51, pfeldman wrote:
> Your message has a target, so you should get resource from the project built
on
> that target first, then callback to any project.

The open resource message only has a url and a line number?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/main/Main.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/main/Main.js:863:
self.runtime.loadModulePromise("sources").then(function() { //sources isn't
autoloaded, but it tracks line messages - load it
On 2015/08/17 21:15:51, pfeldman wrote:
> I think we need to introduce proper markers the UISourceCodes themselves. The
> ones that would bind messages to lines and allow listening to them.

IMO, binding messages to lines is very limiting. If we're overhauling them, we
want to be able to bind messages to arbitrary spans (which may span multiple
lines or only part of a line).

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/resources/ResourcesPanel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/resources/ResourcesPanel.js:2338: return "filesystem:"
+ this._fileSystem.name;
On 2015/08/17 21:15:51, pfeldman wrote:
> On 2015/08/14 00:55:05, wes wrote:
> > On 2015/08/13 21:15:46, pfeldman wrote:
> > > This is only used to restore last used item.
> > 
> > This may be pedantic, but it's the result of going through (literally grep)
> and
> > unifying every 
> > single url returning function which dealt with files (file:// or filesystem:
> or
> > filesystem://) 
> > to all uniformly return `filesystem:` URLs rather than the menagerie of URL
> > prefixes each API 
> > returned before. The lack of consistency made things very confusing. Though
> this
> > one wasn't directly
> > used, it still got caught up in the mass change. One of my other comments
asks
> > which should be
> > considered the 'canonical' URL for a file.
> 
> Right, except for these are used for different purposes and are different on
> purpose.

Okay - but why is there also both filesystem: and filesystem:// URLS? (And the
filesystem: URLs used for workspace files aren't even parsed as valid URLs by
the URL class, BTW, since it lacks the // to delimit the protocol).

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/ConsoleModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:198: var mime =
WebInspector.ResourceType.fromActivePanel();
On 2015/08/17 21:15:51, pfeldman wrote:
> As discussed during our meeting, we should take this out of SDK and place it
on
> the level higher. I'd take this entire
> WebInspector.ConsoleModel.evaluateCommandInConsole method and move it into
your
> language module. Call sites are UI and can depend on your module just fine.

By the time you had commented this I had actually already uploaded a changeset
which did just that. :D

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/ConsoleModel.js:209: //TODO: decide how to handle
errors?
On 2015/08/17 21:15:52, pfeldman wrote:
> - please don't use arrow functions just yet
> - that depends on the kind of error this is. When does the code above throw?

If the transpile call throws on the user's end (and by user, I mean 'person who
registers a language', not necessarily an extension author) throws, it throws. I
could change to manager to have the .catch call... but the correct behavior on
error is really callsite-specific (try again? ignore the registered language and
fall back to callsite's default?), I'd think, so it's probably just worth
handling here. In this case, I don't know if it's better to fallback to
evaluation as JS or reporting the error to the user somehow (or both).

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sdk/RuntimeModel.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:44: this._locationResolver = raw
=> {
On 2015/08/17 21:15:52, pfeldman wrote:
> Your _locationResolver looks very similar to the source mapping that is baked
> into both SDK and workspace. Are you doing the same thing here?

Already gone. :3

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sdk/RuntimeModel.js:363: var location =
this.pauseLocation();
On 2015/08/17 21:15:52, pfeldman wrote:
> This would also go into your module

And so it has.

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js
(right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/source_frame/TextEditorAutocompleteController.js:129:
var oldPrefixRange = this._prefixRange;
On 2015/08/17 21:15:52, pfeldman wrote:
> Let me talk to lushnikov@ about the way we should refactor it.

Any progress on this?

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
File Source/devtools/front_end/sources/SourcesView.js (right):

https://codereview.chromium.org/1264133002/diff/40001/Source/devtools/front_e...
Source/devtools/front_end/sources/SourcesView.js:100: var mimeType =
WebInspector.ResourceType.mimeFromUrl(uiSourceCode.uri());
On 2015/08/17 21:15:52, pfeldman wrote:
> uri is the URI within workspace, contentURL is where it came from (fs or
> network). You should take the mime from the name, i.e. uri.

Alright - I think I've gone through and changed all the things I've added to
look at uri rather than contentURL, since we really care about its binding in
the sources pane.

[pfeldman, 2015-08-25 21:30:21]

I think it makes sense to split this into a number of small CLs that we can
formally review and start landing. In the meanwhile, I'll go over the replies.

[pfeldman, 2016-07-22 18:19:32]

pfeldman@chromium.org changed reviewers:
- pfeldman@chromium.org