Record first install time for an extension. [chromium/src : main]
Anunoy Ghosh (Gerrit)
Attention is currently required from: Daniel Rubery, Devlin Cronin.
Anunoy Ghosh would like Daniel Rubery and Devlin Cronin to review this change.
Record first install time for an extension.
- Add a new extension pref, "first_install_time", that records the
time when an extension is installed.
- Replace the existing extension pref, "install_time", with
"last_update_time". The new name better reflects the timestamp
recorded.
Bug: 1381132
Change-Id: I69179f84a77afc308159b90a448705cb49385e05
---
M chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
M chrome/browser/extensions/extension_prefs_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_unittest.cc
M chrome/browser/safe_browsing/incident_reporting/extension_data_collection_unittest.cc
M extensions/browser/extension_prefs.cc
M extensions/browser/extension_prefs.h
6 files changed, 183 insertions(+), 27 deletions(-)
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Anunoy Ghosh (Gerrit)
Attention is currently required from: Daniel Rubery, Devlin Cronin.
1 comment:
Patchset:
Hi Devlin & Dan,
Thanks in advance for your time! The crbug has a link to the DD for additional context.Dan:
Feel free to review only the files you are OWNER for :-)Devlin:
I found some Preference json files (see [1]) in chrome/test/data/extensions that have the legacy "install_time" pref encoded. The CQ dry run ran fine so I don't think I need to change them but then again maybe we should for completeness. WDYT?[1] - https://source.chromium.org/search?q=%5C%22install_time%5C%22%20%20-f:%2Fgen%2F&start=1
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Anunoy Ghosh (Gerrit)
Attention is currently required from: Daniel Rubery, Devlin Cronin.
Anunoy Ghosh uploaded patch set #6 to this change.
Record first install time for an extension.
- Add a new extension pref, "first_install_time", that records the
time when an extension is installed.
- Replace the existing extension pref, "install_time", with
"last_update_time". The new name better reflects the timestamp
recorded.
A subsequent CL will rename ExtensionPrefs::GetInstallTime to GetLastUpdateTime.
Bug: 1381132
Change-Id: I69179f84a77afc308159b90a448705cb49385e05
---
M chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
M chrome/browser/extensions/extension_prefs_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_unittest.cc
M chrome/browser/safe_browsing/incident_reporting/extension_data_collection_unittest.cc
M extensions/browser/extension_prefs.cc
M extensions/browser/extension_prefs.h
6 files changed, 185 insertions(+), 27 deletions(-)
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Daniel Rubery (Gerrit)
Attention is currently required from: Anunoy Ghosh, Devlin Cronin.
Patch set 6:Code-Review +1
1 comment:
Patchset:
safe_browsing LGTM
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Anunoy Ghosh.
9 comments:
Patchset:
Thanks, Anunoy! A couple comments here, but overall this is looking pretty good : )
File chrome/browser/extensions/extension_prefs_unittest.cc:
EXPECT_NE(base::Time(), first_install_time);
EXPECT_NE(base::Time(), install_time);
EXPECT_NE(first_install_time, install_time);Can we cache the first_install_time from the first check and verify that it's unchanged in response to the update?
Patch Set #6, Line 726:
std::string first_install_time, last_update_time, old_install_time;nit: chromium style usually puts variables on separate lines, even if they're the same type
File extensions/browser/extension_prefs.cc:
Patch Set #6, Line 1649:
install_time.ToInternalValue()Is this an intentional change in the way we store these times? If so, why?
Patch Set #6, Line 185:
// TODO(anunoy): DEPRECATED! Remove after M113. Use kPrefLastUpdateTime instead.Comments are great, but variable names are event better : ) let's rename this to `kPrefDeprecatedInstallTime`.
Patch Set #6, Line 185:
Use kPrefLastUpdateTime insteadnitty nit: put the guidance for what to use outside of the TODO (otherwise, it sounds like the TODO is to use the new pref, but we already do that)
Patch Set #6, Line 1777:
const std::string& pref_key) const {nit: prefer base::StringPiece or const char* to avoid unnecessarily constructing a string here
Patch Set #6, Line 2560:
ext_dict->SetString(kPrefLastUpdateTime, install_time_string);Related to question above about changing how we store the time prefs, this just copies the old value, so it doesn't convert it to instead be using the different (time since epoch) version.
Patch Set #6, Line 2564:
if (!ext_dict->HasKey(kPrefFirstInstallTime)) {Can this ever evaluate to false? When would an extension have the old pref (indicating it hasn't migrated) but also have the new pref (indicating it has)?
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Anunoy Ghosh (Gerrit)
Attention is currently required from: Devlin Cronin.
9 comments:
Patchset:
Hi Devlin,
Thanks for your comments. I have addressed most of them and replied with a couple of follow-on questions.
I had also asked about the following in the first draft of the CL and wanted to get your take on it:
I found some Preference json files (see [1]) in chrome/test/data/extensions that have the legacy "install_time" pref encoded. The CQ dry run ran fine so I don't think I need to change them. WDYT?
File chrome/browser/extensions/extension_prefs_unittest.cc:
EXPECT_NE(base::Time(), first_install_time);
EXPECT_NE(base::Time(), install_time);
EXPECT_NE(first_install_time, install_time);
Can we cache the first_install_time from the first check and verify that it's unchanged in response […]
Good idea. Done.
Patch Set #6, Line 726:
std::string first_install_time, last_update_time, old_install_time;nit: chromium style usually puts variables on separate lines, even if they're the same type
Done
File extensions/browser/extension_prefs.cc:
Patch Set #6, Line 1649:
install_time.ToInternalValue()Is this an intentional change in the way we store these times? If so, why?
Use of To/FromInternalValue is deprecated [1] and generates pre-CL upload warnings. I replaced them as instructed. I believe the actual value stored remains the same as the existing code [2].
However, I am now reconsidering this and leaning towards just continuing to use the old To/FromIntervalValue for consistent usage in this file. Let me know what you think.
Patch Set #6, Line 185:
Use kPrefLastUpdateTime instead
nitty nit: put the guidance for what to use outside of the TODO (otherwise, it sounds like the TODO […]
Done
Patch Set #6, Line 185:
// TODO(anunoy): DEPRECATED! Remove after M113. Use kPrefLastUpdateTime instead.
Comments are great, but variable names are event better : ) let's rename this to `kPrefDeprecatedIns […]
Done
Patch Set #6, Line 1777:
const std::string& pref_key) const {nit: prefer base::StringPiece or const char* to avoid unnecessarily constructing a string here
Good catch, thanks! Done.
Patch Set #6, Line 2560:
ext_dict->SetString(kPrefLastUpdateTime, install_time_string);
Related to question above about changing how we store the time prefs, this just copies the old value […]
I think the new and old storage values are calculated the same way. See my response to the related comment above.
Patch Set #6, Line 2564:
if (!ext_dict->HasKey(kPrefFirstInstallTime)) {
Can this ever evaluate to false? When would an extension have the old pref (indicating it hasn't m […]
You are correct. The pref key existence check is unnecessary and I have removed it.
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Anunoy Ghosh.
5 comments:
Patchset:
Thanks, Anunoy! Only remaining question is about whether we can move the update to how we serialize times to a followup, which I think would make this CL a bit more "obviously safe"
File chrome/browser/extensions/extension_prefs_unittest.cc:
Patch Set #7, Line 678:
auto first_install_time = prefs()->GetFirstInstallTime(extension_->id());nit: we can just use `first_install_time_` here; no need for a second variable
File extensions/browser/extension_prefs.cc:
Patch Set #6, Line 1649:
install_time.ToInternalValue()
Use of To/FromInternalValue is deprecated [1] and generates pre-CL upload warnings. […]
I think I'd prefer moving those changes to a separate CL. If, as we suspect, the implementation is the same (and so the value stored in prefs is unaffected), it shouldn't matter if we do it in a followup (and we can tag in a //base owner on that followup CL to confirm).
File extensions/browser/extension_prefs.cc:
Patch Set #7, Line 187:
// Use kPrefLastUpdateTime instead.nit: Move above the variable (or delete)
Patch Set #7, Line 2364:
if (!extension_dict->HasKey(kPrefFirstInstallTime))nit: it might be worth a 1-line comment about this (indicating we don't overwrite an existing value in order to preserve the first-install nature)
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Anunoy Ghosh (Gerrit)
Attention is currently required from: Devlin Cronin.
7 comments:
Commit Message:
Patch Set #7, Line 1:
Parent: bbbc2ae2 (OOBE - Polymer3 - Check-in oobe-text-button)@rdevlin...@chromium.org:
I found some Preference json files (see [1]) in chrome/test/data/extensions that have the legacy "install_time" pref encoded. I don't think they are used (I tracked one down in the code) and the CQ dry-run ran fine. Should I change them anyway or leave them be? WDYT?
Patchset:
Thanks again for your comments Devlin!
I have addressed them. I did have one outstanding question for you that I have left as a separate comment.
File chrome/browser/extensions/extension_prefs_unittest.cc:
Patch Set #7, Line 678:
auto first_install_time = prefs()->GetFirstInstallTime(extension_->id());nit: we can just use `first_install_time_` here; no need for a second variable
Done
File extensions/browser/extension_prefs.cc:
Patch Set #6, Line 1649:
install_time.ToInternalValue()
I think I'd prefer moving those changes to a separate CL. […]
Sounds good. I've reverted to using To/FromInternalValue.
Patch Set #6, Line 2560:
ext_dict->SetString(kPrefLastUpdateTime, install_time_string);
I think the new and old storage values are calculated the same way. […]
Done
File extensions/browser/extension_prefs.cc:
Patch Set #7, Line 187:
// Use kPrefLastUpdateTime instead.nit: Move above the variable (or delete)
Done
Patch Set #7, Line 2364:
if (!extension_dict->HasKey(kPrefFirstInstallTime))
nit: it might be worth a 1-line comment about this (indicating we don't overwrite an existing value […]
Done
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Anunoy Ghosh.
Patch set 8:Code-Review +1
1 comment:
Patchset:
Thanks, Anunoy! LGTM!
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Anunoy Ghosh.
2 comments:
Commit Message:
Patch Set #7, Line 1:
Parent: bbbc2ae2 (OOBE - Polymer3 - Check-in oobe-text-button)
@rdevlin...@chromium.org: […]
Ah, those. I'd say don't worry about them (as long as the CQ is happy). That was a very old practice where we'd generate extensions from an old profile state, but that's really not wise, since the profile state is now very stale. I don't think it'd make sense to update these, since the rest of it also isn't updated. (My suspicion is that, at some point, we'll need to get rid of those entirely in favor of a programmatically-set-up version in the test. But that's obviously very unrelated to this CL.)
Patchset:
Whoops, missed the outstanding question
To view, visit change 4002889. To unsubscribe, or for help writing mail filters, visit settings.
Anunoy Ghosh (Gerrit)
Attention is currently required from: Anunoy Ghosh.
Patch set 8:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Record first install time for an extension.
- Add a new extension pref, "first_install_time", that records the
time when an extension is installed.
- Replace the existing extension pref, "install_time", with
"last_update_time". The new name better reflects the timestamp
recorded.
A subsequent CL will rename ExtensionPrefs::GetInstallTime to GetLastUpdateTime.
Bug: 1381132
Change-Id: I69179f84a77afc308159b90a448705cb49385e05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4002889
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Reviewed-by: Daniel Rubery <dru...@chromium.org>
Commit-Queue: Anunoy Ghosh <anu...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1074598}
---
M chrome/browser/extensions/extension_message_bubble_controller_unittest.cc
M chrome/browser/extensions/extension_prefs_unittest.cc
M chrome/browser/safe_browsing/extension_telemetry/extension_telemetry_service_unittest.cc
M chrome/browser/safe_browsing/incident_reporting/extension_data_collection_unittest.cc
M extensions/browser/extension_prefs.cc
M extensions/browser/extension_prefs.h
6 files changed, 197 insertions(+), 28 deletions(-)