Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10880025:
[Android] Fix handling of asset files loading. (Closed)
|
Created:
8 years, 3 months ago by mnaganov (inactive) Modified:
8 years, 3 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Android] Fix handling of asset files loading.
This is needed after https://chromiumcodereview.appspot.com/10700117.
BUG=144266
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=154102
Patch Set 1 #
Total comments: 9
Patch Set 2 : Comments addressed #
Total comments: 14
Patch Set 3 : Comments addressed #
Total comments: 6
Patch Set 4 : Comments addressed #
Total comments: 5
Messages
Total messages: 16 (0 generated)
Sami, I think you wrote the original code, please take a look. Joth, need owner's LGTM
https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.cc:15: #include "chrome/common/url_constants.h" prefer to be removing chrome/ deps from here rather than adding new ones. see comment below. https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.cc:117: return false; out of scope for this CL, but this should never fail now -- could add a TODO to remove it https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.cc:132: ProfileManager::GetDefaultProfile()->GetRequestContext(); could you have the context_getter passed in as a param? This will make it much easier when we come to move this to android_webview. (as per go/clank-webview-components) https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.h:12: class AndroidProtocolAdapter { this class really needs some docs. I have already forgotten again which features it supports! https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.h:14: static net::URLRequest::ProtocolFactory Factory; wow. I've never seen this syntax for declaring a function via a fn-pointer typedef before. Learn something every day.
https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.cc:15: #include "chrome/common/url_constants.h" On 2012/08/23 18:23:28, joth wrote: > prefer to be removing chrome/ deps from here rather than adding new ones. see > comment below. Done. https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.cc:117: return false; On 2012/08/23 18:23:28, joth wrote: > out of scope for this CL, but this should never fail now -- could add a TODO to > remove it Removed. https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.cc:132: ProfileManager::GetDefaultProfile()->GetRequestContext(); On 2012/08/23 18:23:28, joth wrote: > could you have the context_getter passed in as a param? This will make it much > easier when we come to move this to android_webview. (as per > go/clank-webview-components) > > Done. https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10880025/diff/1/chrome/browser/android... chrome/browser/android/android_protocol_adapter.h:12: class AndroidProtocolAdapter { On 2012/08/23 18:23:28, joth wrote: > this class really needs some docs. I have already forgotten again which features > it supports! Added a description.
https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:99: network_delegate, did you mean this? Looks like a compile error. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:114: JNIEnv* env, net::URLRequestContextGetter* context) { nit: context => context_getter. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:130: content::BrowserThread::PostTask( I think the modern style for this is: context_getter->GetNetworkTaskRunner()->PostTask(FROM_HERE, Bind(...)) (you can remove the #include browser_thread.h then) https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:133: base::Unretained(context))); I believe the Unretained() shouldn't be needed as URLRequestContextGetter is ref-counted, and indeed could cause use-after-free error. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:20: // - "content" scheme is used for accessing data from Android content Thanks, great docs. nit: "content:" scheme. (aids grepability) https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:24: // - "file" scheme extension for accessing application assets and resources, ditto, android_assets and android_resources. (right?)
https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:99: network_delegate, On 2012/08/24 17:28:28, joth wrote: > did you mean this? Looks like a compile error. The upstream android_stream_reader_url_request_job.cc still has 3 arguments. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:114: JNIEnv* env, net::URLRequestContextGetter* context) { On 2012/08/24 17:28:28, joth wrote: > nit: context => context_getter. Done. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:130: content::BrowserThread::PostTask( On 2012/08/24 17:28:28, joth wrote: > I think the modern style for this is: > > context_getter->GetNetworkTaskRunner()->PostTask(FROM_HERE, Bind(...)) > > (you can remove the #include browser_thread.h then) OK, changed. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:133: base::Unretained(context))); On 2012/08/24 17:28:28, joth wrote: > I believe the Unretained() shouldn't be needed as URLRequestContextGetter is > ref-counted, and indeed could cause use-after-free error. It is ref-counted, but the profile returns a raw pointer to it, so I will need to make proper wrapping into a reference. I don't think the getter will be freed during application initialization. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.h (right): https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:20: // - "content" scheme is used for accessing data from Android content On 2012/08/24 17:28:28, joth wrote: > Thanks, great docs. > > > nit: "content:" scheme. > > (aids grepability) OK. But, technically, the colon isn't a part of the scheme name. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:24: // - "file" scheme extension for accessing application assets and resources, On 2012/08/24 17:28:28, joth wrote: > ditto, android_assets and android_resources. (right?) Done.
https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:99: network_delegate, On 2012/08/24 18:17:22, Mikhail Naganov (Chromium) wrote: > On 2012/08/24 17:28:28, joth wrote: > > did you mean this? Looks like a compile error. > > The upstream android_stream_reader_url_request_job.cc still has 3 arguments. OK you're passing |network_delegate| as param 2, but where is that variable declared? It used to be passed in as a param to this AndroidProtocolAdapter::Factory() function (old linke 78) but you have deleted that. https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:133: base::Unretained(context))); On 2012/08/24 18:17:22, Mikhail Naganov (Chromium) wrote: > On 2012/08/24 17:28:28, joth wrote: > > I believe the Unretained() shouldn't be needed as URLRequestContextGetter is > > ref-counted, and indeed could cause use-after-free error. > > It is ref-counted, but the profile returns a raw pointer to it, so I will need > to make proper wrapping into a reference. I don't think the getter will be freed > during application initialization. make_scoped_refptr(foo) seems no more onerous than base::Unretained(foo), and avoids me having to stop and think will go out of scope (although, I agree, during startup it's extremely unlikely too). (and obv change AddFileSchemeInterceptorOnIOThread to accept scoped_refptr<Foo> )
https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:73: virtual net::URLRequestJob* MaybeIntercept(net::URLRequest* request) const OVERRIDE; Line length? https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:115: InitJNIBindings(env); Maybe add a DCHECK for the result here? Otherwise it's easy to miss that some class wasn't found and the rest of the code starts failing mysteriously. https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.h (left): https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:16: // Register the protocol factories for all supported Android protocol Could update this comment to say "protocol factories and interceptors".
On 2012/08/24 23:22:26, joth wrote: > https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... > File chrome/browser/android/android_protocol_adapter.cc (right): > > https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... > chrome/browser/android/android_protocol_adapter.cc:99: network_delegate, > On 2012/08/24 18:17:22, Mikhail Naganov (Chromium) wrote: > > On 2012/08/24 17:28:28, joth wrote: > > > did you mean this? Looks like a compile error. > > > > The upstream android_stream_reader_url_request_job.cc still has 3 arguments. > > OK you're passing |network_delegate| as param 2, but where is that variable > declared? It used to be passed in as a param to this > AndroidProtocolAdapter::Factory() function (old linke 78) but you have deleted > that. > Sorry, I misunderstood your first comment. There is a refactoring happening in Chromium, and this parameter is back as of r153133, so everything should look valid now. > https://chromiumcodereview.appspot.com/10880025/diff/5001/chrome/browser/andr... > chrome/browser/android/android_protocol_adapter.cc:133: > base::Unretained(context))); > On 2012/08/24 18:17:22, Mikhail Naganov (Chromium) wrote: > > On 2012/08/24 17:28:28, joth wrote: > > > I believe the Unretained() shouldn't be needed as URLRequestContextGetter is > > > ref-counted, and indeed could cause use-after-free error. > > > > It is ref-counted, but the profile returns a raw pointer to it, so I will need > > to make proper wrapping into a reference. I don't think the getter will be > freed > > during application initialization. > > > make_scoped_refptr(foo) seems no more onerous than base::Unretained(foo), and > avoids me having to stop and think will go out of scope (although, I agree, > during startup it's extremely unlikely too). > OK. But this makes me to start mentally rolling all AddRefs and Releases :) > (and obv change AddFileSchemeInterceptorOnIOThread to accept scoped_refptr<Foo> > ) Seems to be unneeded according to this: http://dev.chromium.org/developers/design-documents/threading#Runnable_methods
https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:73: virtual net::URLRequestJob* MaybeIntercept(net::URLRequest* request) const OVERRIDE; On 2012/08/28 11:10:50, Sami wrote: > Line length? Done. https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.cc:115: InitJNIBindings(env); On 2012/08/28 11:10:50, Sami wrote: > Maybe add a DCHECK for the result here? Otherwise it's easy to miss that some > class wasn't found and the rest of the code starts failing mysteriously. Good idea. But wrapping the in a DCHECK will mean it will be wiped out in release builds, so changed to an 'if' with a NOTREACHED branch for the failure. https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... File chrome/browser/android/android_protocol_adapter.h (left): https://chromiumcodereview.appspot.com/10880025/diff/9001/chrome/browser/andr... chrome/browser/android/android_protocol_adapter.h:16: // Register the protocol factories for all supported Android protocol On 2012/08/28 11:10:50, Sami wrote: > Could update this comment to say "protocol factories and interceptors". This would reveal implementation details. Changed to a more generic term "handlers".
joth@ -- what do you think now? I would like to have this fix submitted, as having tests disabled isn't a way to go.
lgtm sorry thought I already sent this one yesterday... :/ just a couple nits. https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... chrome/browser/android/android_protocol_adapter.cc:249: kAssetPrefix(std::string(chrome::kFileScheme) + nit: initializer colon should be on this line https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... chrome/browser/android/android_protocol_adapter.cc:252: kResourcePrefix(std::string(chrome::kFileScheme) + nit: I think technically kConst is only for things constants that are statically defined at compile time. On balance I think these may as well be done that way too (i.e.: static const char[] kFoo = "file://android_asset"). while it means we 'duplicate' parts of the string literals on the flip side it makes it much easier to see (and verify) exactly what those constants contains, and in reality these aren't going to change a whole lot as they're in the public API spec.
https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... chrome/browser/android/android_protocol_adapter.cc:249: kAssetPrefix(std::string(chrome::kFileScheme) + On 2012/08/29 19:49:33, joth wrote: > nit: initializer colon should be on this line Done. https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... chrome/browser/android/android_protocol_adapter.cc:252: kResourcePrefix(std::string(chrome::kFileScheme) + On 2012/08/29 19:49:33, joth wrote: > nit: I think technically kConst is only for things constants that are statically > defined at compile time. > Agree. Changed to asset_prefix_ and resource_prefix_. > On balance I think these may as well be done that way too (i.e.: static const > char[] kFoo = "file://android_asset"). while it means we 'duplicate' parts of > the string literals on the flip side it makes it much easier to see (and verify) > exactly what those constants contains, and in reality these aren't going to > change a whole lot as they're in the public API spec. Well, URI scheme names also doesn't change, but we anyway have constants for them. One reason for using constants is that if you mistype the name of the constant, you'll get a compiler error. For clarity, I've added comments for these constants with their intended values.
new patch upload? (sgtm though from comments, so you can tbr if you want to land without another day passing) https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... File chrome/browser/android/android_protocol_adapter.cc (right): https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... chrome/browser/android/android_protocol_adapter.cc:252: kResourcePrefix(std::string(chrome::kFileScheme) + On 2012/08/30 09:24:38, Mikhail Naganov (Chromium) wrote: > On 2012/08/29 19:49:33, joth wrote: > > nit: I think technically kConst is only for things constants that are > statically > > defined at compile time. > > > > Agree. Changed to asset_prefix_ and resource_prefix_. > > > On balance I think these may as well be done that way too (i.e.: static const > > char[] kFoo = "file://android_asset"). while it means we 'duplicate' parts of > > the string literals on the flip side it makes it much easier to see (and > verify) > > exactly what those constants contains, and in reality these aren't going to > > change a whole lot as they're in the public API spec. > > Well, URI scheme names also doesn't change, but we anyway have constants for > them. One reason for using constants is that if you mistype the name of the > constant, you'll get a compiler error. For clarity, I've added comments for > these constants with their intended values. Ah I wasn't meaning to suggest removing constants, just change them to be true compile-time constants, e.g.: namespace { const char[] kAndroidAssetPrefix = "file://android_asset"; ... }
On 2012/08/30 18:10:40, joth wrote: > new patch upload? > I have committed it already after fixing your last comments. There is a link to the committed revision: https://src.chromium.org/viewvc/chrome?view=rev&revision=154102 > (sgtm though from comments, so you can tbr if you want to land without another > day passing) > > https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... > File chrome/browser/android/android_protocol_adapter.cc (right): > > https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/and... > chrome/browser/android/android_protocol_adapter.cc:252: > kResourcePrefix(std::string(chrome::kFileScheme) + > On 2012/08/30 09:24:38, Mikhail Naganov (Chromium) wrote: > > On 2012/08/29 19:49:33, joth wrote: > > > nit: I think technically kConst is only for things constants that are > > statically > > > defined at compile time. > > > > > > > Agree. Changed to asset_prefix_ and resource_prefix_. > > > > > On balance I think these may as well be done that way too (i.e.: static > const > > > char[] kFoo = "file://android_asset"). while it means we 'duplicate' parts > of > > > the string literals on the flip side it makes it much easier to see (and > > verify) > > > exactly what those constants contains, and in reality these aren't going to > > > change a whole lot as they're in the public API spec. > > > > Well, URI scheme names also doesn't change, but we anyway have constants for > > them. One reason for using constants is that if you mistype the name of the > > constant, you'll get a compiler error. For clarity, I've added comments for > > these constants with their intended values. > > Ah I wasn't meaning to suggest removing constants, just change them to be true > compile-time constants, e.g.: > > namespace { > const char[] kAndroidAssetPrefix = "file://android_asset"; > ... > } That's what I was talking about -- make a typo in the constant value (BTW, it should be "file:///android_asset/", which is automagically derived from the result of other constants concatenation), and you are screwed. While if you make a typo in the constant name, the code will not compile.
On 30 August 2012 13:11, <mnaganov@chromium.org> wrote: > On 2012/08/30 18:10:40, joth wrote: > >> new patch upload? >> > > > I have committed it already after fixing your last comments. There is a > link to > the committed revision: > https://src.chromium.org/**viewvc/chrome?view=rev&**revision=154102<https://s... > > > Ha, of course. I'm too used to gerrit now where a new patchset is always uploaded... Thanks. > (sgtm though from comments, so you can tbr if you want to land without >> another >> day passing) >> > > > https://chromiumcodereview.**appspot.com/10880025/diff/** > 13001/chrome/browser/android/**android_protocol_adapter.cc<https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/android/android_protocol_adapter.cc> > >> File chrome/browser/android/**android_protocol_adapter.cc (right): >> > > > https://chromiumcodereview.**appspot.com/10880025/diff/** > 13001/chrome/browser/android/**android_protocol_adapter.cc#**newcode252<https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/android/android_protocol_adapter.cc#newcode252> > >> chrome/browser/android/**android_protocol_adapter.cc:**252: >> kResourcePrefix(std::string(**chrome::kFileScheme) + >> On 2012/08/30 09:24:38, Mikhail Naganov (Chromium) wrote: >> > On 2012/08/29 19:49:33, joth wrote: >> > > nit: I think technically kConst is only for things constants that are >> > statically >> > > defined at compile time. >> > > >> > >> > Agree. Changed to asset_prefix_ and resource_prefix_. >> > >> > > On balance I think these may as well be done that way too (i.e.: >> static >> const >> > > char[] kFoo = "file://android_asset"). while it means we 'duplicate' >> parts >> of >> > > the string literals on the flip side it makes it much easier to see >> (and >> > verify) >> > > exactly what those constants contains, and in reality these aren't >> going >> > to > >> > > change a whole lot as they're in the public API spec. >> > >> > Well, URI scheme names also doesn't change, but we anyway have >> constants for >> > them. One reason for using constants is that if you mistype the name of >> the >> > constant, you'll get a compiler error. For clarity, I've added comments >> for >> > these constants with their intended values. >> > > Ah I wasn't meaning to suggest removing constants, just change them to be >> true >> compile-time constants, e.g.: >> > > namespace { >> const char[] kAndroidAssetPrefix = "file://android_asset"; >> ... >> } >> > > That's what I was talking about -- make a typo in the constant value (BTW, > it > should be "file:///android_asset/", which is automagically derived from the > result of other constants concatenation), and you are screwed. While if > you make > a typo in the constant name, the code will not compile. > > Yes, that's true. (although many would argue, this is what we have tests for) [I just see the 10 temporary std::string instances created in the process of initializing 2 constants and may head spins :) ] > > https://chromiumcodereview.**appspot.com/10880025/<https://chromiumcodereview... >
On 30 August 2012 13:11, <mnaganov@chromium.org> wrote: > On 2012/08/30 18:10:40, joth wrote: > >> new patch upload? >> > > > I have committed it already after fixing your last comments. There is a > link to > the committed revision: > https://src.chromium.org/**viewvc/chrome?view=rev&**revision=154102<https://s... > > > Ha, of course. I'm too used to gerrit now where a new patchset is always uploaded... Thanks. > (sgtm though from comments, so you can tbr if you want to land without >> another >> day passing) >> > > > https://chromiumcodereview.**appspot.com/10880025/diff/** > 13001/chrome/browser/android/**android_protocol_adapter.cc<https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/android/android_protocol_adapter.cc> > >> File chrome/browser/android/**android_protocol_adapter.cc (right): >> > > > https://chromiumcodereview.**appspot.com/10880025/diff/** > 13001/chrome/browser/android/**android_protocol_adapter.cc#**newcode252<https://chromiumcodereview.appspot.com/10880025/diff/13001/chrome/browser/android/android_protocol_adapter.cc#newcode252> > >> chrome/browser/android/**android_protocol_adapter.cc:**252: >> kResourcePrefix(std::string(**chrome::kFileScheme) + >> On 2012/08/30 09:24:38, Mikhail Naganov (Chromium) wrote: >> > On 2012/08/29 19:49:33, joth wrote: >> > > nit: I think technically kConst is only for things constants that are >> > statically >> > > defined at compile time. >> > > >> > >> > Agree. Changed to asset_prefix_ and resource_prefix_. >> > >> > > On balance I think these may as well be done that way too (i.e.: >> static >> const >> > > char[] kFoo = "file://android_asset"). while it means we 'duplicate' >> parts >> of >> > > the string literals on the flip side it makes it much easier to see >> (and >> > verify) >> > > exactly what those constants contains, and in reality these aren't >> going >> > to > >> > > change a whole lot as they're in the public API spec. >> > >> > Well, URI scheme names also doesn't change, but we anyway have >> constants for >> > them. One reason for using constants is that if you mistype the name of >> the >> > constant, you'll get a compiler error. For clarity, I've added comments >> for >> > these constants with their intended values. >> > > Ah I wasn't meaning to suggest removing constants, just change them to be >> true >> compile-time constants, e.g.: >> > > namespace { >> const char[] kAndroidAssetPrefix = "file://android_asset"; >> ... >> } >> > > That's what I was talking about -- make a typo in the constant value (BTW, > it > should be "file:///android_asset/", which is automagically derived from the > result of other constants concatenation), and you are screwed. While if > you make > a typo in the constant name, the code will not compile. > > Yes, that's true. (although many would argue, this is what we have tests for) [I just see the 10 temporary std::string instances created in the process of initializing 2 constants and may head spins :) ] > > https://chromiumcodereview.**appspot.com/10880025/<https://chromiumcodereview... > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
