Open
Bug 779684
Opened 11 years ago
Updated 4 months ago
Decom nsIFrameTraversal
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
REOPENED
mozilla17
People
(Reporter: dzbarsky, Unassigned)
References
Details
Attachments
(3 files, 1 obsolete file)
45.20 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
6.95 KB,
patch
|
Details | Diff | Splinter Review | |
8.11 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•11 years ago
|
||
Attachment #648138 -
Flags: review?(roc)
Comment 2•11 years ago
|
||
Isn't the entire existence of nsIFrameTraversal extra XPCOM that we don't need?
Comment on attachment 648138 [details] [diff] [review] Patch Review of attachment 648138 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I think we could get rid of nsIFrameEnumerator and have two concrete classes: nsFrameIterator and a subclass nsFrameIteratorVisual that aren't refcounted and are always stack-allocated. That would require a tiny amount of extra code in nsIFrame::GetFrameFromDirection. Alternatively we could have a 'visual' flag in nsFrameIterator and just use conditional branches in Get(First/Last)(Child/Sibling)Inner--- whatever gives simpler code. I'd go for the latter actually, the conditional branch is probably faster than the virtual call anyway.
Reporter | ||
Comment 4•11 years ago
|
||
Ah, I didn't realize we don't need nsIFrameEnumerator at all. Ok, I'll do what you suggested.
Also instead of boolean parameters we should have a flags word parameter.
Reporter | ||
Comment 6•11 years ago
|
||
I've made the changes you suggested.
Attachment #648138 -
Attachment is obsolete: true
Attachment #648138 -
Flags: review?(roc)
Attachment #648990 -
Flags: review?(roc)
Comment on attachment 648990 [details] [diff] [review] Patch Review of attachment 648990 [details] [diff] [review]: ----------------------------------------------------------------- r+ with that. Excellent! ::: layout/base/nsFrameTraversal.h @@ +19,5 @@ > +typedef PRUint32 FrameIteratorFlags; > +#define ITERATOR_FLAGS_NONE 0 > +#define ITERATOR_FLAGS_LOCKSCROLL 1 << 1 > +#define ITERATOR_FLAGS_FOLLOWOOF 1 << 2 > +#define ITERATOR_FLAGS_VISUAL 1 << 3 These aren't namespaced properly. I suggest using an enum in nsFrameIterator to define the values, say FLAG_NONE, FLAG_LOCK_SCROLL, FLAG_FOLLOW_OUT_OF_FLOW and FLAG_VISUAL.
Attachment #648990 -
Flags: review?(roc) → review+
Reporter | ||
Comment 8•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f0e5c7874d
Comment 9•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3f0e5c7874d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 10•11 years ago
|
||
This causes build errors for me: /Users/smontagu/mozwork/inbound/mozilla/dom/base/nsFocusManager.cpp: In member function ‘nsresult nsFocusManager::GetSelectionLocation(nsIDocument*, nsIPresShell*, nsIContent**, nsIContent**)’: /Users/smontagu/mozwork/inbound/mozilla/dom/base/nsFocusManager.cpp:2275: error: ‘FrameIteratorFlags’ is not a class or namespace /Users/smontagu/mozwork/inbound/mozilla/dom/base/nsFocusManager.cpp: In member function ‘nsresult nsFocusManager::GetNextTabbableContent(nsIPresShell*, nsIContent*, nsIContent*, nsIContent*, bool, PRInt32, bool, nsIContent**)’: /Users/smontagu/mozwork/inbound/mozilla/dom/base/nsFocusManager.cpp:2701: error: ‘FrameIteratorFlags’ is not a class or namespace
Reporter | ||
Comment 11•11 years ago
|
||
What compiler is this? Have you tried clobbering?
Comment 12•11 years ago
|
||
Xcode 3.2.6, and clobbering doesn't help.
Comment 13•11 years ago
|
||
I get the same build error ... mac os x 10.7.4, clang 3.0
Reporter | ||
Comment 14•11 years ago
|
||
Can you try removing the |FrameIteratorFlags| part in nsFocusManager? So just have |FLAG_NONE| by itself, etc. The enum values may be getting dumped into the enclosing scope.
Comment 15•11 years ago
|
||
with this it builds for me
Updated•11 years ago
|
Attachment #649310 -
Attachment is patch: true
Comment 16•11 years ago
|
||
Moving |enum FrameIteratorFlags| to nsFrameIterator class and replacing FrameIteratorFlags:: to nsFrameIterator:: should work too
Comment 17•11 years ago
|
||
(In reply to Jan Varga [:janv] from comment #16) > Moving |enum FrameIteratorFlags| to nsFrameIterator class and replacing > FrameIteratorFlags:: to nsFrameIterator:: should work too I think, this is what Robert suggested to do I'll attach a patch in a minute ...
Comment 18•11 years ago
|
||
Reporter | ||
Comment 20•11 years ago
|
||
Jan, you're right, I should have done that originally. Thanks for fixing.
Updated•11 years ago
|
Attachment #649319 -
Flags: review?(roc)
Attachment #649319 -
Flags: review?(roc) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 21•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf69c3fd814
Keywords: checkin-needed
Reporter | ||
Comment 23•11 years ago
|
||
Backed out for causing bug 787822 https://hg.mozilla.org/integration/mozilla-inbound/rev/21cb609fe669
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 24•11 years ago
|
||
(In reply to David Zbarsky (:dzbarsky) from comment #23) > Backed out for causing bug 787822 > https://hg.mozilla.org/integration/mozilla-inbound/rev/21cb609fe669 https://hg.mozilla.org/mozilla-central/rev/21cb609fe669
Comment 25•9 months ago
|
||
The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.
Assignee: dzbarsky → nobody
Updated•4 months ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•