Closed
Bug 1310346
Opened 8 years ago
Closed 8 years ago
[e10s-multi] RDM browser_toolbox_swap_browsers.js test failures
Categories
(DevTools :: Responsive Design Mode, defect)
Tracking
(firefox52 fixed)
RESOLVED
FIXED
Firefox 52
Tracking | Status | |
---|---|---|
firefox52 | --- | fixed |
People
(Reporter: jryans, Unassigned)
References
Details
Attachments
(1 file)
With e10s multi, there are some test failures in browser_toolbox_swap_browsers.js. STR: ./mach mochitest devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js --setpref dom.ipc.processCount=10
Comment hidden (mozreview-request) |
Comment 2•8 years ago
|
||
mozreview-review |
Comment on attachment 8801389 [details] Bug 1310346 - Fix RDM browser_toolbox_swap_browsers.js for e10s-multi. https://reviewboard.mozilla.org/r/86142/#review85116 ::: devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js:45 (Diff revision 1) > + "0: No DevTools connections yet"); > let { toolbox } = yield openInspector(); > - // 2: One for each tab (starting tab plus the one we opened). Only one truly needed, > - // but calling listTabs will create one for each tab. `registerTestActor` calls > - // this, triggering the extra tab's actor to be made. > - yield checkServerConnectionCount(tab.linkedBrowser, 2); > + if (E10S_MULTI_ENABLED) { > + // 1: Two tabs open, but only one per content process. Only one truly > + // needed, but calling listTabs will create one for each tab. > + // `registerTestActor` calls this, triggering the extra tab's actor to be made. I don't follow this comment. It starts by saying there is only one, but then, it seems to say there could be more? ::: devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js:51 (Diff revision 1) > + yield checkServerConnectionCount(tab.linkedBrowser, 1, > + "1: Two tabs open, but only one per content process"); > + } else { > + // 2: One for each tab (starting tab plus the one we opened). Only one truly > + // needed, but calling listTabs will create one for each tab. > + // `registerTestActor` calls this, triggering the extra tab's actor to be made. I totally understand these comments if you keep only the first sentence. ::: devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js:62 (Diff revision 1) > + if (E10S_MULTI_ENABLED) { > + // 2: RDM UI uses an extra connection > + yield checkServerConnectionCount(ui.getViewportBrowser(), 2, > + "2: RDM UI uses an extra connection"); > + } else { > - // 3: RDM UI uses an extra connection > + // 3: RDM UI uses an extra connection Instead of duplicating the comment, it would be great to say why it doesn't use an extra connection in e10s ::: devtools/client/responsive.html/test/browser/browser_toolbox_swap_browsers.js:103 (Diff revision 1) > + yield checkServerConnectionCount(ui.getViewportBrowser(), 2, > + "2: Two tabs open, but only one per content process"); > + } else { > + // 3: One for each tab (starting tab plus the one we opened). Only one truly > + // needed, but calling listTabs will create one for each tab. > + // `registerTestActor` calls this, triggering the extra tab's actor to be made. Same thing here, I would keep only the first sentence in both if/else comments.
Comment 3•8 years ago
|
||
mozreview-review |
Comment on attachment 8801389 [details] Bug 1310346 - Fix RDM browser_toolbox_swap_browsers.js for e10s-multi. https://reviewboard.mozilla.org/r/86142/#review85120
Attachment #8801389 -
Flags: review?(poirot.alex) → review+
Reporter | ||
Comment 4•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8801389 [details] Bug 1310346 - Fix RDM browser_toolbox_swap_browsers.js for e10s-multi. https://reviewboard.mozilla.org/r/86142/#review85116 Okay, I've tried to clarify the comments. Thanks for the review! > I don't follow this comment. > It starts by saying there is only one, but then, it seems to say there could be more? Okay, I'll chop it down to just the first sentence... It's a bit tricky to explain clearly. We're checking the connection count of a specific process, so even though we've triggered connections for all open tabs, only some are counted since we don't ask every process.
Comment hidden (mozreview-request) |
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ff86d2b6fb5f Fix RDM browser_toolbox_swap_browsers.js for e10s-multi. r=ochameau
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ff86d2b6fb5f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•