Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass the schedule time of an element to the element itself #11959

Merged
merged 2 commits into from Nov 9, 2017

Conversation

warrengm
Copy link
Contributor

@warrengm warrengm commented Nov 7, 2017

Passes the time at which layout was scheduled for an element to the element itself. This will be used to measure the time between when layout was scheduled and when layout was completed.

@dvoytenko
Copy link
Contributor

/to @jridgewell for review.

@@ -1912,7 +1912,7 @@ export class Resources {
this.queue_.enqueue(task);
this.schedulePass(this.calcTaskTimeout_(task));
}
task.resource.layoutScheduled();
task.resource.layoutScheduled(task.scheduleTime);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this will overwrite previous schedules. IE, a preload (low priority layout) happens and the element is scheduled, but then the user scrolls and now we schedule a full layout for the element. Which should be the scheduled time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that example I'd prefer the full layout to be the recorded scheduleTime. The ultimate goal is to get the time from when layout was scheduled to when the element finished loading, not including any preloading while the element was hidden.

One edge case that (I think is possible) is when an element is unlaid-out and then scheduler schedules it a second time. But that should happen off-screen (right?) so taking the most recent scheduleTime makes sense to me.

The following case would be undesirable: if the scheduler queues up a layout while a full layout is being executed. Is that possible?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ultimate goal is to get the time from when layout was scheduled to when the element finished loading, not including any preloading while the element was hidden.

What do you mean by hidden? When I say preload, I mean exactly the same thing as a regular layout, but with lower priority in the task queue. As in, layouts are p1 tasks, while preloads (which are layouts and run the same code) are assigned p2. Later scheduled p1s can preempt p2s as you would expect, and p1s and p2s (generally) run in the order they are scheduled (minus preempting).

One edge case that (I think is possible) is when an element is unlaid-out and then scheduler schedules it a second time

I'm assuming you're going to be using this time in #layoutCallback? Then the scheduled time will be when the second layout is scheduled (after it's un-laid out).

if the scheduler queues up a layout while a full layout is being executed. Is that possible

No. A preload can be reassigned to a p1 (in which case, it's exactly the same as a if a normal layout were scheduled), but once it's "layout priority" there's nothing else for it to change to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the info! By hidden, I meant an element that is off the viewport. If I understand correctly, an element might be preloaded before it enters some margin of the viewport (is the margin 3 viewports?). But after the element enters that margin, the element will begin a P1 layout. In that case, I want to use the time at which the P1 layout was scheduled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The margin is element defined. For images, it's anywhere on the page, for facebook it's .75 viewports, ads are 1.5 (I think).

@jridgewell jridgewell merged commit 1e4404e into ampproject:master Nov 9, 2017
neko-fire pushed a commit to 3qnexx/amphtml that referenced this pull request Nov 17, 2017
…t#11959)

* Pass the time at which an element was scheduled to be laid out to the element itself.

* Update comment
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
…t#11959)

* Pass the time at which an element was scheduled to be laid out to the element itself.

* Update comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants