Remove compile time flag ENABLE_PAGE_POPUP . This is part of an overall effort to remove unneeded c…

Remove compile time flag ENABLE_PAGE_POPUP and replace with a runtime flag.

This is part of an overall effort to remove unneeded compile flags and replace with compile time flags.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=150609

[yael.aharon, 2013-05-01 01:15:53]

Can you please review ?
thanks!

[esprehn, 2013-05-01 01:32:01]

What is a Page Popup?

[yael.aharon, 2013-05-01 01:33:02]

On 2013/05/01 01:32:01, esprehn wrote:
> What is a Page Popup?

Calendar, time picker, etc

[yael.aharon1, 2013-05-02 00:27:22]

A question came up if this flag should be converted to runtime flag, since
Android will never ship with this flag turned on.
What do you think?

[tkent, 2013-05-02 00:34:29]

Yael and I talked about this on IRC.

We have three Form-related compile time flags (PAGE_POPUP, CALENDAR_PICKER, and
INPUT_MULTIPLE_FIELDS_UI) which we won't turn on for Android. I'm afraid their
code size is not acceptable for Android and we had better unify these flags into
one compile time flag eventually.

[eseidel, 2013-05-02 01:17:21]

Can we implement this feature in a way which does not require an ifdef and does
not increase Android's binary size an unreasonable amount?

I would prefer (when possible) to remove all #ifdefs and relatedly work to
reduce the code size of runtime features we believe to be expensive in the code
base.

[yael.aharon1, 2013-05-03 20:53:54]

I built content_shell for android with and without this patch, based on r149658.
The size of libcontent_shell_content_view.so without this patch is 1625358708.
The size of libcontent_shell_content_view.so with this patch is 1626095712.

[jamesr, 2013-05-03 20:55:35]

1.5 GB?  That must be debug, right?  The release size is what's more relevant.

[yael.aharon1, 2013-05-03 20:57:52]

On 2013/05/03 20:55:35, jamesr wrote:
> 1.5 GB?  That must be debug, right?  The release size is what's more relevant.

Please correct me if I am wrong, but I thought that building with  "ninja -C
out/Release -j10 content_shell_apk" produces a Release build?

[jamesr, 2013-05-03 21:00:29]

That's probably a release build with symbols - you'd have to strip it to get an
idea of what we would distribute.  I'm not sure exactly how to do that with the
android toolchain.

[yael.aharon1, 2013-05-03 21:09:25]

On 2013/05/03 21:00:29, jamesr wrote:
> That's probably a release build with symbols - you'd have to strip it to get
an
> idea of what we would distribute.  I'm not sure exactly how to do that with
the
> android toolchain.
Sorry, this is the first time I am building for Android. Here are the  numbers
after stripping:
without the patch 30205700
With the patch    30213924

[Peter Beverloo, 2013-05-07 17:04:14]

That's 8 kilobytes, probably 3-4 when compressed in the actual .apk package. 
I'd say we can live with that.

[tkent, 2013-05-07 21:05:24]

Yael, Peter, thank you for the clarification!
We can go ahead with this CL.

[tkent, 2013-05-07 21:17:34]

It seems there is no code to call WebRuntimeFeatures::enablePagePopup(true) for
non-test non-Android environment.

[tkent, 2013-05-07 21:20:22]

https://codereview.chromium.org/14735003/diff/1/Source/WebKit/chromium/src/Ch...
File Source/WebKit/chromium/src/ChromeClientImpl.cpp (left):

https://codereview.chromium.org/14735003/diff/1/Source/WebKit/chromium/src/Ch...
Source/WebKit/chromium/src/ChromeClientImpl.cpp:728: controller = adoptPtr(new
ColorChooserUIController(this, chooserClient));
We need to check the runtime flag, and select either of
ColorChooserPopupUIController and ColorChooserUIController.

[yael.aharon, 2013-05-07 21:25:19]

Enabling the code for non-Android was done in
https://codereview.chromium.org/14668006/ .
I realize runtime flags have changed since I posted this patch and it needs
rebasing.

[tkent, 2013-05-07 21:40:47]

On 2013/05/07 21:25:19, yael.aharon wrote:
> Enabling the code for non-Android was done in
> https://codereview.chromium.org/14668006/ .

With these CLs, you will need to do:
1. Land this
2. Blink gardener rolls Blink revision in Chromium
3. Land 14668006

This is not acceptable because the feature will break between 2 and 3.

You should do:
1. Land only WebRuntimeFeatures change
2. Blink gardener rolls Blink revision in Chromium
3. Land 14668006
4. Land the rest of this CL.

Or you should add
    #if OS(ANDROID)
        enablePagePopup(enable)
    #endif
to WebRuntimeFeatures::enableStableFeatures if Eric is comfortable with it.

Eric, what do you think about introducing OS-ifdef to WebRuntimeFeatures.cpp?

[tkent, 2013-05-07 21:42:15]

>     #if OS(ANDROID)

Oops, it should be #if !OS(ANDROID)

[yael.aharon1, 2013-05-16 00:12:15]

This is the third patch is a list of 3 patches to remove the compile time flag
ENABLE(PAGE_POPUP). 
The first patch is
https://src.chromium.org/viewvc/blink?revision=150413&view=revision and it
rolled into chromium a short time ago. 
Can you please review this?
thanks!

[tkent, 2013-05-16 00:20:42]

lgtm

[commit-bot: I haz the power, 2013-05-17 17:36:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14735003/18001

[commit-bot: I haz the power, 2013-05-17 17:36:59]

Failed to apply patch for Source/core/page/PagePopupController.idl:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/page/PagePopupController.idl
  Hunk #1 FAILED at 30.
  1 out of 1 hunk FAILED -- saving rejects to file
Source/core/page/PagePopupController.idl.rej

Patch:       Source/core/page/PagePopupController.idl
Index: Source/core/page/PagePopupController.idl
diff --git a/Source/core/page/PagePopupController.idl
b/Source/core/page/PagePopupController.idl
index
1ca5c57a968c46fa34bb3b29fee3463da15a68c2..d4d2beef3d81b86d39bad35024971750a3eab181
100644
--- a/Source/core/page/PagePopupController.idl
+++ b/Source/core/page/PagePopupController.idl
@@ -30,7 +30,7 @@
 
 [
     NoInterfaceObject,
-    Conditional=PAGE_POPUP,
+    EnabledAtRuntime=pagePopup,
     ImplementationLacksVTable
 ] interface PagePopupController {
     void setValueAndClosePopup(long numberValue, DOMString stringValue);

[commit-bot: I haz the power, 2013-05-17 18:13:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14735003/28001

[commit-bot: I haz the power, 2013-05-17 19:03:34]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-17 19:56:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yael.aharon@intel.com/14735003/28001

[commit-bot: I haz the power, 2013-05-17 21:17:21]

Change committed as 150609