Wallpaper manager backend APIs

Wallpaper manager backend APIs


BUG=118684
TEST=none


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

[bshe, 2012-07-10 02:03:58]

Hi Satoru.

Here is my CL. I use this->AddRef as a quick walkaround. It's definitely not the
correct way. I just want to get some suggestions from you before I implement a
less hacky way.

Thanks!

https://chromiumcodereview.appspot.com/10754014/diff/1014/chrome/browser/chro...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):

https://chromiumcodereview.appspot.com/10754014/diff/1014/chrome/browser/chro...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:216: this->AddRef();
Here I use a hack to make it work. Do you have any suggestion about how to do it
in a less hacky way? One thing I can think of is create a wrapper class for
fetcher_ which reference to the WallpaperManagerSetWallpaperFunction instance.

[bshe, 2012-07-11 19:48:48]

Hi Rob.

Could you please take a look at this CL? It implements two wallpaper extension
functions:
1. chrome.experimental.wallpaperManager.getStrings: get translated string
2. chrome.experimental.wallpaperManager.setWallpaper: Sets the wallpaper to the
given url and layout.

And dispatches three events:
experimental.wallpaperManager.onDownloadProgress,
experimental.wallpaperManager.onDownloadDone,
experimental.wallpaperManager.onDownloadError

Thanks!

[flackr, 2012-07-12 19:26:12]

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/app/generat...
File chrome/app/generated_resources.grd (right):

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/app/generat...
chrome/app/generated_resources.grd:13722: +      </message>
This seems like a duplicate.

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:83: DictionaryValue*
dict = reinterpret_cast<DictionaryValue*>(result_.get());
Perhaps swap the order of these and result_.reset(dict);

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:182:
source->GetOriginalURL().ExtractFileName() + ".jpg");
Can we copy the extension from the source file rather than assume it's ".jpg"?

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:231: // If the
wallpaper already downloaded, uses it directly.
// If the wallpaper was already downloaded it, use it immediately.

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:253: // If the
wallpaper already downloaded, uses it directly.
This comment doesn't seem to correspond to the code here.

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:274:
WallpaperErrorCode code) {
Are you eventually going to do something with this code? Would it make more
sense to pass the code to JS and have it determine which string to display as a
result?

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:291: if
(!file_util::DirectoryExists(path))
You already check this before calling CreateDirectory. I think this
responsibility should be in one place or the other but not both. You could
DCHECK it here and assume the directory doesn't exist when CreateDirectory is
called or always call CreateDirectory regardless of if it exists.

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:109: // Setups the
url_fetcher on UI thread.
s/Setups/Sets up

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/common/exte...
File chrome/common/extensions/api/experimental_wallpaper_manager.json (right):

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/common/exte...
chrome/common/extensions/api/experimental_wallpaper_manager.json:41: "name":
"layout"
Add "enum": ["list", "of", "acceptable", "values"] as in
chrome/common/extensions/api/experimental_font_settings.json

https://chromiumcodereview.appspot.com/10754014/diff/14003/chrome/common/exte...
chrome/common/extensions/api/experimental_wallpaper_manager.json:55:
"description": "A simple event."
This description is somewhat vague, see experimental_font_settings.json for
defining properties which exist on objects and I imagine there are specific
interesting properties for these event objects.

https://chromiumcodereview.appspot.com/10754014/diff/14003/net/url_request/ur...
File net/url_request/url_fetcher_core.cc (left):

https://chromiumcodereview.appspot.com/10754014/diff/14003/net/url_request/ur...
net/url_request/url_fetcher_core.cc:772: 
Probably don't need to change this file.

[bshe, 2012-07-13 19:28:57]

done. Thanks.

http://codereview.chromium.org/10754014/diff/14003/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/10754014/diff/14003/chrome/app/generated_resou...
chrome/app/generated_resources.grd:13722: +      </message>
On 2012/07/12 19:26:12, flackr wrote:
> This seems like a duplicate.

Removed.

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.cc (right):

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:83: DictionaryValue*
dict = reinterpret_cast<DictionaryValue*>(result_.get());
On 2012/07/12 19:26:12, flackr wrote:
> Perhaps swap the order of these and result_.reset(dict);

Done. Seems result_ is removed and SetResult is used instead.

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:182:
source->GetOriginalURL().ExtractFileName() + ".jpg");
Removed the extension name since it's unnecessary for chromeos.  

On 2012/07/12 19:26:12, flackr wrote:
> Can we copy the extension from the source file rather than assume it's ".jpg"?

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:231: // If the
wallpaper already downloaded, uses it directly.
On 2012/07/12 19:26:12, flackr wrote:
> // If the wallpaper was already downloaded it, use it immediately.

Done.

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:253: // If the
wallpaper already downloaded, uses it directly.
On 2012/07/12 19:26:12, flackr wrote:
> This comment doesn't seem to correspond to the code here.

Removed.

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:274:
WallpaperErrorCode code) {
Yes. I am going to listen to the dispatched event in the js and display message
on the front UI using a butter bar. Passing the code to js sounds fine to me.

Done

On 2012/07/12 19:26:12, flackr wrote:
> Are you eventually going to do something with this code? Would it make more
> sense to pass the code to JS and have it determine which string to display as
a
> result?

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.cc:291: if
(!file_util::DirectoryExists(path))
On 2012/07/12 19:26:12, flackr wrote:
> You already check this before calling CreateDirectory. I think this
> responsibility should be in one place or the other but not both. You could
> DCHECK it here and assume the directory doesn't exist when CreateDirectory is
> called or always call CreateDirectory regardless of if it exists.

You are absolutely right. This function actually was no longer used. So remove
it.

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

http://codereview.chromium.org/10754014/diff/14003/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:109: // Setups the
url_fetcher on UI thread.
On 2012/07/12 19:26:12, flackr wrote:
> s/Setups/Sets up

Done.

http://codereview.chromium.org/10754014/diff/14003/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_wallpaper_manager.json (right):

http://codereview.chromium.org/10754014/diff/14003/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_wallpaper_manager.json:41: "name":
"layout"
On 2012/07/12 19:26:12, flackr wrote:
> Add "enum": ["list", "of", "acceptable", "values"] as in
> chrome/common/extensions/api/experimental_font_settings.json

Done.

[bshe, 2012-07-16 01:18:17]

+mihaip

I have moved the api to wallpaperManagerPrivate as you suggested in
https://chromiumcodereview.appspot.com/10453032/

I will probably abandon issue 10453032 since most of it is covered by this CL.

Thanks!

[Mihai Parparita -not on Chrome, 2012-07-16 23:14:25]

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:40: const int64
kBytesWallpaperDownloadProgressReportInterval = 10240;
Seems like this can just be in the .cc file.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:66:
DECLARE_EXTENSION_FUNCTION_NAME("wallpaperManagerPrivate.setWallpaper");
The file name generally matches the API namespace (e.g.
file_browser_private_api.cc, terminal_private_api.cc), so this file should be
called wallpaper_manager_private_api.cc. That's kind of a mouthful, you may want
to drop the "manager" from the namespace.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:75: // This class is
a wrapper class for URLFetcher. It has a ref counted pointer
To make the header file more readable/hide internals better, I think you can
just forward-declare WallpaperFetcher here, and then do the declaration and
implementation .cc file.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:83: void
WallpaperManager::OnCustomWallpaperLoaded(const std::string& email,
The email parameter appears to be unused (so it can also be removed from
SetWallpaperFromFile).

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/common/exte...
File chrome/common/extensions/api/wallpaper_manager_private.json (right):

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/common/exte...
chrome/common/extensions/api/wallpaper_manager_private.json:14: "nodoc": "true",
Since the root namespace has nodoc, I believe you don't need it for every
function/event too.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/common/exte...
chrome/common/extensions/api/wallpaper_manager_private.json:37: "name": "url"
By making the API taken in a URL, you end up making the API more complex, since
it has to implement file downloading, fire progress and error events, etc. 

XMLHttpRequest already does all that. It can fetch data as ArrayBuffers (which I
believe you can pass into ExtensionFunction implementations) and extensions can
use it to load data from any host (just add "<all_urls>" to the permission list
in the manifest).

Can you implement the wallpaper manager in terms of a simpler
setWallpaper(ArrayBuffer) API, and have the client-side code use XMLHttpRequest?

[flackr, 2012-07-18 13:46:10]

Just a few nits.

http://codereview.chromium.org/10754014/diff/25004/ash/desktop_background/des...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/10754014/diff/25004/ash/desktop_background/des...
ash/desktop_background/desktop_background_resources.h:34: ASH_EXPORT
WallpaperLayout GetLayoutEnum(const char* layout);
nit: sort

http://codereview.chromium.org/10754014/diff/25004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10754014/diff/25004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:8: #include
"ash/desktop_background/desktop_background_controller.h"
nit: Sort

http://codereview.chromium.org/10754014/diff/25004/chrome/common/extensions/d...
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/10754014/diff/25004/chrome/common/extensions/d...
chrome/common/extensions/docs/js/api_page_generator.js:87:
'../../api/experimental_wallpaper_manager.json',
nit: sort

[bshe, 2012-07-18 18:28:56]

@hihai

Could you please take another look? Thanks.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
File chrome/browser/chromeos/extensions/wallpaper_manager_api.h (right):

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:40: const int64
kBytesWallpaperDownloadProgressReportInterval = 10240;
removed.

On 2012/07/16 23:14:25, Mihai Parparita wrote:
> Seems like this can just be in the .cc file.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:66:
DECLARE_EXTENSION_FUNCTION_NAME("wallpaperManagerPrivate.setWallpaper");
On 2012/07/16 23:14:25, Mihai Parparita wrote:
> The file name generally matches the API namespace (e.g.
> file_browser_private_api.cc, terminal_private_api.cc), so this file should be
> called wallpaper_manager_private_api.cc. That's kind of a mouthful, you may
want
> to drop the "manager" from the namespace.

Done.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/extensions/wallpaper_manager_api.h:75: // This class is
a wrapper class for URLFetcher. It has a ref counted pointer
Removed the class in new patch.

On 2012/07/16 23:14:25, Mihai Parparita wrote:
> To make the header file more readable/hide internals better, I think you can
> just forward-declare WallpaperFetcher here, and then do the declaration and
> implementation .cc file.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/browser/chr...
chrome/browser/chromeos/login/wallpaper_manager.cc:83: void
WallpaperManager::OnCustomWallpaperLoaded(const std::string& email,
This will be used in the future. I need to have the email address as key to save
wallpaper related information to local state. Could I keep it for now? But I am
fine to remove it if you insist.

On 2012/07/16 23:14:25, Mihai Parparita wrote:
> The email parameter appears to be unused (so it can also be removed from
> SetWallpaperFromFile).

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/common/exte...
File chrome/common/extensions/api/wallpaper_manager_private.json (right):

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/common/exte...
chrome/common/extensions/api/wallpaper_manager_private.json:14: "nodoc": "true",
On 2012/07/16 23:14:25, Mihai Parparita wrote:
> Since the root namespace has nodoc, I believe you don't need it for every
> function/event too.

Done.

https://chromiumcodereview.appspot.com/10754014/diff/25004/chrome/common/exte...
chrome/common/extensions/api/wallpaper_manager_private.json:37: "name": "url"
On 2012/07/16 23:14:25, Mihai Parparita wrote:
> By making the API taken in a URL, you end up making the API more complex,
since
> it has to implement file downloading, fire progress and error events, etc. 
> 
> XMLHttpRequest already does all that. It can fetch data as ArrayBuffers (which
I
> believe you can pass into ExtensionFunction implementations) and extensions
can
> use it to load data from any host (just add "<all_urls>" to the permission
list
> in the manifest).
> 
> Can you implement the wallpaper manager in terms of a simpler
> setWallpaper(ArrayBuffer) API, and have the client-side code use
XMLHttpRequest?

Done.

[bshe, 2012-07-18 18:35:02]

@flackr

Done. Could you please take another look? Thanks

http://codereview.chromium.org/10754014/diff/25004/ash/desktop_background/des...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/10754014/diff/25004/ash/desktop_background/des...
ash/desktop_background/desktop_background_resources.h:34: ASH_EXPORT
WallpaperLayout GetLayoutEnum(const char* layout);
On 2012/07/18 13:46:10, flackr wrote:
> nit: sort

Done.

http://codereview.chromium.org/10754014/diff/25004/chrome/browser/chromeos/lo...
File chrome/browser/chromeos/login/wallpaper_manager.cc (right):

http://codereview.chromium.org/10754014/diff/25004/chrome/browser/chromeos/lo...
chrome/browser/chromeos/login/wallpaper_manager.cc:8: #include
"ash/desktop_background/desktop_background_controller.h"
On 2012/07/18 13:46:10, flackr wrote:
> nit: Sort

Done.

http://codereview.chromium.org/10754014/diff/25004/chrome/common/extensions/d...
File chrome/common/extensions/docs/js/api_page_generator.js (right):

http://codereview.chromium.org/10754014/diff/25004/chrome/common/extensions/d...
chrome/common/extensions/docs/js/api_page_generator.js:87:
'../../api/experimental_wallpaper_manager.json',
On 2012/07/18 13:46:10, flackr wrote:
> nit: sort

Done.

[bshe, 2012-07-19 22:42:11]

On 2012/07/18 18:35:02, bshe wrote:
> @flackr
> 
> Done. Could you please take another look? Thanks
> 
>
http://codereview.chromium.org/10754014/diff/25004/ash/desktop_background/des...
> File ash/desktop_background/desktop_background_resources.h (right):
> 
>
http://codereview.chromium.org/10754014/diff/25004/ash/desktop_background/des...
> ash/desktop_background/desktop_background_resources.h:34: ASH_EXPORT
> WallpaperLayout GetLayoutEnum(const char* layout);
> On 2012/07/18 13:46:10, flackr wrote:
> > nit: sort
> 
> Done.
> 
>
http://codereview.chromium.org/10754014/diff/25004/chrome/browser/chromeos/lo...
> File chrome/browser/chromeos/login/wallpaper_manager.cc (right):
> 
>
http://codereview.chromium.org/10754014/diff/25004/chrome/browser/chromeos/lo...
> chrome/browser/chromeos/login/wallpaper_manager.cc:8: #include
> "ash/desktop_background/desktop_background_controller.h"
> On 2012/07/18 13:46:10, flackr wrote:
> > nit: Sort
> 
> Done.
> 
>
http://codereview.chromium.org/10754014/diff/25004/chrome/common/extensions/d...
> File chrome/common/extensions/docs/js/api_page_generator.js (right):
> 
>
http://codereview.chromium.org/10754014/diff/25004/chrome/common/extensions/d...
> chrome/common/extensions/docs/js/api_page_generator.js:87:
> '../../api/experimental_wallpaper_manager.json',
> On 2012/07/18 13:46:10, flackr wrote:
> > nit: sort
> 
> Done.

friendly ping? Could you take another look? Thanks!

[Mihai Parparita -not on Chrome, 2012-07-20 00:17:05]

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:39: const char
kWallpaperManagerDomain[] = "obklkkbkpaoaejdabbfldmcfplpdgolj";
We generally refer to these as "extension ID", not "domain"

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:114: delete this;
Should there be a return after the delete?

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:122: delete this;
Ditto here.

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:172:
SetCustomWallpaper(wallpaper, layout_);
This needs a SendResponse(true) call to indicate that the async extension
function call is done (and all the places where you fail due to errors need a
SendResponse(false) call).

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_private_api.h (right):

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:20: namespace
wallpaper_manager_util {
You may want to have this in a separate file, it doesn't really have anything to
do with the API implementation.

[bshe, 2012-07-22 23:59:42]

mihai@

Done. Could you please take another look? Thanks!

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_private_api.cc (right):

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:39: const char
kWallpaperManagerDomain[] = "obklkkbkpaoaejdabbfldmcfplpdgolj";
On 2012/07/20 00:17:05, Mihai Parparita wrote:
> We generally refer to these as "extension ID", not "domain"

Done.

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:114: delete this;
On 2012/07/20 00:17:05, Mihai Parparita wrote:
> Should there be a return after the delete?

Done.

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:122: delete this;
On 2012/07/20 00:17:05, Mihai Parparita wrote:
> Ditto here.

Done.

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.cc:172:
SetCustomWallpaper(wallpaper, layout_);
On 2012/07/20 00:17:05, Mihai Parparita wrote:
> This needs a SendResponse(true) call to indicate that the async extension
> function call is done (and all the places where you fail due to errors need a
> SendResponse(false) call).

Done.

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_private_api.h (right):

http://codereview.chromium.org/10754014/diff/47001/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_private_api.h:20: namespace
wallpaper_manager_util {
On 2012/07/20 00:17:05, Mihai Parparita wrote:
> You may want to have this in a separate file, it doesn't really have anything
to
> do with the API implementation.

Done.

[flackr, 2012-07-23 20:52:46]

http://codereview.chromium.org/10754014/diff/34035/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

http://codereview.chromium.org/10754014/diff/34035/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:49: }  //namespace
wallpaper_manager_util
The original line is correct, there should be a space after the //.

http://codereview.chromium.org/10754014/diff/34035/chrome/common/extensions/a...
File chrome/common/extensions/api/wallpaper_private.json (right):

http://codereview.chromium.org/10754014/diff/34035/chrome/common/extensions/a...
chrome/common/extensions/api/wallpaper_private.json:11: "name": "getStrings",
I'm confused why I can't find anything like this for any other extensions. Is
there a different way to have localized strings in the extension?

[bshe, 2012-07-23 21:01:54]

http://codereview.chromium.org/10754014/diff/34035/chrome/browser/chromeos/ex...
File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):

http://codereview.chromium.org/10754014/diff/34035/chrome/browser/chromeos/ex...
chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:49: }  //namespace
wallpaper_manager_util
On 2012/07/23 20:52:46, flackr wrote:
> The original line is correct, there should be a space after the //.

done

http://codereview.chromium.org/10754014/diff/34035/chrome/common/extensions/a...
File chrome/common/extensions/api/wallpaper_private.json (right):

http://codereview.chromium.org/10754014/diff/34035/chrome/common/extensions/a...
chrome/common/extensions/api/wallpaper_private.json:11: "name": "getStrings",
I actually follow the pattern file browser use:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...

It has a getStrings function to get the localized strings. 
On 2012/07/23 20:52:46, flackr wrote:
> I'm confused why I can't find anything like this for any other extensions. Is
> there a different way to have localized strings in the extension?

[bshe, 2012-07-24 16:01:28]

On 2012/07/23 21:01:54, bshe wrote:
>
http://codereview.chromium.org/10754014/diff/34035/chrome/browser/chromeos/ex...
> File chrome/browser/chromeos/extensions/wallpaper_manager_util.cc (right):
> 
>
http://codereview.chromium.org/10754014/diff/34035/chrome/browser/chromeos/ex...
> chrome/browser/chromeos/extensions/wallpaper_manager_util.cc:49: } 
//namespace
> wallpaper_manager_util
> On 2012/07/23 20:52:46, flackr wrote:
> > The original line is correct, there should be a space after the //.
> 
> done
> 
>
http://codereview.chromium.org/10754014/diff/34035/chrome/common/extensions/a...
> File chrome/common/extensions/api/wallpaper_private.json (right):
> 
>
http://codereview.chromium.org/10754014/diff/34035/chrome/common/extensions/a...
> chrome/common/extensions/api/wallpaper_private.json:11: "name": "getStrings",
> I actually follow the pattern file browser use:
>
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/a...
> 
> It has a getStrings function to get the localized strings. 
> On 2012/07/23 20:52:46, flackr wrote:
> > I'm confused why I can't find anything like this for any other extensions.
Is
> > there a different way to have localized strings in the extension?

@mihai Could you please take another look when you have time? Thanks!

[Mihai Parparita -not on Chrome, 2012-07-25 00:04:57]

LGTM

[flackr, 2012-07-25 00:05:30]

LGTM too

[bshe, 2012-07-25 15:04:49]

Thanks!

Owners:
+sky for ash/*
+jhawkins for:
chrome/browser/ui/webui/options2/browser_options_handler.cc
chrome/browser/resources/chromeos/wallpaper_manager/manifest.json

[James Hawkins, 2012-07-25 15:06:07]

My files LGTM

[sky, 2012-07-25 16:14:42]

http://codereview.chromium.org/10754014/diff/43014/ash/desktop_background/des...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/10754014/diff/43014/ash/desktop_background/des...
ash/desktop_background/desktop_background_resources.h:37: ASH_EXPORT
WallpaperLayout GetLayoutEnum(const char* layout);
Why does this take a const char* ? The caller has a std::string. I would rather
see a const std::string& come in as its safer.

[bshe, 2012-07-25 16:39:36]

sky@

Done. Could you please take another look? Thanks!

http://codereview.chromium.org/10754014/diff/43014/ash/desktop_background/des...
File ash/desktop_background/desktop_background_resources.h (right):

http://codereview.chromium.org/10754014/diff/43014/ash/desktop_background/des...
ash/desktop_background/desktop_background_resources.h:37: ASH_EXPORT
WallpaperLayout GetLayoutEnum(const char* layout);
On 2012/07/25 16:14:42, sky wrote:
> Why does this take a const char* ? The caller has a std::string. I would
rather
> see a const std::string& come in as its safer.

Done.

[sky, 2012-07-25 16:59:40]

LGTM

[commit-bot: I haz the power, 2012-07-25 17:11:14]

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

[commit-bot: I haz the power, 2012-07-25 17:37:03]

Try job failure for 10754014-61001 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-07-25 18:57:23]

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

[commit-bot: I haz the power, 2012-07-25 20:24:11]

Change committed as 148400

[Mihai Parparita -not on Chrome, 2012-08-07 21:16:40]

https://chromiumcodereview.appspot.com/10754014/diff/64002/chrome/common/exte...
File chrome/common/extensions/api/_permission_features.json (right):

https://chromiumcodereview.appspot.com/10754014/diff/64002/chrome/common/exte...
chrome/common/extensions/api/_permission_features.json:296: "extension_types":
["extension", "packaged_app"],
If you need to use this API in a v2 app, you'll need to add "platform_app" to
the list.