Bug 206310

Created attachment 387837 [details]
Patch

Created attachment 387842 [details]
Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Created attachment 387843 [details]
Patch

Comment on attachment 387843 [details]
Patch

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

> Source/WebKit/UIProcess/API/APIContentWorld.cpp:55
> +        world = new ContentWorld();

In general, a pattern of calling an empty constructor and then setting members is an anti-pattern.  Just add to the constructor parameters.

> Source/WebKit/UIProcess/API/APIContentWorld.cpp:56
> +        world->m_identifier = API::UserContentWorld::normalWorldIdentifer();

Is there really something special about this identifier, or could we use just another generated identifier?

> Source/WebKit/UIProcess/API/APIContentWorld.cpp:63
> +    static ContentWorld* world;

I think we might prefer something like NeverDestroyed<RefPtr<ContentWorld>> for some reason.

> Source/WebKit/UIProcess/API/APIContentWorld.cpp:67
> +        world->m_identifier = API::UserContentWorld::generateIdentifier();

It would probably be useful to give this world a name for future debugging help.  But then we wouldn't be able to use those names for other worlds, so maybe not.

> Source/WebKit/UIProcess/API/APIContentWorld.cpp:80
> +ContentWorld::ContentWorld()
> +{
> +}

I think = default is the cool new way to write this.

> Source/WebKit/UIProcess/API/APIContentWorld.h:50
> +    uint64_t m_identifier;

Strongly-typed identifiers are all the new hotness.
using ContentWorldIdentifier = ObjectIdentifier<ContentWorldType>

> Source/WebKit/UIProcess/API/APIUserContentWorld.h:49
> +    friend class ContentWorld;

Could we add the necessary public interface instead of befriending?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:910
> +    

whitespace.

(In reply to Alex Christensen from comment #5)
> Comment on attachment 387843 [details]
> Patch
> 
> > Source/WebKit/UIProcess/API/APIContentWorld.cpp:56
> > +        world->m_identifier = API::UserContentWorld::normalWorldIdentifer();
> 
> Is there really something special about this identifier, or could we use
> just another generated identifier?

Yes, there is something special about it.

> > Source/WebKit/UIProcess/API/APIContentWorld.cpp:63
> > +    static ContentWorld* world;
> 
> I think we might prefer something like NeverDestroyed<RefPtr<ContentWorld>>
> for some reason.
> 
> > Source/WebKit/UIProcess/API/APIContentWorld.cpp:67
> > +        world->m_identifier = API::UserContentWorld::generateIdentifier();
> 
> It would probably be useful to give this world a name for future debugging
> help.  But then we wouldn't be able to use those names for other worlds, so
> maybe not.

Right - These are explicitly name-free on purpose.

> 
> > Source/WebKit/UIProcess/API/APIContentWorld.cpp:80
> > +ContentWorld::ContentWorld()
> > +{
> > +}
> 
> I think = default is the cool new way to write this.
> 
> > Source/WebKit/UIProcess/API/APIContentWorld.h:50
> > +    uint64_t m_identifier;
> 
> Strongly-typed identifiers are all the new hotness.
> using ContentWorldIdentifier = ObjectIdentifier<ContentWorldType>

I dodged that on purpose because the other classes these interact with (InjectedBundleScriptWorld, etc) don't have them.

> 
> > Source/WebKit/UIProcess/API/APIUserContentWorld.h:49
> > +    friend class ContentWorld;
> 
> Could we add the necessary public interface instead of befriending?

Nah, generateIdentifier() should remain private to all who have no business touching it.

Created attachment 387864 [details]
Patch

Comment on attachment 387864 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/API/APIContentWorld.cpp:68
> +ContentWorld::ContentWorld(uint64_t identifier)

Could you add a FIXME here saying we should use ObjectIdentifier?

> Source/WebKit/UIProcess/API/APIUserContentWorld.h:49
> +    friend class ContentWorld;

I still don't think this is great.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3385
> +        send(Messages::WebPageProxy::ScriptValueCallback({ }, ExceptionDetails { "Unable to execute JavaScript: Cannot find specified content world"_s }, callbackID));

Could you add a FIXME somewhere saying to use sendWithAsyncReply rather than two messages with a CallbackID?

Comment on attachment 387864 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/_WKContentWorld.mm:43
> +    return wrapper(API::ContentWorld::sharedWorldWithName(name));

Even though name is non-nullable, do you think it would be worth throwing if name is nil?  Should we add such a test that ignores compiler warnings and does a horrible thing?

(In reply to Alex Christensen from comment #9)
> Comment on attachment 387864 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=387864&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/_WKContentWorld.mm:43
> > +    return wrapper(API::ContentWorld::sharedWorldWithName(name));
> 
> Even though name is non-nullable, do you think it would be worth throwing if
> name is nil?  Should we add such a test that ignores compiler warnings and
> does a horrible thing?

In our brave new world of playing nice with Swift, throwing is strongly frowned upon. I don't think it would pass muster.

In Obj-C land, you shoot yourself in the foot purposefully misbehaving with a non-nullable argument like that.

In Swift land the compiler doesn't even let you.

Created attachment 387871 [details]
Patch

Comment on attachment 387871 [details]
Patch

Clearing flags on attachment: 387871

Committed r254668: <https://trac.webkit.org/changeset/254668>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/58633355>

It looks like the changes in https://trac.webkit.org/changeset/254668/webkit

broke 21 tests with an assert. Tracking in https://bugs.webkit.org/show_bug.cgi?id=206357

(In reply to Truitt Savell from comment #15)
> It looks like the changes in https://trac.webkit.org/changeset/254668/webkit
> 
> broke 21 tests with an assert. Tracking in
> https://bugs.webkit.org/show_bug.cgi?id=206357

Does Mac-debug only run WK1 debug?
Do we seriously not have a WK2-debug EWS bot?

Comment on attachment 387871 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewEvaluateJavaScript.mm:199
> +        isDone = true;
> +    }];
> +    isDone = false;

These tests don't actually wait until isDone is set to true.  This test finishes in one runloop iteration without waiting for any results.  I'm fixing this in a patch I'm working on now.