Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10332156:
Add ability to set RLZ statically for OS_ANDROID (Closed)
|
Created:
8 years, 7 months ago by Isaac (away) Modified:
8 years ago CC:
chromium-reviews, gone Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionAndroid phones have a native RLZ component that generates tags. Chrome for Android needs to query this component and set the tag, rather than using chrome's rlz library. This CL adds the ability to statically set an rlz that was retrieved externally.
BUG=157039
Patch Set 1 #Patch Set 2 : Splitting android functions in prep for client id upload #
Total comments: 2
Patch Set 3 : fixed indentation #
Total comments: 2
Patch Set 4 : Better commit message #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #
Total comments: 7
Messages
Total messages: 26 (0 generated)
LGTM https://chromiumcodereview.appspot.com/10332156/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url.cc:100: NOTREACHED(); Should be 2 space indent.
https://chromiumcodereview.appspot.com/10332156/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url.cc:100: NOTREACHED(); On 2012/05/15 00:16:59, jcivelli wrote: > Should be 2 space indent. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ilevy@chromium.org/10332156/8
Presubmit check for 10332156-8 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/browser/search_engines
Presubmit checks took 4.5s to calculate.
Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!
CL descriptions should describe why a change is done, not just what it does. Either expand the description of this CL, or (better) have a tracking bug for whatever it is you're trying to do and add that bug number to the BUG= line.
Doesn't Android define ENABLE_RLZ in addition to OS_ANDROID? It seems like it should. https://chromiumcodereview.appspot.com/10332156/diff/8/chrome/browser/search_... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/8/chrome/browser/search_... chrome/browser/search_engines/template_url.cc:99: default: Nit: Don't touch this please
ENABLE_RLZ is only set on Chrome builds on mac and windows. Android has different requirements. https://chromiumcodereview.appspot.com/10332156/diff/8/chrome/browser/search_... File chrome/browser/search_engines/template_url.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/8/chrome/browser/search_... chrome/browser/search_engines/template_url.cc:99: default: On 2012/05/15 17:34:16, Peter Kasting wrote: > Nit: Don't touch this please it was giving me a presubmit warning for two commands on a single line -- can I ignore it and still submit through CQ?
On 2012/05/15 19:44:41, Isaac wrote: > ENABLE_RLZ is only set on Chrome builds on mac and windows. Android has > different requirements. That's kind of vague. Should we make the meaning of the #define more explicit or split it into multiple pieces? I'm not a big fan of using "OS_ANDROID" as a proxy for "some RLZ-related behavior that partially overlaps other RLZ-related behavior we have #defines for, that happens to be what Android currently does". > https://chromiumcodereview.appspot.com/10332156/diff/8/chrome/browser/search_... > File chrome/browser/search_engines/template_url.cc (right): > > https://chromiumcodereview.appspot.com/10332156/diff/8/chrome/browser/search_... > chrome/browser/search_engines/template_url.cc:99: default: > On 2012/05/15 17:34:16, Peter Kasting wrote: > > Nit: Don't touch this please > > it was giving me a presubmit warning for two commands on a single line -- can I > ignore it and still submit through CQ? As long as you're not touching it I think you'll be fine.
I could make a ENABLE_STATIC_RLZ flag -- would you prefer that?
OK, after thinking about this more, a couple things: * Your code is not threadsafe. SearchTermsData functions may be called on any thread. Instead of overriding that class' GetRlzParameterValue() function, you should be overriding GetRlzParamterValue(). * We can do this without so many protecting #ifdefs, as follows: (1) Un-protect GetRlzParameterValue() in both classes. Move your setter to UIThreadSearchTermsData, name it SetRlzParameterValue(), and don't protect it. Make the implementation of GetRlzParameterValue() unconditionally return the static string you set if non-empty. Guard the remaining bits with ENABLE_RLZ so that if that define is not set the function just returns the unset empty |rlz_string|. (2) Unprotect the calling code in template_url.cc, which already does what we want for the default "empty string" case. (3) Unprotect the RLZ unittest. Convert the #elif you added to an #else. The result of all this will be fewer ifdefs overall, shorter code, and no OS_ANDROID at all.
Peter, I think I've made the changes you were looking for --w hat do you think? Cheers, Isaac
LGTM https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... File chrome/browser/search_engines/search_terms_data.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... chrome/browser/search_engines/search_terms_data.cc:98: if (!(g_static_rlz == NULL)) Nit: Use != https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... File chrome/browser/search_engines/search_terms_data.h (right): https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... chrome/browser/search_engines/search_terms_data.h:69: static void SetStaticRlz(const string16& rlz); Nit: Call this SetRlzParameterValue(). Write a comment explaining what it does. (While you're at it, write one for UIThreadSearchTermsData::GetRlzParameterValue() too, and maybe the other overrides.) https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... chrome/browser/search_engines/search_terms_data.h:70: private: Nit: Newline above https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... chrome/browser/search_engines/search_terms_data.h:72: static base::LazyInstance<string16>::Leaky g_static_rlz; Nit: Call this |rlz_parameter_value_|
https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... File chrome/browser/search_engines/search_terms_data.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... chrome/browser/search_engines/search_terms_data.cc:98: if (!(g_static_rlz == NULL)) On 2012/05/19 02:00:55, Peter Kasting wrote: > Nit: Use != I would love to but they haven't defined the != operator! I was thinking about submitting a patch for this in a separate CL.
https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... File chrome/browser/search_engines/search_terms_data.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... chrome/browser/search_engines/search_terms_data.cc:98: if (!(g_static_rlz == NULL)) On 2012/05/19 02:09:01, Isaac wrote: > On 2012/05/19 02:00:55, Peter Kasting wrote: > > Nit: Use != > > I would love to but they haven't defined the != operator! I was thinking about > submitting a patch for this in a separate CL. Actually this makes me realize there's a problem here -- I think setting this override to the empty string should cancel the override. That is, NULL and string16() should work the same way. I wonder if that gives us any opportunity to optimize this some, by just always initting the string, so we know it's never NULL... Anyway, adding operator!=() would be good :)
https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... File chrome/browser/search_engines/search_terms_data.cc (right): https://chromiumcodereview.appspot.com/10332156/diff/3009/chrome/browser/sear... chrome/browser/search_engines/search_terms_data.cc:98: if (!(g_static_rlz == NULL)) OK -- will make this and the other changes in a follow up CL. cheers On 2012/05/19 02:13:08, Peter Kasting wrote: > On 2012/05/19 02:09:01, Isaac wrote: > > On 2012/05/19 02:00:55, Peter Kasting wrote: > > > Nit: Use != > > > > I would love to but they haven't defined the != operator! I was thinking > about > > submitting a patch for this in a separate CL. > > Actually this makes me realize there's a problem here -- I think setting this > override to the empty string should cancel the override. That is, NULL and > string16() should work the same way. > > I wonder if that gives us any opportunity to optimize this some, by just always > initting the string, so we know it's never NULL... > > Anyway, adding operator!=() would be good :)
This CL was never closed. What happened here?
Haven't submitted this yet, got bogged down in infra stuff. Still needs to be completed.
On 2012/09/05 21:29:46, Isaac wrote: > Haven't submitted this yet, got bogged down in infra stuff. Still needs to be > completed. OK. Bonus points if you can manage to implement this by making some particular function to get the RLZ data be implemented distinctly on Android versus non-Android, a la a lot of the "browser API" stuff that's currently in progress elsewhere. That's cleaner than adding static variables and setters that non-Android ports don't call.
That seems reasonable. I'll see what I can do. This has to land for the M24 cut so I will be working on it next week.
Hi Peter, dfalcantara is going take over shepherding this feature. Are you still OK with a static setting as an initial patch? The RLZ can change due to user behavior, so the callback you suggested would either always need to callback (i.e. no cache) or else support some sort of cache invalidation. Could you clarify what you're looking for? Cheers, Isaac
> Hi Peter, > > dfalcantara is going take over shepherding this feature. Are you still OK with > a static setting as an initial patch? The RLZ can change due to user behavior, > so the callback you suggested would either always need to callback (i.e. no > cache) or else support some sort of cache invalidation. Could you clarify what > you're looking for? How does RLZ change in response to user behavior? Is this Android-specific? I didn't think any such changes happened on non-Android, and I'm kind of confused why we'd have some on Android either. I would assume the callback function would be implemented distinctly on different platforms and would be responsible for doing any necessary caching internally. The caller side could remain completely ignorant.
The RLZ changes when the user does their first search. That might be it ATM, but there are no guarantees that this behavior won't change in the future (i.e. the RLZ could update more frequently). I think we'll still want the rlz to be set from java land on android; otherwise we'd have to do a JNI call to fetch current RLZ each search.
On 2012/12/01 03:59:00, Isaac wrote: > I think we'll still want the rlz to be set from java land on android; otherwise > we'd have to do a JNI call to fetch current RLZ each search. You can do that behind the Android implementation, though. In other words, don't expose any of that in any cross-platform headers. Just have some generic GetRLZBlah() sort of function. Then for Android, implement that in an Android-specific file as something that reads a value that's set from the Java side, or whatever you want. The critical factor is, we want the simplest and least platform-specific possible API exposed to anyone outsider Android-land.
Abandoning in favor of Dan's https://chromiumcodereview.appspot.com/11448005 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
