Bug 37125

Summary: Remove obsolete MOBILE flag
Product: WebKit Reporter: Laszlo Gombos <laszlo.gombos>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, benm, commit-queue, dacarson, dbates, ddkilzer, eric, koivisto, ostap73, staikos, steveblock
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
1st try, remove the define
none
2nd try
none
3rd try - remove the MOBILE flag none

Description Laszlo Gombos 2010-04-05 16:14:30 PDT
Currently the MOBILE flag is unconditionally disabled in WebCore/config.h.
Comment 1 Laszlo Gombos 2010-04-05 16:30:30 PDT
Created attachment 52587 [details]
1st try, remove the define

Alternatively we can put the define around guards like other feature flags, like this:

#ifndef MOBILE
#define MOBILE 0
#endif

I'm also happy to change the name of the define if there is a better proposal. I think a global define like MOBILE does not quite make sense.
Comment 2 Eric Seidel (no email) 2010-04-05 16:56:12 PDT
I suspect this was added for iPhone:
http://trac.webkit.org/changeset/23281

That or windows mobile. ;p

It probably isn't needed anymore, but only someone on team iPhone (like ddkilzer) would know.
Comment 3 Eric Seidel (no email) 2010-04-05 16:56:31 PDT
This bug is not security sensitive, removing flag.
Comment 4 Laszlo Gombos 2010-04-05 17:54:06 PDT
The MOBILE flag only used in WebCore/html/HTMLTokenizer.cpp at the moment and sets up some sensible defaults for CPU-constrained environments. Those defaults that are controlled by this flag can be changed runtime, so I have no strong preference to keep the build flag around. 

Hope others can chip in as well.
Comment 5 George Staikos 2010-04-05 18:12:21 PDT
This flag is really old - I think it predates everything except S60 browser in terms of mobile webkit.  We don't use it.
Comment 6 Eric Seidel (no email) 2010-04-05 19:44:06 PDT
If we don't here back from ddkilzer and co. then I suggest changing this to #ifndef MOBILE and then I would feel comfortable r+ing it.
Comment 7 Laszlo Gombos 2010-04-05 23:03:00 PDT
Created attachment 52608 [details]
2nd try

Change the patch based on Eric's feedback to keep a default value for the MOBILE flag in config.h but only set it if it was not defined before.

Also took the liberty of renaming the flag to ENABLE_MOBILE_TOKENIZER.
Comment 8 David Kilzer (:ddkilzer) 2010-04-06 08:45:10 PDT
(In reply to comment #6)
> If we don't here back from ddkilzer and co. then I suggest changing this to
> #ifndef MOBILE and then I would feel comfortable r+ing it.

This macro was added for Android in r16821.

http://trac.webkit.org/changeset/16821

You'll have to ask them if they still use it.
Comment 9 David Kilzer (:ddkilzer) 2010-04-06 08:47:42 PDT
(In reply to comment #8)
> (In reply to comment #6)
> > If we don't here back from ddkilzer and co. then I suggest changing this to
> > #ifndef MOBILE and then I would feel comfortable r+ing it.
> 
> This macro was added for Android in r16821.
> 
> http://trac.webkit.org/changeset/16821
> 
> You'll have to ask them if they still use it.

Or maybe it was Nokia.  In either case, it was not added for iPhone and we don't use it.  :)
Comment 10 Ben Murdoch 2010-04-06 10:08:50 PDT
Android does not use this flag.
Comment 11 Eric Seidel (no email) 2010-04-06 16:56:03 PDT
S60 never built out of trunk, right?  And Nokia is now Qt.  So it seems we should just rip out this flag and all the associated code.
Comment 12 Laszlo Gombos 2010-04-06 21:13:33 PDT
Created attachment 52701 [details]
3rd try - remove the MOBILE flag

Thanks for all the feedback.
Comment 13 WebKit Commit Bot 2010-04-06 21:58:36 PDT
Comment on attachment 52701 [details]
3rd try - remove the MOBILE flag

Clearing flags on attachment: 52701

Committed r57193: <http://trac.webkit.org/changeset/57193>
Comment 14 WebKit Commit Bot 2010-04-06 21:58:43 PDT
All reviewed patches have been landed.  Closing bug.