Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1679983003:
Set wallpaper from right click menu in Files app. (Closed)
|
Created:
4 years, 10 months ago by ryoh Modified:
4 years, 10 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, davemoore+watch_chromium.org, hirono Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet wallpaper from right click menu in Files app.
BUG=578935
TEST=manually
Committed: https://crrev.com/09db206e3e766ac2370f336cbf2b7d3368ef816e
Cr-Commit-Position: refs/heads/master@{#377814}
Patch Set 1 #Patch Set 2 : use promise, handle errors #Patch Set 3 : remove unused var #
Total comments: 4
Patch Set 4 : fix nits #
Total comments: 4
Patch Set 5 : update in response to the review #Patch Set 6 : . #
Total comments: 2
Patch Set 7 : Update type annotations of closure compiler temporarily #Patch Set 8 : check chrome.runtime.lastError #Patch Set 9 : add todo #
Total comments: 3
Patch Set 10 : make consistency between internal and external #Patch Set 11 : update the annotation file with generated signatures #
Total comments: 10
Patch Set 12 : use local externs definition. #Patch Set 13 : . #Patch Set 14 : . #Patch Set 15 : renamed #
Total comments: 2
Patch Set 16 : use enum #
Messages
Total messages: 69 (20 generated)
Description was changed from ========== Set wallpaper from right click menu. BUG= ========== to ========== Set wallpaper from right click menu in Files app. BUG=578935 ==========
ryoh@chromium.org changed reviewers: + mtomasz@chromium.org
Description was changed from ========== Set wallpaper from right click menu in Files app. BUG=578935 ========== to ========== Set wallpaper from right click menu in Files app. BUG=578935 TEST=manually ==========
Hi! This patch enables users to set wallpaper from right click context menu in Files app. Closure compiler doesn't know about wallpaper API we use, so closure compilation fails. But it's a false positive error. (I'm working also on it) Please take a look!
lgtm with nits. Sorry for late, if I don't reply in a day please always ping me. https://codereview.chromium.org/1679983003/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1510: fileReader.onError = function(err) { nit: should it be onerror? https://codereview.chromium.org/1679983003/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1518: 'data': arrayBuffer, nit: no need for '' for keys. { data: ..., layout: ... .. }
Thank you for your review! > if I don't reply in a day please always ping me. OK! :-) Closure compiler still reports wrong errors on this patch, so I will commit this after fixing it. https://codereview.chromium.org/1679983003/diff/40001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1510: fileReader.onError = function(err) { On 2016/02/12 04:30:51, mtomasz wrote: > nit: should it be onerror? Done. https://codereview.chromium.org/1679983003/diff/40001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1518: 'data': arrayBuffer, On 2016/02/12 04:30:51, mtomasz wrote: > nit: no need for '' for keys. > { > data: ..., > layout: ... > .. > } Done.
hirono@chromium.org changed reviewers: + hirono@chromium.org
https://codereview.chromium.org/1679983003/diff/60001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1508: resolve(this.result); I'm a bit surprised at onload is invoked with 'this'. I guess it's undocumented behavior. Could you explicitly refer fileReader.result ? https://codereview.chromium.org/1679983003/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1528: if (entries.length === 0 ) { nit: Please remove a space following 0.
Thank you for review, hirono@. Could you take a look? https://codereview.chromium.org/1679983003/diff/60001/ui/file_manager/file_ma... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1508: resolve(this.result); On 2016/02/12 16:44:59, hirono wrote: > I'm a bit surprised at onload is invoked with 'this'. I guess it's undocumented > behavior. Could you explicitly refer fileReader.result ? Done. https://codereview.chromium.org/1679983003/diff/60001/ui/file_manager/file_ma... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1528: if (entries.length === 0 ) { On 2016/02/12 16:44:59, hirono wrote: > nit: Please remove a space following 0. Done.
lgtm with a nit. Thanks! https://codereview.chromium.org/1679983003/diff/100001/ui/file_manager/file_m... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/100001/ui/file_manager/file_m... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1520: }, function() {}); Maybe we should check chrome.runtime.lastError in the completion callback.
Hi, I update the patch. Could you take a look? I added some entries to type annotations of closure compiler temporarily. https://codereview.chromium.org/1679983003/diff/100001/ui/file_manager/file_m... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/100001/ui/file_manager/file_m... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1520: }, function() {}); On 2016/02/16 02:53:09, hirono wrote: > Maybe we should check chrome.runtime.lastError in the completion callback. Wow! What a C-like error handling! Thanks.
Thanks! still LGTM
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org Link to the patchset: https://codereview.chromium.org/1679983003/#ps160001 (title: "add todo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679983003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679983003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ryoh@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hi, rdevlin! We need your LGTM to submit a patch for closure compiler's type annotations. It's needed temporarily. Could you take a look?
ryoh@chromium.org changed reviewers: + fukino@chromium.org
I heard that fukino@ in our team is also an owner.
https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9162: We should instead be generating this from the json schema compiler script.
Hi, devlin. I replied to your review! Could you take a look? https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9162: On 2016/02/17 18:19:36, Devlin wrote: > We should instead be generating this from the json schema compiler script. IIRC, this file is generated from the file hosted in our internal repository. I also send a patch to internal repository. It seems to take time to propagate the changes from internal to external(~10days?). I would like to send this change to M50(the branch will be cut soon), so I also sent a patch to this file.
https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9162: On 2016/02/19 05:31:27, ryoh wrote: > On 2016/02/17 18:19:36, Devlin wrote: > > We should instead be generating this from the json schema compiler script. > > IIRC, this file is generated from the file hosted in our internal repository. > I also send a patch to internal repository. > > It seems to take time to propagate the changes from internal to > external(~10days?). > I would like to send this change to M50(the branch will be cut soon), > so I also sent a patch to this file. The closure compiler isn't automatically rolled - it's done by hand. So there's no guarantee on when it'll actually be updated. The script in the json schema compiler generates some nice externs for the API that might be slightly better to copy-paste in here than these (which appear to be handwritten?). Note also that in the long-term, we hope to transition these to each having their own extern file, rather than a single 10k line one - but we don't have to address that now.
ryoh@chromium.org changed reviewers: + ewa@google.com
On 2016/02/19 17:31:53, Devlin wrote: > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > File third_party/closure_compiler/externs/chrome_extensions.js (right): > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > third_party/closure_compiler/externs/chrome_extensions.js:9162: > On 2016/02/19 05:31:27, ryoh wrote: > > On 2016/02/17 18:19:36, Devlin wrote: > > > We should instead be generating this from the json schema compiler script. > > > > IIRC, this file is generated from the file hosted in our internal repository. > > I also send a patch to internal repository. > > > > It seems to take time to propagate the changes from internal to > > external(~10days?). > > I would like to send this change to M50(the branch will be cut soon), > > so I also sent a patch to this file. > > The closure compiler isn't automatically rolled - it's done by hand. So there's > no guarantee on when it'll actually be updated. The script in the json schema > compiler generates some nice externs for the API that might be slightly better > to copy-paste in here than these (which appear to be handwritten?). Note also > that in the long-term, we hope to transition these to each having their own > extern file, rather than a single 10k line one - but we don't have to address > that now. I see. I also think it's best if we can generate annotation file from schema automatically. Could you tell me some examples of schema files? I will be able to write a schema file for wallpaper API. # Sorry, I can't find nice one. IIUC, all (or most?) of signatures (of chrome extensions) are written by hand. This patch was already landed to our internal repository. ewa@ I added you to cc, I think you are working on these signature files of closure compiler in internal repository. What do you think? Do you think should we write a schema file?
On 2016/02/23 06:05:23, ryoh wrote: > On 2016/02/19 17:31:53, Devlin wrote: > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > File third_party/closure_compiler/externs/chrome_extensions.js (right): > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > third_party/closure_compiler/externs/chrome_extensions.js:9162: > > On 2016/02/19 05:31:27, ryoh wrote: > > > On 2016/02/17 18:19:36, Devlin wrote: > > > > We should instead be generating this from the json schema compiler script. > > > > > > IIRC, this file is generated from the file hosted in our internal > repository. > > > I also send a patch to internal repository. > > > > > > It seems to take time to propagate the changes from internal to > > > external(~10days?). > > > I would like to send this change to M50(the branch will be cut soon), > > > so I also sent a patch to this file. > > > > The closure compiler isn't automatically rolled - it's done by hand. So > there's > > no guarantee on when it'll actually be updated. The script in the json schema > > compiler generates some nice externs for the API that might be slightly better > > to copy-paste in here than these (which appear to be handwritten?). Note also > > that in the long-term, we hope to transition these to each having their own > > extern file, rather than a single 10k line one - but we don't have to address > > that now. > > I see. I also think it's best if we can generate annotation file from schema > automatically. > > Could you tell me some examples of schema files? > I will be able to write a schema file for wallpaper API. > # Sorry, I can't find nice one. > > IIUC, all (or most?) of signatures (of chrome extensions) are written by hand. > This patch was already landed to our internal repository. > > ewa@ > I added you to cc, I think you are working on these signature files of > closure compiler in internal repository. > > What do you think? > Do you think should we write a schema file? There's a tool in chrome to generate the externs. I'm working on making it a better part of our process, but it's not there yet. In the meantime, use "src/ $ python tools/json_schema_compiler/compiler.py chrome/common/extensions/api/wallpaper.json --root=. --generator=externs" to generate the externs, and then copy-paste them in here.
On 2016/02/23 06:29:12, Devlin wrote: > On 2016/02/23 06:05:23, ryoh wrote: > > On 2016/02/19 17:31:53, Devlin wrote: > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > > File third_party/closure_compiler/externs/chrome_extensions.js (right): > > > > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > > third_party/closure_compiler/externs/chrome_extensions.js:9162: > > > On 2016/02/19 05:31:27, ryoh wrote: > > > > On 2016/02/17 18:19:36, Devlin wrote: > > > > > We should instead be generating this from the json schema compiler > script. > > > > > > > > IIRC, this file is generated from the file hosted in our internal > > repository. > > > > I also send a patch to internal repository. > > > > > > > > It seems to take time to propagate the changes from internal to > > > > external(~10days?). > > > > I would like to send this change to M50(the branch will be cut soon), > > > > so I also sent a patch to this file. > > > > > > The closure compiler isn't automatically rolled - it's done by hand. So > > there's > > > no guarantee on when it'll actually be updated. The script in the json > schema > > > compiler generates some nice externs for the API that might be slightly > better > > > to copy-paste in here than these (which appear to be handwritten?). Note > also > > > that in the long-term, we hope to transition these to each having their own > > > extern file, rather than a single 10k line one - but we don't have to > address > > > that now. > > > > I see. I also think it's best if we can generate annotation file from schema > > automatically. > > > > Could you tell me some examples of schema files? > > I will be able to write a schema file for wallpaper API. > > # Sorry, I can't find nice one. > > > > IIUC, all (or most?) of signatures (of chrome extensions) are written by hand. > > This patch was already landed to our internal repository. > > > > ewa@ > > I added you to cc, I think you are working on these signature files of > > closure compiler in internal repository. > > > > What do you think? > > Do you think should we write a schema file? > > There's a tool in chrome to generate the externs. I'm working on making it a > better part of our process, but it's not there yet. In the meantime, use "src/ > $ python tools/json_schema_compiler/compiler.py > chrome/common/extensions/api/wallpaper.json --root=. --generator=externs" to > generate the externs, and then copy-paste them in here. (There's also a long sordid discussion about how it doesn't make sense that we kinda-sorta pull in this externs file from the closure compiler's public repo when it's in fact based on chromium code - but that's a little tangential.)
On 2016/02/23 06:29:12, Devlin wrote: > On 2016/02/23 06:05:23, ryoh wrote: > > On 2016/02/19 17:31:53, Devlin wrote: > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > > File third_party/closure_compiler/externs/chrome_extensions.js (right): > > > > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > > third_party/closure_compiler/externs/chrome_extensions.js:9162: > > > On 2016/02/19 05:31:27, ryoh wrote: > > > > On 2016/02/17 18:19:36, Devlin wrote: > > > > > We should instead be generating this from the json schema compiler > script. > > > > > > > > IIRC, this file is generated from the file hosted in our internal > > repository. > > > > I also send a patch to internal repository. > > > > > > > > It seems to take time to propagate the changes from internal to > > > > external(~10days?). > > > > I would like to send this change to M50(the branch will be cut soon), > > > > so I also sent a patch to this file. > > > > > > The closure compiler isn't automatically rolled - it's done by hand. So > > there's > > > no guarantee on when it'll actually be updated. The script in the json > schema > > > compiler generates some nice externs for the API that might be slightly > better > > > to copy-paste in here than these (which appear to be handwritten?). Note > also > > > that in the long-term, we hope to transition these to each having their own > > > extern file, rather than a single 10k line one - but we don't have to > address > > > that now. > > > > I see. I also think it's best if we can generate annotation file from schema > > automatically. > > > > Could you tell me some examples of schema files? > > I will be able to write a schema file for wallpaper API. > > # Sorry, I can't find nice one. > > > > IIUC, all (or most?) of signatures (of chrome extensions) are written by hand. > > This patch was already landed to our internal repository. > > > > ewa@ > > I added you to cc, I think you are working on these signature files of > > closure compiler in internal repository. > > > > What do you think? > > Do you think should we write a schema file? > > There's a tool in chrome to generate the externs. I'm working on making it a > better part of our process, but it's not there yet. In the meantime, use "src/ > $ python tools/json_schema_compiler/compiler.py > chrome/common/extensions/api/wallpaper.json --root=. --generator=externs" to > generate the externs, and then copy-paste them in here. Thank you! It seems that we can't set type of 'data' as "ArrayBuffer". % python tools/json_schema_compiler/compiler.py chrome/common/extensions/api/wallpaper.json --root=. --generator=externs "Unsupported JSON type ArrayBuffer" Currently, the type of "data" is defined as "binary": https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... and generated file says it's "?|undefined": /** * Sets wallpaper to the image at <em>url</em> or <em>wallpaperData</em> with * the specified <em>layout</em> * @param {{ * data: (?|undefined), * url: (string|undefined), * layout: !chrome.wallpaper.WallpaperLayout, * filename: string, * thumbnail: (boolean|undefined) * }} details * @param {function(?):void} callback * @see https://developer.chrome.com/extensions/wallpaper#method-setWallpaper */ Is it ok?
Lots of points to touch on.
History: due to some strange historical twists, I've ended up being in
charge of the chrome_extensions.js file. I have no association with either
the Chrome or Closure teams.
Over the last year or so there's been some work to try to automate the
generation of these externs. It's a pretty obvious approach to take,
however, I think getting all the details right is challenging as some of
the IDL is under-specified.
A 10K externs file is a Bad Thing. However, it's what we have and, until
it's replaced with something else, it's what we have to deal with.
Yes, the externs in chrome_extensions.js have been written by hand, over
many years, by many different people. They are not sefl-consistent,
however, I've tried to capture the Best Practices in the comments at the
top of the file and have tried to consistently enforce them.
My understanding is that the schema file refers to the IDL file. Is that
correct? If so, doesn't it already exist? I think the suggestion is to run
the externs generator for the wallpaper API and see if we like the results
better than the by-hand ones and, if so, incorporate them.
Evan
On Mon, Feb 22, 2016 at 10:05 PM, <ryoh@chromium.org> wrote:
> On 2016/02/19 17:31:53, Devlin wrote:
> >
>
>
https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co...
> > File third_party/closure_compiler/externs/chrome_extensions.js (right):
> >
> >
>
>
https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co...
> > third_party/closure_compiler/externs/chrome_extensions.js:9162:
> > On 2016/02/19 05:31:27, ryoh wrote:
> > > On 2016/02/17 18:19:36, Devlin wrote:
> > > > We should instead be generating this from the json schema compiler
> script.
> > >
> > > IIRC, this file is generated from the file hosted in our internal
> repository.
> > > I also send a patch to internal repository.
> > >
> > > It seems to take time to propagate the changes from internal to
> > > external(~10days?).
> > > I would like to send this change to M50(the branch will be cut soon),
> > > so I also sent a patch to this file.
> >
> > The closure compiler isn't automatically rolled - it's done by hand. So
> there's
> > no guarantee on when it'll actually be updated. The script in the json
> schema
> > compiler generates some nice externs for the API that might be slightly
> better
> > to copy-paste in here than these (which appear to be handwritten?). Note
> also
> > that in the long-term, we hope to transition these to each having their
> own
> > extern file, rather than a single 10k line one - but we don't have to
> address
> > that now.
>
> I see. I also think it's best if we can generate annotation file from
> schema
> automatically.
>
> Could you tell me some examples of schema files?
> I will be able to write a schema file for wallpaper API.
> # Sorry, I can't find nice one.
>
> IIUC, all (or most?) of signatures (of chrome extensions) are written by
> hand.
> This patch was already landed to our internal repository.
>
> ewa@
> I added you to cc, I think you are working on these signature files of
> closure compiler in internal repository.
>
> What do you think?
> Do you think should we write a schema file?
>
>
> https://codereview.chromium.org/1679983003/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
I do not think it's ok to use "?|undefined" as the type. From the JS Style Guide <https://engdoc.corp.google.com/eng/doc/javascriptguide.xml?cl=head#JavaScript...>, the description of the ? type says: Indicates that the variable can take on any type, and the compiler should not type-check any uses of it. so ALL type-checking would be lost. Not good. Evan On Mon, Feb 22, 2016 at 11:13 PM, <ryoh@chromium.org> wrote: > On 2016/02/23 06:29:12, Devlin wrote: > > On 2016/02/23 06:05:23, ryoh wrote: > > > On 2016/02/19 17:31:53, Devlin wrote: > > > > > > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > > > File third_party/closure_compiler/externs/chrome_extensions.js > (right): > > > > > > > > > > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > > > third_party/closure_compiler/externs/chrome_extensions.js:9162: > > > > On 2016/02/19 05:31:27, ryoh wrote: > > > > > On 2016/02/17 18:19:36, Devlin wrote: > > > > > > We should instead be generating this from the json schema > compiler > > script. > > > > > > > > > > IIRC, this file is generated from the file hosted in our internal > > > repository. > > > > > I also send a patch to internal repository. > > > > > > > > > > It seems to take time to propagate the changes from internal to > > > > > external(~10days?). > > > > > I would like to send this change to M50(the branch will be cut > soon), > > > > > so I also sent a patch to this file. > > > > > > > > The closure compiler isn't automatically rolled - it's done by hand. > So > > > there's > > > > no guarantee on when it'll actually be updated. The script in the > json > > schema > > > > compiler generates some nice externs for the API that might be > slightly > > better > > > > to copy-paste in here than these (which appear to be handwritten?). > Note > > also > > > > that in the long-term, we hope to transition these to each having > their > own > > > > extern file, rather than a single 10k line one - but we don't have to > > address > > > > that now. > > > > > > I see. I also think it's best if we can generate annotation file from > schema > > > automatically. > > > > > > Could you tell me some examples of schema files? > > > I will be able to write a schema file for wallpaper API. > > > # Sorry, I can't find nice one. > > > > > > IIUC, all (or most?) of signatures (of chrome extensions) are written > by > hand. > > > This patch was already landed to our internal repository. > > > > > > ewa@ > > > I added you to cc, I think you are working on these signature files of > > > closure compiler in internal repository. > > > > > > What do you think? > > > Do you think should we write a schema file? > > > > There's a tool in chrome to generate the externs. I'm working on making > it a > > better part of our process, but it's not there yet. In the meantime, use > "src/ > > $ python tools/json_schema_compiler/compiler.py > > chrome/common/extensions/api/wallpaper.json --root=. > --generator=externs" to > > generate the externs, and then copy-paste them in here. > > Thank you! > It seems that we can't set type of 'data' as "ArrayBuffer". > % python tools/json_schema_compiler/compiler.py > chrome/common/extensions/api/wallpaper.json --root=. --generator=externs > "Unsupported JSON type ArrayBuffer" > > Currently, the type of "data" is defined as "binary": > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > and generated file says it's "?|undefined": > > /** > * Sets wallpaper to the image at <em>url</em> or <em>wallpaperData</em> > with > * the specified <em>layout</em> > * @param {{ > * data: (?|undefined), > * url: (string|undefined), > * layout: !chrome.wallpaper.WallpaperLayout, > * filename: string, > * thumbnail: (boolean|undefined) > * }} details > * @param {function(?):void} callback > * @see > https://developer.chrome.com/extensions/wallpaper#method-setWallpaper > */ > > Is it ok? > > https://codereview.chromium.org/1679983003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/23 07:13:11, ryoh wrote: > On 2016/02/23 06:29:12, Devlin wrote: > > On 2016/02/23 06:05:23, ryoh wrote: > > > On 2016/02/19 17:31:53, Devlin wrote: > > > > > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > > > > File third_party/closure_compiler/externs/chrome_extensions.js (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1679983003/diff/160001/third_party/closure_co... > Thank you! > It seems that we can't set type of 'data' as "ArrayBuffer". > % python tools/json_schema_compiler/compiler.py > chrome/common/extensions/api/wallpaper.json --root=. --generator=externs > "Unsupported JSON type ArrayBuffer" > > Currently, the type of "data" is defined as "binary": > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > and generated file says it's "?|undefined": > > /** > * Sets wallpaper to the image at <em>url</em> or <em>wallpaperData</em> with > * the specified <em>layout</em> > * @param {{ > * data: (?|undefined), > * url: (string|undefined), > * layout: !chrome.wallpaper.WallpaperLayout, > * filename: string, > * thumbnail: (boolean|undefined) > * }} details > * @param {function(?):void} callback > * @see https://developer.chrome.com/extensions/wallpaper#method-setWallpaper > */ > > Is it ok? Looks like a bug. Please file one against me to fix that, and go ahead and put the right type in by hand.
Thank you for your reviews. I filed a bug to support "ArrayBuffer" type in schema compiler: https://bugs.chromium.org/p/chromium/issues/detail?id=588993&q=ryoh&colspec=I... And I update this patch with generated type annotations (+ some fix for "ArrayBuffer" type by hand) Could you take a look?
Looking at the chromimum patch: 1. It would be desirable to keep the @see link. 2. The enum declaration doesn't accomplish anything. See section G. in the Best Practices at the top of the file. 3. The description before setWallpaper doesn't need HTML markup. 4. The parameter to the callback is 'binary' so it should also be an ArrayBuffer and not '?'. On Tue, Feb 23, 2016 at 12:08 AM, <ryoh@chromium.org> wrote: > Thank you for your reviews. > > I filed a bug to support "ArrayBuffer" type in schema compiler: > > https://bugs.chromium.org/p/chromium/issues/detail?id=588993&q=ryoh&colspec=I... > > And I update this patch with generated type annotations (+ some fix for > "ArrayBuffer" type by hand) > > Could you take a look? > > https://codereview.chromium.org/1679983003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/02/23 09:41:27, chromium-reviews wrote: > Looking at the chromimum patch: > > 1. It would be desirable to keep the @see link. > 2. The enum declaration doesn't accomplish anything. See section G. in > the Best Practices at the top of the file. > 3. The description before setWallpaper doesn't need HTML markup. > 4. The parameter to the callback is 'binary' so it should also be an > ArrayBuffer and not '?'. Heya Evan - FYI, it's usually easier if you comment directly on the patch (it makes it a bit more clear). :) re enums, the information at the top of the file is out of date. Chrome *does* directly define these enum objects so that they can be used like "chrome.fooApi.BarType.BAZ", so it's important to add these (and encouraged to use them in the code!). Re 3, it doesn't need it, but I'd rather just not care much. :) Since these are autogenerated, I'd like to get in the practice of just being able to type a command and be done, rather than needing any editing. Re 1, I could go either way - I suppose it would make sense to have an @see for the whole namespace, even though we have @sees over types and methods that would suffice. That said, for the time being, I'd prefer to not worry about it (for the same reason as above). Ryoh@, please do change the type to binary as discussed above. :)
I'll repeat my comments, and reply to some of yours, in the patch and we
can finish the discussion there.
Evan
On Tue, Feb 23, 2016 at 8:32 AM, <rdevlin.cronin@chromium.org> wrote:
> On 2016/02/23 09:41:27, chromium-reviews wrote:
> > Looking at the chromimum patch:
> >
> > 1. It would be desirable to keep the @see link.
> > 2. The enum declaration doesn't accomplish anything. See section G. in
> > the Best Practices at the top of the file.
> > 3. The description before setWallpaper doesn't need HTML markup.
> > 4. The parameter to the callback is 'binary' so it should also be an
> > ArrayBuffer and not '?'.
>
> Heya Evan - FYI, it's usually easier if you comment directly on the patch
> (it
> makes it a bit more clear). :)
>
> re enums, the information at the top of the file is out of date. Chrome
> *does*
> directly define these enum objects so that they can be used like
> "chrome.fooApi.BarType.BAZ", so it's important to add these (and
> encouraged to
> use them in the code!). Re 3, it doesn't need it, but I'd rather just not
> care
> much. :) Since these are autogenerated, I'd like to get in the practice of
> just
> being able to type a command and be done, rather than needing any editing.
> Re
> 1, I could go either way - I suppose it would make sense to have an @see
> for the
> whole namespace, even though we have @sees over types and methods that
> would
> suffice. That said, for the time being, I'd prefer to not worry about it
> (for
> the same reason as above).
>
> Ryoh@, please do change the type to binary as discussed above. :)
>
> https://codereview.chromium.org/1679983003/
>
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9166: * @const I was thinking there should be an @see link to the docs but, the @see's before the types and methods is sufficient. https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9178: }; The docs mention the chrome.wallpaper.WallpaperLayout as being an enum, but lists the values as strings (specifically: "STRETCH", "CENTER", or "CENTER_CROPPED"). The docs don't mention the enum value names used to reference the strings. Therefore, a developer does not know to use chrome.wallpaper.WallpaperLayout.STRETCH and will, instead, use "STRETCH". If the API is providing an enum with named fields for the values, then the docs are not complete and that should be fixed. Also, if the API is providing an enum with named fields for the values, the enum should NOT be defined in an externs file. Defining enums in an externs file is no-no as developers will never find them. https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9182: * the specified <em>layout</em> No need for HTML markup in this comment. In this file, it's distracting and not helpful. Since this is auto-generated, it would be best to change the auto-generator to strip markup from descriptions. https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9190: * @param {function(?):void} callback The callback's param should be an ArrayBuffer.
https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9178: }; On 2016/02/23 22:15:47, ewa wrote: > The docs mention the chrome.wallpaper.WallpaperLayout as being an enum, but > lists the values as strings (specifically: "STRETCH", "CENTER", or > "CENTER_CROPPED"). The docs don't mention the enum value names used to > reference the strings. > > Therefore, a developer does not know to use > chrome.wallpaper.WallpaperLayout.STRETCH and will, instead, use "STRETCH". > > If the API is providing an enum with named fields for the values, then the docs > are not complete and that should be fixed. Also, if the API is providing an > enum with named fields for the values, the enum should NOT be defined in an > externs file. Defining enums in an externs file is no-no as developers will > never find them. Yes, the docs should be updated to indicate this. However, that's not a reason to not include them in the extern. Developers *are* using these enums (and chrome is too!) and if we remove them from the externs file, the code will fail to compile (no object named Foo on chrome.bar). Just because something isn't well known doesn't mean the externs should be incorrect. ;) https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9182: * the specified <em>layout</em> On 2016/02/23 22:15:47, ewa wrote: > No need for HTML markup in this comment. In this file, it's distracting and not > helpful. > > Since this is auto-generated, it would be best to change the auto-generator to > strip markup from descriptions. Agreed, it'd be good to not have the markup in the externs. Patches welcome. :)
https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9178: }; Ok, I think we're making different points. For an enum provided by a library, the externs file needs to define the, well, external things. That includes the type name, chrome.wallpaper.WallpaperLayout in this example, and it's members, STRETCH, CENTER and CENTER_CROPPED. It should not be defining the value of the names, 'STRETCH', 'CENTER' and 'CENTER_CROPPED', else we end up with Two Copies of the Truth (always bad). An externs file is never the place to define something, rather, it's the place to describe something that is defined elsewhere and to describe the names used to reference those somethings so the compiler doesn't rename them. Hmm, it's not entirely clear to me how the compiler handles enums in an externs file. For @enum, it probably requires there to be values as it likes valid JS. I ran some experiments and, yes, values are required. The good news is that the compiler did not replace the symbolic name with its value from the externs file (that would have been a big mess). I also tried setting their values to the empty string and that worked. If the param to is declared to be an enum, then the compiler complains if a string is passed in. Since the docs encourage the use of strings, the param probably needs to be support both the enum or a string. ---- The lack of docs is reason to not include enums. The enums may be used in chrome, but's not relevant to this discussion as this is about extensions and extension developers. If extension developers are using them, then they made a leap of faith that the enum member's name is identical to it's value. That said, I have a suggestion below that allows the enum in the externs file before docs have been fixed. ---- How to move forward? My suggestions: 1. The doc-generator needs to be fixed so the docs are fixed. 2. The enum can be included in the externs file, but the values should be empty and uses must also accept strings. By doing the latter, it's fine to include the enum prior to getting the docs fixed. 3. Item 2 should be incorporated into the auto-generator as there are lots of enums that are strings in the current chrome_extensions.js and dropping support of strings will break code.
rdevlin.cronin@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam, since it seems like this discussion is becoming bigger than this particular extern. https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9178: }; On 2016/02/24 01:56:33, ewa wrote: > Ok, I think we're making different points. > > For an enum provided by a library, the externs file needs to define the, well, > external things. That includes the type name, chrome.wallpaper.WallpaperLayout > in this example, and it's members, STRETCH, CENTER and CENTER_CROPPED. It > should not be defining the value of the names, 'STRETCH', 'CENTER' and > 'CENTER_CROPPED', else we end up with Two Copies of the Truth (always bad). > > An externs file is never the place to define something, rather, it's the place > to describe something that is defined elsewhere and to describe the names used > to reference those somethings so the compiler doesn't rename them. > > Hmm, it's not entirely clear to me how the compiler handles enums in an externs > file. For @enum, it probably requires there to be values as it likes valid JS. > I ran some experiments and, yes, values are required. The good news is that the > compiler did not replace the symbolic name with its value from the externs file > (that would have been a big mess). I also tried setting their values to the > empty string and that worked. > > If the param to is declared to be an enum, then the compiler complains if a > string is passed in. Since the docs encourage the use of strings, the param > probably needs to be support both the enum or a string. > > ---- > > The lack of docs is reason to not include enums. The enums may be used in > chrome, but's not relevant to this discussion as this is about extensions and > extension developers. If extension developers are using them, then they made a > leap of faith that the enum member's name is identical to it's value. > > That said, I have a suggestion below that allows the enum in the externs file > before docs have been fixed. > > > ---- > How to move forward? My suggestions: > 1. The doc-generator needs to be fixed so the docs are fixed. > 2. The enum can be included in the externs file, but the values should be empty > and uses must also accept strings. By doing the latter, it's fine to include > the enum prior to getting the docs fixed. > 3. Item 2 should be incorporated into the auto-generator as there are lots of > enums that are strings in the current chrome_extensions.js and dropping support > of strings will break code. > > I would say that anyone consuming *new* externs (which this is) should use enums rather than strings - enums are more resilient. We shouldn't make new externs that permit old unsafe behavior. Re "The enums may be used in chrome, but's not relevant to this discussion as this is about extensions and extension developers." I don't really agree - extensions are a chrome platform. The source of truth is in chrome (and always has been). Chrome's behavior is very relevant, and is what externs should be based upon. Also, at the end of the day, please note that this is the chromium externs file, which, for historical reasons, differs from the internal version (we're working on more noticeably forking here). This is precisely for some of these reasons - that it doesn't make sense to have chrome pull in an external file based on internal stuff.
[snip]
> I would say that anyone consuming *new* externs (which this is) should use
enums
> rather than strings - enums are more resilient. We shouldn't make new externs
> that permit old unsafe behavior.
Strongly agree.
However, that means the docs should be fixed before the enum is included in the
externs
file. As is, a developer reading the docs is not aware of the symbolic names.
That will lead
to developers using string literals and getting messages from the compiler.
> Re "The enums may be used in chrome, but's not relevant to this discussion as
> this is about extensions and extension developers." I don't really agree -
> extensions are a chrome platform. The source of truth is in chrome (and
always
> has been). Chrome's behavior is very relevant, and is what externs should be
> based upon.
The point I'm trying to make is that extension developers rely on the Chrome
Extension
docs and should not be expected to rely on other sources. It becomes a
philosophy
question: if an enum is not documented, does it exist? No.
> Also, at the end of the day, please note that this is the chromium externs
file,
> which, for historical reasons, differs from the internal version (we're
working
> on more noticeably forking here). This is precisely for some of these reasons
-
> that it doesn't make sense to have chrome pull in an external file based on
> internal stuff.
I work on the internal version. If the folks in charge of the external version
wish to
make changes, so be it. I'm happy to share my thoughts and experience regarding
why the internal version is the way it is.
I don't think any of the discussion in this thread motivates differences in the
versions as this is about how we can get the best type-checking possible and
that's valuable to both.
In conclusion:
1. I strongly agree that new enums should be used without the "or string" aspect
as that provides the strongest type-checking.
2. The enum docs need to be complete.
3. When #2 happens, APIs that use older enums can be converted, but will need
to use "or string" for backward compatibility.
not lgtm https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9: // SSS::::::::SS run roll_closure_compiler. P::::PPPPPPPPP how can I make this bigger, exactly?
https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... File third_party/closure_compiler/externs/chrome_extensions.js (right): https://codereview.chromium.org/1679983003/diff/200001/third_party/closure_co... third_party/closure_compiler/externs/chrome_extensions.js:9: // SSS::::::::SS run roll_closure_compiler. P::::PPPPPPPPP On 2016/02/24 19:12:51, Dan Beam wrote: > how can I make this bigger, exactly? fwiw: i found cl/115298731 but it differs from what you're adding here and you didn't run the script to import these (it would've changed more files)
M-50 branch cut is approaching, so how about following these steps? Step 1. Use app's local extern file to tell the compiler about chrome.wallpaper definition for M-50 release. Step 2. Update upstream chrome_extensions.js(cl/115298731) based on an agreement about enum definition. Step 3. Replace local extern with definitions in global chrome_extensions.js using roll_closure_compiler.
On 2016/02/24 23:16:39, fukino wrote: > M-50 branch cut is approaching, so how about following these steps? > > Step 1. Use app's local extern file to tell the compiler about chrome.wallpaper > definition for M-50 release. > Step 2. Update upstream chrome_extensions.js(cl/115298731) based on an agreement > about enum definition. > Step 3. Replace local extern with definitions in global chrome_extensions.js > using roll_closure_compiler. sgtm
As fukino@'s plan, I updated the patch to use local extenrs file. Please take a look!
On 2016/02/25 04:48:11, ryoh wrote: > As fukino@'s plan, > I updated the patch to use local extenrs file. > Please take a look! It looks wallpaper.js is missing, and could you name it as 'chrome_wallpaper.js'?
Sorry for missing files. I added chrome_wallpapar.js
lgtm.
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mtomasz@chromium.org, hirono@chromium.org Link to the patchset: https://codereview.chromium.org/1679983003/#ps280001 (title: "renamed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679983003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679983003/280001
The CQ bit was unchecked by commit-bot@chromium.org
A disapproval has been posted by following reviewers: dbeam@chromium.org. Please make sure to get positive review before another CQ attempt.
On 2016/02/25 05:12:52, commit-bot: I haz the power wrote: > A disapproval has been posted by following reviewers: mailto:dbeam@chromium.org. > Please make sure to get positive review before another CQ attempt. dbeam@ Sorry, could you take a look? How about the fukino@'s plan?
lgtm https://codereview.chromium.org/1679983003/diff/280001/ui/file_manager/file_m... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/280001/ui/file_manager/file_m... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1538: layout: 'CENTER_CROPPED', using the enum even if the @param is a {string} should work. so do that ;)
https://codereview.chromium.org/1679983003/diff/280001/ui/file_manager/file_m... File ui/file_manager/file_manager/foreground/js/file_manager_commands.js (right): https://codereview.chromium.org/1679983003/diff/280001/ui/file_manager/file_m... ui/file_manager/file_manager/foreground/js/file_manager_commands.js:1538: layout: 'CENTER_CROPPED', On 2016/02/25 17:08:55, Dan Beam wrote: > using the enum even if the @param is a {string} should work. so do that ;) Done.
The CQ bit was checked by ryoh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, hirono@chromium.org, mtomasz@chromium.org, fukino@chromium.org Link to the patchset: https://codereview.chromium.org/1679983003/#ps300001 (title: "use enum")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679983003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679983003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ryoh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1679983003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1679983003/300001
Message was sent while issue was closed.
Description was changed from ========== Set wallpaper from right click menu in Files app. BUG=578935 TEST=manually ========== to ========== Set wallpaper from right click menu in Files app. BUG=578935 TEST=manually ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Set wallpaper from right click menu in Files app. BUG=578935 TEST=manually ========== to ========== Set wallpaper from right click menu in Files app. BUG=578935 TEST=manually Committed: https://crrev.com/09db206e3e766ac2370f336cbf2b7d3368ef816e Cr-Commit-Position: refs/heads/master@{#377814} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/09db206e3e766ac2370f336cbf2b7d3368ef816e Cr-Commit-Position: refs/heads/master@{#377814} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
