Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6976030:
Start the event routers with the extension service, so that they're correctly started for each pr... (Closed)
|
Created:
9 years, 7 months ago by Miranda Callahan Modified:
9 years, 5 months ago CC:
chromium-reviews, brettw-cc_chromium.org, jstritar Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionStart the event routers with the extension service, so that they're correctly started for each profile.
BUG=84059
TEST=browser doesn't crash when a second profile is created.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=90797
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : Reland, now that event routers have been fixed #Messages
Total messages: 16 (0 generated)
+jstritar, who was just looking at this.
LGTM On 2011/05/26 17:14:05, Yoyo Zhou wrote: > +jstritar, who was just looking at this. The related issue dealing with this line of code is http://crbug.com/80559 , but I think it's not related. However, I see now I had been assuming profile.cc:293 virtual ExtensionService* GetExtensionService() { return GetOriginalProfile()->GetExtensionService(); } meant there was only one extension service (and I assumed this would change later for multi-profile)... does this just get overridden by ProfileImpl? But if we have multiple event routers under multi-profile, it's possible now they would observe events for the wrong profile, so as in the related issue, we might have event listeners firing twice. Is this easy for you to check, or should I look into it?
On 2011/05/26 17:36:36, Yoyo Zhou wrote: > LGTM > > On 2011/05/26 17:14:05, Yoyo Zhou wrote: > > +jstritar, who was just looking at this. > > The related issue dealing with this line of code is http://crbug.com/80559 , but > I think it's not related. > > However, I see now I had been assuming profile.cc:293 > > virtual ExtensionService* GetExtensionService() { > return GetOriginalProfile()->GetExtensionService(); > } > > meant there was only one extension service (and I assumed this would change > later for multi-profile)... does this just get overridden by ProfileImpl? > > But if we have multiple event routers under multi-profile, it's possible now > they would observe events for the wrong profile, so as in the related issue, we > might have event listeners firing twice. Is this easy for you to check, or > should I look into it? Yoyo, could you briefly look into this? I would appreciate it -- this code area is kind of new to me. Thanks!
On 2011/05/26 17:57:02, Miranda Callahan wrote: > On 2011/05/26 17:36:36, Yoyo Zhou wrote: > > LGTM > > > > On 2011/05/26 17:14:05, Yoyo Zhou wrote: > > > +jstritar, who was just looking at this. > > > > The related issue dealing with this line of code is http://crbug.com/80559 , > but > > I think it's not related. > > > > However, I see now I had been assuming profile.cc:293 > > > > virtual ExtensionService* GetExtensionService() { > > return GetOriginalProfile()->GetExtensionService(); > > } > > > > meant there was only one extension service (and I assumed this would change > > later for multi-profile)... does this just get overridden by ProfileImpl? > > > > But if we have multiple event routers under multi-profile, it's possible now > > they would observe events for the wrong profile, so as in the related issue, > we > > might have event listeners firing twice. Is this easy for you to check, or > > should I look into it? > > Yoyo, could you briefly look into this? I would appreciate it -- this code area > is kind of new to me. Thanks! (And, correct -- there will now be a separate extension service for every profile...)
On 2011/05/26 17:57:02, Miranda Callahan wrote: > On 2011/05/26 17:36:36, Yoyo Zhou wrote: > > LGTM > > > > On 2011/05/26 17:14:05, Yoyo Zhou wrote: > > > +jstritar, who was just looking at this. > > > > The related issue dealing with this line of code is http://crbug.com/80559 , > but > > I think it's not related. > > > > However, I see now I had been assuming profile.cc:293 > > > > virtual ExtensionService* GetExtensionService() { > > return GetOriginalProfile()->GetExtensionService(); > > } > > > > meant there was only one extension service (and I assumed this would change > > later for multi-profile)... does this just get overridden by ProfileImpl? > > > > But if we have multiple event routers under multi-profile, it's possible now > > they would observe events for the wrong profile, so as in the related issue, > we > > might have event listeners firing twice. Is this easy for you to check, or > > should I look into it? > > Yoyo, could you briefly look into this? I would appreciate it -- this code area > is kind of new to me. Thanks! Sure, I will take a look.
On 2011/05/26 18:04:22, Yoyo Zhou wrote: > On 2011/05/26 17:57:02, Miranda Callahan wrote: > > On 2011/05/26 17:36:36, Yoyo Zhou wrote: > > > LGTM > > > > > > On 2011/05/26 17:14:05, Yoyo Zhou wrote: > > > > +jstritar, who was just looking at this. > > > > > > The related issue dealing with this line of code is http://crbug.com/80559 , > > but > > > I think it's not related. > > > > > > However, I see now I had been assuming profile.cc:293 > > > > > > virtual ExtensionService* GetExtensionService() { > > > return GetOriginalProfile()->GetExtensionService(); > > > } > > > > > > meant there was only one extension service (and I assumed this would change > > > later for multi-profile)... does this just get overridden by ProfileImpl? > > > > > > But if we have multiple event routers under multi-profile, it's possible now > > > they would observe events for the wrong profile, so as in the related issue, > > we > > > might have event listeners firing twice. Is this easy for you to check, or > > > should I look into it? > > > > Yoyo, could you briefly look into this? I would appreciate it -- this code > area > > is kind of new to me. Thanks! > > Sure, I will take a look. Yoyo, Jon and I just spent half an hour going over this problem -- I am going to check in this change and Jon is going to work on a comprehensive change to the extension event routers to make them work correctly per-profile. Without this change, the multi-profile menu crashes immediately, and this change does not effect non-multi-profile mode. With this change, we can run the multi-profile menu, though we may get some interbleed of events until Jon checks in the significantly larger fix.
On 2011/05/27 19:55:57, Miranda Callahan wrote: > On 2011/05/26 18:04:22, Yoyo Zhou wrote: > > On 2011/05/26 17:57:02, Miranda Callahan wrote: > > > On 2011/05/26 17:36:36, Yoyo Zhou wrote: > > > > LGTM > > > > > > > > On 2011/05/26 17:14:05, Yoyo Zhou wrote: > > > > > +jstritar, who was just looking at this. > > > > > > > > The related issue dealing with this line of code is http://crbug.com/80559 > , > > > but > > > > I think it's not related. > > > > > > > > However, I see now I had been assuming profile.cc:293 > > > > > > > > virtual ExtensionService* GetExtensionService() { > > > > return GetOriginalProfile()->GetExtensionService(); > > > > } > > > > > > > > meant there was only one extension service (and I assumed this would > change > > > > later for multi-profile)... does this just get overridden by ProfileImpl? > > > > > > > > But if we have multiple event routers under multi-profile, it's possible > now > > > > they would observe events for the wrong profile, so as in the related > issue, > > > we > > > > might have event listeners firing twice. Is this easy for you to check, or > > > > should I look into it? > > > > > > Yoyo, could you briefly look into this? I would appreciate it -- this code > > area > > > is kind of new to me. Thanks! > > > > Sure, I will take a look. > > Yoyo, Jon and I just spent half an hour going over this problem -- I am going to > check in this change and Jon is going to work on a comprehensive change to the > extension event routers to make them work correctly per-profile. Without this > change, the multi-profile menu crashes immediately, and this change does not > effect non-multi-profile mode. With this change, we can run the multi-profile > menu, though we may get some interbleed of events until Jon checks in the > significantly larger fix. (Also, Jon just confirmed that 80559 is indeed a very similar problem, and his fix for that is going to take care of this bug in multiprofile as well.)
From today's discussion, I think this change still makes sense, given that we're going to try to redo the change away from singletons.
On 2011/06/01 21:56:03, Yoyo Zhou wrote: > From today's discussion, I think this change still makes sense, given that we're > going to try to redo the change away from singletons. Something like this is still worth doing, but my only concern is with crbug.com/40144; this CL would change back to having the bookmarks imported after starting up the bookmark event router, which was the cause of that bug. It took a while to figure out how to force Chrome to import bookmarks, but the discussion in crbug.com/46528 has some tips. So I could reproduce it, but I noticed that if "skip_first_run_ui" is true, the bug does not happen, yet the bookmarks are imported. But I can't figure out what actually imports the bookmarks in that case -- FirstRun::AutoImport seems not to be called. Do you happen to know?
On 2011/06/25 01:33:44, Yoyo Zhou wrote: > On 2011/06/01 21:56:03, Yoyo Zhou wrote: > > From today's discussion, I think this change still makes sense, given that > we're > > going to try to redo the change away from singletons. > > Something like this is still worth doing, but my only concern is with > crbug.com/40144; this CL would change back to having the bookmarks imported > after starting up the bookmark event router, which was the cause of that bug. > > It took a while to figure out how to force Chrome to import bookmarks, but the > discussion in crbug.com/46528 has some tips. So I could reproduce it, but I > noticed that if "skip_first_run_ui" is true, the bug does not happen, yet the > bookmarks are imported. But I can't figure out what actually imports the > bookmarks in that case -- FirstRun::AutoImport seems not to be called. Do you > happen to know? Hmm, good question. Did you follow the instructions in bug 46528 to do the importing of bookmarks through a bookmarks file? In this case, there is code to import from a file. If they're not being imported that way, though, let me know (and let me know which OS this is happening on), and I'll trace through.
On 2011/06/27 13:34:44, Miranda Callahan wrote: > On 2011/06/25 01:33:44, Yoyo Zhou wrote: > > On 2011/06/01 21:56:03, Yoyo Zhou wrote: > > > From today's discussion, I think this change still makes sense, given that > > we're > > > going to try to redo the change away from singletons. > > > > Something like this is still worth doing, but my only concern is with > > crbug.com/40144; this CL would change back to having the bookmarks imported > > after starting up the bookmark event router, which was the cause of that bug. > > > > It took a while to figure out how to force Chrome to import bookmarks, but the > > discussion in crbug.com/46528 has some tips. So I could reproduce it, but I > > noticed that if "skip_first_run_ui" is true, the bug does not happen, yet the > > bookmarks are imported. But I can't figure out what actually imports the > > bookmarks in that case -- FirstRun::AutoImport seems not to be called. Do you > > happen to know? > > Hmm, good question. Did you follow the instructions in bug 46528 to do the > importing of bookmarks through a bookmarks file? In this case, there is code to > import from a file. If they're not being imported that way, though, let me know > (and let me know which OS this is happening on), and I'll trace through. Actually, I guess all I learned was consistent with http://www.chromium.org/administrators/configuring-other-preferences : skip_first_run_ui has to be false for any bookmarks to be imported from the specification in master_preferences. The behavior is the same with or without this patch. I don't understand how the silent import from other browsers as described in crbug.com/40144 is supposed to work (the First Run UI doesn't match the old description anymore) -- it seems Windows is the only platform that supports it, but I couldn't make it happen there. There is probably a good chance that this CL would fix crbug.com/86642 though.
Still LGTM
On 2011/06/28 01:32:39, Yoyo Zhou wrote: > Still LGTM Thanks, will get this in ASAP. As far as the silent import goes, bookmarks are no longer silently imported (there was a major change about a year ago), so the bug report from 40144 is obsolete. I can send you a link to a new description of the first run process; where did you read the obsolete description? I'll take that out and replace it with what's actually going on if there's bad information somewhere...
On 2011/06/28 13:22:32, Miranda Callahan wrote: > On 2011/06/28 01:32:39, Yoyo Zhou wrote: > > Still LGTM > > Thanks, will get this in ASAP. > > As far as the silent import goes, bookmarks are no longer silently imported > (there was a major change about a year ago), so the bug report from 40144 is > obsolete. I can send you a link to a new description of the first run process; > where did you read the obsolete description? I'll take that out and replace it > with what's actually going on if there's bad information somewhere... Thanks, that's really helpful. I must have read it in the old changelists / bugs, which are historical artifacts anyway. I need to brush up on my documentation locations.
Change committed as 90797 |
