Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9315007:
Make the Chrome Web Store Icon Syncable (Closed)
|
Created:
8 years, 10 months ago by csharp Modified:
8 years, 10 months ago CC:
chromium-reviews, Aaron Boodman, mihaip+watch_chromium.org Visibility:
Public. |
DescriptionMake the Chrome Web Store Icon Syncable
Mark the CWS as a syncable app so that it page and app launch ordinals are synced.
BUG=112290
TEST=Users should be able to move the Chrome Web store icon and have its positions update to other signed in locations.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=120432
Patch Set 1 #
Total comments: 4
Patch Set 2 : Adding DCHECK instead of NOTREACHED #
Total comments: 5
Patch Set 3 : Adding check to ensure we don't download CWS #
Total comments: 8
Patch Set 4 : Responding to comments #
Total comments: 2
Patch Set 5 : Fixing Spelling Mistake #
Total comments: 4
Patch Set 6 : responding to comments #Patch Set 7 : Changing DCHECK_EQ to DCHECK_STREQ #Patch Set 8 : Casting char* to string #
Messages
Total messages: 28 (0 generated)
I'm not sure if this is the best way to sync the CWS, but I didn't see any better method and this way does appear to work correctly. I'm not sure what I could do to add a test here. Any thoughts?
this will cause every browser instance to try to download the chrome web store app from the gallery. will this work and is this what we want?
https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (left): https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1587: NOTREACHED(); Why is this needed?
I'm not completely sure if this is exactly what we want, but in my testing this does seems to work. The CWS is a Extension::component so every browser instance already has it, so I don't think they it will cause any downloads, but instead will just add the CWS to the list of syncable apps (from what I can tell looking at and playing around with the code). Another option would be to not sync it as an app, but just add the page and app launch ordinal as individual sync values. This would means that later on, when the app options are synced as well (disable notifications and setting how it is opened), they would also have to be added as individual values to sync. https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (left): https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1587: NOTREACHED(); On 2012/02/01 09:18:57, Finnur wrote: > Why is this needed? I took this out because when when the sync data for the CWS is read, it calls this function, which reaches the NOTREACHED (since the CWS is a component), which causes a crash in debug.
I'll leave the validity of the sync issue to akalin (not sure whether this approach is right/wrong) but I have one comment on the NOTREACHED... https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (left): https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1587: NOTREACHED(); Then why not DCHECK if... extension_id != extension_misc::kWebStoreAppId ... instead?
https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (left): https://chromiumcodereview.appspot.com/9315007/diff/1/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1587: NOTREACHED(); On 2012/02/01 15:09:10, Finnur wrote: > Then why not DCHECK if... > extension_id != extension_misc::kWebStoreAppId > ... instead? I'd wanted to avoid making too many special cases for the WebStoreApp, but that is probably a good idea to idea that check (since the webstoreapp is already such a special case).
On 2012/02/01 14:48:22, csharp wrote: > The CWS is a Extension::component so every browser instance already has it, so I > don't think they it will cause any downloads, but instead will just add the CWS > to the list of syncable apps (from what I can tell looking at and playing around > with the code). Ah, you're right I think. Just the same, I'd like an explicit check somewhere that makes sure we don't try to download this. (PendingExtension...) classes Also I guess this is okay for just one special case, but if there are any other similar cases, we should probably do this more systematically.
http://codereview.chromium.org/9315007/diff/6001/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/9315007/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.cc:1588: DCHECK(extension_id == extension_misc::kWebStoreAppId); DCHECK_EQ http://codereview.chromium.org/9315007/diff/6001/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.cc:1588: DCHECK(extension_id == extension_misc::kWebStoreAppId); if id == web store id, perhaps dcheck that enabled == false? http://codereview.chromium.org/9315007/diff/6001/chrome/common/extensions/ext... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9315007/diff/6001/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2974: // The CWS needs to be treated as synable app. synable -> syncable also comment as to why
> Also I guess this is okay for just one special case, but if there are any other > similar cases, we should probably do this more systematically. Sounds good to me, is there anywhere in the code I should put a note about that?
https://chromiumcodereview.appspot.com/9315007/diff/6001/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/6001/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:1588: DCHECK(extension_id == extension_misc::kWebStoreAppId); On 2012/02/02 01:17:46, akalin wrote: > DCHECK_EQ DCHECK_EQ added. I think the other dcheck should be that enabled == IsIncognitoEnabled. Basically just making sure that we aren't trying to modify the value. (By default the CWS app has enabled=true) https://chromiumcodereview.appspot.com/9315007/diff/6001/chrome/common/extens... File chrome/common/extensions/extension.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/6001/chrome/common/extens... chrome/common/extensions/extension.cc:2974: // The CWS needs to be treated as synable app. On 2012/02/02 01:17:46, akalin wrote: > synable -> syncable > > also comment as to why Done.
Few more comments http://codereview.chromium.org/9315007/diff/6002/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_service.cc (right): http://codereview.chromium.org/9315007/diff/6002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.cc:1588: DCHECK_EQ(extension_misc::kWebStoreAppId, extension_id); flip argument order (unlike EXPECT_*, should match the natural arg order) http://codereview.chromium.org/9315007/diff/6002/chrome/browser/extensions/ex... chrome/browser/extensions/extension_service.cc:1592: if (extension_id == extension_misc::kWebStoreAppId) i think this if statement is unnecessary; if you're here, you passed the previous dcheck http://codereview.chromium.org/9315007/diff/6002/chrome/browser/extensions/pe... File chrome/browser/extensions/pending_extension_manager.cc (right): http://codereview.chromium.org/9315007/diff/6002/chrome/browser/extensions/pe... chrome/browser/extensions/pending_extension_manager.cc:67: if (extension_misc::kWebStoreAppId == id) { flip argument order http://codereview.chromium.org/9315007/diff/6002/chrome/common/extensions/ext... File chrome/common/extensions/extension.cc (right): http://codereview.chromium.org/9315007/diff/6002/chrome/common/extensions/ext... chrome/common/extensions/extension.cc:2975: // and we need to make sure its position values are synced. add a note here about doing it more systematically if another case arises
On 2012/02/02 15:26:09, csharp wrote: > > Also I guess this is okay for just one special case, but if there are any > other > > similar cases, we should probably do this more systematically. > > Sounds good to me, is there anywhere in the code I should put a note about that? Yeah, added comment where I think you should put the note. Thanks!
https://chromiumcodereview.appspot.com/9315007/diff/6002/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/6002/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:1588: DCHECK_EQ(extension_misc::kWebStoreAppId, extension_id); On 2012/02/03 01:44:43, akalin wrote: > flip argument order (unlike EXPECT_*, should match the natural arg order) Done. https://chromiumcodereview.appspot.com/9315007/diff/6002/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:1592: if (extension_id == extension_misc::kWebStoreAppId) On 2012/02/03 01:44:43, akalin wrote: > i think this if statement is unnecessary; if you're here, you passed the > previous dcheck Done. https://chromiumcodereview.appspot.com/9315007/diff/6002/chrome/browser/exten... File chrome/browser/extensions/pending_extension_manager.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/6002/chrome/browser/exten... chrome/browser/extensions/pending_extension_manager.cc:67: if (extension_misc::kWebStoreAppId == id) { On 2012/02/03 01:44:43, akalin wrote: > flip argument order Done. https://chromiumcodereview.appspot.com/9315007/diff/6002/chrome/common/extens... File chrome/common/extensions/extension.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/6002/chrome/common/extens... chrome/common/extensions/extension.cc:2975: // and we need to make sure its position values are synced. On 2012/02/03 01:44:43, akalin wrote: > add a note here about doing it more systematically if another case arises Done.
My part LGTM, one nit. I defer the sync part to akalin. https://chromiumcodereview.appspot.com/9315007/diff/7005/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/7005/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:1587: // web store (which is consider an app, and may try to set this value). nit: consider->considered.
https://chromiumcodereview.appspot.com/9315007/diff/7005/chrome/browser/exten... File chrome/browser/extensions/extension_service.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/7005/chrome/browser/exten... chrome/browser/extensions/extension_service.cc:1587: // web store (which is consider an app, and may try to set this value). On 2012/02/03 15:12:52, Finnur wrote: > nit: consider->considered. Done.
LGTM but, can you write an integration test for this? (feel free to do this in another CL) https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/browser/exten... File chrome/browser/extensions/pending_extension_manager.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/browser/exten... chrome/browser/extensions/pending_extension_manager.cc:65: // it is listed as a syncable app (because it values need to be synced) it it -> its https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/common/extens... File chrome/common/extensions/extension.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/common/extens... chrome/common/extensions/extension.cc:2977: // something some systematically should be done. some systematically -> more systematic
https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/browser/exten... File chrome/browser/extensions/pending_extension_manager.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/browser/exten... chrome/browser/extensions/pending_extension_manager.cc:65: // it is listed as a syncable app (because it values need to be synced) it On 2012/02/03 18:09:27, akalin wrote: > it -> its Done. https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/common/extens... File chrome/common/extensions/extension.cc (right): https://chromiumcodereview.appspot.com/9315007/diff/5007/chrome/common/extens... chrome/common/extensions/extension.cc:2977: // something some systematically should be done. On 2012/02/03 18:09:27, akalin wrote: > some systematically -> more systematic Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9315007/4006
Try job failure for 9315007-4006 (retry) on win_rel for step "compile" (clobber build). It's a second try, previously, step "compile" failed. 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/csharp@chromium.org/9315007/11001
On 2012/02/03 19:27:03, I haz the power (commit-bot) wrote: > Try job failure for 9315007-4006 (retry) on win_rel for step "compile" (clobber > build). > It's a second try, previously, step "compile" failed. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Problem was I was using DCHECK_EQ with strings, which isn't your suppose to do. DCHECK_STREQ is the right way
There's no such thing as DCHECK_STREQ in chromium code. You can definitely use DCHECK_EQ with strings; you just have to (explicitly) convert the char* to string.
On 2012/02/03 20:33:13, akalin wrote: > There's no such thing as DCHECK_STREQ in chromium code. You can definitely use > DCHECK_EQ with strings; you just have to (explicitly) convert the char* to > string. Apparently third_party/cld/base/logging.h has DCHECK_STREQ. But don't use that.
On 2012/02/03 20:34:35, akalin wrote: > On 2012/02/03 20:33:13, akalin wrote: > > There's no such thing as DCHECK_STREQ in chromium code. You can definitely > use > > DCHECK_EQ with strings; you just have to (explicitly) convert the char* to > > string. > > Apparently third_party/cld/base/logging.h has DCHECK_STREQ. But don't use that. Hmm, looks like some sort of visual studio bug. that template instantiation should definitely work since there's an implicit conversion from const char[] -> std::string. but doing the explicit conversion should work around that.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/9315007/8008
Change committed as 120432
On 2012/02/03 23:14:07, I haz the power (commit-bot) wrote: > Change committed as 120432 I reverted this as it seemed to cause sync_integration_test failures... I'm guessing given that it was an apps sync test failure involving the word 'ordinal' this patch really stuck out :) Please try to run sync_integration_test try runs on your patch to prevent me from unnecessarily reverting in the future!
On 2012/02/04 00:40:30, timsteele wrote: > On 2012/02/03 23:14:07, I haz the power (commit-bot) wrote: > > Change committed as 120432 > > I reverted this as it seemed to cause sync_integration_test failures... I'm > guessing given that it was an apps sync test failure involving the word > 'ordinal' this patch really stuck out :) > > Please try to run sync_integration_test try runs on your patch to prevent me > from unnecessarily reverting in the future! Since you have to reland this, include the integration tests you were planning to write in this CL. I guess I should have mandated that in the first place... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
