Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25533005:
Eliminate usage of content url constants in Autofill core code. (Closed)
|
Created:
7 years, 2 months ago by blundell Modified:
7 years, 2 months ago CC:
chromium-reviews, benquan, browser-components-watch_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionEliminate usage of content url constants in Autofill core code.
Instead, hardcode the HTTPS scheme (as is done in e.g. //net).
BUG=303006
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228325
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add reference to bug #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/25533005/diff/1/components/autofill/core/brow... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/25533005/diff/1/components/autofill/core/brow... components/autofill/core/browser/autofill_manager.cc:128: return form.source_url().SchemeIs("https"); Hmm, it seems like a shame to lose the named constant for this -- raw strings attract typos. Perhaps the constant should be defined in //net/?
+jam@ I had raised this question with jam@ a while back, and his advice was to simply hardcode the string. I'm fine either way.
On 2013/10/01 21:22:31, blundell wrote: > +jam@ > > I had raised this question with jam@ a while back, and his advice was to simply > hardcode the string. I'm fine either way. John, why does it make sense to hardcode the string in components that might be shared with iOS, but use a constant elsewhere? In fact, I'd expect that we'll eventually to share the majority of the strings in url_constants with iOS code. Do you recommend hardcoding all of them within the shared code?
On 2013/10/01 21:37:46, Ilya Sherman wrote: > On 2013/10/01 21:22:31, blundell wrote: > > +jam@ > > > > I had raised this question with jam@ a while back, and his advice was to > simply > > hardcode the string. I'm fine either way. > > John, why does it make sense to hardcode the string in components that might be > shared with iOS, but use a constant elsewhere? In fact, I'd expect that we'll > eventually to share the majority of the strings in url_constants with iOS code. > Do you recommend hardcoding all of them within the shared code? https://code.google.com/p/chromium/codesearch#search/&q=%22%5C%22https%5C%22%... has 170 hits for "https" so i think this is a common pattern already..
On 2013/10/01 23:38:43, jam wrote: > On 2013/10/01 21:37:46, Ilya Sherman wrote: > > On 2013/10/01 21:22:31, blundell wrote: > > > +jam@ > > > > > > I had raised this question with jam@ a while back, and his advice was to > > simply > > > hardcode the string. I'm fine either way. > > > > John, why does it make sense to hardcode the string in components that might > be > > shared with iOS, but use a constant elsewhere? In fact, I'd expect that we'll > > eventually to share the majority of the strings in url_constants with iOS > code. > > Do you recommend hardcoding all of them within the shared code? > > https://code.google.com/p/chromium/codesearch#search/&q=%2522%255C%2522https%... > has 170 hits for "https" so i think this is a common pattern already.. I appreciate that the current status quo is not ideal. That's a pretty weak argument for making things worse, though. Many of the hits for raw uses of "https" are in the chrome/ or content/ layers, where the named constant is visible. And there are only 53 hits for the named constant. Would you support removing the named constant entirely? Let's think beyond just "https". What about kJavaScriptScheme, kMailToScheme, kViewSourceScheme? Would you recommend hardcoding all of those as well?
On 2013/10/01 23:49:59, Ilya Sherman wrote: > On 2013/10/01 23:38:43, jam wrote: > > On 2013/10/01 21:37:46, Ilya Sherman wrote: > > > On 2013/10/01 21:22:31, blundell wrote: > > > > +jam@ > > > > > > > > I had raised this question with jam@ a while back, and his advice was to > > > simply > > > > hardcode the string. I'm fine either way. > > > > > > John, why does it make sense to hardcode the string in components that might > > be > > > shared with iOS, but use a constant elsewhere? In fact, I'd expect that > we'll > > > eventually to share the majority of the strings in url_constants with iOS > > code. > > > Do you recommend hardcoding all of them within the shared code? > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%252522%25255C%252522... > > has 170 hits for "https" so i think this is a common pattern already.. > > I appreciate that the current status quo is not ideal. That's a pretty weak > argument for making things worse, though. Many of the hits for raw uses of > "https" are in the chrome/ or content/ layers, where the named constant is > visible. And there are only 53 hits for the named constant. Would you support > removing the named constant entirely? > > Let's think beyond just "https". What about kJavaScriptScheme, kMailToScheme, > kViewSourceScheme? Would you recommend hardcoding all of those as well? i honestly don't care that much about this topic, it seems very minor compared to all the other issues we have. someone could put this constant in net and change all the code to use it.
On 2013/10/02 21:07:20, jam wrote: > On 2013/10/01 23:49:59, Ilya Sherman wrote: > > On 2013/10/01 23:38:43, jam wrote: > > > On 2013/10/01 21:37:46, Ilya Sherman wrote: > > > > On 2013/10/01 21:22:31, blundell wrote: > > > > > +jam@ > > > > > > > > > > I had raised this question with jam@ a while back, and his advice was to > > > > simply > > > > > hardcode the string. I'm fine either way. > > > > > > > > John, why does it make sense to hardcode the string in components that > might > > > be > > > > shared with iOS, but use a constant elsewhere? In fact, I'd expect that > > we'll > > > > eventually to share the majority of the strings in url_constants with iOS > > > code. > > > > Do you recommend hardcoding all of them within the shared code? > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%25252522%2525255C%25... > > > has 170 hits for "https" so i think this is a common pattern already.. > > > > I appreciate that the current status quo is not ideal. That's a pretty weak > > argument for making things worse, though. Many of the hits for raw uses of > > "https" are in the chrome/ or content/ layers, where the named constant is > > visible. And there are only 53 hits for the named constant. Would you > support > > removing the named constant entirely? > > > > Let's think beyond just "https". What about kJavaScriptScheme, kMailToScheme, > > kViewSourceScheme? Would you recommend hardcoding all of those as well? > > i honestly don't care that much about this topic, it seems very minor compared > to all the other issues we have. someone could put this constant in net and > change all the code to use it. Ilya, would you be OK with me filing a bug for this task and landing this CL as-is at the current time?
On 2013/10/04 18:56:08, blundell wrote: > On 2013/10/02 21:07:20, jam wrote: > > On 2013/10/01 23:49:59, Ilya Sherman wrote: > > > On 2013/10/01 23:38:43, jam wrote: > > > > On 2013/10/01 21:37:46, Ilya Sherman wrote: > > > > > On 2013/10/01 21:22:31, blundell wrote: > > > > > > +jam@ > > > > > > > > > > > > I had raised this question with jam@ a while back, and his advice was > to > > > > > simply > > > > > > hardcode the string. I'm fine either way. > > > > > > > > > > John, why does it make sense to hardcode the string in components that > > might > > > > be > > > > > shared with iOS, but use a constant elsewhere? In fact, I'd expect that > > > we'll > > > > > eventually to share the majority of the strings in url_constants with > iOS > > > > code. > > > > > Do you recommend hardcoding all of them within the shared code? > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%2525252522%252525255... > > > > has 170 hits for "https" so i think this is a common pattern already.. > > > > > > I appreciate that the current status quo is not ideal. That's a pretty weak > > > argument for making things worse, though. Many of the hits for raw uses of > > > "https" are in the chrome/ or content/ layers, where the named constant is > > > visible. And there are only 53 hits for the named constant. Would you > > support > > > removing the named constant entirely? > > > > > > Let's think beyond just "https". What about kJavaScriptScheme, > kMailToScheme, > > > kViewSourceScheme? Would you recommend hardcoding all of those as well? > > > > i honestly don't care that much about this topic, it seems very minor compared > > to all the other issues we have. someone could put this constant in net and > > change all the code to use it. > > Ilya, would you be OK with me filing a bug for this task and landing this CL > as-is at the current time? Sure, LGTM.
On 2013/10/07 21:36:54, Ilya Sherman wrote: > On 2013/10/04 18:56:08, blundell wrote: > > On 2013/10/02 21:07:20, jam wrote: > > > On 2013/10/01 23:49:59, Ilya Sherman wrote: > > > > On 2013/10/01 23:38:43, jam wrote: > > > > > On 2013/10/01 21:37:46, Ilya Sherman wrote: > > > > > > On 2013/10/01 21:22:31, blundell wrote: > > > > > > > +jam@ > > > > > > > > > > > > > > I had raised this question with jam@ a while back, and his advice > was > > to > > > > > > simply > > > > > > > hardcode the string. I'm fine either way. > > > > > > > > > > > > John, why does it make sense to hardcode the string in components that > > > might > > > > > be > > > > > > shared with iOS, but use a constant elsewhere? In fact, I'd expect > that > > > > we'll > > > > > > eventually to share the majority of the strings in url_constants with > > iOS > > > > > code. > > > > > > Do you recommend hardcoding all of them within the shared code? > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#search/&q=%252525252522%2525252... > > > > > has 170 hits for "https" so i think this is a common pattern already.. > > > > > > > > I appreciate that the current status quo is not ideal. That's a pretty > weak > > > > argument for making things worse, though. Many of the hits for raw uses > of > > > > "https" are in the chrome/ or content/ layers, where the named constant is > > > > visible. And there are only 53 hits for the named constant. Would you > > > support > > > > removing the named constant entirely? > > > > > > > > Let's think beyond just "https". What about kJavaScriptScheme, > > kMailToScheme, > > > > kViewSourceScheme? Would you recommend hardcoding all of those as well? > > > > > > i honestly don't care that much about this topic, it seems very minor > compared > > > to all the other issues we have. someone could put this constant in net and > > > change all the code to use it. > > > > Ilya, would you be OK with me filing a bug for this task and landing this CL > > as-is at the current time? > > Sure, LGTM. Added bug reference.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/25533005/14001
Step "update" is always a major failure. Look at the try server FAQ for more details. 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/blundell@chromium.org/25533005/14001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/25533005/14001
Message was sent while issue was closed.
Change committed as 228325 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
