Bug 90521 – Add ENABLE_DIALOG_ELEMENT and skeleton files

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Matt Falkenhagen]

Add ENABLE_DIALOG_TAG and skeleton files.

[Matt Falkenhagen]

Created attachment 150727 [details]
Patch

[Hajime Morrita]

Comment on attachment 150727 [details]
Patch

You may want to add RuntimeFeatures flags in addition to compilation flag. See <context> in HTMLTagNaes.in as an example.
Then you can enable the compilation flag for chromium by default, and disable the runtime flag at the same time.

The window.internals object can be used to turn the flag on in the test.
This trick allows us to enable the feature-under-development only in the test, and keep hiding it from the wild web.


It's OK to add RuntimeFeatures flags in the separate patch. I'd happy to r+ in that case.

[Matt Falkenhagen]

Thanks for the review!

I think I'll try adding RuntimeFeature flags in a separate patch. By the way, are you talking about <content> and the contextConditional flag in HTMLTagNames.in? I don't see <context>.

[Hajime Morrita]

(In reply to comment #3)
> Thanks for the review!
> 
> I think I'll try adding RuntimeFeature flags in a separate patch. By the way, are you talking about <content> and the contextConditional flag in HTMLTagNames.in? I don't see <context>.
Oops. You're right. "context" was typo,

[Matt Falkenhagen]

Created attachment 150885 [details]
Patch

[Matt Falkenhagen]

I've modified the patch to include the runtime feature flag.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Hajime Morrita]

Comment on attachment 150885 [details]
Patch

Looks good.All we need is Kent-san's stamp.

[Kent Tamura]

Comment on attachment 150885 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Add ENABLE_DIALOG_TAG and skeleton files

I guess you followed ENABLE_PROGRESS_TAG and ENABLE_METER_TAG, but IMO we should name this flag 'ENABLE_DIALOG_ELEMENT' and we should rename ENABLE_PROGRESS_TAG, ENABLE_METER_TAG, and ENABLE_DATALIST to ENABLE_*_ELEMENT for consistency.

> Source/WebCore/dom/ContextFeatures.h:43
> +        DialogTag,

You should explain why we need per-context flag in ChangeLog.

> Source/WebKit/chromium/public/WebRuntimeFeatures.h:144
> +    WEBKIT_EXPORT static void enableDialogTag(bool);
> +    WEBKIT_EXPORT static bool isDialogTagEnabled();
> +

WebKit API change LGTM.

[Matt Falkenhagen]

(In reply to comment #9)
> (From update of attachment 150885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150885&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        Add ENABLE_DIALOG_TAG and skeleton files
> 
> I guess you followed ENABLE_PROGRESS_TAG and ENABLE_METER_TAG, but IMO we should name this flag 'ENABLE_DIALOG_ELEMENT' and we should rename ENABLE_PROGRESS_TAG, ENABLE_METER_TAG, and ENABLE_DATALIST to ENABLE_*_ELEMENT for consistency.

Makes sense to me. Is it OK to do all the renaming in a separate patch?

> 
> > Source/WebCore/dom/ContextFeatures.h:43
> > +        DialogTag,
> 
> You should explain why we need per-context flag in ChangeLog.
> 
> > Source/WebKit/chromium/public/WebRuntimeFeatures.h:144
> > +    WEBKIT_EXPORT static void enableDialogTag(bool);
> > +    WEBKIT_EXPORT static bool isDialogTagEnabled();
> > +
> 
> WebKit API change LGTM.

[Kent Tamura]

Comment on attachment 150885 [details]
Patch

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

>>> Source/WebCore/ChangeLog:3
>>> +        Add ENABLE_DIALOG_TAG and skeleton files
>> 
>> I guess you followed ENABLE_PROGRESS_TAG and ENABLE_METER_TAG, but IMO we should name this flag 'ENABLE_DIALOG_ELEMENT' and we should rename ENABLE_PROGRESS_TAG, ENABLE_METER_TAG, and ENABLE_DATALIST to ENABLE_*_ELEMENT for consistency.
> 
> Makes sense to me. Is it OK to do all the renaming in a separate patch?

Yes, of course.

[Matt Falkenhagen]

Created attachment 151184 [details]
Patch

[Matt Falkenhagen]

Comment on attachment 151184 [details]
Patch

Thanks for the reviews. I've added a line to the ChangeLog about the per-context flag.

[Kent Tamura]

(In reply to comment #11)
> (From update of attachment 150885 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=150885&action=review
> 
> >>> Source/WebCore/ChangeLog:3
> >>> +        Add ENABLE_DIALOG_TAG and skeleton files
> >> 
> >> I guess you followed ENABLE_PROGRESS_TAG and ENABLE_METER_TAG, but IMO we should name this flag 'ENABLE_DIALOG_ELEMENT' and we should rename ENABLE_PROGRESS_TAG, ENABLE_METER_TAG, and ENABLE_DATALIST to ENABLE_*_ELEMENT for consistency.
> > 
> > Makes sense to me. Is it OK to do all the renaming in a separate patch?
> 
> Yes, of course.

Ah, did you mean you didn't apply ENABLE_DIALOG_ELEMENT in this bug?
We had better apply ENABLE_DIALOG_ELEMENT now, and rename the remaining later.

[Matt Falkenhagen]

(In reply to comment #14)
> (In reply to comment #11)
> > (From update of attachment 150885 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=150885&action=review
> > 
> > >>> Source/WebCore/ChangeLog:3
> > >>> +        Add ENABLE_DIALOG_TAG and skeleton files
> > >> 
> > >> I guess you followed ENABLE_PROGRESS_TAG and ENABLE_METER_TAG, but IMO we should name this flag 'ENABLE_DIALOG_ELEMENT' and we should rename ENABLE_PROGRESS_TAG, ENABLE_METER_TAG, and ENABLE_DATALIST to ENABLE_*_ELEMENT for consistency.
> > > 
> > > Makes sense to me. Is it OK to do all the renaming in a separate patch?
> > 
> > Yes, of course.
> 
> Ah, did you mean you didn't apply ENABLE_DIALOG_ELEMENT in this bug?
> We had better apply ENABLE_DIALOG_ELEMENT now, and rename the remaining later.

Oh I see. I'll change it to use ENABLE_DIALOG_ELEMENT now.

[Gyuyoung Kim]

Comment on attachment 151184 [details]
Patch

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

> Source/cmake/WebKitFeatures.cmake:35
> +    WEBKIT_OPTION_DEFINE(ENABLE_DIRECTORY_UPLOAD "Toggle Directory upload support" OFF)

Though I don't like to add a code unrelated to bug, if you wanna add DIRECTORY_UPLOAD feature to here, please add this to cmakeconfig.h.cmake as well.

[Matt Falkenhagen]

Comment on attachment 151184 [details]
Patch

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

>> Source/cmake/WebKitFeatures.cmake:35
>> +    WEBKIT_OPTION_DEFINE(ENABLE_DIRECTORY_UPLOAD "Toggle Directory upload support" OFF)
> 
> Though I don't like to add a code unrelated to bug, if you wanna add DIRECTORY_UPLOAD feature to here, please add this to cmakeconfig.h.cmake as well.

I actually didn't add it here. I just noticed a misspelling: Toogle -> Toggle.

[Gyuyoung Kim]

Comment on attachment 151184 [details]
Patch

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

>>> Source/cmake/WebKitFeatures.cmake:35
>>> +    WEBKIT_OPTION_DEFINE(ENABLE_DIRECTORY_UPLOAD "Toggle Directory upload support" OFF)
>> 
>> Though I don't like to add a code unrelated to bug, if you wanna add DIRECTORY_UPLOAD feature to here, please add this to cmakeconfig.h.cmake as well.
> 
> I actually didn't add it here. I just noticed a misspelling: Toogle -> Toggle.

I'm not sure if we can fix code unrelated to this patch together. I would not object to fix this.

[Matt Falkenhagen]

Created attachment 151189 [details]
Patch

[Matt Falkenhagen]

(In reply to comment #15)
> Oh I see. I'll change it to use ENABLE_DIALOG_ELEMENT now.

Added new patch using ENABLE_DIALOG_ELEMENT.

(In reply to comment #18)
> I'm not sure if we can fix code unrelated to this patch together. I would not object to fix this.

Sorry, I'd just fixed it since it was right below the line I added. I'll be more careful to not to change unrelated code in the future.

[Kent Tamura]

Comment on attachment 151189 [details]
Patch

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

> Source/WebCore/html/HTMLDialogElement.cpp:1
> +#include "config.h"

Please add copyright header.

> Source/WebCore/html/HTMLDialogElement.h:1
> +#ifndef HTMLDialogElement_h

ditto.

> Source/WebCore/html/HTMLDialogElement.idl:1
> +module html {

ditto.

[Matt Falkenhagen]

Created attachment 151198 [details]
Patch

[Matt Falkenhagen]

Added copyright notices.

[Kent Tamura]

Comment on attachment 151198 [details]
Patch

Looks ok.

[WebKit Review Bot]

Comment on attachment 151198 [details]
Patch

Rejecting attachment 151198 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
MLDirectoryElement.o
  CXX(target) out/Debug/obj.target/webcore_html/Source/WebCore/html/HTMLDivElement.o
Source/WebCore/html/HTMLDialogElement.cpp: In constructor 'WebCore::HTMLDialogElement::HTMLDialogElement(const WebCore::QualifiedName&, WebCore::Document*)':
Source/WebCore/html/HTMLDialogElement.cpp:38: error: 'dialogElement' was not declared in this scope
make: *** [out/Debug/obj.target/webcore_html/Source/WebCore/html/HTMLDialogElement.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/13168127

[Matt Falkenhagen]

Created attachment 151218 [details]
Patch

[Matt Falkenhagen]

Fixed the build failure. I'd mistakenly changed an instance of dialogTag to dialogElement when doing the flag renaming, causing a failure on debug builds.

[Kent Tamura]

Comment on attachment 151218 [details]
Patch

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

> Source/WebCore/html/HTMLDialogElement.h:44
> +    void show();
> +private:

nit: Need a blank line before "private:".

[WebKit Review Bot]

Comment on attachment 151218 [details]
Patch

Rejecting attachment 151218 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
urations/FeatureDefines.xcconfig
patching file Source/WebKit2/Configurations/FeatureDefines.xcconfig
patching file Source/cmake/WebKitFeatures.cmake
patching file Source/cmakeconfig.h.cmake
patching file Tools/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Tools/Scripts/webkitperl/FeatureList.pm
patching file ChangeLog

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Kent Tamura']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13174016

[Matt Falkenhagen]

Created attachment 151222 [details]
fix conflict

[Kent Tamura]

Comment on attachment 151222 [details]
fix conflict

ok

[WebKit Review Bot]

Comment on attachment 151222 [details]
fix conflict

Clearing flags on attachment: 151222

Committed r122107: <http://trac.webkit.org/changeset/122107>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 90794

[Matt Falkenhagen]

It looks like the patch broke the Mac debug build bots:

http://build.webkit.org/builders/Apple%20Lion%20Debug%20%28Build%29/builds/1137/steps/compile-webkit/logs/stdio
http://build.webkit.org/builders/Apple%20SnowLeopard%20Debug%20%28Build%29/builds/1133/steps/compile-webkit/logs/stdio

The entries for JSHTMLDialogElement look incorrect in WebCore.xcodeproj/project.pbxproj.

[Matt Falkenhagen]

Created attachment 151391 [details]
fix xcodeproj changes

[WebKit Review Bot]

Comment on attachment 151391 [details]
fix xcodeproj changes

Clearing flags on attachment: 151391

Committed r122195: <http://trac.webkit.org/changeset/122195>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.