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

Status: RESOLVED FIXED

(Remote Protocol :: CDP, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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/

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 3

[Jacob Stenson]

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

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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!

Comment 5

[Jacob Stenson]

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

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 7

[Jacob Stenson]

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.

Comment 8

[Jacob Stenson]

Attachment: Bug 1591989 - [remote] Don't use 'frameId' for Page.loadEventFired event. r=whimboo

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 10

[Jacob Stenson]

Apologies. I am new to all this.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0af2df439cd
[remote] Don't use 'frameId' for Page.loadEventFired event. r=whimboo,remote-protocol-reviewers

Comment 13

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/d0af2df439cd