Status

defect
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
Firefox 55
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

Attachments

(2 attachments)

The patch for bug 1349363 touched the devtools hack to force the user to turn off e10s-multi if they want to debug service workers. I rushed that patch a little bit and after talking things over with Alexandre on IRC, we identified some followups to fix. The content of the patch seems to be correct, these are mostly form fixes:
  - Update the hardcoded strings to reflect what we're actually doing now (disabling e10s-multi until the next experiment instead of indefinitely).
  - Add a pref observer for dom.ipc.multiOptOut to update the debugger UI's state.
  - Add some additional comments to clarify what's happening and why since it's pretty tricky.
Comment hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
Comment on attachment 8859800 [details]
Bug 1357909 - Tweak the strings explaining to the user what's happening.

https://reviewboard.mozilla.org/r/131788/#review134772

Looks good but if my knowledges about l10n are not outdated, you have to change l10n key if you end up changing the value. (like append `2` to l10n keys)

::: devtools/client/locales/en-US/aboutdebugging.properties:131
(Diff revision 1)
>  multiProcessWarningTitle = Service Worker debugging is not compatible with multiple content processes at the moment.
>  
>  # LOCALIZATION NOTE (multiProcessWarningMessage):
>  # This string is displayed in the warning section for multi-e10s in
>  # about:debugging#workers
> -multiProcessWarningMessage = The preference “dom.ipc.processCount” can be set to 1 to force a single content process.
> +multiProcessWarningMessage = The preference “dom.ipc.multiOptOut” can be set to 1 to force a single content process for the current version.

Today it is reseted to 1, but I imagine this is going to change over time depending on E10S_MULTI_EXPERIMENT value?
If that's the case, we may change that phrasing to not mention a precise value.
To something like this?
"The preference “dom.ipc.multiOptOut” can be modified to force a single content process for the current version."
Attachment #8859800 - Flags: review?(poirot.alex) → review+
Strings can't be changed without also changing their names.. (the l10n tools don't handle this well). So you need to update the names here too
Comment hidden (mozreview-request)

Comment 5

2 years ago
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bae82ca67cf8
Tweak the strings explaining to the user what's happening. r=ochameau

Comment 6

2 years ago
Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/634530ff65a6
Tweak the strings explaining to the user what's happening. r=ochameau

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/634530ff65a6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8859800 [details]
Bug 1357909 - Tweak the strings explaining to the user what's happening.

Approval Request Comment
[Feature/Bug causing the regression]: e10s-multi
[User impact if declined]: The text in the service worker debugger won't be up to date.
[Is this code covered by automated tests?]: n/a
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: n/a
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple string changes
[String changes made/needed]: Yes.
Attachment #8859800 - Flags: approval-mozilla-beta?
Hi :flod,
This string change might need your review.
Flags: needinfo?(francesco.lodolo)
We should not be landing string changes in mozilla-beta, as a general rule but especially in this cycle, where localization tools and localizers are already trying to deal with Dawn.

Is it an option to hard-code these messages in a beta-only patch?
Flags: needinfo?(francesco.lodolo)
Hi :mrbkap,
Do you have any updates for the patch?
Flags: needinfo?(mrbkap)
The strings were apparently already hardcoded for Aurora. I've copy/pasted them for this patch for beta.
Attachment #8863500 - Flags: review?(poirot.alex)
Flags: needinfo?(mrbkap)
Attachment #8863500 - Flags: approval-mozilla-beta?
Attachment #8859800 - Flags: approval-mozilla-beta?
Comment on attachment 8863500 [details] [diff] [review]
Patch for beta

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

Sorry for the review delay, the review request wasn't appearing in my bugzilla dashboard (bugzilla-todos), may be because the bug was marked resolved fixed?

Anyway, this patch looks good.
Attachment #8863500 - Flags: review?(poirot.alex) → review+
Comment on attachment 8863500 [details] [diff] [review]
Patch for beta

Fix a service worker debugger issue. Beta54+. Should be in 54 beta 6.
Attachment #8863500 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.