Bug 90521

Created attachment 150727 [details] Patch

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.

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

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

Created attachment 150885 [details] Patch

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

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 on attachment 150885 [details] Patch Looks good.All we need is Kent-san's stamp.

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.

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

Created attachment 151184 [details] Patch

Comment on attachment 151184 [details] Patch Thanks for the reviews. I've added a line to the ChangeLog about the per-context flag.

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

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

Created attachment 151189 [details] Patch

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

Created attachment 151198 [details] Patch

Added copyright notices.

Comment on attachment 151198 [details] Patch Looks ok.

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

Created attachment 151218 [details] Patch

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

Created attachment 151222 [details] fix conflict

Comment on attachment 151222 [details] fix conflict ok

Comment on attachment 151222 [details] fix conflict Clearing flags on attachment: 151222 Committed r122107: <http://trac.webkit.org/changeset/122107>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by 90794

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.

Created attachment 151391 [details] fix xcodeproj changes

Comment on attachment 151391 [details] fix xcodeproj changes Clearing flags on attachment: 151391 Committed r122195: <http://trac.webkit.org/changeset/122195>

All reviewed patches have been landed. Closing bug.