Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2006023002:
Log some information about the state of the homepage on Android. (Closed)
DescriptionLog some information about the state of the homepage on Android.
BUG=612653
Committed: https://crrev.com/42da7e24e22f88b02bd4dc64af9206b6ab40d84c
Cr-Commit-Position: refs/heads/master@{#397046}
Patch Set 1 #Patch Set 2 : Do not log homepage stats natively as it does not apply to Android. #
Total comments: 10
Patch Set 3 : Move to share the same logic with desktop #
Total comments: 6
Patch Set 4 : Update comments #
Total comments: 4
Patch Set 5 : Nits #
Total comments: 8
Patch Set 6 : comment nits #Patch Set 7 : moar nits #Patch Set 8 : Rebase #Patch Set 9 : Fix debug clang error about unused function (ifdef for android) #Patch Set 10 : JNI doesn't support null strings :-/ #
Messages
Total messages: 41 (13 generated)
tedchoc@chromium.org changed reviewers: + mpearson@chromium.org
PTAL I was originally going to share the logic in PrefMetricsService directly, but it seemed a "bit" odd. Mainly because the homepage state on Android is not stored as part of Chrome prefs at all and it seemed a bit misleading. If I had to though, I guess I could pull out a public method on PrefMetricsService called RecordHomepagePrefs that took the boolean and url values for the homepage bits (on all other platforms this would be called from RecordLaunchPrefs but not on android). Then Android would call it probably from the entry point I added in my change here but passing the values as stored in android prefs. Wasn't convinced it was worth it, but don't feel to strongly here. Thoughts?
On 2016/05/23 21:34:15, Ted C wrote: > PTAL > > I was originally going to share the logic in PrefMetricsService directly, but it > seemed a "bit" odd. > > Mainly because the homepage state on Android is not stored as part of Chrome > prefs at all and it seemed a bit misleading. > > If I had to though, I guess I could pull out a public method on > PrefMetricsService called RecordHomepagePrefs that took the boolean and url > values for the homepage bits (on all other platforms this would be called from > RecordLaunchPrefs but not on android). Then Android would call it probably from > the entry point I added in my change here but passing the values as stored in > android prefs. Wasn't convinced it was worth it, but don't feel to strongly > here. The way I see it, refactoring doesn't get you cleanliness benefit here. The benefit you get from piping through the values is support for the logging of the type of URLs for non-NTP home pages. Whether this is worthwhile depends in my mind on two things: - how often do users have non-NTP home pages? Is this a decent fraction of our install base? If so, we should try to understand them better. - is there an increasing risk of malware / UwS on Android changing people's settings? If so, we should have logging in place beforehand to understand the problem if it grows. You are a better judge to answer these questions than me. --mark
https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:114: // provider must be contacted before we can fully resolve this. Also, we do not store please rewrite this sentence without "we"--it's unclear who "we" refers to here (Android? Chrome? Chrome for Android code?) https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:118: PartnerBrowserCustomizations.setOnInitializeAsyncFinished(new Runnable() { Will this be logged on startup (well technically profile open) like the regular Chrome logging? i.e, the same frequency / trigger? https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:124: "Settings.ShowHomeButton", isHomepageEnabled); Does show home button really mean the same thing as home page is enabled?! I would think (without researching the code) the latter has to do with starting up with a home page... https://codereview.chromium.org/2006023002/diff/20001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/20001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:112: SampleNewTabPageURL(profile_); Is this appropriate for Android? (chrome::kChromeUINewTabURL) https://codereview.chromium.org/2006023002/diff/20001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:114: int restore_on_startup = prefs_->GetInteger(prefs::kRestoreOnStartup); And this whole block here for that matter?
On 2016/05/24 04:31:19, Mark P wrote: > On 2016/05/23 21:34:15, Ted C wrote: > > PTAL > > > > I was originally going to share the logic in PrefMetricsService directly, but > it > > seemed a "bit" odd. > > > > Mainly because the homepage state on Android is not stored as part of Chrome > > prefs at all and it seemed a bit misleading. > > > > If I had to though, I guess I could pull out a public method on > > PrefMetricsService called RecordHomepagePrefs that took the boolean and url > > values for the homepage bits (on all other platforms this would be called from > > RecordLaunchPrefs but not on android). Then Android would call it probably > from > > the entry point I added in my change here but passing the values as stored in > > android prefs. Wasn't convinced it was worth it, but don't feel to strongly > > here. > > The way I see it, refactoring doesn't get you cleanliness benefit here. The > benefit > you get from piping through the values is support for the logging of the type > of URLs for non-NTP home pages. If we go down this route, does the existing logic belong in PrefMetricService? We would be calling it using non chrome pref data. Does that matter? Certainly easier to keep it in its current position and just document the discrepancy between desktop and Android. Whether this is worthwhile depends in my > mind on two things: > - how often do users have non-NTP home pages? Is this a decent fraction of > our install base? If so, we should try to understand them better. Honestly, we have very little visibility. I would think more are non-ntp than not (on Android you can't enable the homepage unless an OEM/carrier forces it). > - is there an increasing risk of malware / UwS on Android changing people's > settings? If so, we should have logging in place beforehand to understand > the problem if it grows. This requires root on Android, which should be uncommon (but not unheard of). > > You are a better judge to answer these questions than me. > > --mark
Moved to sharing the same logic and calling from java to native. https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java (right): https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:114: // provider must be contacted before we can fully resolve this. Also, we do not store On 2016/05/24 04:31:26, Mark P wrote: > please rewrite this sentence without "we"--it's unclear who "we" refers to here > (Android? Chrome? Chrome for Android code?) removed this comment entirely as it is documented in native https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:118: PartnerBrowserCustomizations.setOnInitializeAsyncFinished(new Runnable() { On 2016/05/24 04:31:26, Mark P wrote: > Will this be logged on startup (well technically profile open) like the regular > Chrome logging? i.e, the same frequency / trigger? It will be logged once on startup. No multi-profile support. The main this this location ensures is that it will not be overlogged since we trigger new Chrome activities fairly often for different use cases. This will be called at most a single time for each browser process start. https://codereview.chromium.org/2006023002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/DeferredStartupHandler.java:124: "Settings.ShowHomeButton", isHomepageEnabled); On 2016/05/24 04:31:26, Mark P wrote: > Does show home button really mean the same thing as home page is enabled?! > I would think (without researching the code) the latter has to do with starting > up with a home page... Nah, isHomepageEnabled is what we use for button state. It means there is an external provider on the device and the user has not turned it off in preferences (the preferences is only exposed if there is an external provider). https://codereview.chromium.org/2006023002/diff/20001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/20001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:112: SampleNewTabPageURL(profile_); On 2016/05/24 04:31:26, Mark P wrote: > Is this appropriate for Android? (chrome::kChromeUINewTabURL) We don't allow overriding the new tab URL on Android. https://codereview.chromium.org/2006023002/diff/20001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:114: int restore_on_startup = prefs_->GetInteger(prefs::kRestoreOnStartup); On 2016/05/24 04:31:26, Mark P wrote: > And this whole block here for that matter? We also don't use desktop restore either, but I'm not entirely sure what this following block does. To me, it looks like neither of these apply on Android.
generally lgtm baring comments below https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:112: #if !defined(OS_ANDROID) Please comment here why we skip it on Android (by restating the reason that you put in the .h file and also referencing where the code in Android that calls RecordHomePageLaunchMetrics is). https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:119: SampleNewTabPageURL(profile_); Given your answers to the comments on the earlier patchset, I think you should ifdef this out (and comment why). ditto for ifdefing out the startup settings (and comment) and while you're here, please comment why the pinned tabs is ifdefed out on Android. I assume // Android does not support pinned tabs. https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:31: // homepage is determined by external providers on the device, so this will "determined by external providers on the device" is vague. Perhaps, something like "set by third parties is stored in XYZ (outside of Chrome)"
tedchoc@chromium.org changed reviewers: + gab@chromium.org
+gab for OWNERS of chrome/browser/prefs/ https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:112: #if !defined(OS_ANDROID) On 2016/05/24 18:41:53, Mark P wrote: > Please comment here why we skip it on Android (by restating the reason that you > put in the .h file and also referencing where the code in Android that calls > RecordHomePageLaunchMetrics is). Done. https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:119: SampleNewTabPageURL(profile_); On 2016/05/24 18:41:53, Mark P wrote: > Given your answers to the comments on the earlier patchset, I think you should > ifdef this out (and comment why). > > ditto for ifdefing out the startup settings (and comment) > > and while you're here, please comment why the pinned tabs is ifdefed out on > Android. I assume > // Android does not support pinned tabs. Done. https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/40001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:31: // homepage is determined by external providers on the device, so this will On 2016/05/24 18:41:53, Mark P wrote: > "determined by external providers on the device" > is vague. > Perhaps, something like "set by third parties is stored in XYZ (outside of > Chrome)" Done.
additional nits, still lgtm https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:114: // it will be logged later once all the dependent information is available. nit: point readers to the call site for this other call. https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:34: // this functionality is exposed to allow it to be called once all the nit: extra space
https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:114: // it will be logged later once all the dependent information is available. On 2016/05/24 19:07:17, Mark P wrote: > nit: point readers to the call site for this other call. Done https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/60001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:34: // this functionality is exposed to allow it to be called once all the On 2016/05/24 19:07:17, Mark P wrote: > nit: extra space Done.
c/b/prefs review https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:111: void PrefMetricsService::RecordLaunchPrefs() { How about #if !defined(OS_ANDROID) on this entire method (with a meta-comment explaining why each subsection either doesn't apply to Android or is done later on Android)? Seems weird to have many ifdef blocks that amount to an empty body on Android. https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:35: // necessary information is available. This comment should state what this method does, not how it's called. The caller that omits OS_ANDROID is a good place to document how it's done differently on Android (ref. other comment about changing if-def semantics).
drive-by reviewer commenting on another reviewer's comments :-) --mark https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:111: void PrefMetricsService::RecordLaunchPrefs() { On 2016/05/25 14:03:24, gab wrote: > How about #if !defined(OS_ANDROID) on this entire method (with a meta-comment > explaining why each subsection either doesn't apply to Android or is done later > on Android)? Seems weird to have many ifdef blocks that amount to an empty body > on Android. I know it looks weird, but (i) these are independent decisions so I think they should look like independent decisions to people reading the code and (ii) when someone comes to the code to glance at metrics recording, they usually only look in the near vicinity, not, say, 50 lines up to see if there's an ifdef covering something larger than what should be the single conceptual unit they're looking at. I like this current structure. https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:35: // necessary information is available. On 2016/05/25 14:03:24, gab wrote: > This comment should state what this method does, not how it's called. > > The caller that omits OS_ANDROID is a good place to document how it's done > differently on Android (ref. other comment about changing if-def semantics). I agree with gab@ on this one; you can move everything but the first sentence of this comment into the implementation.
https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.cc:111: void PrefMetricsService::RecordLaunchPrefs() { On 2016/05/25 17:01:53, Mark P wrote: > On 2016/05/25 14:03:24, gab wrote: > > How about #if !defined(OS_ANDROID) on this entire method (with a meta-comment > > explaining why each subsection either doesn't apply to Android or is done > later > > on Android)? Seems weird to have many ifdef blocks that amount to an empty > body > > on Android. > > I know it looks weird, but (i) these are independent decisions so I think they > should look like independent decisions to people reading the code and (ii) when > someone comes to the code to glance at metrics recording, they usually only look > in the near vicinity, not, say, 50 lines up to see if there's an ifdef covering > something larger than what should be the single conceptual unit they're looking > at. I like this current structure. Ok, works for me then.
https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:35: // necessary information is available. On 2016/05/25 17:01:53, Mark P wrote: > On 2016/05/25 14:03:24, gab wrote: > > This comment should state what this method does, not how it's called. > > > > The caller that omits OS_ANDROID is a good place to document how it's done > > differently on Android (ref. other comment about changing if-def semantics). > > I agree with gab@ on this one; you can move everything but the first sentence of > this comment into the implementation. I personally think the comment about this being triggered during initialization on all ports except android is relevant in the header. Then readers will understand quickly whether this is something they should be worrying about (w/o having to look into the code). I agree reasoning why android is different is not important here and should be/is already in the code. But if you still disagree strongly then I'll remove it as I don't want that to be the thing that holds this up.
https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:35: // necessary information is available. On 2016/05/25 18:13:07, Ted C wrote: > On 2016/05/25 17:01:53, Mark P wrote: > > On 2016/05/25 14:03:24, gab wrote: > > > This comment should state what this method does, not how it's called. > > > > > > The caller that omits OS_ANDROID is a good place to document how it's done > > > differently on Android (ref. other comment about changing if-def semantics). > > > > I agree with gab@ on this one; you can move everything but the first sentence > of > > this comment into the implementation. > > I personally think the comment about this being triggered during initialization > on all ports except android is relevant in the header. Then readers will > understand quickly whether this is something they should be worrying about (w/o > having to look into the code). I agree reasoning why android is different is > not important here and should be/is already in the code. > > But if you still disagree strongly then I'll remove it as I don't want that to > be the thing that holds this up. If you want to mention where it's triggered, that mention should be on the constructor of PrefMetricsService (where the trigger is). Also, if you're going to state that it's not triggered on Android, keeping the comment of what else is done on Android alongside makes sense too I'd say?
https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_metrics_service.h (right): https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_metrics_service.h:35: // necessary information is available. On 2016/05/25 18:23:40, gab wrote: > On 2016/05/25 18:13:07, Ted C wrote: > > On 2016/05/25 17:01:53, Mark P wrote: > > > On 2016/05/25 14:03:24, gab wrote: > > > > This comment should state what this method does, not how it's called. > > > > > > > > The caller that omits OS_ANDROID is a good place to document how it's done > > > > differently on Android (ref. other comment about changing if-def > semantics). > > > > > > I agree with gab@ on this one; you can move everything but the first > sentence > > of > > > this comment into the implementation. > > > > I personally think the comment about this being triggered during > initialization > > on all ports except android is relevant in the header. Then readers will > > understand quickly whether this is something they should be worrying about > (w/o > > having to look into the code). I agree reasoning why android is different is > > not important here and should be/is already in the code. > > > > But if you still disagree strongly then I'll remove it as I don't want that to > > be the thing that holds this up. > > If you want to mention where it's triggered, that mention should be on the > constructor of PrefMetricsService (where the trigger is). > > Also, if you're going to state that it's not triggered on Android, keeping the > comment of what else is done on Android alongside makes sense too I'd say? I can't say I agree, but I'll just remove it entirely.
Well in practice this is a method called by a method called by the constructor except on Android where the second method is called but no-ops. Feels weird to document that flow on the header of this method. I agree with you it's easier to read if documented, just not here? Le mer. 25 mai 2016 14:28, <tedchoc@chromium.org> a écrit : > > > https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... > File chrome/browser/prefs/pref_metrics_service.h (right): > > > https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... > chrome/browser/prefs/pref_metrics_service.h:35: // necessary information > is available. > On 2016/05/25 18:23:40, gab wrote: > > On 2016/05/25 18:13:07, Ted C wrote: > > > On 2016/05/25 17:01:53, Mark P wrote: > > > > On 2016/05/25 14:03:24, gab wrote: > > > > > This comment should state what this method does, not how it's > called. > > > > > > > > > > The caller that omits OS_ANDROID is a good place to document how > it's done > > > > > differently on Android (ref. other comment about changing if-def > > semantics). > > > > > > > > I agree with gab@ on this one; you can move everything but the > first > > sentence > > > of > > > > this comment into the implementation. > > > > > > I personally think the comment about this being triggered during > > initialization > > > on all ports except android is relevant in the header. Then readers > will > > > understand quickly whether this is something they should be worrying > about > > (w/o > > > having to look into the code). I agree reasoning why android is > different is > > > not important here and should be/is already in the code. > > > > > > But if you still disagree strongly then I'll remove it as I don't > want that to > > > be the thing that holds this up. > > > > If you want to mention where it's triggered, that mention should be on > the > > constructor of PrefMetricsService (where the trigger is). > > > > Also, if you're going to state that it's not triggered on Android, > keeping the > > comment of what else is done on Android alongside makes sense too I'd > say? > > I can't say I agree, but I'll just remove it entirely. > > https://codereview.chromium.org/2006023002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/05/25 19:58:30, chromium-reviews wrote: > Well in practice this is a method called by a method called by the > constructor except on Android where the second method is called but no-ops. > Feels weird to document that flow on the header of this method. I agree > with you it's easier to read if documented, just not here? Again, I have to disagree. If I'm looking at a method and determining whether I should call it, I won't look everywhere else in the class header to look for any references to it. If it's not all in a single place, I see no positive benefit in it. But again, this is your code. Tell me where to put it and I'll put it there as I don't think this is important to this change and I want to land this sooner than later. > > Le mer. 25 mai 2016 14:28, <mailto:tedchoc@chromium.org> a écrit : > > > > > > > > https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... > > File chrome/browser/prefs/pref_metrics_service.h (right): > > > > > > > https://codereview.chromium.org/2006023002/diff/80001/chrome/browser/prefs/pr... > > chrome/browser/prefs/pref_metrics_service.h:35: // necessary information > > is available. > > On 2016/05/25 18:23:40, gab wrote: > > > On 2016/05/25 18:13:07, Ted C wrote: > > > > On 2016/05/25 17:01:53, Mark P wrote: > > > > > On 2016/05/25 14:03:24, gab wrote: > > > > > > This comment should state what this method does, not how it's > > called. > > > > > > > > > > > > The caller that omits OS_ANDROID is a good place to document how > > it's done > > > > > > differently on Android (ref. other comment about changing if-def > > > semantics). > > > > > > > > > > I agree with gab@ on this one; you can move everything but the > > first > > > sentence > > > > of > > > > > this comment into the implementation. > > > > > > > > I personally think the comment about this being triggered during > > > initialization > > > > on all ports except android is relevant in the header. Then readers > > will > > > > understand quickly whether this is something they should be worrying > > about > > > (w/o > > > > having to look into the code). I agree reasoning why android is > > different is > > > > not important here and should be/is already in the code. > > > > > > > > But if you still disagree strongly then I'll remove it as I don't > > want that to > > > > be the thing that holds this up. > > > > > > If you want to mention where it's triggered, that mention should be on > > the > > > constructor of PrefMetricsService (where the trigger is). > > > > > > Also, if you're going to state that it's not triggered on Android, > > keeping the > > > comment of what else is done on Android alongside makes sense too I'd > > say? > > > > I can't say I agree, but I'll just remove it entirely. > > > > https://codereview.chromium.org/2006023002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2016/05/25 22:14:04, Ted C (OOO till 5.31) wrote: > On 2016/05/25 19:58:30, chromium-reviews wrote: > > Well in practice this is a method called by a method called by the > > constructor except on Android where the second method is called but no-ops. > > Feels weird to document that flow on the header of this method. I agree > > with you it's easier to read if documented, just not here? > > Again, I have to disagree. If I'm looking at a method and determining > whether I should call it, I won't look everywhere else in the class header > to look for any references to it. If it's not all in a single place, I see > no positive benefit in it. If you're determining whether you should all it means you have the data required to call it (which you don't on Android so can't make that mistake). If you're wondering who calls it, you look at callers. Callers have the comment so that looks fine to me. lgtm, thanks > But again, this is your code. Tell me where to put it and I'll put it > there as I don't think this is important to this change and I want to > land this sooner than later.
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mpearson@chromium.org, gab@chromium.org Link to the patchset: https://codereview.chromium.org/2006023002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006023002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006023002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2006023002/#ps160001 (title: "Fix debug clang error about unused function (ifdef for android)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006023002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006023002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tedchoc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from gab@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2006023002/#ps180001 (title: "JNI doesn't support null strings :-/")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006023002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006023002/180001
On 2016/06/01 00:04:21, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) Sorry for the churn all. Switched to release recently and that didn't have the clang error or the DCHECK I was hitting. Built and ran it in Debug so hopefully this is the last one.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by tedchoc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2006023002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2006023002/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Log some information about the state of the homepage on Android. BUG=612653 ========== to ========== Log some information about the state of the homepage on Android. BUG=612653 Committed: https://crrev.com/42da7e24e22f88b02bd4dc64af9206b6ab40d84c Cr-Commit-Position: refs/heads/master@{#397046} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/42da7e24e22f88b02bd4dc64af9206b6ab40d84c Cr-Commit-Position: refs/heads/master@{#397046} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
