Status

()

REOPENED
7 years ago
6 years ago

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

7 years ago
Created attachment 648138 [details] [diff] [review]
Patch
Attachment #648138 - Flags: review?(roc)
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.
(Assignee)

Comment 4

7 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.
(Assignee)

Comment 6

7 years ago
Created attachment 648990 [details] [diff] [review]
Patch

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+
https://hg.mozilla.org/mozilla-central/rev/b3f0e5c7874d
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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
(Assignee)

Comment 11

7 years ago
What compiler is this?  Have you tried clobbering?
Xcode 3.2.6, and clobbering doesn't help.
I get the same build error ...
mac os x 10.7.4, clang 3.0
(Assignee)

Comment 14

7 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.
Created attachment 649310 [details] [diff] [review]
Remove |FrameIteratorFlags|

with this it builds for me
Attachment #649310 - Attachment is patch: true
Moving |enum FrameIteratorFlags| to nsFrameIterator class and replacing FrameIteratorFlags:: to nsFrameIterator:: should work too
(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 ...
Created attachment 649319 [details] [diff] [review]
move FrameIteratorFlags to nsFrameIterator class
(Assignee)

Comment 20

7 years ago
Jan, you're right, I should have done that originally.  Thanks for fixing.

Updated

7 years ago
Attachment #649319 - Flags: review?(roc)
Keywords: checkin-needed

Updated

6 years ago
Depends on: 787822
(Assignee)

Comment 23

6 years ago
Backed out for causing bug 787822
https://hg.mozilla.org/integration/mozilla-inbound/rev/21cb609fe669
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
You need to log in before you can comment on or make changes to this bug.