simplify nsDocumentViewer::FindContainerView

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tnikkel, Assigned: tnikkel)

Tracking

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
While working on bug 1234622 I noticed a bunch of simplifications we can make.
(Assignee)

Comment 1

2 years ago
Created attachment 8734123 [details] [diff] [review]
Refactor nsIPresShell::GetRealPrimaryFrameFor
Attachment #8734123 - Flags: review?(dholbert)
(Assignee)

Comment 2

2 years ago
Created attachment 8734124 [details] [diff] [review]
Simplify nsDocumentViewer::FindContainerView even more
Attachment #8734124 - Flags: review?(dholbert)
[Adding dependency on bug 1234622, because it looks like the patches don't apply cleanly unless I layer them on top of that bug's patch.]
Depends on: 1234622
Comment on attachment 8734123 [details] [diff] [review]
Refactor nsIPresShell::GetRealPrimaryFrameFor

>-PresShell::GetRealPrimaryFrameFor(nsIContent* aContent) const
>-{
>-  if (aContent->GetComposedDoc() != GetDocument()) {
>-    return nullptr;
>-  }

The only thing I was wondering about here was, why do we have this early-return (which you're having to remove in this patch as part of refactoring)?

Did some HG Archeology to trace back the history of this code. It dates back to before bug 500882 -- at that point, content *had* to go through the PresShell/FrameManager to find out what its primary frame was. So, that's why this presshell API exists.

When bug 500882 created nsIContent::GetPrimaryFrame, it added this early-return as well, presumably as a precaution & to preserve prior behavior of this API (because FrameManager would presumably have returned null if it was given a nsIContent from the wrong document):
 http://hg.mozilla.org/mozilla-central/diff/8b6f32659aa6/layout/base/nsPresShell.cpp#l1.104

But this early-return doesn't seem to have been particularly important (and doesn't seem to matter for the one remaining caller, the caller that you're simplifying here). So I think we're good to clean this up.

r=me
Attachment #8734123 - Flags: review?(dholbert) → review+
Comment on attachment 8734124 [details] [diff] [review]
Simplify nsDocumentViewer::FindContainerView even more

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

r=me
Attachment #8734124 - Flags: review?(dholbert) → review+
(Assignee)

Comment 6

2 years ago
(In reply to Daniel Holbert [:dholbert] from comment #4)
> The only thing I was wondering about here was, why do we have this
> early-return (which you're having to remove in this patch as part of
> refactoring)?
> 
> Did some HG Archeology to trace back the history of this code.

Oh yeah, sorry, I did the same digging and came to the same conclusion. I guess I didn't explain that anywhere, I could have saved you the time.
Cool, no worries; the archeology wasn't too intense. (it doesn't date back to prehgstoric times)

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ffcc3708103a
https://hg.mozilla.org/mozilla-central/rev/4bbf54c1bcd9
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
(Assignee)

Comment 10

2 years ago
Backed this out to see if it's causing bug 1262027.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d813b20917fc269ebc213cb3013b1cb3b056a99c
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0ec1a2d3b94
https://hg.mozilla.org/mozilla-central/rev/25c1e1cbb744
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.