Bug 66823

Summary: Add the request info to the WebPageProxy::CreateNewPage message
Product: WebKit Reporter: Alice Liu <alice.barraclough>
Component: WebKit2Assignee: Alice Liu <alice.barraclough>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch w/ fixed style
none
patch with fixes for MiniBrowser on gtk and windows
none
patch - keeps nightlies working
none
patch - with fix for minibrowser win andersca: review+

Description Alice Liu 2011-08-23 16:58:51 PDT
Add the request info to the WebPageProxy::CreateNewPage message.  WebKit clients will then have more information, such as the request url, to help them decide on how to handle a CreateNewPage message.
Comment 1 Alice Liu 2011-08-23 17:12:09 PDT
Created attachment 104937 [details]
patch
Comment 2 WebKit Review Bot 2011-08-23 17:14:35 PDT
Attachment 104937 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp:53:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alice Liu 2011-08-23 17:25:05 PDT
Created attachment 104942 [details]
patch w/ fixed style
Comment 4 Alice Liu 2011-08-23 17:40:14 PDT
Created attachment 104946 [details]
patch with fixes for MiniBrowser on gtk and windows
Comment 5 Alexey Proskuryakov 2011-08-24 10:46:15 PDT
Sending initial request to the client seems quite surprising. What is the client going to do with this information? The request can result in a redirect, so a completely different resource will be used in the end.

Also, it seems like this patch would break nightly builds due to changing API.
Comment 6 Alice Liu 2011-08-24 11:03:38 PDT
(In reply to comment #5)
> Sending initial request to the client seems quite surprising. What is the client going to do with this information? The request can result in a redirect, so a completely different resource will be used in the end.

I'm not sure I see why you consider the redirect issue to be a problem.  A client can definitely make use of initial request info. your comment seems to assume that the client will be using the resource as specified and that no updates to it will be made.  that's not necessarily the case.  The request info is merely being provided so that the client can act on the createNewPage message as it sees fit.  

> Also, it seems like this patch would break nightly builds due to changing API.

i did try to address the API change where I was able to find it, namely the mini browser implementations for mac, win and gtk.  Did i miss something, and could you be more specific about where to find what i missed? 

thanks
Comment 7 Alexey Proskuryakov 2011-08-24 11:24:17 PDT
> I'm not sure I see why you consider the redirect issue to be a problem.  A client can definitely make use of initial request info.

Due to my weak imagination, I cannot fathom any valid use for this info in the client, which makes me wonder about it.

> Did i miss something, and could you be more specific about where to find what i missed? 

Quoting myself, "it seems like this patch would break nightly builds". Those need to work with Safari 5.1.
Comment 8 Alice Liu 2011-08-25 16:18:58 PDT
Created attachment 105272 [details]
patch - keeps nightlies working
Comment 9 WebKit Review Bot 2011-08-25 16:21:22 PDT
Attachment 105272 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/UIProcess/WebUIClient.cpp:48:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/UIProcess/WebUIClient.cpp:72:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebKit2/Shared/APIClientTraits.h:46:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Alice Liu 2011-08-25 16:31:34 PDT
I won't be uploading a new patch just to to fix the style.  I'll just say that that I have fixed the null-comparison style, but i'm not going to fix the brace style.
Comment 11 Alice Liu 2011-08-25 22:21:41 PDT
Created attachment 105312 [details]
patch - with fix for minibrowser win
Comment 12 WebKit Review Bot 2011-08-25 22:25:48 PDT
Attachment 105312 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1

Source/WebKit2/Shared/APIClientTraits.h:46:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.