Devtools: API To set the red/yellow squiggles for a file via DI

API To set the red/yellow squiggles for a file via DI

Known internally as "LineMessage"s. Additionally, Extends them to
take start and end line/column sets, to prepare for a
future where the UI actually supports rendering multiline
squiggles.

This migrates https://codereview.chromium.org/1361863002/

This work was split from https://codereview.chromium.org/1264133002/

BUG=484261

[wes, 2015-10-23 20:36:01]

Description was changed from

==========
API To set the red/yellow squiggles for a file via DI

Known internally as "LineMessage"s. Additionally, Extends them to
take start and end line/column sets, to prepare for a
future where the UI actually supports rendering multiline
squiggles.

This migrates https://codereview.chromium.org/1361863002/

This work was split from https://codereview.chromium.org/1264133002/

BUG=484261
==========

to

==========
API To set the red/yellow squiggles for a file via DI

Known internally as "LineMessage"s. Additionally, Extends them to
take start and end line/column sets, to prepare for a
future where the UI actually supports rendering multiline
squiggles.

This migrates https://codereview.chromium.org/1361863002/

This work was split from https://codereview.chromium.org/1264133002/

BUG=484261
==========

[wes, 2015-10-23 20:36:01]

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

[pfeldman, 2015-10-26 18:08:49]

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js
(right):

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:614:
this.clearMessages();
We should allow for concurrent error messages, you should not clear existing
ones.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:764:
lineNumber: function()
We can not remove these in favor of the one above.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:772:
columnNumber: function()
We can not remove these in favor of the one above.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:783:
return  this.messageText() === another.messageText() &&
poor formatting.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:1384:
frame.setMessagesForSource(container.messages().map(function(m){
Revealer is for revealing, not for providing messages, as I mentioned in the
previous review, they should be added as markers on UISourceCode instead.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js
(right):

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:714:
WebInspector.UISourceCodeMessage = function(text, kind, location) {
Lets store a collection of these on the UISourceCode along with the event on
their addition / removal.

[wes, 2015-10-26 22:16:32]

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js
(right):

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:614:
this.clearMessages();
On 2015/10/26 18:08:49, pfeldman wrote:
> We should allow for concurrent error messages, you should not clear existing
> ones.

Acknowledged.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:764:
lineNumber: function()
On 2015/10/26 18:08:49, pfeldman wrote:
> We can not remove these in favor of the one above.

Is there an action for me on this comment, or is this just a remark for
posterity saying you understand why it's still here?

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:772:
columnNumber: function()
On 2015/10/26 18:08:49, pfeldman wrote:
> We can not remove these in favor of the one above.

Same as above?

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/source_frame/SourceFrame.js:783:
return  this.messageText() === another.messageText() &&
On 2015/10/26 18:08:49, pfeldman wrote:
> poor formatting.

Acknowledged.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js
(right):

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/sources/SourcesPanel.js:1384:
frame.setMessagesForSource(container.messages().map(function(m){
On 2015/10/26 18:08:49, pfeldman wrote:
> Revealer is for revealing, not for providing messages, as I mentioned in the
> previous review, they should be added as markers on UISourceCode instead.

Acknowledged.

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js
(right):

https://codereview.chromium.org/1416793005/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:714:
WebInspector.UISourceCodeMessage = function(text, kind, location) {
On 2015/10/26 18:08:49, pfeldman wrote:
> Lets store a collection of these on the UISourceCode along with the event on
> their addition / removal.

Acknowledged.

[wes, 2015-10-26 22:19:59]

I've redone it to use events rather than a revealer and utilize individual
message add/remove methods. Though this feels like it's an easy place for API
consumers to have race conditions when doing work asynchronously.

[pfeldman, 2015-10-27 17:37:34]

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js
(right):

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js:98:
_onLineMessageAdded: function(evt) {
Here and below, { goes the next line. Annotate evt.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js:100:
this.addMessageToSource(new WebInspector.SourceFrameMessage(m.text(),
WebInspector.SourceFrameMessage.Level[m.kind()], m.location()));
Lets move the level enum into the UISourceCodeMessage.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js:103:
_onLineMessageRemoved: function(evt) {
ditto

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js
(right):

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:68:
LineMessageAdded: "LineMessageAdded",
These are not exactly line messages, MessageAdded? SourceMessageAdded?

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:577:
addLineMessage: function(message) {
addLineMessage: function(level, text, location)
{
    // construct object
    return message;
}

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:591:
removeLineMessage: function(message) {
Should we remove messages by identity objects?

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:602:
__proto__: WebInspector.Object.prototype
messages() accessor and clearMessages() would come handy for the clients that
are created post-message addition.

[pfeldman, 2015-10-27 17:44:04]

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js
(right):

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:580: if
(m.equal(message)) {
There is no reason to dedupe them here.

[pfeldman, 2015-10-27 17:48:04]

I actually need these messages on UISourceCode too. Do you want me to take over
the UISourceCode part? I'll migrate existing messages from being
UISourceCodeFrame based to the new schema as well.

[wes, 2015-10-27 21:59:17]

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js
(right):

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js:98:
_onLineMessageAdded: function(evt) {
On 2015/10/27 17:37:34, pfeldman wrote:
> Here and below, { goes the next line. Annotate evt.

Acknowledged.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js:100:
this.addMessageToSource(new WebInspector.SourceFrameMessage(m.text(),
WebInspector.SourceFrameMessage.Level[m.kind()], m.location()));
On 2015/10/27 17:37:34, pfeldman wrote:
> Lets move the level enum into the UISourceCodeMessage.

The actual enum is going to need to be moved out of source and into workspace
and renamed, then.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/sources/UISourceCodeFrame.js:103:
_onLineMessageRemoved: function(evt) {
On 2015/10/27 17:37:34, pfeldman wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js
(right):

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:68:
LineMessageAdded: "LineMessageAdded",
On 2015/10/27 17:37:34, pfeldman wrote:
> These are not exactly line messages, MessageAdded? SourceMessageAdded?

MessageAdded is more succinct, so if its meaning is still clear, I think I'd at
least prefer it.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:577:
addLineMessage: function(message) {
On 2015/10/27 17:37:34, pfeldman wrote:
> addLineMessage: function(level, text, location)
> {
>     // construct object
>     return message;
> }

Seems fine.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:580: if
(m.equal(message)) {
On 2015/10/27 17:44:04, pfeldman wrote:
> There is no reason to dedupe them here.

Alright - we do still dedupe them in the display part, I believe.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:591:
removeLineMessage: function(message) {
On 2015/10/27 17:37:34, pfeldman wrote:
> Should we remove messages by identity objects?

If we don't dedupe them on entry, then we should probably only remove based on
reference equality, rather than deep equality. It just means that consumers have
to keep around the messages they care about.

https://codereview.chromium.org/1416793005/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:602:
__proto__: WebInspector.Object.prototype
On 2015/10/27 17:37:34, pfeldman wrote:
> messages() accessor and clearMessages() would come handy for the clients that
> are created post-message addition.

For sure. clearMessages() is going to fire many menu removed events, though (or
need to have its own event).

[pfeldman, 2015-10-27 22:15:47]

I uploaded a number of patches that are introducing the uisourcecodemessage and
migrate existing code onto it:

https://codereview.chromium.org/1417163003/
https://codereview.chromium.org/1427653002/
https://codereview.chromium.org/1403373013/

They are under review / landing, but they are not using the text ranges, just
the line/column numbers. I.e. my code just moves things around so that we
migrated to the new messages.

[wes, 2015-10-27 22:34:37]

On 2015/10/27 22:15:47, pfeldman wrote:
> I uploaded a number of patches that are introducing the uisourcecodemessage
and
> migrate existing code onto it:
> 
> https://codereview.chromium.org/1417163003/
> https://codereview.chromium.org/1427653002/
> https://codereview.chromium.org/1403373013/
> 
> They are under review / landing, but they are not using the text ranges, just
> the line/column numbers. I.e. my code just moves things around so that we
> migrated to the new messages.

I suppose I'm going to want to discard this cl and just add a new cl which
expands the location parameter to text ranges once those land?

[pfeldman, 2015-10-27 22:36:32]

> I suppose I'm going to want to discard this cl and just add a new cl which
> expands the location parameter to text ranges once those land?

I think so. Sorry about that - one thing pulled the other and it turned into a
non-trivial thing that required to move substantial amount of code.

[pfeldman, 2015-10-29 22:56:50]

All should be landed by now.

[paulirish, 2015-11-11 23:43:46]

wes, are you clear on the next steps for this one?