1317511 - Share captive portal state with the content process

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Attachment: Share captive portal state with the content process

MozReview-Commit-ID: 5FnM9DNDWwL

Comment 2

[Daniel Stenberg [:bagder]]

Comment on attachment 8811297 [details] [diff] [review]
Share captive portal state with the content process

Review of attachment 8811297 [details] [diff] [review]:
-----------------------------------------------------------------

Just one question.

::: netwerk/base/CaptivePortalService.cpp
@@ +163,5 @@
>  
> +  if (XRE_GetProcessType() != GeckoProcessType_Default) {
> +    // Doesn't do anything when called in the content process.
> +    return NS_OK;
> +  }

But do you still want the LOG() to happen when not doing anything? The same comment for RecheckCaptivePortal.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Theoretically no one should be calling Start, Stop in the content process. 
RecheckCaptivePortal() might be implemented in a followup - in case we want content process events to trigger rechecks.
The logs will still useful in case we observe weird behaviour.

Rebasing on m-c and pushing to inbound...

Comment 4

[Valentin Gosu [:valentin] (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1eedf8ccdc1ef3980b6097d89cb3d665bd4b8d81
Bug 1317511 - Share captive portal state with the content process r=bagder

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1eedf8ccdc1e

Comment 6

[areya]

https://hg.mozilla.org/mozilla-central/rev/1eedf8ccdc1e

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Can you request uplift to aurora since we are also considering uplift for bug 989197? Thanks.

Comment 8

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8811297 [details] [diff] [review]
Share captive portal state with the content process

Approval Request Comment
[User impact if declined]: Can't uplift bug 989197 (see bug 989197 comment 141)
[Is this code covered by automated tests?]: No, but the frontend work in bug 989197 (which uses this) is covered.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[Is the change risky?]: I don't think so, but :valentin can comment further.
[Why is the change risky/not risky?]: The patch exposes a state variable to the content process. A frontend feature that uses this has been in Nightly for a while and all appears to work fine.
[String changes made/needed]: -

Comment 9

[Valentin Gosu [:valentin] (he/him)]

Should be safe to uplift.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8811297 [details] [diff] [review]
Share captive portal state with the content process

captive portal work for aurora52

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm hitting a conflict uplifting this to aurora, could you take a look?

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Attachment: [aurora] Share captive portal state with the content process

Comment 13

[Valentin Gosu [:valentin] (he/him)]

I fixed the merge conflicts. Didn't push to try, but it should build.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/11814a11cdbb