Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Devtools followup for bug 1349363

RESOLVED FIXED in Firefox 54

Status

()

Firefox
Developer Tools
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

3 months ago
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.
(Assignee)

Updated

3 months ago
Blocks: 1349363
Comment hidden (mozreview-request)

Comment 2

3 months 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

3 months 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

3 months 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
https://hg.mozilla.org/mozilla-central/rev/634530ff65a6
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
https://hg.mozilla.org/mozilla-central/rev/bae82ca67cf8
(Assignee)

Comment 9

3 months ago
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?

Updated

3 months ago
status-firefox54: --- → affected
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)
(Assignee)

Comment 13

3 months ago
Created attachment 8863500 [details] [diff] [review]
Patch for beta

The strings were apparently already hardcoded for Aurora. I've copy/pasted them for this patch for beta.
Attachment #8863500 - Flags: review?(poirot.alex)
(Assignee)

Updated

3 months ago
Flags: needinfo?(mrbkap)
Attachment #8863500 - Flags: approval-mozilla-beta?
(Assignee)

Updated

3 months ago
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+

Comment 16

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5b3b3ab6875c
status-firefox54: affected → fixed
You need to log in before you can comment on or make changes to this bug.