Bug 157257

Summary: Change IDL enumerations to be nested in their C++ class instead of at WebCore namespace level
Product: WebKit Reporter: Darin Adler <darin>
Component: BindingsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, cdumez
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Darin Adler 2016-05-02 00:36:48 PDT
Change IDL enumerations to be nested in their C++ class instead of at WebCore namespace level
Comment 1 Darin Adler 2016-05-02 01:01:12 PDT
Created attachment 277897 [details]
Patch
Comment 2 Darin Adler 2016-05-02 01:01:50 PDT
OK, Chris, Alex, here is the change we discussed.
Comment 3 Darin Adler 2016-05-02 07:19:02 PDT
Build failures on GTK and iOS should be quick to fix; just a couple call sites that need the class prefixes.
Comment 4 Darin Adler 2016-05-02 08:26:18 PDT
Created attachment 277907 [details]
Patch
Comment 5 Darin Adler 2016-05-02 08:27:06 PDT
New patch may fix the GTK and iOS builds.
Comment 6 Darin Adler 2016-05-02 08:47:11 PDT
Created attachment 277908 [details]
Patch
Comment 7 Chris Dumez 2016-05-02 08:59:42 PDT
iOS still does not build:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:404:75: error: no type named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
static inline HTMLMediaElementEnums::VideoFullscreenMode toFullscreenMode(HTMLMediaElement::VideoPresentationMode mode)
                                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                          HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:407:10: error: no member named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
    case HTMLMediaElement::VideoPresentationMode::Fullscreen:
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:409:10: error: no member named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
    case HTMLMediaElement::VideoPresentationMode::PictureInPicture:
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:411:10: error: no member named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
    case HTMLMediaElement::VideoPresentationMode::Inline:
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:436:8: error: no type named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
static HTMLMediaElement::VideoPresentationMode toPresentationMode(HTMLMediaElement::VideoFullscreenMode mode)
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
       HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:439:16: error: no member named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
        return HTMLMediaElement::VideoPresentationMode::Fullscreen;
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
               HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:442:16: error: no member named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
        return HTMLMediaElement::VideoPresentationMode::PictureInPicture;
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
               HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:445:16: error: no member named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
        return HTMLMediaElement::VideoPresentationMode::Inline;
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
               HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:448:12: error: no member named 'VideoPresentationMode' in 'WebCore::HTMLMediaElement'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
    return HTMLMediaElement::VideoPresentationMode::Inline;
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:451:1: error: unknown type name 'VideoPresentationMode'; did you mean 'HTMLVideoElement::VideoPresentationMode'?
VideoPresentationMode HTMLVideoElement::webkitPresentationMode() const
^~~~~~~~~~~~~~~~~~~~~
HTMLVideoElement::VideoPresentationMode
In file included from /Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.cpp:28:
/Volumes/Data/EWS/WebKit/Source/WebCore/html/HTMLVideoElement.h:81:16: note: 'HTMLVideoElement::VideoPresentationMode' declared here
    enum class VideoPresentationMode { Fullscreen, PictureInPicture, Inline };
               ^
10 errors generated.
Comment 8 Darin Adler 2016-05-02 09:00:59 PDT
Created attachment 277909 [details]
Patch
Comment 9 Chris Dumez 2016-05-02 09:21:43 PDT
Comment on attachment 277909 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277909&action=review

r=me

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:815
> +sub PrefixIsWordsThatMatchStartOrEndWords

I find this name to bit unclear. If we cannot find a better name, I suggest a comment explaining what this routine does.
Comment 10 Chris Dumez 2016-05-02 09:22:43 PDT
(In reply to comment #9)
> Comment on attachment 277909 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277909&action=review
> 
> r=me
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:815
> > +sub PrefixIsWordsThatMatchStartOrEndWords
> 
> I find this name to bit unclear. If we cannot find a better name, I suggest
> a comment explaining what this routine does.

BTW, I think the code looks much nicer now after this change.

However, it looks like iOS is still red.
Comment 11 Darin Adler 2016-05-02 09:44:15 PDT
I’ll land this tonight, I guess, after fixing iOS.
Comment 12 Darin Adler 2016-05-02 22:20:40 PDT
Created attachment 277982 [details]
Patch
Comment 13 Darin Adler 2016-05-02 22:35:23 PDT
Will land this as soon as I see the iOS build succeeding on the EWS.
Comment 14 Darin Adler 2016-05-02 22:46:59 PDT
Committed r200361: <http://trac.webkit.org/changeset/200361>