|
|
Chromium Code Reviews|
Created:
7 years, 11 months ago by achuithb Modified:
7 years, 10 months ago Reviewers:
satorux1 CC:
chromium-reviews, achuith+watch_chromium.org, oshima+watch_chromium.org, stevenjb+watch_chromium.org, nkostylev+watch_chromium.org, davemoore+watch_chromium.org, tfarina Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionUse DriveResourceMetadataInterface in unit tests.
BUG=165844
TEST=unit tests pass.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179448
Patch Set 1 #Patch Set 2 : fix unit test #
Total comments: 3
Patch Set 3 : rebase #
Messages
Total messages: 17 (0 generated)
All usage of DriveResourceMetadata needs to switch to DriveResourceMetadataInterface.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/12049085/1
One of the unit tests was failing - the fix is not trivial so could you please take another look, Satoru-san.
https://codereview.chromium.org/12049085/diff/3004/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right): https://codereview.chromium.org/12049085/diff/3004/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:314: base_names.push_back(entries->at(i).base_name()); nit: std::vector::at is informally banned. use entries[i] instead
https://codereview.chromium.org/12049085/diff/3004/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right): https://codereview.chromium.org/12049085/diff/3004/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:314: base_names.push_back(entries->at(i).base_name()); On 2013/01/26 17:46:10, tfarina wrote: > nit: std::vector::at is informally banned. > > use entries[i] instead Can't use entries[i] since entries is a pointer; I'd have to do (*entries)[i], which doesn't seem like much of an improvement. Do you have a pointer to the discussion where at() was informally banned? What was the rationale?
LGTM https://codereview.chromium.org/12049085/diff/3004/chrome/browser/chromeos/dr... File chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc (right): https://codereview.chromium.org/12049085/diff/3004/chrome/browser/chromeos/dr... chrome/browser/chromeos/drive/drive_resource_metadata_unittest.cc:314: base_names.push_back(entries->at(i).base_name()); On 2013/01/26 20:39:14, achuith.bhandarkar wrote: > On 2013/01/26 17:46:10, tfarina wrote: > > nit: std::vector::at is informally banned. > > > > use entries[i] instead > > Can't use entries[i] since entries is a pointer; I'd have to do (*entries)[i], > which doesn't seem like much of an improvement. Do you have a pointer to the > discussion where at() was informally banned? What was the rationale? ->at looks just fine to me. I guess there is no need to add yet another style rule on this minor matter.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/12049085/3004
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/12049085/3004
Sorry for I got bad news for ya. Compile failed with a clobber build on mac. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/12049085/3004
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/12049085/3004
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/12049085/17006
Message was sent while issue was closed.
Change committed as 179448 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
