Re: Expose liveedit compile error data in SDK (issue 11833010)

1 view

Skip to first unread message

apa...@chromium.org

unread,

Jan 21, 2013, 9:25:02 AM1/21/13

to peter...@gmail.com, chromedevtoo...@googlegroups.com

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
File plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java
(right):

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java#newcode138
plugins/org.chromium.sdk/src/org/chromium/sdk/UpdatableScript.java:138:
* Describeds failure caused by compile error.
Describes

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java
File
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java
(right):

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java#newcode41
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java:41:
interface Position {
Sounds like an inappropriate name. This resembles a Range much more (and
we name it that way in WebCore), otherwise it's going to confuse and
frustrate users.

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java#newcode47
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java:47:
interface PointPosition {
Why "PointPosition"? A plain "Position" would work fine.

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java#newcode48
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java:48:
long position();
Is this actually an offset()? We should be very careful when inventing
new type names.

https://codereview.chromium.org/11833010/

apa...@chromium.org

unread,

Jan 21, 2013, 9:43:46 AM1/21/13

to peter...@gmail.com, chromedevtoo...@googlegroups.com

LGTM with comments

https://codereview.chromium.org/11833010/

peter...@gmail.com

unread,

Jan 21, 2013, 11:01:32 AM1/21/13

to apa...@chromium.org, chromedevtoo...@googlegroups.com

On 2013/01/21 14:25:02, apavlov wrote:
> Describes

Done.

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java
File
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java
(right):

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java#newcode41
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java:41:
interface Position {

On 2013/01/21 14:25:02, apavlov wrote:
> Sounds like an inappropriate name. This resembles a Range much more
(and we name
> it that way in WebCore), otherwise it's going to confuse and frustrate
users.

Done.

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java#newcode47
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java:47:
interface PointPosition {

On 2013/01/21 14:25:02, apavlov wrote:
> Why "PointPosition"? A plain "Position" would work fine.

Done.

https://codereview.chromium.org/11833010/diff/2001/plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java#newcode48
plugins/org.chromium.sdk/src/org/chromium/sdk/internal/v8native/protocol/input/ChangeLiveBody.java:48:
long position();

On 2013/01/21 14:25:02, apavlov wrote:
> Is this actually an offset()? We should be very careful when inventing
new type
> names.