Closed Bug 1610285 Opened 4 years ago Closed 2 months ago

For overlay scrollbars "layoutViewport" of Page.getLayoutMetrics should not take scrollbar width and height into account

Categories

(Remote Protocol :: CDP, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: whimboo, Unassigned)

References

Details

Attachments

(3 obsolete files)

With bug 1544417 implemented we can now set specific widths and heights of the browser. But when working on the implementation of the clip option for screenshots (bug 1587845) I noticed that taken screenshots are too narrow compared to Chrome. And that exactly 30 pixels by a device pixel ratio of 2.0. This is exactly twice the size of the scrollbars.

Checking with Chrome I can see that it only applies the scrollbar width/height when actually taking a screenshot.

Side-effect right now is that continuously repeating the Page.setDeviceMetricsOverride and Page.getLayoutMetrics commands causes a reduction of the browser by 15px for each cycle. That should not happen.

Whiteboard: [puppeteer-beta-mvp]

Not actually sure why I can only see that in the Puppeteer's full screenshot example for https://www.nytimes.com/, but maybe related to some reflows? But as it turned out this is only the case for overlay scrollbars. See this code in Blink:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc?l=711-735&rcl=aaa0d1910aaeb9d932c508a59c3f3cc0a37bba77

Maybe this is something we have to look into when it comes to mobile support.

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Depends on: 1608468
Priority: P1 → P3
Summary: layoutViewport of Page.getLayoutMetrics should not take scrollbar width and height into account → For overlay scrollbars "layoutViewport" of Page.getLayoutMetrics should not take scrollbar width and height into account
Whiteboard: [puppeteer-beta-mvp]

nsRange instances are allocated a lot in the heap especially by editor and
spellchecker. The allocation cost is too bad for benchmarks. Therefore,
we should reuse released instances as far as possible. For managing it in
static factory methods of nsRange, we need to hide nsRange constructor.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED

This patch makes nsRange::Create() reuse its instances automatically.
It's difficult to consider the limit of cache since nsRange instance is
created not so many in most cases, but only Find and Spellchecker sometimes
create too many instances.

Depends on D61237

Previously, I added Selection::mCachedRange to save allocation cost of
nsRange. However, with the previous patch, we don't need the hack anymore
since ranges removed by Selection::RemoveAllRanges() are always kept in
the global cache of nsRange. Therefore, we can remove the ugly hack right
now.

Depends on D61238

Sorry for the bug spam, I used wrong bug #.

Assignee: masayuki → nobody
Status: ASSIGNED → NEW

Comment on attachment 9123536 [details]
Bug 1610285 - part 3: Remove dirty hack of Selection::mCachedRange r=smaug!

Revision D61239 was moved to bug 1612085. Setting attachment 9123536 [details] to obsolete.

Attachment #9123536 - Attachment is obsolete: true

Comment on attachment 9123535 [details]
Bug 1610285 - part 2: Make nsRange instances reused r=smaug!

Revision D61238 was moved to bug 1612085. Setting attachment 9123535 [details] to obsolete.

Attachment #9123535 - Attachment is obsolete: true

Comment on attachment 9123534 [details]
Bug 1610285 - part 1: Hide constructor of nsRange r=smaug!

Revision D61237 was moved to bug 1612085. Setting attachment 9123534 [details] to obsolete.

Attachment #9123534 - Attachment is obsolete: true
Component: CDP: Page → CDP
Severity: normal → S3

We are not going to fix this bug for our CDP implementation. One should use WebDriver BiDi these days.

Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: