Bug 28941 – Add webSocketEnabled flag in Settings

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Fumitoshi Ukai]

Add webSocketEnabled flag in Settings to control WebSocket availability.

[Fumitoshi Ukai]

Created attachment 38977 [details]
add webSocketEnabled in Settings


---
 3 files changed, 23 insertions(+), 0 deletions(-)

[Eric Seidel (no email)]

Comment on attachment 38977 [details]
add webSocketEnabled in Settings

Why?  This doesn't do anything yet?  Are there other web sockets code paths you're about to add?  Generally we don't add dead code (unless it's about to be used...), which seems to be exactly what this patch is doing.

[Fumitoshi Ukai]

(In reply to comment #2)
> (From update of attachment 38977 [details])
> Why?  This doesn't do anything yet?  Are there other web sockets code paths
> you're about to add?  Generally we don't add dead code (unless it's about to be
> used...), which seems to be exactly what this patch is doing.

I want this code to support --enable-web-sockets switch (in chromium).
Will add hooks once WebSocket API code (bug 28038) is landed.

[Mark Rowe (bdash)]

Why is it necessary for this to be a runtime setting rather than a compile-time choice?

[Eric Seidel (no email)]

I expect his response will be that "chromium makes all new features command-line switches, so they need to be run-time switchable."  Chromium is constantly shipping from tip of tree (the developer branch) so in-progress features like this default to off, but can be enabled by passing command-line switches.

[Brett Wilson (Google)]

Eric is correct. We like to ship off trunk without worrying about half-completed features. On the dev channel, we can make a feature available for testing by interested devlopers, and still eventually push that build to stable without major changes.

[Alexey Proskuryakov]

It would be nice to mention this policy on webkit-dev, along with some guidelines on when these runtime switches can be removed.

[Eric Seidel (no email)]

Great minds think alike:
http://groups.google.com/group/chromium-dev/browse_thread/thread/be626eabfdf0ca48

[Eric Seidel (no email)]

Comment on attachment 38977 [details]
add webSocketEnabled in Settings

My understanding of the new policy is that this should have "experimental" in the name.

[Fumitoshi Ukai]

Created attachment 40226 [details]
rename webSocketsEnabled to experimentalWebSocketEnabled

[Eric Seidel (no email)]

Comment on attachment 40226 [details]
rename webSocketsEnabled to experimentalWebSocketEnabled

I dont' think the explicit resetDefaultsToConsistentValues reset calls are really necessary, but they're certainly not a bad idea either.

LGTM.  If you want this auto-committed, you'll need to set cq? as well.  I can't remember if you're a committer yet or not.

[WebKit Commit Bot]

Comment on attachment 40226 [details]
rename webSocketsEnabled to experimentalWebSocketEnabled

Rejecting patch 40226 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Fumitoshi Ukai]

(In reply to comment #12)
> (From update of attachment 40226 [details])
> Rejecting patch 40226 from commit-queue.
> 
> [e-mail] does not have committer permissions according to
> http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

Hmm, I was too early to set commit-queue+?

[WebKit Commit Bot]

Comment on attachment 40226 [details]
rename webSocketsEnabled to experimentalWebSocketEnabled

Rejecting patch 40226 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.

[Eric Seidel (no email)]

Clearly you did not use bugzilla-tool to do the commit, or run the layout tests, since it looks like you broke all platforms:
http://build.webkit.org/changes/1106
http://build.webkit.org/waterfall

:(

Assuming I'm reading the buildbot output correctly.

I'm going to roll out this patch.

[Eric Seidel (no email)]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 40226 [details] [details])
> > Rejecting patch 40226 from commit-queue.
> > 
> > [e-mail] does not have committer permissions according to
> > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
> 
> Hmm, I was too early to set commit-queue+?

No, you just haven't added yourself to committers.py yet.  You can't set commit-queue+ until you do.

[Eric Seidel (no email)]

I'm glad to see that you have commit bit now!  But please get another committer to walk you through your next commit, since there were several process errors here.  Most notably, you did not watch the bots after committing.

You also did not update the bug with the committed revision (yes, we need to document that somewhere).  And you did not close the bug after committing.

bugzilla-tool land-diff can do all of these things for you, although you're also welcome to perform the actions manually.

[Fumitoshi Ukai]

(In reply to comment #16)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > (From update of attachment 40226 [details] [details] [details])
> > > Rejecting patch 40226 from commit-queue.
> > > 
> > > [e-mail] does not have committer permissions according to
> > > http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py.
> > 
> > Hmm, I was too early to set commit-queue+?
> 
> No, you just haven't added yourself to committers.py yet.  You can't set
> commit-queue+ until you do.

I believe I added myself to committers.py (r48852) before I set commit-queue+...

[Fumitoshi Ukai]

(In reply to comment #15)
> Clearly you did not use bugzilla-tool to do the commit, or run the layout
> tests, since it looks like you broke all platforms:
> http://build.webkit.org/changes/1106
> http://build.webkit.org/waterfall
> 
> :(
> 
> Assuming I'm reading the buildbot output correctly.
> 
> I'm going to roll out this patch.

Sorry, I've prepared a fix in http://bugs.webkit.org/show_bug.cgi?id=29840.
Do you roll back this?  If so, I'll merge the fix here.

[Eric Seidel (no email)]

(In reply to comment #18)
> I believe I added myself to committers.py (r48852) before I set
> commit-queue+...

Sorry, that's my fault.  I was running the commit-queue in a way such that it was not auto-updating committers.py correctly.  I've fixed that now.

[Eric Seidel (no email)]

My apologies for my aggressive tone above.  It was undeserved.  Thank you for so quick a response!

[Fumitoshi Ukai]

(In reply to comment #21)
> My apologies for my aggressive tone above.  It was undeserved.  Thank you for
> so quick a response!

No problem. I should be more carefully before I committed.
Thanks for your help!

[Fumitoshi Ukai]

Landed as r48861

[Sam Weinig]

I am confused why this flag is called experimentalWebSocketsEnabled (I see in one comment it was stated this is a new rule.  Where was this rule established).  Do we plan on changing the setting once the implementation is done?  Does this work for things like "WebSocket" in window?  window.hasOwnProperty("WebSocket")?

[Fumitoshi Ukai]

(In reply to comment #24)
> I am confused why this flag is called experimentalWebSocketsEnabled (I see in
> one comment it was stated this is a new rule.  Where was this rule
> established).  

In this thread?
https://lists.webkit.org/pipermail/webkit-dev/2009-September/009881.html

> Do we plan on changing the setting once the implementation is
> done?  Does this work for things like "WebSocket" in window? 
> window.hasOwnProperty("WebSocket")?

I assumed a web developer would check with 'typeof window.WebSocket == "undefined"', as described in https://lists.webkit.org/pipermail/webkit-dev/2009-September/010000.html