Closed Bug 1591989 Opened 3 months ago Closed 2 months ago

Don't use `frameId` for Page.loadEventFired event

Categories

(Remote Protocol :: Page, defect, P3)

defect

Tracking

(firefox72 fixed)

RESOLVED FIXED
Firefox 72
Tracking Status
firefox72 --- fixed

People

(Reporter: whimboo, Assigned: stensonj, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(1 file)

Page.loadEventFired should only emit the timestamp but not a frameId:

https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/remote/domains/content/Page.jsm#144

When making changes to this file make sure that all the browser-chrome tests are still passing by running the following command:

./mach test remote/test/browser/

Looks like there is an expected discrepancy between the CDP documentation and our wanted implementation. See bug 1543095 which explicitly added this property. I will remove the mentored flag until we sorted it out.

Mentor: hskupin
Flags: needinfo?(poirot.alex)
Flags: needinfo?(ato)
See Also: → 1543095
Whiteboard: [lang=js]

I discussed it with Andreas and we should indeed get this removed to be consistent with the CDP protocol.

Mentor: hskupin
Flags: needinfo?(poirot.alex)
Flags: needinfo?(ato)
Whiteboard: [lang=js]

Hi there, I'd like to take on this bug. I am new to debugging mozilla code and also new to bugzilla so I may need some guidance completing it if that is okay. From what I gather in your description I would need to remove the 'frameId' on line 144 and then ensure that the tests still pass by running that command? Thanks

Flags: needinfo?(hskupin)

Hi Jacob! It's great to have you here, and that you are interested to help with this bug. Feel free to also join us on IRC if you want in case of further questions. It makes it easier as having to write back and forth here on Bugzilla.

Otherwise your summarized description sounds correct. Please see https://firefox-source-docs.mozilla.org/remote/ in how to get started. The bug will be assigned to you once the first patch got uploaded. Thanks!

Flags: needinfo?(hskupin)

Thanks for your response. How would I go about joining you on IRC? I have a few more questions if that is ok.

Flags: needinfo?(hskupin)

You would need an IRC client. Firefox should already suggest you one when clicking on this link: irc://irc.mozilla.org/#interop

Flags: needinfo?(hskupin)

Hi there, I believe I have removed 'frameId' successfuly. I have ran ./mach test remote/test/browser/ which nets 0 'unexpected results' and is 'OK'. I will commit this to phabricator.

Flags: needinfo?(hskupin)

Great. thank you. I will have a look at latest tomorrow. Note that there is no need to set needinfo for each and every comment given that I read all of them. Thanks.

Flags: needinfo?(hskupin)

Apologies. I am new to all this.

No problem at all. It was just a gently info. Great to see your contribution!

Attachment #9108763 - Attachment description: Bug 1591989 - Don't use 'frameId' for Page.loadEventFired event, r=whimboo → Bug 1591989 - [remote] Don't use 'frameId' for Page.loadEventFired event. r=whimboo
Assignee: nobody → stensonj
Status: NEW → ASSIGNED
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0af2df439cd
[remote] Don't use 'frameId' for Page.loadEventFired event. r=whimboo,remote-protocol-reviewers
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72
You need to log in before you can comment on or make changes to this bug.