Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
114143002:
[Mac] Account management view for the new avatar bubble. (Closed)
|
Created:
7 years ago by noms (inactive) Modified:
6 years, 11 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Mac] Account management view for the new avatar bubble.
This also adds the inline sign-in views to the bubble.
Screenshot: https://drive.google.com/file/d/0B1B1Up4p2NRMbGRkQ3FpWFZweVU/edit?usp=sharing
BUG=324036
TEST=Start Chrome with --new-profile-management and --enable-inline-sign-in flags.
For a local profile, click on the avatar button and press the "Sign in to
Chromium" link from the avatar bubble. This should display an inline sign-in
view inside the bubble. For a signed in profile, press the "Manage accounts"
link. This should show you the email accounts linked to this profile. Clicking
the big "add account" button should show you the inline sign-in view
embedded in the bubble. Once an email has been added, the account
management view should refresh and display this new email account.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243193
Patch Set 1 : #
Total comments: 23
Patch Set 2 : alexei comments #Patch Set 3 : rebase #
Total comments: 10
Patch Set 4 : moar alexei comments #
Total comments: 8
Patch Set 5 : rebase #Patch Set 6 : alexei comments #
Total comments: 2
Patch Set 7 : fix base:: compile errors #
Total comments: 6
Patch Set 8 : nits #
Messages
Total messages: 14 (0 generated)
Hi Alexei, Here's another CL from the "new avatar bubble" series. This one adds a new view that lets you managed the profile accounts, as well as implements the embedded gaia sign in/add account views. Please take a look!
https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:43: scoped_ptr<ProfileChooserControllerInternal::Observer> observer_; Can this be scoped_ptr<OAuth2TokenService::Observer>? https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:51: // List of the full, un-elided accounts for the active profile. Mention what the keys are and what the values are. The current description doesn't tell me enough to understand what this map actually is. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:67: - (BubbleViewMode) viewMode; Nit: Remove space before viewMode. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:83: namespace ProfileChooserControllerInternal { Assuming you can do scoped_ptr<OAuth2TokenService::Observer> for the member, this should be in an anon namespace instead. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:85: class Observer : public OAuth2TokenService::Observer { I'd call this class something more descriptive than Observer if it's in the anon namespace. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:114: }; Nit: DISALLOW_... https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:551: std::vector<std::string>accounts( Nit: Space after > and use = for consistency with primaryAccount above. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:569: } This method is rather long, I suggest making a helper method for adding the accounts buttons. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:584: Profile* profile = browser_->profile(); Nit: Just inline this into the call, since you only use it once. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:592: GURL(signin::GetPromoURL(source, false)), Nit: signin::GetPromoURL() really should return a GURL - it doesn't make sense for each caller to do the conversion. Feel free to clean this up in a separate CL instead of here. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:645: base::string16 elidedEmail = gfx::ElideEmail( Nit: Can you make a helper function in the anon namespace that elides the email and returns it as an NSString*?
https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:43: scoped_ptr<ProfileChooserControllerInternal::Observer> observer_; I actually want the observer to listen to AvatarMenu changes too (in a future CL the avatar bubble can modify the profile name, which should trigger a model update). I've added this to the observer to make it clear, even though the update doesn't actually happen. I've also moved it away from the internal namespace (I saw this being used around in objective-c lands of Chrome. Is this not a thing?) On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Can this be scoped_ptr<OAuth2TokenService::Observer>? https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:51: // List of the full, un-elided accounts for the active profile. On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Mention what the keys are and what the values are. The current description > doesn't tell me enough to understand what this map actually is. Done. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:67: - (BubbleViewMode) viewMode; On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Nit: Remove space before viewMode. Done. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:85: class Observer : public OAuth2TokenService::Observer { On 2013/12/17 20:32:43, Alexei Svitkine wrote: > I'd call this class something more descriptive than Observer if it's in the anon > namespace. Done. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:114: }; On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Nit: DISALLOW_... Done. I've started a jar when I put away 5 dollars every time I miss this :( https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:551: std::vector<std::string>accounts( On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Nit: Space after > and use = for consistency with primaryAccount above. Done. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:569: } On 2013/12/17 20:32:43, Alexei Svitkine wrote: > This method is rather long, I suggest making a helper method for adding the > accounts buttons. Done. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:584: Profile* profile = browser_->profile(); On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Nit: Just inline this into the call, since you only use it once. Done. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:592: GURL(signin::GetPromoURL(source, false)), It does return a GURL. I don't know why sometimes (here) it's getting copy constructed. Anyway, I took out the copy constructor bit. On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Nit: signin::GetPromoURL() really should return a GURL - it doesn't make sense > for each caller to do the conversion. Feel free to clean this up in a separate > CL instead of here. https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:645: base::string16 elidedEmail = gfx::ElideEmail( On 2013/12/17 20:32:43, Alexei Svitkine wrote: > Nit: Can you make a helper function in the anon namespace that elides the email > and returns it as an NSString*? Done.
https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:43: scoped_ptr<ProfileChooserControllerInternal::Observer> observer_; On 2013/12/19 11:44:44, Monica Dinculescu wrote: > I actually want the observer to listen to AvatarMenu changes too (in a future CL > the avatar bubble can modify the profile name, which should trigger a model > update). I've added this to the observer to make it clear, even though the > update doesn't actually happen. I've also moved it away from the internal > namespace (I saw this being used around in objective-c lands of Chrome. Is this > not a thing?) I see. It might be a thing, but I'm not sure its commonly used. Its definitely inconsistent with the general style of using hacker_naming_convention for namespaces in Chrome and it looks pretty weird, so I prefer not doing it that way. (And in cases where I've done something similar, I've not done it that way and my reviewers were fine with it - for example search for BookmarkContextMenuDelegateBridge). I think we often call these helpers *Bridge, since its a bridge class between C++ and ObjC, plus its good to pick a name thats unlikely to conflict with something else in the codebase. How about ActiveProfileObserverBridge? https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:49: // integers used to tag the UI buttons, and the values are the original email Nit: grammar, emails (plural) Also, I'd stick the word "generated" to describe the tags for the UI buttons, to make it clear that they're not coming from somewhere. https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:123: // profile. Rather than describing what the code below does, describe _why_ it does that. e.g. why it only applies to account management view and not others? Same for above. https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:246: DCHECK([sender tag] > 0); // Should not be called for the primary account. DCHECK_GT https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:276: ProfileOAuth2TokenServiceFactory::GetForProfile(browser_->profile()); Would ProfileOAuth2TokenServiceFactory::GetForProfile() just return NULL if its a guest? If so, I'd just base the logic off of that (if not null). (You can then still have a DCHECK(oauth2TokenService || isGuestSession_);) Also, does it make sense to cache a ptr to the service rather than looking it up in a number of places? https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:575: - (NSView*) createAccountsListWithRect:(NSRect)rect { Nit: Remove space
https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/114143002/diff/60001/chrome/browser/ui/cocoa/... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:43: scoped_ptr<ProfileChooserControllerInternal::Observer> observer_; Bridge! I love it. Done. On 2013/12/19 21:01:02, Alexei Svitkine wrote: > On 2013/12/19 11:44:44, Monica Dinculescu wrote: > > I actually want the observer to listen to AvatarMenu changes too (in a future > CL > > the avatar bubble can modify the profile name, which should trigger a model > > update). I've added this to the observer to make it clear, even though the > > update doesn't actually happen. I've also moved it away from the internal > > namespace (I saw this being used around in objective-c lands of Chrome. Is > this > > not a thing?) > > I see. It might be a thing, but I'm not sure its commonly used. Its definitely > inconsistent with the general style of using hacker_naming_convention for > namespaces in Chrome and it looks pretty weird, so I prefer not doing it that > way. (And in cases where I've done something similar, I've not done it that way > and my reviewers were fine with it - for example search for > BookmarkContextMenuDelegateBridge). > > I think we often call these helpers *Bridge, since its a bridge class between > C++ and ObjC, plus its good to pick a name thats unlikely to conflict with > something else in the codebase. How about ActiveProfileObserverBridge? https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.h (right): https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.h:49: // integers used to tag the UI buttons, and the values are the original email On 2013/12/19 21:01:02, Alexei Svitkine wrote: > Nit: grammar, emails (plural) > > Also, I'd stick the word "generated" to describe the tags for the UI buttons, to > make it clear that they're not coming from somewhere. Done. https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:123: // profile. On 2013/12/19 21:01:02, Alexei Svitkine wrote: > Rather than describing what the code below does, describe _why_ it does that. > e.g. why it only applies to account management view and not others? Same for > above. Done. https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:246: DCHECK([sender tag] > 0); // Should not be called for the primary account. On 2013/12/19 21:01:02, Alexei Svitkine wrote: > DCHECK_GT Done. https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:276: ProfileOAuth2TokenServiceFactory::GetForProfile(browser_->profile()); Hui who originally implemented this preferred not to cache a ptr, as it might mask future problems (the service is null but shoudn't be, and since we don't try to add tokens to a null service, we could miss it). That's also why she suggested I check for a guest session, and not a null service, though I guess the DCHECK would point that out. Is The guest would indeed return a null service, and we know that for a guest we don't ever want to try to save tokens. Does it sort of make sense then to leave it as is? On 2013/12/19 21:01:02, Alexei Svitkine wrote: > Would ProfileOAuth2TokenServiceFactory::GetForProfile() just return NULL if its > a guest? If so, I'd just base the logic off of that (if not null). (You can then > still have a DCHECK(oauth2TokenService || isGuestSession_);) > > Also, does it make sense to cache a ptr to the service rather than looking it up > in a number of places? https://codereview.chromium.org/114143002/diff/120001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:575: - (NSView*) createAccountsListWithRect:(NSRect)rect { On 2013/12/19 21:01:02, Alexei Svitkine wrote: > Nit: Remove space Done.
https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:102: ActiveProfileObserverBridge(ProfileChooserController* controller) Nit: explicit https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:249: DCHECK_GT([sender tag], 0); // Should not be called for the primary account. Nit: Also add DCHECK(ContainsKey(currentProfileAccounts_, [sender tag])); (ContainsKey() from base/stl_util.h). https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:303: [[[self window] contentView] setSubviews:[NSArray array]]; Nit: You make a contentView variable below. Move this line below that and call it on contentView directly. https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:598: currentProfileAccounts_[i] = accounts[i]; Should this function clear currentProfileAccounts_ first?
https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:102: ActiveProfileObserverBridge(ProfileChooserController* controller) On 2013/12/20 21:34:14, asvitkine (OOO until Jan 6th) wrote: > Nit: explicit Done. https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:249: DCHECK_GT([sender tag], 0); // Should not be called for the primary account. On 2013/12/20 21:34:14, asvitkine (OOO until Jan 6th) wrote: > Nit: Also add DCHECK(ContainsKey(currentProfileAccounts_, [sender tag])); > (ContainsKey() from base/stl_util.h). Done. https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:303: [[[self window] contentView] setSubviews:[NSArray array]]; On 2013/12/20 21:34:14, asvitkine (OOO until Jan 6th) wrote: > Nit: You make a contentView variable below. Move this line below that and call > it on contentView directly. Done. https://codereview.chromium.org/114143002/diff/140001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:598: currentProfileAccounts_[i] = accounts[i]; Done. It was cleared above, in createCurrentProfileAccountsView (which called this function), but you're right that it makes more sense to be in here. On 2013/12/20 21:34:14, asvitkine (OOO until Jan 6th) wrote: > Should this function clear currentProfileAccounts_ first?
looks like more stuff moved to base:: namespace underneath you over the holidays, so you have some compile failures
https://codereview.chromium.org/114143002/diff/230001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/230001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:601: if (primaryAccount != accounts[i]) { It occurs to me that this logic probably needs to happen on multiple platforms. If so, I think it may be useful to make a C++ helper class that wraps this stuff (i.e. querying the primary account, querying the list of accounts, saving off currentProfileAccounts_ there and being able to ask it which one is the primary one, etc). Then, that class could be used by both Cocoa and Aura views. If you'd like you can add a TODO to make this change later.
Yup, I had noticed the base:: changes as well. Fixed! https://codereview.chromium.org/114143002/diff/230001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm (right): https://codereview.chromium.org/114143002/diff/230001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller.mm:601: if (primaryAccount != accounts[i]) { Created crbug.com/331805. I'd rather fix it in a separate CL, as this one is already pretty giant. On 2014/01/06 16:14:33, Alexei Svitkine wrote: > It occurs to me that this logic probably needs to happen on multiple platforms. > If so, I think it may be useful to make a C++ helper class that wraps this stuff > (i.e. querying the primary account, querying the list of accounts, saving off > currentProfileAccounts_ there and being able to ask it which one is the primary > one, etc). > > Then, that class could be used by both Cocoa and Aura views. > > If you'd like you can add a TODO to make this change later. Done.
Thanks! lgtm with nits https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm (right): https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm:178: SetAuthenticatedUsername(kEmail); Nit: Indentation is off. Should be 4 more than the previous line. https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm:180: ->UpdateCredentials(kEmail, kLoginToken); Nit: put -> on previous line. Same below. https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm:230: EXPECT_EQ(NULL, [primaryAccount action]); Nit: NULL -> nil (Obj-C) here and below.
Woohoo, thanks! This CL spanned two years. :) https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm (right): https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm:178: SetAuthenticatedUsername(kEmail); On 2014/01/06 16:34:58, Alexei Svitkine wrote: > Nit: Indentation is off. Should be 4 more than the previous line. Done. https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm:180: ->UpdateCredentials(kEmail, kLoginToken); On 2014/01/06 16:34:58, Alexei Svitkine wrote: > Nit: put -> on previous line. Same below. Done. https://codereview.chromium.org/114143002/diff/320001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/browser/profile_chooser_controller_unittest.mm:230: EXPECT_EQ(NULL, [primaryAccount action]); On 2014/01/06 16:34:58, Alexei Svitkine wrote: > Nit: NULL -> nil (Obj-C) here and below. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/114143002/420001
Message was sent while issue was closed.
Change committed as 243193 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
