779684 - Decom nsIFrameTraversal

Status: REOPENED

Description

[David Zbarsky (:dzbarsky)]

Comment 1

[David Zbarsky (:dzbarsky)]

Created attachment 648138 [details] [diff] [review]
Patch

Comment 2

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

Isn't the entire existence of nsIFrameTraversal extra XPCOM that we don't need?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 4

[David Zbarsky (:dzbarsky)]

Ah, I didn't realize we don't need nsIFrameEnumerator at all.  Ok, I'll do what you suggested.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also instead of boolean parameters we should have a flags word parameter.

Comment 6

[David Zbarsky (:dzbarsky)]

Created attachment 648990 [details] [diff] [review]
Patch

I've made the changes you suggested.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 8

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f0e5c7874d

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b3f0e5c7874d

Comment 10

[Simon Montagu :smontagu]

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

Comment 11

[David Zbarsky (:dzbarsky)]

What compiler is this?  Have you tried clobbering?

Comment 12

[Simon Montagu :smontagu]

Xcode 3.2.6, and clobbering doesn't help.

Comment 13

[Jan Varga [:janv]]

I get the same build error ...
mac os x 10.7.4, clang 3.0

Comment 14

[David Zbarsky (:dzbarsky)]

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

[Simon Montagu :smontagu]

Created attachment 649310 [details] [diff] [review]
Remove |FrameIteratorFlags|

with this it builds for me

Comment 16

[Jan Varga [:janv]]

Moving |enum FrameIteratorFlags| to nsFrameIterator class and replacing FrameIteratorFlags:: to nsFrameIterator:: should work too

Comment 17

[Jan Varga [:janv]]

(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

[Jan Varga [:janv]]

Created attachment 649319 [details] [diff] [review]
move FrameIteratorFlags to nsFrameIterator class

Comment 19

[Jan Varga [:janv]]

https://tbpl.mozilla.org/?tree=Try&rev=68c3f10a92c5

Comment 20

[David Zbarsky (:dzbarsky)]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf69c3fd814

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8bf69c3fd814

Comment 23

[David Zbarsky (:dzbarsky)]

Backed out for causing bug 787822
https://hg.mozilla.org/integration/mozilla-inbound/rev/21cb609fe669

Comment 24

[Ed Morley [:emorley]]

(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