Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
107813005:
Files.app: Use types and constants generated by the IDL compiler in the event router. (Closed)
|
Created:
6 years, 12 months ago by hirono Modified:
6 years, 11 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, rginda+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFiles.app: Use types and constants generated by the IDL compiler in the event router.
Event names and dictionary values in the event router of Files.app can be
replaced with constants and types generated by the IDL compiler.
BUG=330638
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243525
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comment + Replace one more dictionary value with a IDL generated type. #Patch Set 3 : Reupload. #Patch Set 4 : Fix tests. #
Messages
Total messages: 21 (0 generated)
PTAL the CL? Thanks!
https://codereview.chromium.org/107813005/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/107813005/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:96: // Converts the job info to its JSON (Value) form. nit: Please update the comment. https://codereview.chromium.org/107813005/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:546: std::vector<linked_ptr<file_browser_private::FileTransferStatus> > How about ScopedVector? https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/6i8vKfmHnJQ
Thanks! https://codereview.chromium.org/107813005/diff/1/chrome/browser/chromeos/exte... File chrome/browser/chromeos/extensions/file_manager/event_router.cc (right): https://codereview.chromium.org/107813005/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:96: // Converts the job info to its JSON (Value) form. On 2013/12/25 01:19:56, mtomasz wrote: > nit: Please update the comment. Done. https://codereview.chromium.org/107813005/diff/1/chrome/browser/chromeos/exte... chrome/browser/chromeos/extensions/file_manager/event_router.cc:546: std::vector<linked_ptr<file_browser_private::FileTransferStatus> > On 2013/12/25 01:19:56, mtomasz wrote: > How about ScopedVector? > > https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/6i8vKfm... Because IDL generated methods takes std::vector<linked_ptr<file_browser_private::FileTransferStatus> >, we cannot use ScopedVector here.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/107813005/60001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
@benwells PTAL event_names.cc? Thanks!
On 2013/12/25 03:21:27, hirono wrote: > @benwells PTAL event_names.cc? Thanks! Why does the global event names file need to know about these? Ideally core extensions code knows nothing about specific apis.
On 2014/01/06 00:57:23, benwells wrote: > On 2013/12/25 03:21:27, hirono wrote: > > @benwells PTAL event_names.cc? Thanks! > > Why does the global event names file need to know about these? Ideally core > extensions code knows nothing about specific apis. The global event names file included Files.app's event names that is not used other than Files.app code. We can remove them to make the global file more independent on the specific apis.
On 2014/01/06 02:30:45, hirono wrote: > On 2014/01/06 00:57:23, benwells wrote: > > On 2013/12/25 03:21:27, hirono wrote: > > > @benwells PTAL event_names.cc? Thanks! > > > > Why does the global event names file need to know about these? Ideally core > > extensions code knows nothing about specific apis. > > The global event names file included Files.app's event names that is not used > other than Files.app code. > We can remove them to make the global file more independent on the specific > apis. Yeah, I think that would be a good idea. Then you also won't need my lg tm for this change :)
On 2014/01/07 00:16:44, benwells wrote: > On 2014/01/06 02:30:45, hirono wrote: > > On 2014/01/06 00:57:23, benwells wrote: > > > On 2013/12/25 03:21:27, hirono wrote: > > > > @benwells PTAL event_names.cc? Thanks! > > > > > > Why does the global event names file need to know about these? Ideally core > > > extensions code knows nothing about specific apis. > > > > The global event names file included Files.app's event names that is not used > > other than Files.app code. > > We can remove them to make the global file more independent on the specific > > apis. > > Yeah, I think that would be a good idea. Then you also won't need my lg tm for > this change :) Oh, I misunderstood it. Thanks!
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/107813005/240001
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/107813005/450001
Failed to get patchset properties (patchset not found?)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/107813005/240001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/107813005/460003
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/107813005/460003
Message was sent while issue was closed.
Change committed as 243525 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
