1257383 - Get rid of ScrollableLayerGuid::mPresShellId

Status: RESOLVED WONTFIX

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

When subframe scrolling was introduced in bug 866232, the pres shell id was made of the components of ScrollableLayerGuid, the key that is meant to uniquely identify a scroll frame in the compositor.

The reasoning that was given at the time is (bug 866232 comment 9):

  "In theory we might be able to do away with presShellId and just use 
   the scrollId but then there's special scrollIds like ROOT_SCROLL_ID 
   and such that we'd have to disambiguate. I felt it was better to 
   just keep all three."

ROOT_SCROLL_ID was removed in bug 900092. The only other "special" scroll id is NULL_SCROLL_ID, but that's interpreted as "no scroll frame", which there's no need to disambiguate with another identifier.

I therefore propose removing ScrollableLayerGuid::mPresShellId.

Note that I'm *not* proposing removing pres shell ids altogether. In addition to being used in ScrollableLayerGuid, they are stored as a member of FrameMetrics, and as far as I can tell, *that* use is still relevant [1].

[1] https://dxr.mozilla.org/mozilla-central/rev/341344bdec8f10bf50646cd6ef2355361435cbf6/dom/ipc/TabChild.cpp#245

Comment 1

[Botond Ballo [:botond]]

Kats, let me know if you agree with the reasoning here.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Yup I think that makes sense, we should be able to drop the presshell id from the guid if we leave it in the FM.

Comment 3

[Botond Ballo [:botond]]

In the short time since I've filed this bug, ScrollableLayerGuid has acquired another client (in bug 1257315) which does use mPresShellId. Given that, I'm now inclined to WONTFIX this unless some objects. (The alternative is to split ScrollableLayerGuid into two classes, one that stores the pres shell id and one that doesn't.)

Comment 4

[Kartikaya Gupta (email:[e-mail])]

WONTFIX'ing per comment 3, feel free to reopen if you still want to do this.