Closed Bug 1163572 Opened 5 years ago Closed 5 years ago

Override RequestContentRepaint to detect root-frame updates

Categories

(Core :: Panning and Zooming, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: danilo.eu, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(3 files, 8 obsolete files)

7.69 KB, patch
botond
: review+
Details | Diff | Splinter Review
3.14 KB, patch
botond
: review+
Details | Diff | Splinter Review
5.85 KB, patch
botond
: review+
Details | Diff | Splinter Review
Override RequestContentRepaint to detect root-frame updates (see
TabChild::UpdateFrameHandler) and call UpdateRootFrame when appropriate
Attached patch wip.patch (obsolete) — Splinter Review
Attachment #8604064 - Flags: review?(bugmail.mozilla)
Attachment #8604064 - Flags: review?(botond)
Blocks: apz-fennec
Attached patch Bug_1163572.patch (obsolete) — Splinter Review
Attachment #8604064 - Attachment is obsolete: true
Attachment #8604064 - Flags: review?(bugmail.mozilla)
Attachment #8604064 - Flags: review?(botond)
Attachment #8604067 - Flags: review?(bugmail.mozilla)
Attachment #8604067 - Flags: review?(botond)
Attached patch Bug_1163572.patch (obsolete) — Splinter Review
sent the wrong file... This is the right one
Attachment #8604067 - Attachment is obsolete: true
Attachment #8604067 - Flags: review?(bugmail.mozilla)
Attachment #8604067 - Flags: review?(botond)
Attachment #8604068 - Flags: review?(bugmail.mozilla)
Attachment #8604068 - Flags: review?(botond)
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 &&
Attachment #8604068 - Flags: review?(bugmail.mozilla) → review+
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.
Attachment #8604068 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Attached patch Bug_1163572.patch (obsolete) — Splinter Review
r+ from latest patch.
Attachment #8604068 - Attachment is obsolete: true
Attachment #8604733 - Flags: review+
Whiteboard: [gfx-noted]
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
Assignee: nobody → danilo.eu
Keywords: checkin-needed
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!
Flags: needinfo?(danilo.eu)
Keywords: checkin-needed
Sure.
Flags: needinfo?(danilo.eu)
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..
Attached patch Bug_1163572.patch (obsolete) — Splinter Review
r+ from latest review.

Try submission:
https://hg.mozilla.org/try/rev/d98d1af3ebb8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d98d1af3ebb8
Attachment #8604733 - Attachment is obsolete: true
Attachment #8612277 - Flags: review+
Keywords: checkin-needed
sorry had to back this out for crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=10309866&repo=mozilla-inbound
Flags: needinfo?(danilo.eu)
I can take a look into this.
Assignee: danilo.eu → bugmail.mozilla
Flags: needinfo?(danilo.eu)
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.
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
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 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?
Attachment #8621656 - Flags: review?(botond) → review+
Attachment #8621658 - Flags: review?(botond) → review+
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 :)
Attachment #8621660 - Flags: review?(botond) → review+
(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.
You need to log in before you can comment on or make changes to this bug.