Disaply compile error position in diff viewer (issue 12035015)

[peter...@gmail.com]

Reviewers: apavlov,

Description:
Disaply compile error position in diff viewer


Please review this at https://codereview.chromium.org/12035015/

SVN Base: https://chromedevtools.googlecode.com/svn/trunk

Affected files:

plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/LiveEditDiffViewer.java

plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/Messages.java

plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PreviewLoader.java

plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java

plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java

plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/messages.properties

[apa...@chromium.org]

Please re-upload the patch, as I'm getting "error: old chunk mismatch" for
all
diffs

https://codereview.chromium.org/12035015/

[peter...@gmail.com]

On 2013/01/22 07:16:55, apavlov wrote:
> Please re-upload the patch, as I'm getting "error: old chunk mismatch"
> for all
> diffs

I'm sorry about this.

As a missing cover letter:
when liveedit wizard shows preview diff, it now can recognize compile error
as a
separate case and standard diff view with fake input: the only "function"
mark-up will be the compile error position.

https://codereview.chromium.org/12035015/

[apa...@chromium.org]

LGTM with comments


https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java
(right):

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java#newcode106
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java:106:
return Arrays.asList(
OK, Arrays.asList() DOES accept a vararg :)

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java#newcode116
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java:116:
return "Compile error";
Do you want to i18n-ize this, too? (the same way you did about
Messages.PushResultParser_SCRIPT)

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java
(right):

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java#newcode307
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java:307:
"Error in getting preview: " + e.toString(), MessagePriority.WARNING));
"Failed to create preview". Don't you want to i18n-ize it?

I also suggest that you use NLS.bind() here.

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java#newcode369
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java:369:
new Message (messageString, MessagePriority.BLOCKING_PROBLEM));
stray whitespace before '('

https://codereview.chromium.org/12035015/

[peter...@gmail.com]

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java
(right):

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java#newcode106
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java:106:
return Arrays.asList(

On 2013/01/24 13:06:27, apavlov wrote:
> OK, Arrays.asList() DOES accept a vararg :)

Done.

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java#newcode116
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushResultParser.java:116:
return "Compile error";

On 2013/01/24 13:06:27, apavlov wrote:
> Do you want to i18n-ize this, too? (the same way you did about
> Messages.PushResultParser_SCRIPT)

Done.

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java
File
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java
(right):

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java#newcode307
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java:307:
"Error in getting preview: " + e.toString(), MessagePriority.WARNING));

On 2013/01/24 13:06:27, apavlov wrote:
> "Failed to create preview". Don't you want to i18n-ize it?

> I also suggest that you use NLS.bind() here.

Done.

https://codereview.chromium.org/12035015/diff/6003/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java#newcode369
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/WizardLogicBuilder.java:369:
new Message (messageString, MessagePriority.BLOCKING_PROBLEM));

On 2013/01/24 13:06:27, apavlov wrote:
> stray whitespace before '('

Done.

https://codereview.chromium.org/12035015/