Re: Support set variable value for WebKit protocol (issue 12287017)

2 views

Skip to first unread message

apa...@chromium.org

unread,

Feb 18, 2013, 6:34:08 AM2/18/13

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

peter...@gmail.com

unread,

Feb 18, 2013, 6:32:52 PM2/18/13

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

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipContextBuilder.java
File
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipContextBuilder.java
(right):

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipContextBuilder.java#newcode569
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipContextBuilder.java:569:
ScopeHolderParams holderParams, int number) {

On 2013/02/18 11:34:08, apavlov wrote:
> "number"? Should it be "scopeIndex" instead?

Done.

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipContextBuilder.java#newcode691
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipContextBuilder.java:691:
private final int scopeNumber;

On 2013/02/18 11:34:08, apavlov wrote:
> scopeIndex?

Done.

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java
File
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java
(right):

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java#newcode665
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java:665:
VariableImpl(JsValue jsValue, String name, VariableValueChanger
valueChanger) {

On 2013/02/18 11:34:08, apavlov wrote:
> While we are here, it is a good idea to swap the JsValue and String
arguments,
> since typically the leftmost arguments are passed into the super
constructor to
> improve readability (like, "the first arguments are the most important
ones.
> Important enough to go into the superclass").

Done.

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java#newcode674
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java:674:
@Override public boolean isMutable() {

On 2013/02/18 11:34:08, apavlov wrote:
> Blank line

We usually keep trivial and really simple methods as 3-liners and
together with each other packed without spaces.

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java#newcode677
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java:677:
@Override public JsValue getValue() {

On 2013/02/18 11:34:08, apavlov wrote:
> Ditto

Same

https://codereview.chromium.org/12287017/diff/2001/plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java#newcode725
plugins/org.chromium.sdk.wipbackend.dev/src/org/chromium/sdk/internal/wip/WipValueBuilder.java:725:
@Override public JsObjectProperty asObjectProperty() {

On 2013/02/18 11:34:08, apavlov wrote:
> Ditto

The same

https://codereview.chromium.org/12287017/

Reply all

Reply to author

Forward

0 new messages