Closed
Bug 1261185
Opened 9 years ago
Closed 9 years ago
Complete test coverage for browser.windows.create
Categories
(WebExtensions :: Untriaged, defect)
WebExtensions
Untriaged
Tracking
(firefox48 fixed)
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: bsilverberg, Assigned: bsilverberg)
References
Details
Attachments
(1 file)
browser.windows.create is missing coverage for:
* passing an invalid |tabId|
* passing an array of URLs to open in |url|
* passing in a value for |left|, |top|, |width| and |height|
Missing coverage can be seen at https://people.mozilla.org/~kmaglione/webextension-test-coverage/browser/components/extensions/ext-windows.js.html
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → bob.silverberg
Status: NEW → ASSIGNED
Iteration: --- → 48.2 - Apr 4
Flags: blocking-webextensions+
Comment 1•9 years ago
|
||
I didn't get a chance to update the coverage tests last week, so some of this is already fixed.
Assignee | ||
Comment 2•9 years ago
|
||
Oh. I've already started working on it. Do you think you'll be able to find time to generate a new coverage report so I can know what needs to be done?
Comment 3•9 years ago
|
||
I updated it just before I posted that comment.
Assignee | ||
Comment 4•9 years ago
|
||
Thanks Kris. Before I saw your comment I added explicit tests for |left|, |top|, |width| and |height|. I see from the report that we are not officially lacking coverage in that area, but it seems like we still don't have actual tests for them. I.e., create a window with one of those options and verify that the window has the requested option. Do you think it's still worth adding tests for that? I found while doing it that I did get expected values for |left|, |width| and |height|, but for |top| I did not, and I'm not sure if that's a bug or not. It seemed that when I created a window and specified a value for |top|, after the window was created, if I asked for its |top| value the number was always off by around 12. Is that a concern, or is it perhaps expected?
Flags: needinfo?(kmaglione+bmo)
Comment 5•9 years ago
|
||
(In reply to Bob Silverberg [:bsilverberg] from comment #4)
> I see from the report that we are not officially lacking coverage in that
> area, but it seems like we still don't have actual tests for them. I.e.,
> create a window with one of those options and verify that the window has the
> requested option.
It's tested pretty thoroughly here:
https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_windows_size.js
> I found while doing it that I did get expected values for |left|, |width|
> and |height|, but for |top| I did not, and I'm not sure if that's a bug or
> not. It seemed that when I created a window and specified a value for |top|,
> after the window was created, if I asked for its |top| value the number was
> always off by around 12. Is that a concern, or is it perhaps expected?
Different platforms impose different restrictions on window size and position.
On OS-X, the window has to be below the top menu bar, and can't be larger than
the available area of the screen, so certain values won't be fully respected.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 6•9 years ago
|
||
Thanks Kris. I had looked at `browser_ext_windows_create.js` but not `browser_ext_windows_size.js`. This bug now only needs coverage added for:
* passing an invalid |tabId|
* passing an array of URLs to open in |url|
Assignee | ||
Comment 7•9 years ago
|
||
Add coverage for:
* passing an invalid |tabId|
* passing an array of URLs to open in |url|
Review commit: https://reviewboard.mozilla.org/r/44165/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/44165/
Attachment #8737894 -
Flags: review?(kmaglione+bmo)
Comment 8•9 years ago
|
||
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag
https://reviewboard.mozilla.org/r/44165/#review40785
::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:19
(Diff revision 1)
> };
>
> + let promiseTabUpdated = () => {
> + return new Promise(resolve => {
> + browser.tabs.onUpdated.addListener(function listener() {
> + browser.tabs.onUpdated.removeListener(listener);
We should check for `updated.url`, and I think we probably need to wait for two onUpdated events.
::: browser/components/extensions/test/browser/browser_ext_windows_create_tabId.js:121
(Diff revision 1)
> + Promise.resolve(window)
> + ]);
> + }).then(([, window]) => {
> + return browser.tabs.query({windowId: window.id});
> + }).then(tabs => {
> + browser.test.assertEq(2, tabs.length, "2 tabs were opened in new window");
We should also check that the URLs are correct.
Attachment #8737894 -
Flags: review?(kmaglione+bmo)
Assignee | ||
Comment 9•9 years ago
|
||
https://reviewboard.mozilla.org/r/44165/#review40785
> We should also check that the URLs are correct.
I mentioned why I am not doing that in the commit comment. I will see if I can use `updated.url` to wait for the correct URLs. Luca also mentioned that I might be able to use `webNavigation.onCommitted` but wasn't sure if we should mix API calls in a test.
Assignee | ||
Comment 10•9 years ago
|
||
https://reviewboard.mozilla.org/r/44165/#review40785
> I mentioned why I am not doing that in the commit comment. I will see if I can use `updated.url` to wait for the correct URLs. Luca also mentioned that I might be able to use `webNavigation.onCommitted` but wasn't sure if we should mix API calls in a test.
Update: Luca suggests that `webNavigation.onCompleted` would be better.
Comment 11•9 years ago
|
||
https://reviewboard.mozilla.org/r/44165/#review40785
> Update: Luca suggests that `webNavigation.onCompleted` would be better.
I think `onUpdated` is best. We want to know when the internal state of the tab objects has been updated, which doesn't really have a well-defined correlation with navigation events.
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44165/diff/1-2/
Attachment #8737894 -
Flags: review?(kmaglione+bmo)
Comment 13•9 years ago
|
||
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag
https://reviewboard.mozilla.org/r/44165/#review40843
Attachment #8737894 -
Flags: review?(kmaglione+bmo) → review+
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8737894 [details]
MozReview Request: Bug 1261185 - Complete test coverage for browser.windows.create, r?kmag
Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44165/diff/2-3/
Assignee | ||
Comment 16•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Iteration: 48.2 - Apr 4 → 48.3 - Apr 18
Keywords: checkin-needed
Comment 17•9 years ago
|
||
Keywords: checkin-needed
Comment 18•9 years ago
|
||
backed out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=8443526&repo=fx-team
Flags: needinfo?(bob.silverberg)
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
I cannot reproduce this on my machine, either when running that test in isolation or when running all the tests in `browser/components/extensions/test/browser/`. I notice from the failure [1], that this only seems to happen on OS X 10.10 and I'm on 10.11, so maybe that's why I cannot reproduce.
Kris, do you have any suggestions? Does anything in the new code look like an obvious culprit for these window leaks?
Everyone: Does anyone else on the team have a box with OS X 10.10 on it?
[1] https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=27df9bc5af2ca94e203e132013a5958bd60e85fc&selectedJob=8437274
Flags: needinfo?(bob.silverberg) → needinfo?(kmaglione+bmo)
Assignee | ||
Comment 21•9 years ago
|
||
I do see some leaks reported from a tab process (as opposed to the default process), which can be seen in my sample output at [1], but they don't trigger any failures, they appear to be the same both with and without my patch, and I don't see those window leaks. I just realized that I have a Mac Mini with 10.10 on it, so I'm going to try to repro on that.
[1] https://gist.github.com/bobsilverberg/b7a1f38b6cdde8b535306ac25bd701b2
Assignee | ||
Comment 22•9 years ago
|
||
I tested this on my 10.10 machine and get the same results. I do not see these window leaks when running either a single test nor the full suite, and I tried both e10s and non-e10s. Do we know if this was a consistent issue after landing this change, or was it intermittent?
Comment 23•9 years ago
|
||
If you can't reproduce this, I'd say push to try and see if you can reproduce again there.
If you do manage to reproduce, the easiest thing to do is disable code until you can't. If that doesn't help, someone's going to have to look through cycle collector graphs.
Flags: needinfo?(kmaglione+bmo)
Assignee | ||
Comment 24•9 years ago
|
||
Pushing to try again to see if it reproduces: https://treeherder.mozilla.org/#/jobs?repo=try&revision=325df11c4aaa
Assignee | ||
Comment 25•9 years ago
|
||
The try I submitted earlier does not display the same leaks as those pointed to in comment #18, and I am unable to reproduce them locally, so let's try landing this again and see what happens.
Keywords: checkin-needed
Comment 26•9 years ago
|
||
Keywords: checkin-needed
Comment 27•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•7 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•