[webkit-reviews] review granted: [Bug 216985] <input type="datetime-local"> not show calendar UI when it's inside ShadowDOM : [Attachment 410715] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 216985: <input type="datetime-local"> not show calendar UI when it's inside
ShadowDOM
https://bugs.webkit.org/show_bug.cgi?id=216985

Attachment 410715: Patch

https://bugs.webkit.org/attachment.cgi?id=410715&action=review




--- Comment #11 from Darin Adler <darin at apple.com> ---
Comment on attachment 410715
  --> https://bugs.webkit.org/attachment.cgi?id=410715
Patch

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

Seems the right way to do this. But we should not explicitly pass the Settings
in any case where it can be avoided. I suggest removing it anywhere we can
rather than passing it everywhere. I suspect that functions that do not allow a
null frame don’t need the Settings argument although I might have missed
something.

> Source/WebCore/Scripts/SettingsTemplates/Settings.h.erb:40
> +    WEBCORE_EXPORT static Ref<Settings> create(Page*);

Can this really work with a Page of null? Of not maybe change to a Page&?

> Source/WebCore/dom/DOMImplementation.h:46
> +    static Ref<Document> createDocument(const String& mimeType, Frame*,
const Settings&, const URL&);

Another option is to use the name contentType.

> Source/WebCore/dom/Document.cpp:617
> +    auto document = adoptRef(*new Document(nullptr,
contextDocument.m_settings.get(), URL()));

Doesn’t this work without the explicit call to get()?

> Source/WebCore/dom/Document.cpp:4010
> +	       return XMLDocument::createXHTML(nullptr, m_settings.get(),
url());

Same here.

> Source/WebCore/dom/Document.cpp:4011
> +	   return XMLDocument::create(nullptr, m_settings.get(), url());

And here.

> Source/WebCore/dom/Document.cpp:4013
> +    return create(m_settings.get(), url());

Here.

> Source/WebCore/dom/Document.cpp:7142
> +	   m_templateDocument = HTMLDocument::create(nullptr, m_settings.get(),
aboutBlankURL());

Here.

> Source/WebCore/dom/Document.cpp:7144
> +	   m_templateDocument = create(m_settings.get(), aboutBlankURL());

Here.

> Source/WebCore/dom/Document.h:343
> +    static Ref<Document> createNonRenderedPlaceholder(Frame&, const
Settings&, const URL&);

Seems we do not need the Settings argument here.

> Source/WebCore/html/HTMLDocument.h:33
> +    static Ref<HTMLDocument> createSynthesizedDocument(Frame&, const
Settings&, const URL&);

Seems we do not need the Settings argument here.

> Source/WebCore/html/ImageDocument.h:37
> +    static Ref<ImageDocument> create(Frame& frame, const Settings& settings,
const URL& url)

Might not need Settings argument here.

> Source/WebCore/html/PluginDocument.h:37
> +    static Ref<PluginDocument> create(Frame& frame, const Settings&
settings, const URL& url)

Might not need it here.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:114
> +static CachedResource* createResource(CachedResource::Type type,
CachedResourceRequest&& request, const PAL::SessionID& sessionID, const
CookieJar* cookieJar, const Settings& settings)

Change return type to smart pointer?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:153
> +static CachedResource* createResource(CachedResourceRequest&& request,
CachedResource& resource)

Ditto?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:155
> +    switch (resource.type()) {

Is there a way to make this less repetitive?

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:1081
> +    CachedResourceHandle<CachedResource> resource = createResource(type,
WTFMove(request), sessionID, &cookieJar, settings);

Maybe auto if we fix the return type above

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:39
> +    : CachedSVGDocument(WTFMove(request), resource.sessionID(),
resource.cookieJar(), resource.m_settings.get())

Less get()?

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:59
> +	   m_document = SVGDocument::create(nullptr, m_settings.get(),
response().url());

Likely don’t need the get() here

> Source/WebCore/page/ios/FrameIOS.mm:86
> +    auto document = HTMLDocument::createSynthesizedDocument(*this,
settings(), url);

Maybe this is a case where we do not have to pass Settings.

> Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:63
> +    auto document = Document::createNonRenderedPlaceholder(mainFrame,
mainFrame.settings(), data.scriptURL);

Maybe this function is a case where we do not need to pass Settings.

> Source/WebCore/xml/DOMParser.cpp:45
> +    auto document = DOMImplementation::createDocument(contentType, nullptr,
m_settings.get(), URL { });

May not need get here.