Closed Bug 1361051 Opened 7 years ago Closed 7 years ago

newly added mozilla::FrameType shadows existing mozilla::dom::FrameType

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: bkelly, Assigned: emilio)

References

Details

Attachments

(1 file, 3 obsolete files)

Bug 1361051: rename mozilla::FrameType to mozilla::LayoutFrameType.
59 bytes, text/x-review-board-request
xidorn
: review+
Details 
Bug 1360241 just added the mozilla::FrameType type to the tree.  This is a problem because there is an existing mozilla::dom::FrameType defined in a webidl binding.  Having the new type shadow the old type makes it much harder to work on code that might get the headers from both included.

Please change mozilla::FrameType to mozilla::layout::FrameType or something.
Flags: needinfo?(emilio+bugs)
For example, when trying to build my wip patches in bug 1293277 I now get this:

 9:53.58 In file included from /srv/mozilla-central/obj-x86_64-pc-linux-gnu-optdebug/dom/base/Unified_cpp_dom_base1.cpp:101:0:
 9:53.58 /srv/mozilla-central/dom/base/Element.cpp: In member function ‘nsIScrollableFrame* mozilla::dom::Element::GetScrollFrame(nsIFrame**, bool)’:
 9:53.58 /srv/mozilla-central/dom/base/Element.cpp:651:32: error: cannot convert ‘mozilla::FrameTyp ’ to ‘mozilla::dom::FrameType’ in initialization
 9:53.58    FrameType type = frame->Type();
 9:53.58                                 ^
 9:53.58 /srv/mozilla-central/dom/base/Element.cpp:652:15: error: ‘Menu’ is not a member of ‘mozilla::dom::FrameType’
 9:53.58    if (type != FrameType::Menu && type != FrameType::ComboboxControl) {
 9:53.58                ^
 9:53.58 /srv/mozilla-central/dom/base/Element.cpp:652:42: error: ‘ComboboxControl’ is not a member of ‘mozilla::dom::FrameType’
 9:53.58    if (type != FrameType::Menu && type != FrameType::ComboboxControl) {

I haven't touched Element.cpp, but it gets the binding header included via unified build.
Blocks: 1293277
I don't think we want to add mozilla::layout namespace.

It seems to me that we are going to remove FrameType from WebIDL in bug 1290936. Can we just do that instead of doing the renaming?
Flags: needinfo?(bkelly)
(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #2)
> It seems to me that we are going to remove FrameType from WebIDL in bug
> 1290936. Can we just do that instead of doing the renaming?

That is not happening any time soon.  We need ancestorOrigins to finish getting spec'd and implemented first.  And its unclear if ancestorOrigins is really an adequate replacement.  Some people don't want FrameType removed at all.

(In reply to Xidorn Quan [:xidorn] UTC+10 (less responsive 15/Apr-3/May) from comment #2)
> I don't think we want to add mozilla::layout namespace.

It seems to exist already.  From layout/base/nsLayoutUtils.cpp:

using namespace mozilla::layout;

Of course, this file also does:

using namespace mozilla;
using namespace mozilla::dom;

So just moving your new FrameType to a mozilla::layout wouldn't help anyway.  This file would still get confused.
Flags: needinfo?(bkelly)
Can we rename dom::FrameType instead?

The mozilla::css and mozilla::layout namespaces exist, but I think we want to get rid of them IIUC, and this would be a step in the opposite direction.

Seems like mozilla::dom::FrameType is only used in a handful of places, so perhaps we can rename it instead? (not sure how easy it is, or whether the WebIDL bindings generator supports something like that already though).
Flags: needinfo?(emilio+bugs)
This type was previously named nsCSSFrameType.  Why did you rename to mozilla::FrameType instead of mozilla::CSSFrameType?  DOM has had "frames" for a long time.

I can rename mozilla::dom::FrameType, but this requires a name re-mapping in Binding.conf.  This makes it harder to read the webidl/c++.
(In reply to Ben Kelly [not reviewing due to deadline][:bkelly] from comment #5)
> This type was previously named nsCSSFrameType.  Why did you rename to
> mozilla::FrameType instead of mozilla::CSSFrameType?  DOM has had "frames"
> for a long time.

That type was _not_ nsCSSFrameType. The API before I changed how frame types worked is that there was a virtual function that returned a |nsIAtom*|. So I didn't rename the type, I added a new type for what atoms were used before, in order to be able to store them more efficiently.

nsCSSFrameType apparently still exists, and it's a plain integer. Its usage[1] is far lower than mozilla::FrameType's (just look at the size of the patch at bug 1360241).

I guess I can rename to mozilla::CSSFrameType, though I'd prefer not, because I think that's not really representative of its purpose (it's the type of a layout frame, which isn't necessarily/always related to CSS).

> I can rename mozilla::dom::FrameType, but this requires a name re-mapping in
> Binding.conf.  This makes it harder to read the webidl/c++.

I would prefer that, but I understand the concern too... I can definitely rename mozilla::FrameType to something else (LayoutFrameType, perhaps?), though I'd be somewhat hesitant to do so, because the name isn't immediately better, and it's used all over the place, which makes the change noisier...

Anyway I don't want to spend a lot of time bikesheding on what should we do, so perhaps just renaming to LayoutFrameType is the easiest thing...

[1]: http://searchfox.org/mozilla-central/search?q=nsCSSFrameType&case=false&regexp=false&path=
Ok.  I'll rename mozilla::dom::FrameType.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Actually, it turns out we don't have a way to remap webidl enumerations to different names.  We can only do that for interfaces, methods, and attributes right now.
Assignee: bkelly → nobody
Status: ASSIGNED → NEW
I'll rename to LayoutFrameType instead, it's probably good enough :)
Assignee: nobody → emilio+bugs
Just in case, all the four patches are to be squashed before landing.
Comment on attachment 8863421 [details]
Bug 1361051: rename mozilla::FrameType to mozilla::LayoutFrameType.

https://reviewboard.mozilla.org/r/135188/#review138244

The commands look good to me. I also took a brief look at the patch, and it seems fine.
Attachment #8863421 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8863422 [details]
Bug 1361051: clang-format-diff of the previous commit.

https://reviewboard.mozilla.org/r/135190/#review138248

Some changes doesn't really make much sense... but fine.
Attachment #8863422 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8863423 [details]
Bug 1361051: Rename it in dom/base, dom/events and dom/html.

https://reviewboard.mozilla.org/r/135192/#review138250
Attachment #8863423 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8863424 [details]
Bug 1361051: clang-format-diff of that.

https://reviewboard.mozilla.org/r/135194/#review138252
Attachment #8863424 - Flags: review?(xidorn+moz) → review+
Attachment #8863422 - Attachment is obsolete: true
Attachment #8863423 - Attachment is obsolete: true
Attachment #8863424 - Attachment is obsolete: true
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fbc54f707dc4
rename mozilla::FrameType to mozilla::LayoutFrameType. r=xidorn
https://hg.mozilla.org/mozilla-central/rev/fbc54f707dc4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: