Comment on attachment 517836[details][diff][review]
Patch to update for NM 0.9 connection states. Not compile tested...
Patch compiles (and works) fine, as long as the patch for bug 627672 is also applied. (It compiles with or without the other patch, but it's kind of worthless without it.)

Comment on attachment 517836[details][diff][review]
Patch to update for NM 0.9 connection states. Not compile tested...
Please document that NM_STATE_CONNECTED_OLD is what old dbus versions send? Possibly even with a number attached to "old" when describing them (like "before version NNN").
Also, remove the extraneous parens in the assignment to mLinkUp.
r=me with those changes. I'm really sorry for the lag here....

Martin - is there some code change in this that is actually needed for bug 627672? If there isn't, the dependency seems to be in the opposite direction, as this patch is non-functional without the changes in 627672 (as I found when I tested it long ago).

(In reply to Nick Hurley from comment #9)
> Martin - is there some code change in this that is actually needed for bug
> 627672? If there isn't, the dependency seems to be in the opposite
> direction, as this patch is non-functional without the changes in 627672 (as
> I found when I tested it long ago).
The patch works as expected (I've just tested it on latest trunk, with a patch from 627672). The patch works w/o 627672 - it fixes the DBUS/NM interface for NM >= 0.9, no matter how it's used later.

(In reply to Martin Stránský from comment #11)
> The patch works as expected (I've just tested it on latest trunk, with a
> patch from 627672). The patch works w/o 627672 - it fixes the DBUS/NM
> interface for NM >= 0.9, no matter how it's used later.
Right, I believe it compiles with or without the patch from 627672, but does the detection of link state (what this patch actually fixes) work without that patch? Last time I tested, it didn't, which is why I marked this as depending on 627672. If things have changed (or the other patch breaks gecko without this one), then let's go ahead and get this landed, otherwise it doesn't seem to make sense to me to land this when it does nothing useful without the other patch.

(In reply to Nick Hurley from comment #12)
> (In reply to Martin Stránský from comment #11)
> > The patch works as expected (I've just tested it on latest trunk, with a
> > patch from 627672). The patch works w/o 627672 - it fixes the DBUS/NM
> > interface for NM >= 0.9, no matter how it's used later.
>
> Right, I believe it compiles with or without the patch from 627672, but does
> the detection of link state (what this patch actually fixes) work without
> that patch? Last time I tested, it didn't, which is why I marked this as
> depending on 627672. If things have changed (or the other patch breaks gecko
> without this one), then let's go ahead and get this landed, otherwise it
> doesn't seem to make sense to me to land this when it does nothing useful
> without the other patch.
I do not believe this patch depends on the one in 627672. Obviously if bug 627672's patch is required to get D-Bus working in the first place, then this patch wont' have any effect when applied. But there's nothing in this patch that would *regress* the current situation, and then when bug 627672 got fixed things would magically work.

(In reply to Dan Williams from comment #13)
> I do not believe this patch depends on the one in 627672. Obviously if bug
> 627672's patch is required to get D-Bus working in the first place, then
> this patch wont' have any effect when applied. But there's nothing in this
> patch that would *regress* the current situation, and then when bug 627672
> got fixed things would magically work.
We're in agreement on everything except the dependency :) My view is that, since this patch is a no-op without bug 627672, we shouldn't land this until 627672 does, as the functionality is unverifiable without that other patch. I'm cc'ing Jason to get a necko peer's opinion on this matter. If he says land it, then let's land it and just tell QA to wait to verify until 627672 also lands.