The root frame of a web page is always supposed to have ID 0. It looks like WebNavigationFrames.jsm doesn't always do this. getAllFrames seems to return the window ID for the root frame instead of 0. Also, findFrame probably shouldn't return the root frame if you pass in its window ID.

I thought we had a test for that.

I looked over the test and I don't see anything that specifically tests that the root frame ID is 0. I did notice that this sort looks wrong: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js#141

Luca pointed out this code to me on IRC: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-webNavigation.js#63 I still think it would be better if we changed this to do the conversion in the JSM. It seems less confusing that way. Or else maybe we could call it getAllWindows instead of getAllFrames to make it clear that you need to convert the IDs yourself.

(In reply to Bill McCloskey (:billm) from comment #3) > Luca pointed out this code to me on IRC: > https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ > ext-webNavigation.js#63 > > I still think it would be better if we changed this to do the conversion in > the JSM. It seems less confusing that way. Or else maybe we could call it > getAllWindows instead of getAllFrames to make it clear that you need to > convert the IDs yourself. So, the assigned frameId should be correct, but I agree: we can make it more explicit by renaming the helpers and messages that aren't already carrying the frameId and adding a new explicit test case on the root frame's framId/parentFrameId. The sort seems to work correctly (which, maybe, is because the |Array.prototype.sort|| already knows how to sort an array of numbers, so if we extract and return a number from the current object, it can then order the array of objects based on that number) but I didn't found this behaviour documented nowhere, so it is definitely better if I fix that sort to use the documented behaviour instead. I've a patch for these changes but I would prefer to rebase it on the patch from Bug 1239349. I think the summary of this bugzilla issue doesn't reflect anymore the real issue we are going to solve, if we are going to use this issue number in the patch which will introduce these changes, I'd like to rename the issue, e.g. "Make explicit where the frameId is assigned for webNavigation's getFrame/getAllFrames results". how it sounds to you, guys? Anyway, I'm assigning this to me.

(In reply to Luca Greco [:rpl] from comment #4) > The sort seems to work correctly (which, maybe, is because the > |Array.prototype.sort|| already knows how to sort an array of numbers, so if > we extract and return a number from the current object, it can then order > the array of objects based on that number) but I didn't found this behaviour > documented nowhere, so it is definitely better if I fix that sort to use the > documented behaviour instead. The argument to sort needs to compare its two arguments and return the result, though. In this case, it's just returns a fairly arbitrary value. I think it comes up with the right result purely by luck.

(In reply to Kris Maglione [:kmag] from comment #5) > (In reply to Luca Greco [:rpl] from comment #4) > > The sort seems to work correctly (which, maybe, is because the > > |Array.prototype.sort|| already knows how to sort an array of numbers, so if > > we extract and return a number from the current object, it can then order > > the array of objects based on that number) but I didn't found this behaviour > > documented nowhere, so it is definitely better if I fix that sort to use the > > documented behaviour instead. > > The argument to sort needs to compare its two arguments and return the > result, though. In this case, it's just returns a fairly arbitrary value. I > think it comes up with the right result purely by luck. That was what I though initially, but it seems too consistent to be only luck (I tried in a couple of javascript REPL and even nodejs has the same behaviour). But in any case, I'm not going to leverage such an undocumented (or, maybe, very lucky) behaviour in our code, so, even besides from the reason why it works (which was more like a "scientific interest" ;-)), I'm going to change it as it should be by looking at the docs.

(In reply to Luca Greco [:rpl] from comment #6) > That was what I though initially, but it seems too consistent to be only > luck (I tried in a couple of javascript REPL and even nodejs has the same > behaviour). js> [9, 8, 7, 6, 1, 4, 3, 5, 2].sort(key => key) [2, 5, 3, 4, 1, 6, 7, 8, 9] js> [1, 2, 3, 4, 5, 6, 7, 8, 9].sort(key => key) [9, 8, 7, 6, 5, 4, 3, 2, 1]

(In reply to Kris Maglione [:kmag] from comment #7) > js> [9, 8, 7, 6, 1, 4, 3, 5, 2].sort(key => key) > [2, 5, 3, 4, 1, 6, 7, 8, 9] > js> [1, 2, 3, 4, 5, 6, 7, 8, 9].sort(key => key) > [9, 8, 7, 6, 5, 4, 3, 2, 1] ouch, from now on I will remember of this as "out of luck on choosing the numbers when I tried it in the REPL". One more reason to change it.

Review commit: https://reviewboard.mozilla.org/r/35651/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35651/

I thought that it could be a good idea to fix the sorting first, because it is going to change only the test file and for this reason it doesn't conflict with the patch attached to Bug 1239349. Then I'm going to push to mozreview, in a different patch, the change which renames the internal helpers methods and message managers' events as discussed.

Comment on attachment 8721340 [details] MozReview Request: Bug 1249144 - [webext] fix sorting collected frame details in webNavigation tests. r?kmag https://reviewboard.mozilla.org/r/35651/#review32341 ::: browser/components/extensions/test/browser/browser_ext_webNavigation_getFrames.js:144 (Diff revision 1) > + el2 = el2 ? el2.frameId : -1; It would be better to create new variables for these rather than reuse the argument variables.

Comment on attachment 8721340 [details] MozReview Request: Bug 1249144 - [webext] fix sorting collected frame details in webNavigation tests. r?kmag Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35651/diff/1-2/

- rename message manager event names ("WebNavigation:GetAllWindows" and "WebNavigation:GetWindow") to remark that it doesn't already has the resolved frameId assigned - add new assertions to explicitly test the frameId / parentFrameIds assigned to the toplevel frame Review commit: https://reviewboard.mozilla.org/r/36659/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36659/

The patch from Bug 1239349 has landed on fx-team, I've just rebase on it and pushed on mozreview the second part of the changes related to this issue (which renames the message manager events and WebNavigationFrames helpers to make it clear that the "internal" windowId is not yet been converted into the "external/public" frameId and adds an explicit assertion to test the frameId and parentFrameId of the root frame are both set to the values that we expect).

Comment on attachment 8723673 [details] MozReview Request: Bug 1249144 - [webext] Test explicitly the frameId/parentFrameId associated to the toplevel frame. r?billm https://reviewboard.mozilla.org/r/36659/#review33781 Hi Luca, Kris made a lot of changes to WebNavigatonFrames.jsm in bug 1213993. I'm pretty happy with it right now. Can you keep the change to browser_ext_webNavigation_getFrames.js and drop everything else? Thanks, Bill

Comment on attachment 8723673 [details] MozReview Request: Bug 1249144 - [webext] Test explicitly the frameId/parentFrameId associated to the toplevel frame. r?billm Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36659/diff/1-2/

(In reply to Bill McCloskey (:billm) from comment #17) > Comment on attachment 8723673 [details] > MozReview Request: Bug 1249144 - [webext] Test explicitly the > frameId/parentFrameId associated to the toplevel frame. r?billm > > https://reviewboard.mozilla.org/r/36659/#review33781 > > Hi Luca, > Kris made a lot of changes to WebNavigatonFrames.jsm in bug 1213993. I'm > pretty happy with it right now. Can you keep the change to > browser_ext_webNavigation_getFrames.js and drop everything else? > Thanks, > Bill Sure, I've just pushed the updated patch (with just the changes in the test case). I'm adding the checkin-needed keyword (and removing the leave-open keyword)