1615305 - Show lazy load as Cause for requests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P3)

Description

[:Harald Kirschner :digitarald]

The cause column should show lazy as the primary cause or add (lazy) as sub-cause if we want to keep both fields around.

Comment 1

[:Harald Kirschner :digitarald]

This needs to be exposed by platform: https://searchfox.org/mozilla-central/source/devtools/server/actors/network-monitor/network-observer.js#680

Lazy does not seem to add a new content policy (vs preload): https://searchfox.org/mozilla-central/source/dom/base/nsContentUtils.h#3398 , so maybe we need a different way to tell the network observer.

Comment 2

[:Harald Kirschner :digitarald]

Hiro offered to look into how lazy load can be exposed as cause for requests.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

One important thing I should note is that lazy-loading doesn't trigger any network requests during the image is out-of-view. So we need to send a new notification to devtools in HTMLImageElement::SetLazyLoading when the image gets in the state waiting for being visible (i.e. lazy-load) and maybe send another notification in HTMLImageelement::StopLazyLoadingAndStartLoadIfNeeded when the image is no longer lazy-loading state. I don't think the latter is necessary because once after the image became non-lazy state, the image will be loaded thus a new network request appears.

I am not sure about the actually way to send the notifications, probably need to ask someone familiar with network or DOM.

Harald, is this sufficient for you guys?

Comment 4

[Hiroyuki Ikezoe (:hiro)]

I just realized in the network panel there is a column to show the filename of the image (I guess it's picked from src attribute), and the filename can be changed during lazy-loading state. So I am not quite sure what we should do for users in such cases, and for the notification we need to send a notification that src attribute is changed too.

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

One important thing I should note is that lazy-loading doesn't trigger any network requests during the image is out-of-view. So we need to send a new notification to devtools in HTMLImageElement::SetLazyLoading when the image gets in the state waiting for being visible (i.e. lazy-load) and maybe send another notification in HTMLImageelement::StopLazyLoadingAndStartLoadIfNeeded when the image is no longer lazy-loading state. I don't think the latter is necessary because once after the image became non-lazy state, the image will be loaded thus a new network request appears.

If I understand the feature suggestion correctly - the Network panel should indicate when an HTTP requests (e.g. for images) where caused by lazy loading strategy (e.g. img element using loading="lazy"). In such case we'll just display "lazy" (or similar label) for such requests.

This means that the net panel isn't that much interested in set of images that are supposed to be lazy loaded - rather in a flag that says that actual HTTP request for image has triggered due to lazy loading technique (e.g. channel.loadInfo.lazyLoad == true)

@Harald, is that accurate?

I am not sure about the actually way to send the notifications, probably need to ask someone familiar with network or DOM.

The network panel is usually consuming notifications through nsIObserverService
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIObserverService

e.g. http-on-modify-request, http-on-opening-request, etc.
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Observer_Notifications

But, not sure whether we need a new notification, I am rather expecting a flag ...

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)

I just realized in the network panel there is a column to show the filename of the image (I guess it's picked from src attribute), and the filename can be changed during lazy-loading state. So I am not quite sure what we should do for users in such cases, and for the notification we need to send a notification that src attribute is changed too.

The Network panel is not using DOM elements, it's solely consuming Necko events about HTTP requests so, the name is URL from nsIChannel (HTTP request)

Honza

Comment 6

[:Harald Kirschner :digitarald]

@Harald, is that accurate?

Yes. We already have a cause field that we would probably use for that.

Comment 7

[:Harald Kirschner :digitarald]

Is this something your team can help with or should we look to other teams, like Necko?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add `lazy` prefx for lazy loading images

I was totally misunderstanding what we need in the network panel. I didn't even know there is Cause column. :/

So, if I understand correctly we just want to show a different string in the Cause column? If the corresponding image data hasn't started loading, we don't need to show anything right?

Then, it's pretty easy to tell whether the corresponding image is lazy-loading or not, we just need to check the channel.loadFlags.

Here is a patch to append lazy- prefix string for lazy loading image. I hope this can help you guys what we have to do in devtools.

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

Hiro, this is great, thanks for the update!

Harald, this looks nice&simple, should we polish the label (e.g. show rather img (lazy)) and land?
I am attaching a screenshot.

Honza

Comment 10

[:Harald Kirschner :digitarald]

Thank you for the screenshot. Let's land this as is and revisit when we have other fields like preload. Lazy loading only applies to images, so this works well as is.

Comment 11

[:Harald Kirschner :digitarald]

Honza, do you what's needed to land this?

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Hiro, can you please uplift the attached patch to Phab and ask me for review, please?
It works just fine and we can land it.

Please let me know even if you don't have time to work on this, I can find someone else to land it.

Thanks!
Honza

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1615305 - Add `lazy-` prefix for lazy-loading images. r?honza

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Done. And I also pushed a try; https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d3505d9e1d59e37f13d31b10a076fd4410728ee

Comment 15

[Hiroyuki Ikezoe (:hiro)]

I didn't notice that html_cause-test-page.html I touched is also used for browser_net_initiator.js test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d06b5b41b6e53b447c32a9ca80b1c71515b3e78

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Here is a try with a proper test fix.
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=295871762&revision=8edaacf597c95a5ddde8a3699afa4491f6bccbf3

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Thanks Hiro, this is great help!

The patch is R+ but, let's land it after 77 merge (next week Tuesday)

Honza

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcff183d3130
Add `lazy-` prefix for lazy-loading images. r=Honza

Comment 19

[Andrei Ciure[:aciure]]

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