Bug 79078

Created attachment 127918 [details] Patch

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.

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

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.

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?

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.

(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.

(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?

Created attachment 129387 [details] Patch 2 Remove Chrome changes, move HTMLPopup* to html/

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?

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.

Created attachment 129639 [details] Patch 3 Renamed to PagePopup

Created attachment 129643 [details] Patch 4 Fix a typo in a comment

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...

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;

Created attachment 129823 [details] Patch for landing

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