Provide warning message for setParameters

Provide warning message for setParameters

This CL adds warning message for setParameters API of vpnProvider.
The vpnProvider extension API is further filled to stable channel
from dev channel.

BUG=467911
TBR=stevenjb

Committed: https://crrev.com/e5ad31358cc693c737cf34268b4abd520994bfc3
Cr-Commit-Position: refs/heads/master@{#321325}

[kaliamoorthi, 2015-03-17 15:28:53]

kaliamoorthi@chromium.org changed reviewers:
+ kalman@chromium.org, stevenjb@chromium.org

[kaliamoorthi, 2015-03-17 15:28:53]

PTAL

[not at google - send to devlin, 2015-03-17 16:03:00]

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/_perm...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/_perm...
extensions/common/api/_permission_features.json:432: "channel": "stable",
Has this gone through launch review?

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
extensions/common/api/vpn_provider.idl:83: // |warning|: Warning message if any.
What is the difference between this and the message in chrome.runtime.lastError?

[kaliamoorthi, 2015-03-17 16:13:36]

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/_perm...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/_perm...
extensions/common/api/_permission_features.json:432: "channel": "stable",
On 2015/03/17 16:03:00, kalman wrote:
> Has this gone through launch review?

saswat@, who is the PM for this feature can answer this question.

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
extensions/common/api/vpn_provider.idl:83: // |warning|: Warning message if any.
On 2015/03/17 16:03:00, kalman wrote:
> What is the difference between this and the message in
chrome.runtime.lastError?

chrome.runtime.lastError is used to indicate error, i.e., the API call was not
successful. Warning is used to indicate that the API call was successful though
some parameters were not set. For instance, some parameter settings were ignored
since they were incorrect.

[not at google - send to devlin, 2015-03-17 16:16:04]

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
extensions/common/api/vpn_provider.idl:83: // |warning|: Warning message if any.
On 2015/03/17 16:13:36, kaliamoorthi wrote:
> On 2015/03/17 16:03:00, kalman wrote:
> > What is the difference between this and the message in
> chrome.runtime.lastError?
> 
> chrome.runtime.lastError is used to indicate error, i.e., the API call was not
> successful. Warning is used to indicate that the API call was successful
though
> some parameters were not set. For instance, some parameter settings were
ignored
> since they were incorrect.

I don't know of any other API that has this - either it succeeded, or it failed.
Generally speaking there is no such thing as a freeform message for
it-kind-of-failed.

I'd try to either:
- just print the warning to the console (similar to how deprecation warnings
work), don't make it part of the API.
- make the result an enum or something, that way the caller can know exactly
what went wrong.

[kaliamoorthi, 2015-03-17 17:25:31]

PTAL

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
File extensions/common/api/vpn_provider.idl (right):

https://codereview.chromium.org/1014053002/diff/1/extensions/common/api/vpn_p...
extensions/common/api/vpn_provider.idl:83: // |warning|: Warning message if any.
On 2015/03/17 16:16:04, kalman wrote:
> On 2015/03/17 16:13:36, kaliamoorthi wrote:
> > On 2015/03/17 16:03:00, kalman wrote:
> > > What is the difference between this and the message in
> > chrome.runtime.lastError?
> > 
> > chrome.runtime.lastError is used to indicate error, i.e., the API call was
not
> > successful. Warning is used to indicate that the API call was successful
> though
> > some parameters were not set. For instance, some parameter settings were
> ignored
> > since they were incorrect.
> 
> I don't know of any other API that has this - either it succeeded, or it
failed.
> Generally speaking there is no such thing as a freeform message for
> it-kind-of-failed.
> 
> I'd try to either:
> - just print the warning to the console (similar to how deprecation warnings
> work), don't make it part of the API.
> - make the result an enum or something, that way the caller can know exactly
> what went wrong.

Moved the warning message to a console print.

[not at google - send to devlin, 2015-03-17 17:34:14]

https://codereview.chromium.org/1014053002/diff/20001/extensions/common/api/_...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/1014053002/diff/20001/extensions/common/api/_...
extensions/common/api/_permission_features.json:432: "channel": "stable",
Please take this out of this change.

[kaliamoorthi, 2015-03-17 17:41:07]

PTAL

https://codereview.chromium.org/1014053002/diff/20001/extensions/common/api/_...
File extensions/common/api/_permission_features.json (right):

https://codereview.chromium.org/1014053002/diff/20001/extensions/common/api/_...
extensions/common/api/_permission_features.json:432: "channel": "stable",
On 2015/03/17 17:34:14, kalman wrote:
> Please take this out of this change.

Done.

[not at google - send to devlin, 2015-03-17 17:48:41]

My lg no longer needed - up to stevenjb now.

[kaliamoorthi, 2015-03-18 17:04:22]

Patchset #4 (id:60001) has been deleted

[kaliamoorthi, 2015-03-18 17:04:31]

Patchset #4 (id:80001) has been deleted

[kaliamoorthi, 2015-03-19 09:05:45]

kaliamoorthi@chromium.org changed reviewers:
+ pneubeck@chromium.org

[kaliamoorthi, 2015-03-19 09:07:38]

kaliamoorthi@chromium.org changed reviewers:
- kalman@chromium.org

[kaliamoorthi, 2015-03-19 09:07:38]

-kalman@, +pneubeck.

Hi Philipp,

Steven seems to be OOO, this CL has some changes in components owned by us but
LGTM power is still with Steven.

PTAL. The changes are mostly plumbing related.

[pneubeck (no reviews), 2015-03-19 09:22:39]

I think this change is fine for now.
In general however, I think it would be much safer if the API itself would do as
many sanity checks as possible to restrict the impact that an extension can have
(and better too restrictive than to liberal)

I mean, we're passing these parameters directly to a system daemon with root
permissions and that daemon is probably not built with direct communication to
3rd party code in mind.

https://codereview.chromium.org/1014053002/diff/100001/chromeos/dbus/shill_th...
File chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc (right):

https://codereview.chromium.org/1014053002/diff/100001/chromeos/dbus/shill_th...
chromeos/dbus/shill_third_party_vpn_driver_client_unittest.cc:129:
writer.AppendString(std::string());
you could use a nonempty string and test the result in line 136.

[pneubeck (no reviews), 2015-03-19 09:22:46]

lgtm

[kaliamoorthi, 2015-03-19 09:36:24]

On 2015/03/19 09:22:46, pneubeck wrote:
> lgtm

Thanks Philipp. I'll TBR stevenjb just to satisfy the scripts.

[kaliamoorthi, 2015-03-19 09:37:28]

The CQ bit was checked by kaliamoorthi@chromium.org

[kaliamoorthi, 2015-03-19 09:37:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
pneubeck@chromium.org
Link to the patchset: https://codereview.chromium.org/1014053002/#ps120001
(title: "Fix comment from Philipp")

[commit-bot: I haz the power, 2015-03-19 09:37:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1014053002/120001

[commit-bot: I haz the power, 2015-03-19 10:10:02]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-03-19 10:10:35]

Patchset 5 (id:??) landed as
https://crrev.com/e5ad31358cc693c737cf34268b4abd520994bfc3
Cr-Commit-Position: refs/heads/master@{#321325}