Closed Bug 1317511 Opened 3 years ago Closed 3 years ago

Share captive portal state with the content process

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files)

No description provided.
MozReview-Commit-ID: 5FnM9DNDWwL
Attachment #8811297 - Flags: review?(daniel)
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.
Attachment #8811297 - Flags: review?(daniel) → review+
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...
https://hg.mozilla.org/mozilla-central/rev/1eedf8ccdc1e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Can you request uplift to aurora since we are also considering uplift for bug 989197? Thanks.
Flags: needinfo?(valentin.gosu)
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]: -
Attachment #8811297 - Flags: approval-mozilla-aurora?
Should be safe to uplift.
Flags: needinfo?(valentin.gosu)
Comment on attachment 8811297 [details] [diff] [review]
Share captive portal state with the content process

captive portal work for aurora52
Attachment #8811297 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I'm hitting a conflict uplifting this to aurora, could you take a look?
Flags: needinfo?(valentin.gosu)
I fixed the merge conflicts. Didn't push to try, but it should build.
Flags: needinfo?(valentin.gosu)
You need to log in before you can comment on or make changes to this bug.