Add some error messages for setting wallpaper failures on Chrome(c++) side.

Add some error messages for setting wallpaper failures on Chrome(c++) side.


We have error handling of wallpaper functions on Javascript side. This cl adds
some error messages on Chrome side and eventually display these messages in a
butter bar.


BUG=152844
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=162864

[bshe, 2012-10-11 14:57:47]

Hi Nikita.

Could you please take a look at this small CL? I added some error messages when
setting wallpaper failed on c++ side(we previously only handle failures in js).
These error messages will be displayed in butter bar eventually.

Again, the strings I use in this CL is not final. I will ask pm for more
accurate strings. At the same time, could you please take a look?

Thanks!

Biao

[bshe, 2012-10-15 15:00:34]

Friendly ping?

On 2012/10/11 14:57:47, bshe wrote:
> Hi Nikita.
> 
> Could you please take a look at this small CL? I added some error messages
when
> setting wallpaper failed on c++ side(we previously only handle failures in
js).
> These error messages will be displayed in butter bar eventually.
> 
> Again, the strings I use in this CL is not final. I will ask pm for more
> accurate strings. At the same time, could you please take a look?
> 
> Thanks!
> 
> Biao

[Nikita (slow), 2012-10-15 15:14:11]

Sorry for delay, was out of office on Thu/Fri.

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:102:
function_->OnFailureOrCancel("Loading canceled");
I believe these should be taken from strings i.e. localized.

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/wallpaper_manager/js/butter_bar.js
(right):

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/wallpaper_manager/js/butter_bar.js:262: var
progressString = 'Loading';
String should be localized.

[bshe, 2012-10-15 15:24:48]

On 2012/10/15 15:14:11, Nikita Kostylev wrote:
> Sorry for delay, was out of office on Thu/Fri.
> 
>
http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
> File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):
> 
>
http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/wallpaper_private_api.cc:102:
> function_->OnFailureOrCancel("Loading canceled");
> I believe these should be taken from strings i.e. localized.
> 
>
http://codereview.chromium.org/11092078/diff/1006/chrome/browser/resources/ch...
> File chrome/browser/resources/chromeos/wallpaper_manager/js/butter_bar.js
> (right):
> 
>
http://codereview.chromium.org/11092078/diff/1006/chrome/browser/resources/ch...
> chrome/browser/resources/chromeos/wallpaper_manager/js/butter_bar.js:262: var
> progressString = 'Loading';
> String should be localized.

Sorry. Didn't know you were out of office. I am waiting for kuscher to finialize
these strings. http://code.google.com/p/chromium/issues/detail?id=152844
And after I got them, I will change these strings to localized strings. At the
same time, other than string issues, do you have any other concern? Thanks!

[Nikita (slow), 2012-10-15 15:48:38]

Don't have any other comments so far but I would like to take a look at CL once
you'll have those strings as a part of it.

[bshe, 2012-10-17 00:13:50]

Hi Nikita.

Now the string has finalized. Could you please take another look?

Note that patchset 3 also include some diff from rebase. So you can ignore the
delta 2 in patchset 3 for file:
chrome/browser/resources/chromeos/wallpaper_manager/js/wallpaper_manager.js

Thanks!

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:102:
function_->OnFailureOrCancel("Loading canceled");
Pre discussion with kuscher, when wallpaper decoding is canceled, we dont want
any error message. We just show a new downloading bar. (it is handled in
javascript.)

On 2012/10/15 15:14:11, Nikita Kostylev wrote:
> I believe these should be taken from strings i.e. localized.

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:111:
function_->OnFailureOrCancel("Loading failed");
Changed to localized string.

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:189: this, "Failed
to create directory"));
Pre discussion with kuscher, this case should be rare enough that a console
error message should be enough. Also line 215

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/resources/ch...
File chrome/browser/resources/chromeos/wallpaper_manager/js/butter_bar.js
(right):

http://codereview.chromium.org/11092078/diff/1006/chrome/browser/resources/ch...
chrome/browser/resources/chromeos/wallpaper_manager/js/butter_bar.js:262: var
progressString = 'Loading';
Pre discussion with kuscher, we just want to show the 100 downloading bar for
some time. So removed.

On 2012/10/15 15:14:11, Nikita Kostylev wrote:
> String should be localized.

[Nikita (slow), 2012-10-17 10:54:24]

lgtm

[commit-bot: I haz the power, 2012-10-17 15:58:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/11092078/9001

[commit-bot: I haz the power, 2012-10-17 15:58:12]

Presubmit check for 11092078-9001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/common/extensions/api

Presubmit checks took 3.6s to calculate.

[bshe, 2012-10-17 16:37:16]

+mihaip

Could you please take a look and also owners?

Thanks!

[Mihai Parparita -not on Chrome, 2012-10-18 00:30:54]

https://chromiumcodereview.appspot.com/11092078/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://chromiumcodereview.appspot.com/11092078/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:137:
SetResult(Value::CreateStringValue(error));
The more idiomatic (with other APIs) way of doing this is to use SetError (you
can then access the error via chrome.runtime.lastError in your callbacl).

[bshe, 2012-10-18 17:34:59]

Thanks!

https://chromiumcodereview.appspot.com/11092078/diff/9001/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

https://chromiumcodereview.appspot.com/11092078/diff/9001/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:137:
SetResult(Value::CreateStringValue(error));
I made the change and use SetError as you suggested. But I am not sure if I am
using it the right way. Could you please take another look?

On 2012/10/18 00:30:54, Mihai Parparita wrote:
> The more idiomatic (with other APIs) way of doing this is to use SetError (you
> can then access the error via chrome.runtime.lastError in your callbacl).

[Mihai Parparita -not on Chrome, 2012-10-18 20:55:49]

LGTM, you're using chrome.runtime.lastError correctly.

[commit-bot: I haz the power, 2012-10-18 21:05:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/11092078/18001

[commit-bot: I haz the power, 2012-10-19 00:39:47]

Change committed as 162864