1163572 - Override RequestContentRepaint to detect root-frame updates

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Danilo Cesar Lemes de Paula]

Override RequestContentRepaint to detect root-frame updates (see
TabChild::UpdateFrameHandler) and call UpdateRootFrame when appropriate

Comment 1

[Danilo Cesar Lemes de Paula]

Attachment: wip.patch

Comment 2

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1163572.patch

Comment 3

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1163572.patch

sent the wrong file... This is the right one

Comment 4

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

Comment on attachment 8604068 [details] [diff] [review]
Bug_1163572.patch

Review of attachment 8604068 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine, but as a follow-up we should change the signature of UpdateRootFrame to take a nsIContent* instead of nsIPresShell* and reuse more code that way. At the TabChild callsite, TabChild::GetPresShell() should return the same value as the shell we get from the nsIContent* so there shouldn't be a functional change - but I could be wrong.

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +83,5 @@
> +  if (aFrameMetrics.GetIsRoot()) {
> +      nsCOMPtr<nsIDocument> doc = targetContent->GetComposedDoc();
> +      nsCOMPtr<nsIPresShell> shell = doc ? doc->GetShell() : nullptr;
> +      if (shell) {
> +          if (aFrameMetrics.GetPresShellId() == shell->GetPresShellId()) {

Combine the two if conditions with &&

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8604068 [details] [diff] [review]
Bug_1163572.patch

Review of attachment 8604068 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/ChromeProcessController.cpp
@@ +78,5 @@
>    nsCOMPtr<nsIContent> targetContent = nsLayoutUtils::FindContentFor(aFrameMetrics.GetScrollId());
> +  FrameMetrics metrics = aFrameMetrics;
> +  // TODO: https://bugzilla.mozilla.org/show_bug.cgi?id=1158424
> +  // When that bug is ready, it should give us the difference between mIsRoot
> +  // and mIsContentRoot. The former should be used here.

According to the plan laid out in bug 1158424, calling UpdateRootFrame should happen for frames with mIsRootContent, not mIsProcessRoot. mIsRoot is going to be renamed to mIsRootContent, so I don't think any changes to this code will be needed (beyond being part of the rename), and I don't think this comment is necessary.

Comment 6

[Danilo Cesar Lemes de Paula]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8604068 [details] [diff] [review]
> Bug_1163572.patch
> 
> Review of attachment 8604068 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine, but as a follow-up we should change the signature of
> UpdateRootFrame to take a nsIContent* instead of nsIPresShell* and reuse
> more code that way. 
OK!

> At the TabChild callsite, TabChild::GetPresShell()
> should return the same value as the shell we get from the nsIContent* so
> there shouldn't be a functional change - but I could be wrong.
I can add some printfs to check if that's true.

 
> ::: gfx/layers/apz/util/ChromeProcessController.cpp
> @@ +83,5 @@
> > +  if (aFrameMetrics.GetIsRoot()) {
> > +      nsCOMPtr<nsIDocument> doc = targetContent->GetComposedDoc();
> > +      nsCOMPtr<nsIPresShell> shell = doc ? doc->GetShell() : nullptr;
> > +      if (shell) {
> > +          if (aFrameMetrics.GetPresShellId() == shell->GetPresShellId()) {
> 
> Combine the two if conditions with &&

OK

Comment 7

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1163572.patch

r+ from latest patch.

Comment 8

[Danilo Cesar Lemes de Paula]

Try server
remote:   https://hg.mozilla.org/try/rev/e08d5fd1db8c
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08d5fd1db8c

Comment 9

[Danilo Cesar Lemes de Paula]

A better try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2bcf89e1c37

Comment 10

[Carsten Book [:Tomcat]]

Hi, this patch failed to apply:

patching file gfx/layers/apz/util/APZCCallbackHelper.cpp
Hunk #2 FAILED at 201
1 out of 2 hunks FAILED -- saving rejects to file gfx/layers/apz/util/APZCCallbackHelper.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh Bug_1163572.patch

Could you take a look, thanks!

Comment 11

[Danilo Cesar Lemes de Paula]

Sure.

Comment 12

[Danilo Cesar Lemes de Paula]

That's odd, I was able to apply the patch without problems and was also able to rebase with today's mozila-central. Also Try didn't complain about it.

I'm sending it again anyway..

Comment 13

[Danilo Cesar Lemes de Paula]

Attachment: Bug_1163572.patch

r+ from latest review.

Try submission:
https://hg.mozilla.org/try/rev/d98d1af3ebb8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d98d1af3ebb8

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/99011d150a1d

Comment 15

[Carsten Book [:Tomcat]]

sorry had to back this out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=10309866&repo=mozilla-inbound

Comment 16

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d4f25a9ef1f

Comment 17

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

I can take a look into this.

Comment 18

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

The call to FindContentFor in TabChild was asserting because the metrics had a NULL_SCROLL_ID. In general the NULL_SCROLL_ID checks seem to be all over the place, I'll see if I can clean this up.

Comment 19

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

I split Danilo's patch into two parts with some modifications and added a third part as well. Try push at [1]; I want to finish up bug 1055557 as well before uploading patches and requesting review, in case fixing that uncovers errors here.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdbd184a4e18

Comment 20

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

The above try push had some failures. I diagnosed one and fixed it in this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9f0798ff632 but apparently there's still another problem. It doesn't help that the log doesn't provide any useful information.

Comment 21

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

Attachment: Part 1 - Stop passing nsIPresShell to UpdateRootFrame

Comment 22

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

Attachment: Part 2 - Have ChromeProcessController handle root-frame requests

Comment 23

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

Attachment: Part 3 - UpdateSubFrame doesn't need an nsIContent

Comment 24

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

Try push with these patches is green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=158ae1c9cc5a

Comment 25

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

Attachment: Part 1 - Stop passing nsIPresShell to UpdateRootFrame

Comment 26

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

Attachment: Part 2 - Have ChromeProcessController handle root-frame requests

Comment 27

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

Attachment: Part 3 - UpdateSubFrame doesn't need an nsIContent

Comment 28

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

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d40c3d2be64f includes these patches.

Comment 29

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

Here's a better try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0f8759aba23

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8621656 [details] [diff] [review]
Part 1 - Stop passing nsIPresShell to UpdateRootFrame

Review of attachment 8621656 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +179,5 @@
>    if (aMetrics.GetScrollId() == FrameMetrics::NULL_SCROLL_ID) {
>      return;
>    }
> +  nsIContent* content = nsLayoutUtils::FindContentFor(aMetrics.GetScrollId());
> +  MOZ_ASSERT(content);

Prior to this, we'd handle a null content gracefully (ScrollFrame() checks for it). Deliberate change?

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8621660 [details] [diff] [review]
Part 3 - UpdateSubFrame doesn't need an nsIContent

Review of attachment 8621660 [details] [diff] [review]:
-----------------------------------------------------------------

I like the interface simplification here!

::: gfx/layers/apz/util/APZCCallbackHelper.cpp
@@ +231,5 @@
> +    return;
> +  }
> +  nsIContent* content = nsLayoutUtils::FindContentFor(aMetrics.GetScrollId());
> +  if (!content) {
> +    return;

And here we're becoming more permissive :)

Comment 32

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

(In reply to Botond Ballo [:botond] from comment #30)
> Prior to this, we'd handle a null content gracefully (ScrollFrame() checks
> for it). Deliberate change?

Oops, not really deliberate. I'll change it to be consistent with UpdateSubFrame and do graceful handling the same way.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/362921d1696b
https://hg.mozilla.org/integration/mozilla-inbound/rev/be8d3f286a35
https://hg.mozilla.org/integration/mozilla-inbound/rev/24bd667a035a

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/362921d1696b
https://hg.mozilla.org/mozilla-central/rev/be8d3f286a35
https://hg.mozilla.org/mozilla-central/rev/24bd667a035a