Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(125)

Issue 14942008: Clean up ifdef around FilePath creation (Closed)

Created:
7 years, 7 months ago by mrunal
Modified:
7 years, 6 months ago
CC:
chromium-reviews, asanka, cbentzel+watch_chromium.org, feature-media-reviews_chromium.org, benjhayden+dwatch_chromium.org, tfarina, jam, joi+watch-content_chromium.org, Aaron Boodman, darin-cc_chromium.org, tzik+watch_chromium.org, chromium-apps-reviews_chromium.org, kinuko+watch, jochen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Clean up ifdef around FilePath creation BUG=102853 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203292

Patch Set 1 #

Total comments: 5

Patch Set 2 : Clean up ifdef around FilePath creation #

Total comments: 4

Patch Set 3 : Clean up ifdef around FilePath creation #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -52 lines) Patch
M chrome/browser/extensions/extension_protocols.cc View 1 chunk +1 line, -5 lines 0 comments Download
M chrome/browser/importer/firefox3_importer.cc View 1 1 chunk +1 line, -5 lines 0 comments Download
M chrome/browser/net/url_fixer_upper_unittest.cc View 1 chunk +1 line, -5 lines 0 comments Download
M chrome/browser/ui/webui/fileicon_source.cc View 1 1 chunk +2 lines, -9 lines 0 comments Download
M chrome/common/extensions/extension.cc View 1 1 chunk +1 line, -5 lines 0 comments Download
M chrome/common/extensions/extension_file_util.cc View 1 chunk +1 line, -9 lines 0 comments Download
M chrome/common/extensions/extension_file_util_unittest.cc View 1 chunk +2 lines, -7 lines 0 comments Download
M chrome/common/logging_chrome.cc View 1 2 1 chunk +2 lines, -7 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
mrunal
Can someone take a look please?
7 years, 7 months ago (2013-05-14 18:59:50 UTC) #1
benjhayden
Asanka, PTAL at download_prefs.cc. I thought that avoided AsUTF8Unsafe() for some reason?
7 years, 7 months ago (2013-05-14 19:02:09 UTC) #2
asanka
https://chromiumcodereview.appspot.com/14942008/diff/1/chrome/browser/download/download_prefs.cc File chrome/browser/download/download_prefs.cc (right): https://chromiumcodereview.appspot.com/14942008/diff/1/chrome/browser/download/download_prefs.cc#newcode69 chrome/browser/download/download_prefs.cc:69: base::FilePath path = base::FilePath::FromUTF8Unsafe(extensions[i]); On POSIX, the extension isn't ...
7 years, 7 months ago (2013-05-14 19:16:11 UTC) #3
mrunal
https://chromiumcodereview.appspot.com/14942008/diff/1/chrome/browser/download/download_prefs.cc File chrome/browser/download/download_prefs.cc (right): https://chromiumcodereview.appspot.com/14942008/diff/1/chrome/browser/download/download_prefs.cc#newcode69 chrome/browser/download/download_prefs.cc:69: base::FilePath path = base::FilePath::FromUTF8Unsafe(extensions[i]); Ok, I will remove the ...
7 years, 7 months ago (2013-05-15 01:38:58 UTC) #4
kinuko
It might work better to split this CL and ask each module owners to take ...
7 years, 7 months ago (2013-05-15 02:29:23 UTC) #5
mrunal
Hi Kinuko, I've split this patch into three patches including this one. Remaining two can ...
7 years, 7 months ago (2013-05-16 02:26:09 UTC) #6
mrunal
ping! Anyone for review?
7 years, 7 months ago (2013-05-21 01:12:58 UTC) #7
kinuko
These changes still span multiple modules (chrome/ consists of a large number of modules and ...
7 years, 7 months ago (2013-05-21 03:30:21 UTC) #8
mrunal
Thanks Kinuko. I will split this patch into even smaller patches.
7 years, 7 months ago (2013-05-21 04:52:31 UTC) #9
wtc
Patch set 2 LGTM. I reviewed the entire CL, but you need to get other ...
7 years, 7 months ago (2013-05-21 17:59:10 UTC) #10
mrunal
https://codereview.chromium.org/14942008/diff/11001/chrome/browser/net/url_fixer_upper_unittest.cc File chrome/browser/net/url_fixer_upper_unittest.cc (right): https://codereview.chromium.org/14942008/diff/11001/chrome/browser/net/url_fixer_upper_unittest.cc#newcode248 chrome/browser/net/url_fixer_upper_unittest.cc:248: struct fixup_case { Is it ok if I submit ...
7 years, 6 months ago (2013-05-28 23:03:12 UTC) #11
mrunal
As per wtc's comments in logging_chrome.cc I have removed curly braces. As per kinuko's suggestion ...
7 years, 6 months ago (2013-05-28 23:14:58 UTC) #12
wtc
On 2013/05/28 23:14:58, mrunal wrote: > > As per kinuko's suggestion should I still split ...
7 years, 6 months ago (2013-05-28 23:54:29 UTC) #13
mrunal
On 2013/05/28 23:54:29, wtc wrote: > However, it is important to have someone check your ...
7 years, 6 months ago (2013-05-29 23:58:03 UTC) #14
James Hawkins
webui/ lgtm
7 years, 6 months ago (2013-05-30 00:04:07 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/14942008/22001
7 years, 6 months ago (2013-05-30 00:12:35 UTC) #16
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 6 months ago (2013-05-30 01:24:55 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mrunal.kapade@intel.com/14942008/22001
7 years, 6 months ago (2013-05-30 23:57:32 UTC) #18
commit-bot: I haz the power
7 years, 6 months ago (2013-05-31 01:15:18 UTC) #19
Message was sent while issue was closed.
Change committed as 203292

Powered by Google App Engine
This is Rietveld 408576698