Closed Bug 1259246 Opened 8 years ago Closed 8 years ago

simplify nsDocumentViewer::FindContainerView

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(2 files)

While working on bug 1234622 I noticed a bunch of simplifications we can make.
[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+
(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)
https://hg.mozilla.org/mozilla-central/rev/ffcc3708103a
https://hg.mozilla.org/mozilla-central/rev/4bbf54c1bcd9
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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 → ---
https://hg.mozilla.org/mozilla-central/rev/e0ec1a2d3b94
https://hg.mozilla.org/mozilla-central/rev/25c1e1cbb744
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.