Bug 92339 - [Forms] Introduce runtime feature flags for input type datetime, datetimelocal, month, time, week
Summary: [Forms] Introduce runtime feature flags for input type datetime, datetimeloca...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks: 88970
  Show dependency tree / graph
 
Reported: 2012-07-25 23:31 PDT by yosin
Modified: 2012-07-26 18:27 PDT (History)
8 users (show)

See Also:

Attachments
Patch 1 (22.21 KB, patch)
2012-07-26 02:15 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 2 (22.49 KB, patch)
2012-07-26 02:58 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 3 (23.30 KB, patch)
2012-07-26 03:08 PDT, yosin 		no flags Details | Formatted Diff | Diff
Patch 4 (23.21 KB, patch)
2012-07-26 18:25 PDT, yosin 		no flags Details | Formatted Diff | Diff
Show Obsolete (3) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description yosin 2012-07-25 23:31:41 PDT 
Using runtime feature flags brings us following benefit:
 1. Detecting compilation failure on each local build on Chromium developer
 2. Detecting behavior change via layout tests
Comment 1 yosin 2012-07-26 02:15:36 PDT 
Created attachment 154585 [details]
Patch 1
Comment 2 WebKit Review Bot 2012-07-26 02:17:37 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 3 Adam Barth 2012-07-26 02:21:10 PDT 
Comment on attachment 154585 [details]
Patch 1

WebKit API change LGTM.  Please be sure to disable these features appropriately on chromium-android.
Comment 4 Adam Barth 2012-07-26 02:21:59 PDT 
Comment on attachment 154585 [details]
Patch 1

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

> Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:202
> +#if OS(ANDROID)

Is this the best way to do this?  I would have expected the embedder to do this work.
Comment 5 yosin 2012-07-26 02:51:59 PDT 
(In reply to comment #4)
> (From update of attachment 154585 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154585&action=review
> 
> > Source/WebCore/bindings/generic/RuntimeEnabledFeatures.cpp:202
> > +#if OS(ANDROID)
> 
> Is this the best way to do this?  I would have expected the embedder to do this work.

No, this isn't beautiful. We should not have this kind of #if in RuntimeEnabledFeatures.cpp

I'll do following three steps to make RuntimeEnabledFeatures.cpp clean:

1. RuntimEnabledFeatures.cpp: Change #if OS(ANDROID) to #if PLATFORM(CHROMIUM) && !OS(ANDROID) to set false, otherwise set true -- keep current behavior
2. content/renderrer/render_thread_impl.cc: RenderThreadImpl::EnsureWebKitInitialized() of Chromium side, I'll add enableInputTypeDateTime(false), ... for !OS(ANDROID)
3. Then, remove #if PLATFORM(CHROMIUM) && !OS(ANDROID) from RuntimeEnabledFeatures.cpp
Comment 6 Kent Tamura 2012-07-26 02:54:44 PDT 
Comment on attachment 154585 [details]
Patch 1

Please touch InputType.h to rebuild *InputType.cpp.  Without it, Chromium-win bots will fail to build.
Comment 7 yosin 2012-07-26 02:58:27 PDT 
Created attachment 154592 [details]
Patch 2
Comment 8 yosin 2012-07-26 03:08:49 PDT 
Created attachment 154597 [details]
Patch 3
Comment 9 Adam Barth 2012-07-26 08:35:34 PDT 
> I'll do following three steps to make RuntimeEnabledFeatures.cpp clean:

That sounds great.  Thanks@
Comment 10 yosin 2012-07-26 18:09:07 PDT 
Comment on attachment 154597 [details]
Patch 3

Could you review this patch?
Thanks in advance.
Comment 11 Kent Tamura 2012-07-26 18:19:08 PDT 
Comment on attachment 154597 [details]
Patch 3

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

The code looks ok.

> Source/WebCore/ChangeLog:3
> +        [Forms] Introduce runtime feature flags for input type datetime, datetimelocal, month, time, week for Chromium port

Flags will be introduced to WebCore.  "for Chromium port" is false.
Comment 12 yosin 2012-07-26 18:25:19 PDT 
Created attachment 154804 [details]
Patch 4
Comment 13 yosin 2012-07-26 18:27:29 PDT 
Comment on attachment 154804 [details]
Patch 4

Clearing flags on attachment: 154804

Committed r123826: <http://trac.webkit.org/changeset/123826>
Comment 14 yosin 2012-07-26 18:27:38 PDT 
All reviewed patches have been landed.  Closing bug.