Bug 79078 – Add HTML Popup API to ChromeClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Kent Tamura]

Add HTML Popup API to Chrome.  This is required to implement a calendar picker for <input type=date>.

[Kent Tamura]

Created attachment 127918 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 127918 [details]
Patch

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

> Source/WebCore/page/Chrome.cpp:561
> +    return m_client->openHTMLPopup(client, originBoundsInRootView);

if client() is public, why do we need these on Chrome?

> Source/WebCore/platform/HTMLPopup.h:32
> +#define HTMLPopup_h

Not sure if HTMLPopup is a good name. HTML is typically used as a prefix for HTML elements.

[Dimitri Glazkov (Google)]

Adding Adam, since he's cleaning up in this area.

[Adam Barth]

Comment on attachment 127918 [details]
Patch

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

This code shouldn't be in Platform.  I'm not sure what you're trying to achieve so it's a bit hard for me to recommend a better course of action.

>> Source/WebCore/page/Chrome.cpp:561
>> +    return m_client->openHTMLPopup(client, originBoundsInRootView);
> 
> if client() is public, why do we need these on Chrome?

We shouldn't need them.

> Source/WebCore/platform/HTMLPopup.h:39
> +class HTMLPopup {

Things in the Platform directory shouldn't know anything about HTML.  There's some sort of layering confusion here.

[Kent Tamura]

Comment on attachment 127918 [details]
Patch

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

>>> Source/WebCore/page/Chrome.cpp:561
>>> +    return m_client->openHTMLPopup(client, originBoundsInRootView);
>> 
>> if client() is public, why do we need these on Chrome?
> 
> We shouldn't need them.

Ok, I'll remove Chrome functions.
I just followed what other functions in Chrome did.

>> Source/WebCore/platform/HTMLPopup.h:32
>> +#define HTMLPopup_h
> 
> Not sure if HTMLPopup is a good name. HTML is typically used as a prefix for HTML elements.

How about DocumentPopup?

>> Source/WebCore/platform/HTMLPopup.h:39
>> +class HTMLPopup {
> 
> Things in the Platform directory shouldn't know anything about HTML.  There's some sort of layering confusion here.

Yeah, I also wonder what was the best directory.  I put it to platform/ because these classes doesn't depend on non-platform classes.

Is it ok to put it into WebCore/html/ because it would be a functionality to realize UI of HTML elements?

[Adam Barth]

Comment on attachment 127918 [details]
Patch

Yeah, WebCore/html seems appropriate.  It looks like it's HTML-related because it has an htmlSource() and it is used to implement HTML elements.

Question: How are you planning to create the htmlSource text?  We want to make sure we avoid XSS.

[Kent Tamura]

(In reply to comment #6)
> Question: How are you planning to create the htmlSource text?  We want to make sure we avoid XSS.

The source text will be built in WebCore C++ code.  We will need to pass some values specified in the main HTML document.  e.g. Passing min/max/step/value values of <input type=date>.
We have to escape such values.

[Kent Tamura]

(In reply to comment #5)
> >> +#define HTMLPopup_h
> > 
> > Not sure if HTMLPopup is a good name. HTML is typically used as a prefix for HTML elements.
> 
> How about DocumentPopup?

Does anyone have a good idea of the name?

[Kent Tamura]

Created attachment 129387 [details]
Patch 2

Remove Chrome changes, move HTMLPopup* to html/

[Dimitri Glazkov (Google)]

Comment on attachment 129387 [details]
Patch 2

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

> Source/WebCore/ChangeLog:13
> +        * html/HTMLPopupClient.h: Added.

I am sorry for jerking you around, but I don't think html is the right place for this. I could be wrong, but I think this directory is loosely associated with the "stuff in HTML spec." (aside from workers and some other stuff in HTML spec that probably should've been its own spec anyway).

Since we have no intention of making this an author-facing API, perhaps this should live in Source/page?

I also noticed that in https://bugs.webkit.org/show_bug.cgi?id=53961, you actually create a new Page for the popup. Perhaps this should be called something like a PagePopup?

[Kent Tamura]

Comment on attachment 129387 [details]
Patch 2

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

>> Source/WebCore/ChangeLog:13
>> +        * html/HTMLPopupClient.h: Added.
> 
> I am sorry for jerking you around, but I don't think html is the right place for this. I could be wrong, but I think this directory is loosely associated with the "stuff in HTML spec." (aside from workers and some other stuff in HTML spec that probably should've been its own spec anyway).
> 
> Since we have no intention of making this an author-facing API, perhaps this should live in Source/page?
> 
> I also noticed that in https://bugs.webkit.org/show_bug.cgi?id=53961, you actually create a new Page for the popup. Perhaps this should be called something like a PagePopup?

page/PagePopup* sounds good.  I'll update the patch.

[Kent Tamura]

Created attachment 129639 [details]
Patch 3

Renamed to PagePopup

[Kent Tamura]

Created attachment 129643 [details]
Patch 4

Fix a typo in a comment

[Dimitri Glazkov (Google)]

Comment on attachment 129643 [details]
Patch 4

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

> Source/WebCore/page/PagePopupClient.h:50
> +    virtual String htmlSource() = 0;

This seems really hacky. After building this string by combining embedded resources, you will still need to use a loader to get this into the popup. Would it be better to implement a new kind of loader that just allows pulling stuff from resources? Well, that may have its own problems, such as performance. We don't want the popup to take forever to load...

[Kent Tamura]

Comment on attachment 129643 [details]
Patch 4

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

>> Source/WebCore/page/PagePopupClient.h:50
>> +    virtual String htmlSource() = 0;
> 
> This seems really hacky. After building this string by combining embedded resources, you will still need to use a loader to get this into the popup. Would it be better to implement a new kind of loader that just allows pulling stuff from resources? Well, that may have its own problems, such as performance. We don't want the popup to take forever to load...

ok,  I'll change this to:
  virtual void writeDocument(DocumentWriter&) = 0;

[Kent Tamura]

Created attachment 129823 [details]
Patch for landing

[Kent Tamura]

Committed r109513: <http://trac.webkit.org/changeset/109513>