Bug 94058

Summary: [BlackBerry] [DRT] 5 tests failed for incorrect JS console message format
Product: WebKit Reporter: Xiaobo Wang <xiaobwang>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mifenton, mxie, rwlbuis, staikos, tonikitoo, webkit.review.bot, yong.li.webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
patch
rwlbuis: review+
patch updated none

Description Xiaobo Wang 2012-08-14 18:58:14 PDT
From RIM PR #191441

Failed cases:
1 fast/frames/sandboxed-iframe-history-denied.html
2 fast/frames/sandboxed-iframe-navigation-targetlink.html
3 fast/frames/sandboxed-iframe-navigation-top-by-name-denied.html
4 fast/frames/sandboxed-iframe-navigation-top-denied.html
5 fast/frames/seamless/seamless-window-location-sandbox.html

All tests failed because of incorrect console message format.
Need to fix in DumpRenderTree code.
Here's the text diff for one of the tests.

---/tmp/developer/layout-test-results/fast/frames/sandboxed-iframe-navigation-top-denied-expected.txt
+++/tmp/developer/layout-test-results/fast/frames/sandboxed-iframe-navigation-top-denied-actual.txt
@@ -1,4 +1,6 @@
-CONSOLE MESSAGE: Unsafe JavaScript attempt to initiate a navigation change for frame with URL navigate-top-to-fail.html.
+CONSOLE MESSAGE: line 0: Unsafe JavaScript attempt to initiate a navigation change for frame with URL file:///developer/LayoutTests/fast/frames/sandboxed-iframe-navigation-top-denied.html from frame with URL file:///developer/LayoutTests/fast/frames/resources/navigate-top-to-fail.html.
+
+JS Console: :0: Unsafe JavaScript attempt to initiate a navigation change for frame with URL file:///developer/LayoutTests/fast/frames/sandboxed-iframe-navigation-top-denied.html from frame with URL file:///developer/LayoutTests/fast/frames/resources/navigate-top-to-fail.html.

 This test verifies that a sandboxed IFrame cannot navigate the top-level frame without allow-top-navigation. This test passes if the navigation does not occur.
Comment 1 Xiaobo Wang 2012-08-14 19:58:08 PDT
Created attachment 158486 [details]
patch

Patch for this issue. The Source part is patched by Rob Buis.
Comment 2 Xiaobo Wang 2012-08-15 01:58:16 PDT
Adding Yong, Antonio, and George.
Can any of you help to review?
Comment 3 Rob Buis 2012-08-15 05:47:38 PDT
Comment on attachment 158486 [details]
patch

Thanks for this patch, looks good!
Comment 4 Yong Li 2012-08-15 07:08:38 PDT
Comment on attachment 158486 [details]
patch

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

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:653
> +        String remaining = message.substring(pos, message.length() - pos);

", message.length() - pos" is not necessary

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:657
> +            baseName = remaining.substring(indexBase + 1, remaining.length() - indexBase - 1);

ditto. Also it should be called "fileName" rather than "baseName"? "Base" sounds like base URL / the folder path...
Comment 5 Xiaobo Wang 2012-08-15 19:22:09 PDT
(In reply to comment #4)
> (From update of attachment 158486 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158486&action=review
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:653
> > +        String remaining = message.substring(pos, message.length() - pos);
> 
> ", message.length() - pos" is not necessary
> 
> > Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:657
> > +            baseName = remaining.substring(indexBase + 1, remaining.length() - indexBase - 1);
> 
> ditto. Also it should be called "fileName" rather than "baseName"? "Base" sounds like base URL / the folder path...

Good suggestions Yong. I agree "fileName" is better. Actually I borrowed the name from the UNIX command "basename" which usually returns the file name of a full path. Will update the patch soon.
Comment 6 Xiaobo Wang 2012-08-16 01:18:23 PDT
Created attachment 158745 [details]
patch updated

Updated according to comments from Yong Li.
Comment 7 Yong Li 2012-08-16 07:17:27 PDT
Comment on attachment 158745 [details]
patch updated

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

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:662
> +    printf("%s\n", newMessage.utf8().data());

I have introduced a class ReadOnlyLatinString that will give better performance if you are sure latin1() won't lose any info and in most cases the String has a 8-bit internal buffer. But this is minor thing, especially this is only for DRT.
Comment 8 WebKit Review Bot 2012-08-16 07:55:10 PDT
Comment on attachment 158745 [details]
patch updated

Clearing flags on attachment: 158745

Committed r125778: <http://trac.webkit.org/changeset/125778>
Comment 9 WebKit Review Bot 2012-08-16 07:55:14 PDT
All reviewed patches have been landed.  Closing bug.