Bug 90521 - Add ENABLE_DIALOG_ELEMENT and skeleton files
Summary: Add ENABLE_DIALOG_ELEMENT and skeleton files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on: 90794
Blocks: 84635
  Show dependency treegraph
 
Reported: 2012-07-04 00:33 PDT by Matt Falkenhagen
Modified: 2012-07-09 22:45 PDT (History)
12 users (show)

See Also:


Attachments
Patch (42.36 KB, patch)
2012-07-04 00:38 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (45.99 KB, patch)
2012-07-05 00:18 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (46.35 KB, patch)
2012-07-08 22:13 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (46.59 KB, patch)
2012-07-08 23:27 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (50.62 KB, patch)
2012-07-09 00:29 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (50.61 KB, patch)
2012-07-09 02:21 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
fix conflict (50.75 KB, patch)
2012-07-09 03:26 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
fix xcodeproj changes (50.83 KB, patch)
2012-07-09 20:50 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Falkenhagen 2012-07-04 00:33:14 PDT
Add ENABLE_DIALOG_TAG and skeleton files.
Comment 1 Matt Falkenhagen 2012-07-04 00:38:29 PDT
Created attachment 150727 [details]
Patch
Comment 2 Hajime Morrita 2012-07-04 02:34:27 PDT
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.
Comment 3 Matt Falkenhagen 2012-07-04 18:59:14 PDT
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>.
Comment 4 Hajime Morrita 2012-07-04 20:06:33 PDT
(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,
Comment 5 Matt Falkenhagen 2012-07-05 00:18:45 PDT
Created attachment 150885 [details]
Patch
Comment 6 Matt Falkenhagen 2012-07-05 00:20:54 PDT
I've modified the patch to include the runtime feature flag.
Comment 7 WebKit Review Bot 2012-07-05 00:23:23 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 8 Hajime Morrita 2012-07-05 02:21:48 PDT
Comment on attachment 150885 [details]
Patch

Looks good.All we need is Kent-san's stamp.
Comment 9 Kent Tamura 2012-07-08 20:15:30 PDT
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.
Comment 10 Matt Falkenhagen 2012-07-08 21:01:09 PDT
(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.
Comment 11 Kent Tamura 2012-07-08 21:03:29 PDT
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.
Comment 12 Matt Falkenhagen 2012-07-08 22:13:37 PDT
Created attachment 151184 [details]
Patch
Comment 13 Matt Falkenhagen 2012-07-08 22:15:07 PDT
Comment on attachment 151184 [details]
Patch

Thanks for the reviews. I've added a line to the ChangeLog about the per-context flag.
Comment 14 Kent Tamura 2012-07-08 22:18:50 PDT
(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.
Comment 15 Matt Falkenhagen 2012-07-08 22:21:48 PDT
(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.
Comment 16 Gyuyoung Kim 2012-07-08 22:50:30 PDT
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.
Comment 17 Matt Falkenhagen 2012-07-08 23:05:20 PDT
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.
Comment 18 Gyuyoung Kim 2012-07-08 23:14:32 PDT
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.
Comment 19 Matt Falkenhagen 2012-07-08 23:27:16 PDT
Created attachment 151189 [details]
Patch
Comment 20 Matt Falkenhagen 2012-07-08 23:38:05 PDT
(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.
Comment 21 Kent Tamura 2012-07-09 00:14:35 PDT
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.
Comment 22 Matt Falkenhagen 2012-07-09 00:29:38 PDT
Created attachment 151198 [details]
Patch
Comment 23 Matt Falkenhagen 2012-07-09 00:32:11 PDT
Added copyright notices.
Comment 24 Kent Tamura 2012-07-09 00:34:58 PDT
Comment on attachment 151198 [details]
Patch

Looks ok.
Comment 25 WebKit Review Bot 2012-07-09 01:40:50 PDT
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
Comment 26 Matt Falkenhagen 2012-07-09 02:21:17 PDT
Created attachment 151218 [details]
Patch
Comment 27 Matt Falkenhagen 2012-07-09 02:24:00 PDT
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.
Comment 28 Kent Tamura 2012-07-09 02:25:42 PDT
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:".
Comment 29 WebKit Review Bot 2012-07-09 02:29:11 PDT
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
Comment 30 Matt Falkenhagen 2012-07-09 03:26:55 PDT
Created attachment 151222 [details]
fix conflict
Comment 31 Kent Tamura 2012-07-09 03:28:10 PDT
Comment on attachment 151222 [details]
fix conflict

ok
Comment 32 WebKit Review Bot 2012-07-09 07:42:17 PDT
Comment on attachment 151222 [details]
fix conflict

Clearing flags on attachment: 151222

Committed r122107: <http://trac.webkit.org/changeset/122107>
Comment 33 WebKit Review Bot 2012-07-09 07:42:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 WebKit Review Bot 2012-07-09 08:25:02 PDT
Re-opened since this is blocked by 90794
Comment 35 Matt Falkenhagen 2012-07-09 08:37:31 PDT
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.
Comment 36 Matt Falkenhagen 2012-07-09 20:50:58 PDT
Created attachment 151391 [details]
fix xcodeproj changes
Comment 37 WebKit Review Bot 2012-07-09 22:45:36 PDT
Comment on attachment 151391 [details]
fix xcodeproj changes

Clearing flags on attachment: 151391

Committed r122195: <http://trac.webkit.org/changeset/122195>
Comment 38 WebKit Review Bot 2012-07-09 22:45:44 PDT
All reviewed patches have been landed.  Closing bug.