Set user-visible machine names and devices types for synced sessions.

Set user-visible machine names and devices types for synced sessions.

On OSX:
- Read the computer name, falling back to reading hw.model and stripping any trialing numbers. This will give us
  something like MacBookPro, iMac, MacBookAir.
On Linux:
- Use distro name which gives something like "Ubuntu 10.04.2 LTS".
On Windows:
- Use the computer name
On ChromeOS:
- Use "Chromebook"

BUG=94693, 59672
TEST=enable session sync, sync two clients, and then look at the entries in about:sync and see that the "client_name" is set

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99391

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99450

[Yaron, 2011-08-30 20:26:09]

I'll add sky@ to look at the non sync bits if you like this approach. This
depends on http://codereview.chromium.org/7811001/

[Nicolas Zea, 2011-08-30 21:29:58]

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:50: std::string
SessionModelAssociator::current_machine_name_ = "";
You can remove this line (default value for a string is empty string anyways).

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:493: local_session_syncid_
= sync_api::kInvalidId;
Should probably clear current_machine_name and current_machine_tag here.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:509: // TODO(zea): We need
a better way of creating a machine tag. The directory
This TODO no longer applies.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:524: current_machine_name_
= base::GetLinuxDistro();
should probably check for the "Unknown" case and replace it with "Linux".

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:528: if
(!sysctlbyname("hw.model", modelBuffer, &length, NULL, 0)) {
I'm not familiar with these API's, so my main concern here is whether they may
block on file i/o in some way, leading to jank (Thakis may be a good reviewer to
verify this). Also, I wonder if this functionality (as well as the windows call
below) are better off as methods in base::SysInfo. Something like GetOSXHwModel
and GetWinComputerName.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:535: // If we didn't find a
number in the string, Just use Mac OS X.
Why not just use the full string?

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:551: #endif
else "Unknown OS"

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.h:410: // Local client name.
Change to "Unique client tag"

[Nicolas Zea, 2011-08-30 23:12:45]

Could you also add 59672 to the bug list?

[Yaron, 2011-08-31 03:26:12]

Comments addressed and added additional reviewers.

sky for chrome/browser/sessions
darin for base/win and chrome/browser
thakis for base/mac - please see nzea's concern about blocking. Worst case, I
can imagine we can initialize it in a similar manner to how GetLinuxDistro is
done for Linux

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:50: std::string
SessionModelAssociator::current_machine_name_ = "";
On 2011/08/30 21:29:58, nzea wrote:
> You can remove this line (default value for a string is empty string anyways).

But since it's static, I need to have a concrete initialization somewhere

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:493: local_session_syncid_
= sync_api::kInvalidId;
On 2011/08/30 21:29:58, nzea wrote:
> Should probably clear current_machine_name and current_machine_tag here.

I cleared current_machine_tag_ but not current_machine_name_. The reason being
that the latter is static and doesn't need to be tied to an instance of a SMA,
but we lazily initialize it when we eventually do need it. If you'd prefer, I
can make it a member and clear it out too.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:509: // TODO(zea): We need
a better way of creating a machine tag. The directory
On 2011/08/30 21:29:58, nzea wrote:
> This TODO no longer applies.

Well not entirely. The last bit of being human-readable goes away but the
comment about starting/stopping Chrome still applies

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:524: current_machine_name_
= base::GetLinuxDistro();
On 2011/08/30 21:29:58, nzea wrote:
> should probably check for the "Unknown" case and replace it with "Linux".

Done

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:528: if
(!sysctlbyname("hw.model", modelBuffer, &length, NULL, 0)) {
On 2011/08/30 21:29:58, nzea wrote:
> I'm not familiar with these API's, so my main concern here is whether they may
> block on file i/o in some way, leading to jank (Thakis may be a good reviewer
to
> verify this). Also, I wonder if this functionality (as well as the windows
call
> below) are better off as methods in base::SysInfo. Something like
GetOSXHwModel
> and GetWinComputerName.

Split off GetComputerName to win_util and GetHardwareModelName to mac_util and
I'll add the relevant owners to check. I didn't add it to base:SysInfo because
we're intentionally grabbing different values for different platforms.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:535: // If we didn't find a
number in the string, Just use Mac OS X.
On 2011/08/30 21:29:58, nzea wrote:
> Why not just use the full string?

Hmm. I suppose that could be better

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:551: #endif
On 2011/08/30 21:29:58, nzea wrote:
> else "Unknown OS"

I added a catch-all case instead (because it also handles errors from
platform-specific codepaths)

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.h (right):

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.h:410: // Local client name.
On 2011/08/30 21:29:58, nzea wrote:
> Change to "Unique client tag"

Done.

[sky, 2011-08-31 03:35:15]

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1429:
g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
Why do we need the linux distro if we're not going to use it?

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_types.h (right):

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/sessions/sess...
chrome/browser/sessions/session_types.h:211: enum DeviceType {
enum before constructor.

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/sessions/sess...
chrome/browser/sessions/session_types.h:218: DeviceType device_type;
If this isn't used in chrome/browser/sessions, I don't see the point of putting
it here. If we do need something like this, it should match the typedefs we have
for the platforms: OS_WIN, OS_MACOSX, OS_CHROMEOS...

[Nico, 2011-08-31 03:55:50]

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm#newcode370
base/mac/mac_util.mm:370: }
Don't you want [[NSHost currentHost] localizedName];?

[Nico, 2011-08-31 03:58:22]

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm#newcode370
base/mac/mac_util.mm:370: }
On 2011/08/31 03:55:50, Nico wrote:
> Don't you want [[NSHost currentHost] localizedName];?

(If so, you'll probably need some kind of fallback on 10.5)

[darin (slow to review), 2011-08-31 05:52:01]

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.cc
File base/win/win_util.cc (right):

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.cc#newcode80
base/win/win_util.cc:80: return "";
return std::string();

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.h
File base/win/win_util.h (right):

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.h#newcode8
base/win/win_util.h:8: // In general, you should not be adding stuff to this
file.
this seems like appropriate advice in this case.  why bother adding code to
base?

wouldn't it be fine to just create a file in sync/glue/ for this?  maybe give it
a more domain specific name?

[Nicolas Zea, 2011-08-31 18:23:00]

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:50: std::string
SessionModelAssociator::current_machine_name_ = "";
On 2011/08/31 03:26:12, Yaron wrote:
> On 2011/08/30 21:29:58, nzea wrote:
> > You can remove this line (default value for a string is empty string
anyways).
> 
> But since it's static, I need to have a concrete initialization somewhere

Woops, I didn't read this properly. Yeah, this definitely needs to be a member
variable. I'm pretty sure this won't work in a multi-profile situation with
multiple sync services, especially if we support the users renaming their
sessions.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:493: local_session_syncid_
= sync_api::kInvalidId;
On 2011/08/31 03:26:12, Yaron wrote:
> On 2011/08/30 21:29:58, nzea wrote:
> > Should probably clear current_machine_name and current_machine_tag here.
> 
> I cleared current_machine_tag_ but not current_machine_name_. The reason being
> that the latter is static and doesn't need to be tied to an instance of a SMA,
> but we lazily initialize it when we eventually do need it. If you'd prefer, I
> can make it a member and clear it out too.

See above comment.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:509: // TODO(zea): We need
a better way of creating a machine tag. The directory
On 2011/08/31 03:26:12, Yaron wrote:
> On 2011/08/30 21:29:58, nzea wrote:
> > This TODO no longer applies.
> 
> Well not entirely. The last bit of being human-readable goes away but the
> comment about starting/stopping Chrome still applies

Tim and I have chatted about this before and we're not planning on changing it.
The idea is to eventually support users deleting stale sessions (for example the
laptop run over by a truck scenario), which should take care of this. So we can
just get rid of the TODO entirely.

[Yaron, 2011-08-31 23:23:21]

Comments addressed and the code has been pushed back into sync/glue/. So I
suppose thakis and darin can drop off if you want.

nzea, I've changed it from "Machine name" to "session name" since the former was
a bit of a misnomer as you pointed out with user-configurable names. I also
moved the retrieval of system information async.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:50: std::string
SessionModelAssociator::current_machine_name_ = "";
On 2011/08/31 18:23:00, nzea wrote:
> On 2011/08/31 03:26:12, Yaron wrote:
> > On 2011/08/30 21:29:58, nzea wrote:
> > > You can remove this line (default value for a string is empty string
> anyways).
> > 
> > But since it's static, I need to have a concrete initialization somewhere
> 
> Woops, I didn't read this properly. Yeah, this definitely needs to be a member
> variable. I'm pretty sure this won't work in a multi-profile situation with
> multiple sync services, especially if we support the users renaming their
> sessions.

Done.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:493: local_session_syncid_
= sync_api::kInvalidId;
On 2011/08/31 18:23:00, nzea wrote:
> On 2011/08/31 03:26:12, Yaron wrote:
> > On 2011/08/30 21:29:58, nzea wrote:
> > > Should probably clear current_machine_name and current_machine_tag here.
> > 
> > I cleared current_machine_tag_ but not current_machine_name_. The reason
being
> > that the latter is static and doesn't need to be tied to an instance of a
SMA,
> > but we lazily initialize it when we eventually do need it. If you'd prefer,
I
> > can make it a member and clear it out too.
> 
> See above comment.

Done.

http://codereview.chromium.org/7740055/diff/3001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:509: // TODO(zea): We need
a better way of creating a machine tag. The directory
On 2011/08/31 18:23:00, nzea wrote:
> On 2011/08/31 03:26:12, Yaron wrote:
> > On 2011/08/30 21:29:58, nzea wrote:
> > > This TODO no longer applies.
> > 
> > Well not entirely. The last bit of being human-readable goes away but the
> > comment about starting/stopping Chrome still applies
> 
> Tim and I have chatted about this before and we're not planning on changing
it.
> The idea is to eventually support users deleting stale sessions (for example
the
> laptop run over by a truck scenario), which should take care of this. So we
can
> just get rid of the TODO entirely.

Done.

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm#newcode370
base/mac/mac_util.mm:370: }
On 2011/08/31 03:58:22, Nico wrote:
> On 2011/08/31 03:55:50, Nico wrote:
> > Don't you want [[NSHost currentHost] localizedName];?
> 
> (If so, you'll probably need some kind of fallback on 10.5)

No the thinking was that people identify their computers by the device itself.
So it would be typical for someone to think of it as "oh that's my Macbook Air"
and that the computer name is less meaningful. This would be a slightly worse
experience for users with multiple devices of the same type (which seems rare),
but otherwise seems like a usability win. In a later release, we'll also add the
ability to customize this string which would support that case. Note that based
on Darin's feedback, I've moved this into session_model_associator

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.cc
File base/win/win_util.cc (right):

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.cc#newcode80
base/win/win_util.cc:80: return "";
On 2011/08/31 05:52:01, darin wrote:
> return std::string();

Done.

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.h
File base/win/win_util.h (right):

http://codereview.chromium.org/7740055/diff/7004/base/win/win_util.h#newcode8
base/win/win_util.h:8: // In general, you should not be adding stuff to this
file.
On 2011/08/31 05:52:01, darin wrote:
> this seems like appropriate advice in this case.  why bother adding code to
> base?
> 
> wouldn't it be fine to just create a file in sync/glue/ for this?  maybe give
it
> a more domain specific name?

This was based on feedback in the original review about making this more generic
but point taken. I suppose the next person wanting the computer name will see
it's already implemented and re-use it. Interestingly enough, the same comment
is not present in mac_util.h but I'll do the same there.

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/browser_main.cc
File chrome/browser/browser_main.cc (right):

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/browser_main....
chrome/browser/browser_main.cc:1429:
g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
On 2011/08/31 03:35:16, sky wrote:
> Why do we need the linux distro if we're not going to use it?

Ya, I was too cavalier about it. Done in the sync code now.

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_types.h (right):

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/sessions/sess...
chrome/browser/sessions/session_types.h:211: enum DeviceType {
On 2011/08/31 03:35:16, sky wrote:
> enum before constructor.

Done.

http://codereview.chromium.org/7740055/diff/7004/chrome/browser/sessions/sess...
chrome/browser/sessions/session_types.h:218: DeviceType device_type;
On 2011/08/31 03:35:16, sky wrote:
> If this isn't used in chrome/browser/sessions, I don't see the point of
putting
> it here. If we do need something like this, it should match the typedefs we
have
> for the platforms: OS_WIN, OS_MACOSX, OS_CHROMEOS...

Well it composes SessionWindows and is related to SessionWindow/SessionTab so I
can see the original motivation to keep it nearby. However, it is only used by
session sync so I moved it out.

As for the comments about matching the typedefs, are you saying I should have
one for each OS in build/build_config.h? Or that I shouldn't need the DeviceType
enum because I can somehow reuse the defines? Or just that the naming should
match the defines?

[Nico, 2011-08-31 23:27:13]

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm#newcode370
base/mac/mac_util.mm:370: }
On 2011/08/31 23:23:22, Yaron wrote:
> On 2011/08/31 03:58:22, Nico wrote:
> > On 2011/08/31 03:55:50, Nico wrote:
> > > Don't you want [[NSHost currentHost] localizedName];?
> > 
> > (If so, you'll probably need some kind of fallback on 10.5)
> 
> No the thinking was that people identify their computers by the device itself.
> So it would be typical for someone to think of it as "oh that's my Macbook
Air"
> and that the computer name is less meaningful. This would be a slightly worse
> experience for users with multiple devices of the same type (which seems
rare),
> but otherwise seems like a usability win. In a later release, we'll also add
the
> ability to customize this string which would support that case. Note that
based
> on Darin's feedback, I've moved this into session_model_associator

That string usually looks like "Nico Weber's MacBook" i.e. it includes the name.
But users can change it in sysprefs and it's what mac users expect to see. I'm
pretty sure you want to use the function I mentioned.

[sky, 2011-08-31 23:32:19]

chrome/browser/sessions LGTM .

[Nicolas Zea, 2011-09-01 00:03:36]

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:142:
header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_PC);
Shouldn't this be TYPE_OTHER?

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:560:
g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
BrowserThread::PostTask(BrowserThread::FILE, ...)

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:594: local_session_syncid_
= id;
We should probably overwrite the local session name here too, to handle the case
where we either set a custom name or someone else changed our name.

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:1181: std::string
SessionModelAssociator::GetComputerName() {
Given that these are not safe to run on the UI thread, they really shouldn't be
part of the SessionModelAssociator. But for now just add a TODO(zea): pull these
into a separate class that lives on the FILE thread.

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session.h (right):

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session.h:35: std::string client_name;
session_name (may as well have them match)

[Yaron, 2011-09-01 20:16:17]

Thanks for you help with the Mac stuff, Nico. Should be ready for another look.

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm
File base/mac/mac_util.mm (right):

http://codereview.chromium.org/7740055/diff/7004/base/mac/mac_util.mm#newcode370
base/mac/mac_util.mm:370: }
On 2011/08/31 23:27:13, Nico wrote:
> On 2011/08/31 23:23:22, Yaron wrote:
> > On 2011/08/31 03:58:22, Nico wrote:
> > > On 2011/08/31 03:55:50, Nico wrote:
> > > > Don't you want [[NSHost currentHost] localizedName];?
> > > 
> > > (If so, you'll probably need some kind of fallback on 10.5)
> > 
> > No the thinking was that people identify their computers by the device
itself.
> > So it would be typical for someone to think of it as "oh that's my Macbook
> Air"
> > and that the computer name is less meaningful. This would be a slightly
worse
> > experience for users with multiple devices of the same type (which seems
> rare),
> > but otherwise seems like a usability win. In a later release, we'll also add
> the
> > ability to customize this string which would support that case. Note that
> based
> > on Darin's feedback, I've moved this into session_model_associator
> 
> That string usually looks like "Nico Weber's MacBook" i.e. it includes the
name.
> But users can change it in sysprefs and it's what mac users expect to see. I'm
> pretty sure you want to use the function I mentioned.

Ah, ok, fair enough. Added that for 10.6 and left the existing code as a
fallback.

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
File chrome/browser/sync/glue/session_model_associator.cc (right):

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:142:
header_s->set_device_type(sync_pb::SessionHeader_DeviceType_TYPE_PC);
On 2011/09/01 00:03:36, nzea wrote:
> Shouldn't this be TYPE_OTHER?

Done.

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:560:
g_browser_process->file_thread()->message_loop()->PostTask(FROM_HERE,
On 2011/09/01 00:03:36, nzea wrote:
> BrowserThread::PostTask(BrowserThread::FILE, ...)

Done.

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:594: local_session_syncid_
= id;
On 2011/09/01 00:03:36, nzea wrote:
> We should probably overwrite the local session name here too, to handle the
case
> where we either set a custom name or someone else changed our name.

Done.

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/ses...
chrome/browser/sync/glue/session_model_associator.cc:1181: std::string
SessionModelAssociator::GetComputerName() {
On 2011/09/01 00:03:36, nzea wrote:
> Given that these are not safe to run on the UI thread, they really shouldn't
be
> part of the SessionModelAssociator. But for now just add a TODO(zea): pull
these
> into a separate class that lives on the FILE thread.

Done. Made them static in the interim

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/syn...
File chrome/browser/sync/glue/synced_session.h (right):

http://codereview.chromium.org/7740055/diff/8001/chrome/browser/sync/glue/syn...
chrome/browser/sync/glue/synced_session.h:35: std::string client_name;
On 2011/09/01 00:03:36, nzea wrote:
> session_name (may as well have them match)

Done. I'll update the proto to session_name in a follow-up change (the name
doesn't matter as long as the id is constant, right?)

[Nico, 2011-09-01 20:20:41]

Cool!

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator_mac.mm (right):

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:17: - (NSString
*)localizedName;
No space after *

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:27: } else {
No else after return, that makes the function less indented

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:29: NSString*
computerName = (NSString *)SCDynamicStoreCopyComputerName(
No space in front of *. Also, we don't like C-style casts, you can use the
CFToNSCast() function from base/mac/foundation_util.h.

Also, the docs say you need to release computer_name, so put computerName into a
scoped_nsobject

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:32: return
base::SysNSStringToUTF8([myHost localizedName]);
s/\[myHost localizedName\]/computerName/ :-)

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:46: } else {
No else after return

[Yaron, 2011-09-01 22:19:46]

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator_mac.mm (right):

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:17: - (NSString
*)localizedName;
On 2011/09/01 20:20:41, Nico wrote:
> No space after *

Done. Or did you mean to add a space after the ")"

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:27: } else {
On 2011/09/01 20:20:41, Nico wrote:
> No else after return, that makes the function less indented

Done.

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:29: NSString*
computerName = (NSString *)SCDynamicStoreCopyComputerName(
On 2011/09/01 20:20:41, Nico wrote:
> No space in front of *. Also, we don't like C-style casts, you can use the
> CFToNSCast() function from base/mac/foundation_util.h.
> 
> Also, the docs say you need to release computer_name, so put computerName into
a
> scoped_nsobject

Done.

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:32: return
base::SysNSStringToUTF8([myHost localizedName]);
On 2011/09/01 20:20:41, Nico wrote:
> s/\[myHost localizedName\]/computerName/ :-)

Done. Ugh.

http://codereview.chromium.org/7740055/diff/13001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:46: } else {
On 2011/09/01 20:20:41, Nico wrote:
> No else after return

Done.

http://codereview.chromium.org/7740055/diff/18001/chrome/browser/sync/glue/se...
File chrome/browser/sync/glue/session_model_associator_mac.mm (right):

http://codereview.chromium.org/7740055/diff/18001/chrome/browser/sync/glue/se...
chrome/browser/sync/glue/session_model_associator_mac.mm:31:
scoped_nsobject<NSString>
computerName(base::mac::CFToNSCast(SCDynamicStoreCopyComputerName(
Oops. I'll fix the 80 cols in the next go.

[Nico, 2011-09-01 23:03:19]

chrome/browser/sync/glue/session_model_associator_mac.mm LGTM

[Nicolas Zea, 2011-09-01 23:58:29]

Lgtm
On Sep 1, 2011 4:03 PM, <thakis@chromium.org> wrote:
> chrome/browser/sync/glue/session_model_associator_mac.mm LGTM
>
> http://codereview.chromium.org/7740055/

[Yaron, 2011-09-02 00:58:49]

On 2011/09/01 23:58:29, nzea wrote:
> Lgtm
> On Sep 1, 2011 4:03 PM, <mailto:thakis@chromium.org> wrote:
> > chrome/browser/sync/glue/session_model_associator_mac.mm LGTM
> >
> > http://codereview.chromium.org/7740055/

Last round of failures seem to just be flaky tests and unrelated. Sending to
commit-queue. Thanks for the reviews.

[commit-bot: I haz the power, 2011-09-02 01:04:43]

Presubmit check for 7740055-19002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit Warnings **
You might be calling functions intended only for testing from
production code. Please verify that the following usages are OK,
and email joi@chromium.org if you are seeing false positives:
  chrome/browser/sync/glue/session_model_associator.cc:214
    if (waiting_for_change_) QuitLoopForTest(); \
  chrome/browser/sync/glue/session_model_associator.cc:233
    if (waiting_for_change_) QuitLoopForTest();

Presubmit checks took 1.7s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2011-09-02 01:46:30]

Presubmit check for 7740055-19002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
You might be calling functions intended only for testing from
production code. Please verify that the following usages are OK,
and email joi@chromium.org if you are seeing false positives:
  chrome/browser/sync/glue/session_model_associator.cc:214
    if (waiting_for_change_) QuitLoopForTest(); \
  chrome/browser/sync/glue/session_model_associator.cc:233
    if (waiting_for_change_) QuitLoopForTest();

Presubmit checks took 4.1s to calculate.

[Yaron, 2011-09-02 03:28:18]

On 2011/09/02 01:46:30, I haz the power (commit-bot) wrote:
> Presubmit check for 7740055-19002 failed and returned exit status 1.
> 
> Running presubmit commit checks ...
> 
> ** Presubmit Warnings **
> You might be calling functions intended only for testing from
> production code. Please verify that the following usages are OK,
> and email mailto:joi@chromium.org if you are seeing false positives:
>   chrome/browser/sync/glue/session_model_associator.cc:214
>     if (waiting_for_change_) QuitLoopForTest(); \
>   chrome/browser/sync/glue/session_model_associator.cc:233
>     if (waiting_for_change_) QuitLoopForTest();
> 
> Presubmit checks took 4.1s to calculate.

Is there a way to override the presubmit check?

[(unused - use chromium), 2011-09-02 04:42:10]

On Thu, Sep 1, 2011 at 8:28 PM,  <yfriedman@chromium.org> wrote:
> On 2011/09/02 01:46:30, I haz the power (commit-bot) wrote:
>>
>> Presubmit check for 7740055-19002 failed and returned exit status 1.
>
>> Running presubmit commit checks ...
>
>> ** Presubmit Warnings **
>> You might be calling functions intended only for testing from
>> production code. Please verify that the following usages are OK,
>> and email mailto:joi@chromium.org if you are seeing false positives:
>>   chrome/browser/sync/glue/session_model_associator.cc:214
>>     if (waiting_for_change_) QuitLoopForTest(); \
>>   chrome/browser/sync/glue/session_model_associator.cc:233
>>     if (waiting_for_change_) QuitLoopForTest();
>
>> Presubmit checks took 4.1s to calculate.
>
> Is there a way to override the presubmit check?

Ask Joi? :-)

If you're sure that this is harmless, you can commit manually. I don't
know if the commit bot supports it.

>
> http://codereview.chromium.org/7740055/
>

[yfriedman, 2011-09-02 05:38:59]

Ya I did but he's ooo
On Sep 1, 2011 9:42 PM, "Nico Weber" <thakis@google.com> wrote:
> On Thu, Sep 1, 2011 at 8:28 PM, <yfriedman@chromium.org> wrote:
>> On 2011/09/02 01:46:30, I haz the power (commit-bot) wrote:
>>>
>>> Presubmit check for 7740055-19002 failed and returned exit status 1.
>>
>>> Running presubmit commit checks ...
>>
>>> ** Presubmit Warnings **
>>> You might be calling functions intended only for testing from
>>> production code. Please verify that the following usages are OK,
>>> and email mailto:joi@chromium.org if you are seeing false positives:
>>>   chrome/browser/sync/glue/session_model_associator.cc:214
>>>     if (waiting_for_change_) QuitLoopForTest(); \
>>>   chrome/browser/sync/glue/session_model_associator.cc:233
>>>     if (waiting_for_change_) QuitLoopForTest();
>>
>>> Presubmit checks took 4.1s to calculate.
>>
>> Is there a way to override the presubmit check?
>
> Ask Joi? :-)
>
> If you're sure that this is harmless, you can commit manually. I don't
> know if the commit bot supports it.
>
>>
>> http://codereview.chromium.org/7740055/
>>

[Yaron, 2011-09-02 16:32:14]

On 2011/09/02 05:38:59, yfriedman_google.com wrote:
> Ya I did but he's ooo
> On Sep 1, 2011 9:42 PM, "Nico Weber" <mailto:thakis@google.com> wrote:
> > On Thu, Sep 1, 2011 at 8:28 PM, <mailto:yfriedman@chromium.org> wrote:
> >> On 2011/09/02 01:46:30, I haz the power (commit-bot) wrote:
> >>>
> >>> Presubmit check for 7740055-19002 failed and returned exit status 1.
> >>
> >>> Running presubmit commit checks ...
> >>
> >>> ** Presubmit Warnings **
> >>> You might be calling functions intended only for testing from
> >>> production code. Please verify that the following usages are OK,
> >>> and email mailto:joi@chromium.org if you are seeing false positives:
> >>>   chrome/browser/sync/glue/session_model_associator.cc:214
> >>>     if (waiting_for_change_) QuitLoopForTest(); \
> >>>   chrome/browser/sync/glue/session_model_associator.cc:233
> >>>     if (waiting_for_change_) QuitLoopForTest();
> >>
> >>> Presubmit checks took 4.1s to calculate.
> >>
> >> Is there a way to override the presubmit check?
> >
> > Ask Joi? :-)
> >
> > If you're sure that this is harmless, you can commit manually. I don't
> > know if the commit bot supports it.
> >
> >>
> >> http://codereview.chromium.org/7740055/
> >>

I'm not a committer yet so I don't believe I can commit manually. Is it possible
for you to sponsor me for provisional or land on my behalf? I think nzea is at
an offsite. I can try and find someone else if you're busy.

[Yaron, 2011-09-02 19:54:22]

On 2011/09/02 16:32:14, Yaron wrote:
> On 2011/09/02 05:38:59, http://yfriedman_google.com wrote:
> > Ya I did but he's ooo
> > On Sep 1, 2011 9:42 PM, "Nico Weber" <mailto:thakis@google.com> wrote:
> > > On Thu, Sep 1, 2011 at 8:28 PM, <mailto:yfriedman@chromium.org> wrote:
> > >> On 2011/09/02 01:46:30, I haz the power (commit-bot) wrote:
> > >>>
> > >>> Presubmit check for 7740055-19002 failed and returned exit status 1.
> > >>
> > >>> Running presubmit commit checks ...
> > >>
> > >>> ** Presubmit Warnings **
> > >>> You might be calling functions intended only for testing from
> > >>> production code. Please verify that the following usages are OK,
> > >>> and email mailto:joi@chromium.org if you are seeing false positives:
> > >>>   chrome/browser/sync/glue/session_model_associator.cc:214
> > >>>     if (waiting_for_change_) QuitLoopForTest(); \
> > >>>   chrome/browser/sync/glue/session_model_associator.cc:233
> > >>>     if (waiting_for_change_) QuitLoopForTest();
> > >>
> > >>> Presubmit checks took 4.1s to calculate.
> > >>
> > >> Is there a way to override the presubmit check?
> > >
> > > Ask Joi? :-)
> > >
> > > If you're sure that this is harmless, you can commit manually. I don't
> > > know if the commit bot supports it.
> > >
> > >>
> > >> http://codereview.chromium.org/7740055/
> > >>
> 
> I'm not a committer yet so I don't believe I can commit manually. Is it
possible
> for you to sponsor me for provisional or land on my behalf? I think nzea is at
> an offsite. I can try and find someone else if you're busy.


Uploaded a fix and confirmed that cros unittests passes on trybot:
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...