Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(59)

Issue 279073003: Add a function triggering extension installed to ExtensionRegistryObserver (Closed)

Created:
6 years, 7 months ago by limasdf
Modified:
6 years, 7 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Add a function triggering extension installed to ExtensionRegistryObserver. This can be replace chrome::NOTIFICATION_EXTENSION_INSTALLED with ExtensionRegistryObserver. And apply it to WebStoreInstaller and InstallTracker. R=kalman@chromium.org BUG=354459 TEST=unit_test, manual(install app from webstore) Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272143

Patch Set 1 : #

Total comments: 9

Patch Set 2 : feedback #

Total comments: 13

Patch Set 3 : feedback2 #

Total comments: 8

Patch Set 4 : feedback3 #

Total comments: 1

Patch Set 5 : fix test after lgtm #

Total comments: 4

Patch Set 6 : #

Total comments: 6

Patch Set 7 : add more DCHECK and test code #

Unified diffs Side-by-side diffs Delta from patch set Stats (+138 lines, -63 lines) Patch
M chrome/browser/extensions/extension_service.cc View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/extensions/install_tracker.h View 1 2 2 chunks +6 lines, -2 lines 0 comments Download
M chrome/browser/extensions/install_tracker.cc View 1 2 3 chunks +22 lines, -23 lines 0 comments Download
M chrome/browser/extensions/webstore_installer.h View 1 2 5 chunks +16 lines, -2 lines 0 comments Download
M chrome/browser/extensions/webstore_installer.cc View 1 2 5 chunks +38 lines, -36 lines 0 comments Download
M extensions/browser/extension_registry.h View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download
M extensions/browser/extension_registry.cc View 1 2 3 4 5 6 1 chunk +12 lines, -0 lines 0 comments Download
M extensions/browser/extension_registry_observer.h View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
M extensions/browser/extension_registry_unittest.cc View 1 2 3 4 5 6 4 chunks +18 lines, -0 lines 0 comments Download

Messages

Total messages: 18 (0 generated)
limasdf
Please take a look.
6 years, 7 months ago (2014-05-14 15:36:44 UTC) #1
not at google - send to devlin
https://codereview.chromium.org/279073003/diff/170001/chrome/browser/extensions/install_tracker.h File chrome/browser/extensions/install_tracker.h (right): https://codereview.chromium.org/279073003/diff/170001/chrome/browser/extensions/install_tracker.h#newcode70 chrome/browser/extensions/install_tracker.h:70: InstalledExtensionInfo info) OVERRIDE; (note that this should be a ...
6 years, 7 months ago (2014-05-14 18:17:50 UTC) #2
limasdf
Sorry i"m late. PTAL. https://codereview.chromium.org/279073003/diff/170001/chrome/browser/extensions/install_tracker.h File chrome/browser/extensions/install_tracker.h (right): https://codereview.chromium.org/279073003/diff/170001/chrome/browser/extensions/install_tracker.h#newcode70 chrome/browser/extensions/install_tracker.h:70: InstalledExtensionInfo info) OVERRIDE; On 2014/05/14 ...
6 years, 7 months ago (2014-05-16 01:02:35 UTC) #3
not at google - send to devlin
more complicated than I thought. also remember to rename the INSTALLED/UNINSTALLED notifications to DEPRECATED when ...
6 years, 7 months ago (2014-05-16 13:49:03 UTC) #4
limasdf
Could you take a look? https://codereview.chromium.org/279073003/diff/210001/chrome/browser/extensions/extension_service.cc File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/279073003/diff/210001/chrome/browser/extensions/extension_service.cc#newcode88 chrome/browser/extensions/extension_service.cc:88: using extensions::InstalledExtensionInfo; On 2014/05/16 ...
6 years, 7 months ago (2014-05-16 20:39:15 UTC) #5
not at google - send to devlin
naming and comment nits now. lgtm, submit once resolved. https://codereview.chromium.org/279073003/diff/210001/chrome/browser/extensions/extension_service.cc File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/279073003/diff/210001/chrome/browser/extensions/extension_service.cc#newcode88 chrome/browser/extensions/extension_service.cc:88: ...
6 years, 7 months ago (2014-05-16 23:03:36 UTC) #6
limasdf
Thanks. After submit, I'll upload following CLs soon. 1. chrome::NOTIFICATION_EXTENSION_INSTALLED -> DEPRECATED postfix 2. Implement ...
6 years, 7 months ago (2014-05-17 06:16:18 UTC) #7
limasdf
The CQ bit was checked by limasdf@gmail.com
6 years, 7 months ago (2014-05-17 06:40:38 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/279073003/240001
6 years, 7 months ago (2014-05-17 06:41:47 UTC) #9
limasdf
Hm I removed DCHECK. PTAL. https://codereview.chromium.org/279073003/diff/240001/extensions/browser/extension_registry.cc File extensions/browser/extension_registry.cc (right): https://codereview.chromium.org/279073003/diff/240001/extensions/browser/extension_registry.cc#newcode59 extensions/browser/extension_registry.cc:59: DCHECK(!GenerateInstalledExtensionsSet()->Contains(extension->id())); I'm going to ...
6 years, 7 months ago (2014-05-17 13:15:10 UTC) #10
limasdf
hope you take a look my previous comment.
6 years, 7 months ago (2014-05-20 15:37:45 UTC) #11
not at google - send to devlin
https://codereview.chromium.org/279073003/diff/280001/extensions/browser/extension_registry.cc File extensions/browser/extension_registry.cc (right): https://codereview.chromium.org/279073003/diff/280001/extensions/browser/extension_registry.cc#newcode58 extensions/browser/extension_registry.cc:58: const std::string& old_name) { I think the DCHECK actually ...
6 years, 7 months ago (2014-05-20 18:27:12 UTC) #12
limasdf
Thanks for the review. I'll request last ptal. https://codereview.chromium.org/279073003/diff/280001/extensions/browser/extension_registry.cc File extensions/browser/extension_registry.cc (right): https://codereview.chromium.org/279073003/diff/280001/extensions/browser/extension_registry.cc#newcode58 extensions/browser/extension_registry.cc:58: const ...
6 years, 7 months ago (2014-05-21 15:35:07 UTC) #13
not at google - send to devlin
only 2 more changes! and lgtm after those. https://codereview.chromium.org/279073003/diff/300001/extensions/browser/extension_registry.cc File extensions/browser/extension_registry.cc (right): https://codereview.chromium.org/279073003/diff/300001/extensions/browser/extension_registry.cc#newcode60 extensions/browser/extension_registry.cc:60: GenerateInstalledExtensionsSet()->Contains(extension->id())); ...
6 years, 7 months ago (2014-05-21 16:33:36 UTC) #14
limasdf
yep gogogo. https://codereview.chromium.org/279073003/diff/300001/extensions/browser/extension_registry.cc File extensions/browser/extension_registry.cc (right): https://codereview.chromium.org/279073003/diff/300001/extensions/browser/extension_registry.cc#newcode60 extensions/browser/extension_registry.cc:60: GenerateInstalledExtensionsSet()->Contains(extension->id())); On 2014/05/21 16:33:36, kalman wrote: > ...
6 years, 7 months ago (2014-05-21 21:40:46 UTC) #15
limasdf
The CQ bit was checked by limasdf@gmail.com
6 years, 7 months ago (2014-05-21 21:40:55 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/limasdf@gmail.com/279073003/320001
6 years, 7 months ago (2014-05-21 21:42:51 UTC) #17
commit-bot: I haz the power
6 years, 7 months ago (2014-05-22 08:45:20 UTC) #18
Message was sent while issue was closed.
Change committed as 272143

Powered by Google App Engine
This is Rietveld 408576698