Closed
Bug 1259246
Opened 8 years ago
Closed 8 years ago
simplify nsDocumentViewer::FindContainerView
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: tnikkel, Assigned: tnikkel)
References
Details
Attachments
(2 files)
9.06 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.21 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
While working on bug 1234622 I noticed a bunch of simplifications we can make.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8734123 -
Flags: review?(dholbert)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8734124 -
Flags: review?(dholbert)
Comment 3•8 years ago
|
||
[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 4•8 years ago
|
||
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 5•8 years ago
|
||
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•8 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.
Comment 7•8 years ago
|
||
Cool, no worries; the archeology wasn't too intense. (it doesn't date back to prehgstoric times)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffcc3708103a https://hg.mozilla.org/integration/mozilla-inbound/rev/4bbf54c1bcd9
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ffcc3708103a https://hg.mozilla.org/mozilla-central/rev/4bbf54c1bcd9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 10•8 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 → ---
Assignee | ||
Comment 11•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d813b20917fc269ebc213cb3013b1cb3b056a99c
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0ec1a2d3b94 https://hg.mozilla.org/integration/mozilla-inbound/rev/25c1e1cbb744
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/e0ec1a2d3b94 https://hg.mozilla.org/mozilla-central/rev/25c1e1cbb744
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•