Chrome changes to extract the code from V8Proxy that special-cases...

Chrome changes to extract the code from V8Proxy that special-cases
when scripts are allowed despite user preferences disabling them.

Adds more accessors and comments to WebSecurityOrigin.

Removes no longer necessary webkit_glue functions.

Removes no longer necessary TemporaryGlue.h file.

R=abarth
BUG=none
TEST=browser features like the new tab page and history view should
still work when passing --disable-javascript to chrome.  similarly,
file and ftp directory listings should remain functional when that
command line flag is specified.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30797

[abarth-chromium, 2009-11-02 19:37:51]

http://codereview.chromium.org/351013/diff/1/11
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/351013/diff/1/11#newcode2431
Line 2431: return false;  // Uninitialized document?
You're not handling the case where the security origin is the string "null". 
This occurs for data URLs and noAccess origins.

http://trac.webkit.org/browser/trunk/WebCore/page/SecurityOrigin.cpp#L292

Did you test loading the various DOM UI pages with JavaScript disabled?  I
thought we had noAccess turned on for most / all of these.

http://codereview.chromium.org/351013/diff/1/11#newcode2434
Line 2434: return false;  // Web site protocols must honor settings.
Why are these special-cased?  Seems like they should fall though the end of the
function.  Is performance really a concern here?

http://codereview.chromium.org/351013/diff/1/7
File webkit/api/public/WebFrameClient.h (right):

http://codereview.chromium.org/351013/diff/1/7#newcode93
Line 93: WebNavigationPolicy defaultPolicy, bool isRedirect) { return
defaultPolicy; }
This seems unrelated

http://codereview.chromium.org/351013/diff/1/7#newcode109
Line 109: WebFrame*, const WebURLError&) { }
ditto

http://codereview.chromium.org/351013/diff/1/7#newcode115
Line 115: virtual void willSubmitForm(WebFrame*, const WebForm&) { }
ditto

http://codereview.chromium.org/351013/diff/1/7#newcode121
Line 121: double interval, double fireTime) { }
ditto

[darin (slow to review), 2009-11-02 23:33:02]

the small tweaks in WebFrameClient are just incidental cleanups.

http://codereview.chromium.org/351013/diff/1/11
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/351013/diff/1/11#newcode2431
Line 2431: return false;  // Uninitialized document?
On 2009/11/02 19:37:51, abarth wrote:
> You're not handling the case where the security origin is the string "null". 
> This occurs for data URLs and noAccess origins.
> 
> http://trac.webkit.org/browser/trunk/WebCore/page/SecurityOrigin.cpp#L292
> 
> Did you test loading the various DOM UI pages with JavaScript disabled?  I
> thought we had noAccess turned on for most / all of these.

"null" is not a valid GURL.

besides that detail, i am just copying the implementation of V8Proxy::isEnabled
from here:
http://trac.webkit.org/browser/trunk/WebCore/bindings/v8/V8Proxy.cpp#L626

http://codereview.chromium.org/351013/diff/1/11#newcode2434
Line 2434: return false;  // Web site protocols must honor settings.
On 2009/11/02 19:37:51, abarth wrote:
> Why are these special-cased?  Seems like they should fall though the end of
the
> function.  Is performance really a concern here?

I have no idea.  I just copied the code.  See link above.

[abarth-chromium, 2009-11-03 00:17:10]

http://codereview.chromium.org/351013/diff/1/11
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/351013/diff/1/11#newcode2431
Line 2431: return false;  // Uninitialized document?
I don't think this code is correct.  In the case where JavaScript is disabled
and our built-in pages have noAccess set, this code will disable JavaScript and
the old code will not.

http://codereview.chromium.org/351013/diff/1/11#newcode2434
Line 2434: return false;  // Web site protocols must honor settings.
Ok.

[darin (slow to review), 2009-11-03 00:53:42]

On Mon, Nov 2, 2009 at 5:17 PM, <abarth@chromium.org> wrote:

>
> http://codereview.chromium.org/351013/diff/1/11
> File chrome/renderer/render_view.cc (right):
>
> http://codereview.chromium.org/351013/diff/1/11#newcode2431
> Line 2431: return false;  // Uninitialized document?
> I don't think this code is correct.  In the case where JavaScript is
> disabled and our built-in pages have noAccess set, this code will
> disable JavaScript and the old code will not.


You are correct as indeed my testing confirms.  I should have caught
this before sending this out to you :(

Reading the comments in SecurityOrigin.h, it is easy to get the
impression that isEmpty() iff toString() == "null".

I will most likely add an isEmpty method to WebSecurityOrigin.

-Darin


>
>
> http://codereview.chromium.org/351013/diff/1/11#newcode2434
> Line 2434: return false;  // Web site protocols must honor settings.
> Ok.
>
>
> http://codereview.chromium.org/351013
>

[darin (slow to review), 2009-11-03 05:13:27]

OK, patch updated.  This one actually works :-/

[abarth-chromium, 2009-11-03 05:21:08]

On 2009/11/03 05:13:27, darin wrote:
> OK, patch updated.  This one actually works :-/

Great!  LGTM.  Too bad WebSecurityOrigin is growing a bigger interface, but it's
better than having everyone parse toString and screw it up.