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

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: bkelly, Assigned: emilio)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

7 months ago
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)
(Reporter)

Comment 1

7 months ago
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)
(Reporter)

Comment 3

7 months ago
(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)
(Assignee)

Comment 4

7 months ago
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)
(Reporter)

Comment 5

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

Comment 6

7 months ago
(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=
(Reporter)

Comment 7

7 months ago
Ok.  I'll rename mozilla::dom::FrameType.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(Reporter)

Comment 8

7 months ago
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
(Assignee)

Comment 9

7 months ago
I'll rename to LayoutFrameType instead, it's probably good enough :)
Assignee: nobody → emilio+bugs
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 14

7 months ago
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+
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8863422 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8863423 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8863424 - Attachment is obsolete: true

Comment 20

7 months ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fbc54f707dc4
rename mozilla::FrameType to mozilla::LayoutFrameType. r=xidorn

Comment 21

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fbc54f707dc4
Status: NEW → RESOLVED
Last Resolved: 7 months 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.