Closed
Bug 1088710
Opened 8 years ago
Closed 6 years ago
Make character encoding widget work in e10s
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
VERIFIED
FIXED
Firefox 47
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(4 files)
As far as I can tell from the work in bug 1080787, the button/menu itself is just not working properly, and that's why the tests are failing.
Flags: firefox-backlog+
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1088540
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #1) > > *** This bug has been marked as a duplicate of bug 1088540 *** Err, this really isn't a dupe. It's still broken and the tests are indicative of this brokenness.
tracking-e10s:
--- → ?
Assignee | ||
Updated•6 years ago
|
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee | ||
Comment 3•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36303/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36303/
Attachment #8723022 -
Flags: review?(mconley)
Assignee | ||
Comment 4•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36305/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36305/
Attachment #8723023 -
Flags: review?(mconley)
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•6 years ago
|
Attachment #8723023 -
Flags: review?(mconley)
Assignee | ||
Comment 5•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #4) > Created attachment 8723023 [details] > MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley > > Review commit: https://reviewboard.mozilla.org/r/36305/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/36305/ The trypush shows that one of these tests (but not the other?) still fails on Linux. I just re-ran that test locally on Windows, where I wrote the patch, and it passes, even if I run the directory up to that test. I don't know why it would behave differently on Linux. There are no Windows try results yet. I'll look at it some more tomorrow (and potentially investigate on my linux vm) to see what's going on here. In general, what this cset is fixing is that onLocationChange can fire before the document's charset has been set appropriately, and in e10s we never update it again after that. Which then breaks the test's expectation that the charset is unicode, rather than 'western' (iso-8859-1). I don't know why this fix would somehow work on my Windows machine and not on the Linux machine, unless httpd.js is sending a charset as a http header by default, and that charset changes per OS, or something?
Comment 6•6 years ago
|
||
Comment on attachment 8723022 [details] MozReview Request: Bug 1088710 - part 1: fix some leftover gunk from pre-australis menubutton, r?mconley https://reviewboard.mozilla.org/r/36303/#review32965 r=me with caveats - see below. Thanks for cleaning this up! ::: toolkit/modules/CharsetMenu.jsm:120 (Diff revision 1) > - build: function(parent, showAccessKeys=true, showDetector=true) { > + build: function(parent, showDetector=true) { This is going to break other consumers of CharsetMenu.build. I can see that both SeaMonkey and Thunderbird use it. Both are passing "true" as the second argument, so I would wager that there aren't too many consumers passing "false" for the second argument... (I just checked add-ons MXR and I only see a few users there as well). Can we detect when the second argument is being passed, and show a Deprecated warning, and just ignore it instead? And then maybe file bugs to update the SM / TB callers? Then maybe set addon-compat on this bug, or maybe dev-doc-needed or something?
Attachment #8723022 -
Flags: review?(mconley) → review+
![]() |
||
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) > Can we detect when the second argument is being passed, and show a > Deprecated warning, and just ignore it instead? And then maybe file bugs to > update the SM / TB callers? There's a third argument that becomes the second argument post-patch (an argument actually still actively used by seamonkey/thunderbird code), so I don't really see how that should work. I could keep the argument as-is and just make it no-op and add a comment about that? Would that be better? (In reply to :Gijs Kruitbosch from comment #5) > The trypush shows that one of these tests (but not the other?) still fails > on Linux. I just re-ran that test locally on Windows, where I wrote the > patch, and it passes, even if I run the directory up to that test. I don't > know why it would behave differently on Linux. There are no Windows try > results yet. I'll look at it some more tomorrow (and potentially investigate > on my linux vm) to see what's going on here. > > In general, what this cset is fixing is that onLocationChange can fire > before the document's charset has been set appropriately, and in e10s we > never update it again after that. Which then breaks the test's expectation > that the charset is unicode, rather than 'western' (iso-8859-1). I don't > know why this fix would somehow work on my Windows machine and not on the > Linux machine, unless httpd.js is sending a charset as a http header by > default, and that charset changes per OS, or something? The test passes on my local linux vm as well. Not sure why this would be different on infra. Any ideas? :-\
Flags: needinfo?(mconley)
Comment 8•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #7) > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) > > Can we detect when the second argument is being passed, and show a > > Deprecated warning, and just ignore it instead? And then maybe file bugs to > > update the SM / TB callers? > > There's a third argument that becomes the second argument post-patch (an > argument actually still actively used by seamonkey/thunderbird code), so I > don't really see how that should work. > > I could keep the argument as-is and just make it no-op and add a comment > about that? Would that be better? > Yeah, I guess that's what I meant. :) > (In reply to :Gijs Kruitbosch from comment #5) > > The trypush shows that one of these tests (but not the other?) still fails > > on Linux. I just re-ran that test locally on Windows, where I wrote the > > patch, and it passes, even if I run the directory up to that test. I don't > > know why it would behave differently on Linux. There are no Windows try > > results yet. I'll look at it some more tomorrow (and potentially investigate > > on my linux vm) to see what's going on here. > > > > In general, what this cset is fixing is that onLocationChange can fire > > before the document's charset has been set appropriately, and in e10s we > > never update it again after that. Which then breaks the test's expectation > > that the charset is unicode, rather than 'western' (iso-8859-1). I don't > > know why this fix would somehow work on my Windows machine and not on the > > Linux machine, unless httpd.js is sending a charset as a http header by > > default, and that charset changes per OS, or something? > > The test passes on my local linux vm as well. Not sure why this would be > different on infra. Any ideas? :-\ Sounds like a race. Here's a screenshot from the failure: http://mozilla-releng-blobs.s3.amazonaws.com/blobs/try/sha512/c338c890c14c4e62812c83726606cd35c8c7276f1e594fd66b1673e826889fa6dcc11d3fe8037787160c6f65343d726f0832d1ae41d94a8938de7d24e9cdb0e1 It looks as if we only ever run the code to properly check the current items once onViewShowing has fired. Are you sure that's fired? I'd log the hell out of this, is how I'd approach it.
Flags: needinfo?(mconley)
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 8723022 [details] MozReview Request: Bug 1088710 - part 1: fix some leftover gunk from pre-australis menubutton, r?mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36303/diff/1-2/
Assignee | ||
Comment 10•6 years ago
|
||
Comment on attachment 8723023 [details] MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36305/diff/1-2/
Attachment #8723023 -
Flags: review?(mconley)
Updated•6 years ago
|
Attachment #8723023 -
Flags: review?(mconley) → review+
Comment 11•6 years ago
|
||
Comment on attachment 8723023 [details] MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley https://reviewboard.mozilla.org/r/36305/#review33467 \o/ Great job!
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #11) > Comment on attachment 8723023 [details] > MozReview Request: Bug 1088710 - part 2: make it work on e10s, r?mconley > > https://reviewboard.mozilla.org/r/36305/#review33467 > > \o/ Great job! Couldn't have done it without the suggestion! ... however, surprisingly, this still dies on Linux debug. Opt looks to be fine now, but not debug: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2742cf050977 The screenshot still looks the same. Which is odd because the test asserts that the subview has been shown as well, and that assertion is passing. Wat. :-\ ... actually, that screenshot (and the previous one as well) looks like the page hasn't finished loading. There's text on the test page, which isn't shown in the screenshot, and the spinner is still going.
Assignee | ||
Comment 13•6 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/36925/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36925/
Attachment #8724217 -
Flags: review?(mconley)
Comment 14•6 years ago
|
||
Comment on attachment 8724217 [details] MozReview Request: Bug 1088710 - part 3: wait for STATE_STOP as well as load, r?mconley https://reviewboard.mozilla.org/r/36925/#review33533 Thanks Gijs! ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:86 (Diff revision 1) > - openNewForegroundTab(tabbrowser, opening = "about:blank", aWaitForLoad = true) { > + openNewForegroundTab(tabbrowser, opening = "about:blank", aWaitForLoad = true, aWaitForStateStop = true) { This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false. ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm:207 (Diff revision 1) > + QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener2, Ci.nsIWebProgressListener]) Nit: linebreak between the two interfaces please, and add a trailing comma while you're at it?
Attachment #8724217 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 15•6 years ago
|
||
https://reviewboard.mozilla.org/r/36925/#review33533 > This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false. D'oh. I meant false; I must have brainfarted and gone with true because that's what the other default was.
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #15) > https://reviewboard.mozilla.org/r/36925/#review33533 > > > This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false. > > D'oh. I meant false; I must have brainfarted and gone with true because > that's what the other default was. with default false instead: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbd28c7f512c
Assignee | ||
Comment 17•6 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #16) > (In reply to :Gijs Kruitbosch from comment #15) > > https://reviewboard.mozilla.org/r/36925/#review33533 > > > > > This is going to change the assumption of a lot of tests. Hopefully this is going to be green on push - if not, we should default this to false. > > > > D'oh. I meant false; I must have brainfarted and gone with true because > > that's what the other default was. > > with default false instead: > > remote: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cbd28c7f512c Annnd the non-e10s web progress is much pickier and needs something that's weak-reference-able, and a filter value: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a5609b37687
Comment 18•6 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/23ee4ba3d927 https://hg.mozilla.org/integration/fx-team/rev/decc59e7a6d3 https://hg.mozilla.org/integration/fx-team/rev/7ac5c16b31f4
Comment 19•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/23ee4ba3d927 https://hg.mozilla.org/mozilla-central/rev/decc59e7a6d3 https://hg.mozilla.org/mozilla-central/rev/7ac5c16b31f4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 6 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Assignee | ||
Comment 20•6 years ago
|
||
Assignee | ||
Comment 21•6 years ago
|
||
Comment on attachment 8725672 [details] [diff] [review] Patch for aurora Approval Request Comment [Feature/regressing bug #]: character encoding menu + widget [User impact if declined]: they don't work correctly in e10s mode [Describe test coverage new/current, TreeHerder]: this includes fixing and enabling existing tests [Risks and why]: reasonably low risk. Can't really break this more than it already is in e10s, doesn't touch non-e10s code too much [String/UUID change made/needed]: nope
Attachment #8725672 -
Flags: review+
Attachment #8725672 -
Flags: approval-mozilla-aurora?
Regression with e10s, tracking and marking affected for 46.
Comment on attachment 8725672 [details] [diff] [review] Patch for aurora Has tests, and fixes an e10s regression. Yay! Please uplift.
Attachment #8725672 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d90312d3824a
Comment 25•6 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/d90312d3824a
Comment 26•6 years ago
|
||
[bugday-20160323] Status: RESOLVED,FIXED --> VERIFIED Comments: STR: Properly working. Component: Name Firefox Version 46.0b4 Build ID 20160322075646 Update Channel beta User Agent Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0 OS Windows 7 SP1 x86_64 Actual Results: As expected Expected Results: Web page automatically loads and showing changes when encoding changes.
Assignee | ||
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•