In LiveEdit API expose important control-flow parameter (fixing TODO) (issue 12323002)

[peter...@gmail.com]

Reviewers: apavlov,

Description:
In LiveEdit API expose important control-flow parameter (fixing TODO)


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

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

Affected files:
M
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/PushChangesAction.java
M
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PreviewLoader.java
M
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushChangesWizard.java
M
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
M
plugins/org.chromium.sdk.wipbackend.protocol_1_0/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
M plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
M
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/ScriptImpl.java


Index:
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/PushChangesAction.java
diff --git
a/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/PushChangesAction.java
b/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/PushChangesAction.java
index
9d2b0b0760cbf9058a8d7df03f5c97fdf5dfd377..a7de5e6e3ced0bd317a5ebf68a71bc38ae0d51ec
100644
---
a/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/PushChangesAction.java
+++
b/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/actions/PushChangesAction.java
@@ -40,7 +40,7 @@ public class PushChangesAction extends V8ScriptAction {

UpdatableScript.UpdateCallback callback = new
UpdatableScript.UpdateCallback() {
@Override
- public void success(Object report, ChangeDescription
changeDescription) {
+ public void success(boolean resumed, Object report,
ChangeDescription changeDescription) {
ChromiumDebugPlugin.log(new Status(IStatus.OK,
ChromiumDebugPlugin.PLUGIN_ID,
"Script has been successfully updated on remote: " + report));
//$NON-NLS-1$
}
Index:
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PreviewLoader.java
diff --git
a/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PreviewLoader.java
b/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PreviewLoader.java
index
f4413880de68366f3ff17b826873da6670db7e7b..270f8d033cfdbc0db379aaefe025de1135986955
100644
---
a/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PreviewLoader.java
+++
b/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PreviewLoader.java
@@ -91,7 +91,7 @@ class PreviewLoader implements
ValueSource<Optional<PreviewLoader.Data>> {
});
done(result);
}
- public void success(Object report,
+ public void success(boolean resumed, Object report,
final UpdatableScript.ChangeDescription changeDescription) {
Optional<Data> result;
if (changeDescription == null) {
Index:
plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushChangesWizard.java
diff --git
a/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushChangesWizard.java
b/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushChangesWizard.java
index
e64d2de2d0d8b4852550b83c6345bd790101e300..2c2a5fad9a2e5f06eedfdb759a5fe97b512f831d
100644
---
a/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushChangesWizard.java
+++
b/plugins/org.chromium.debug.ui/src/org/chromium/debug/ui/liveedit/PushChangesWizard.java
@@ -258,7 +258,7 @@ public class PushChangesWizard {
input[0] = LiveEditResultDialog.createTextInput(text, changesPlan,
failure);
}
- public void success(Object report,
+ public void success(boolean resumed, Object report,
final UpdatableScript.ChangeDescription changeDescription) {
if (changeDescription == null) {
input[0] = LiveEditResultDialog.createTextInput(
Index:
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
diff --git
a/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
b/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
index
2f90f82f79f867f9e6839207e64a55161e530cab..ea2d36d05ed7bbc665ca2c23f4fca7e4c6c1f02e
100644
---
a/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
+++
b/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
@@ -56,6 +56,7 @@ class WipScriptImpl extends ScriptBase<String> {
new GenericCallback<SetScriptSourceData>() {
@Override
public void success(SetScriptSourceData value) {
+ // TODO: support 'step in recommended' here.
RelayOk relayOk =
possiblyUpdateCallFrames(preview, value, updateCallback,
guard.getRelay());
guard.discharge(relayOk);
@@ -112,7 +113,7 @@ class WipScriptImpl extends ScriptBase<String> {
}
ChangeDescription wrappedChangeDescription =
UpdateResultParser.wrapChangeDescription(liveEditResult);
- updateCallback.success(null, wrappedChangeDescription);
+ updateCallback.success(false, null, wrappedChangeDescription);
}
}
}
Index:
plugins/org.chromium.sdk.wipbackend.protocol_1_0/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
diff --git
a/plugins/org.chromium.sdk.wipbackend.protocol_1_0/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
b/plugins/org.chromium.sdk.wipbackend.protocol_1_0/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
index
3e7dbadb8148c59ce89d88936b2365218a2cdfc6..3d4dd13d55ef2d263576dcba91e934e164918150
100644
---
a/plugins/org.chromium.sdk.wipbackend.protocol_1_0/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
+++
b/plugins/org.chromium.sdk.wipbackend.protocol_1_0/src/org/chromium/sdk/internal/wip/WipScriptImpl.java
@@ -109,7 +109,7 @@ class WipScriptImpl extends ScriptBase<String> {
}
ChangeDescription wrappedChangeDescription =
UpdateResultParser.wrapChangeDescription(liveEditResult);
- updateCallback.success(null, wrappedChangeDescription);
+ updateCallback.success(false, null, wrappedChangeDescription);
}
}
}
Index: plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
diff --git
a/plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
b/plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
index
cf5c95e9d22bd4a3cb447d7d5cdd562829ffe32b..1fd907ee0617b50230a76e500e3595262fab9268
100644
--- a/plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
+++ b/plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
@@ -11,11 +11,11 @@ import java.util.List;
*/
public interface UpdatableScript {
/**
- * Demands that script text should be replaced with a new one if
possible. VM may get resumed
- * after this command (this is defined by {@link
ChangeDescription#isStackModified()}). In this
- * case a standard 'suspended' notification is expected in a moment.
+ * Demands that script text should be replaced with a new one if
possible. A technical VM step may
+ * be automatically scheduled after this command (this is defined
+ * by {@link ChangeDescription#isStackModified()}), that is a technical
requirement of V8.
+ * VM should pause back in a moment, standard 'suspended' notification
will be visible.
* @param newSource new text of script
- * TODO: add an explicit output parameter about whether VM was resumed
or not.
*/
RelayOk setSourceOnRemote(String newSource, UpdateCallback callback,
SyncCallback syncCallback);

@@ -32,12 +32,13 @@ public interface UpdatableScript {
* {@link DebugEventListener#scriptContentChanged} will
* be called additionally. Besides, a current context may be dismissed
and recreated after this
* event. The order of all listed event notifications is not currently
specified.
+ * @param resumed true if VM has been resumed to make post-change
technical step
* @param report unspecified implementation-dependent report for
debugging purposes;
* may be null
* @param changeDescription describes live editing change that has
been applied or is planned
* to be applied; may be null if backend or VM does not support
*/
- void success(Object report, ChangeDescription changeDescription);
+ void success(boolean resumed, Object report, ChangeDescription
changeDescription);
void failure(String message, Failure details);
}

Index:
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/ScriptImpl.java
diff --git
a/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/ScriptImpl.java
b/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/ScriptImpl.java
index
addc7877446f5957a56123376b5467832a883675..baf66f942384f704c93ad3bc83a3b0973761dea9
100644
---
a/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/ScriptImpl.java
+++
b/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/ScriptImpl.java
@@ -62,6 +62,7 @@ public class ScriptImpl extends ScriptBase<Long> {
}

LiveEditResult resultDescription = body.getResultDescription();
+ boolean resumed = false;
if (!previewOnly) {
ScriptLoadCallback scriptCallback = new ScriptLoadCallback() {
@Override
@@ -88,6 +89,7 @@ public class ScriptImpl extends ScriptBase<Long> {
// was sent so the context was dropped. Ignore this case.
} else {
debugContext.continueVm(DebugContext.StepAction.IN, 0, null);
+ resumed = true;
}
} else {
if (resultDescription != null &&
resultDescription.stack_modified()) {
@@ -97,7 +99,7 @@ public class ScriptImpl extends ScriptBase<Long> {
}

if (callback != null) {
- callback.success(body.getChangeLog(),
+ callback.success(resumed, body.getChangeLog(),
UpdateResultParser.wrapChangeDescription(resultDescription));
}
}

[apa...@chromium.org]

LGTM with a comment

You don't seem to use the added boolean, do you? You need another TODO to
make a
real use of it

https://codereview.chromium.org/12323002/

[peter...@gmail.com]

On 2013/02/20 08:40:23, apavlov wrote:
> LGTM with a comment

> You don't seem to use the added boolean, do you? You need another TODO to
> make
a
> real use of it

In fact I don't need this value, because Eclipse framework is quite
forgiving.
But anyway I see it as important part of our contract, that the client must
be
aware of cases when VM changes from resumed to paused and vice-versa.

https://codereview.chromium.org/12323002/

[peter...@gmail.com]

Committed manually as r1140 (presubmit successful).

https://codereview.chromium.org/12323002/