We should provide an API to developers so that they can set whether the WKWebView should always allow scaling of the web page, regardless of author intent. <rdar://problem/26425468>
<rdar://problem/26631538>
Created attachment 280472 [details] initial patch
Created attachment 280475 [details] patch fix build failure
Created attachment 280478 [details] patch Another build fix for mac ports
Comment on attachment 280478 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=280478&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:246 > + @seealso backForwardList. This comment should be removed @seealso backForwardList. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214 > +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 Is all the iOS versioning required? This code will never run on an older device right > Tools/WebKitTestRunner/TestController.cpp:2144 > +#if PLATFORM(COCOA) Should this be IOS
Comment on attachment 280478 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=280478&action=review >> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214 >> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > > Is all the iOS versioning required? This code will never run on an older device right Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well?
Comment on attachment 280478 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=280478&action=review >>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214 >>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 >> >> Is all the iOS versioning required? This code will never run on an older device right > > Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well? It seems like that should be kept there so people compiling against different versions know whats there or not, but internally the versioning seems uncessary to me. Maybe someone else has a different opinion
Comment on attachment 280478 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=280478&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248 > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0); The naming of this API is surprising (and the fact that it's API at all, to start out with). I cc'd andersca for thoughts. >>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214 >>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 >>> >>> Is all the iOS versioning required? This code will never run on an older device right >> >> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well? > > It seems like that should be kept there so people compiling against different versions know whats there or not, but internally the versioning seems uncessary to me. Maybe someone else has a different opinion The versioning should definitely not be here, there's no reason for it and it absolutely can be run on different versions (and should be usable and testable there).
(In reply to comment #8) > Comment on attachment 280478 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=280478&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248 > > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0); > > The naming of this API is surprising (and the fact that it's API at all, to > start out with). I cc'd andersca for thoughts. > > >>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214 > >>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > >>> > >>> Is all the iOS versioning required? This code will never run on an older device right > >> > >> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well? New APIs should be versioned. We use WK_IOS_TBA for new APIs. I don't like the name. > > > > It seems like that should be kept there so people compiling against different versions know whats there or not, but internally the versioning seems uncessary to me. Maybe someone else has a different opinion > > The versioning should definitely not be here, there's no reason for it and > it absolutely can be run on different versions (and should be usable and > testable there).
(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 280478 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=280478&action=review > > > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248 > > > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0); > > > > The naming of this API is surprising (and the fact that it's API at all, to > > start out with). I cc'd andersca for thoughts. > > > > >>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214 > > >>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > > >>> > > >>> Is all the iOS versioning required? This code will never run on an older device right > > >> > > >> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well? > > New APIs should be versioned. We use WK_IOS_TBA for new APIs. I don't like > the name. The interface should have availability macros, yes, but the implementation shouldn't be #if'd out, should it?
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Comment on attachment 280478 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=280478&action=review > > > > > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:248 > > > > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, 10_0); > > > > > > The naming of this API is surprising (and the fact that it's API at all, to > > > start out with). I cc'd andersca for thoughts. > > > > > > >>>> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:2214 > > > >>>> +#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > > > >>> > > > >>> Is all the iOS versioning required? This code will never run on an older device right > > > >> > > > >> Because we set the property WK_AVAILABLE(NA, 10_0), do you think we should remove that as well? > > > > New APIs should be versioned. We use WK_IOS_TBA for new APIs. I don't like > > the name. Do you have any suggestion for the name? > > The interface should have availability macros, yes, but the implementation > shouldn't be #if'd out, should it? I think the code won't compile if the version is lower than the availability macro.
(In reply to comment #11) > > The interface should have availability macros, yes, but the implementation > > shouldn't be #if'd out, should it? > > I think the code won't compile if the version is lower than the availability > macro. No. The availability macro (WK_AVAILABLE) is unrelated to what code actually builds, and is just an annotation. You only want the availability macro, not the #if __IPHONE_OS_VERSION_MIN_REQUIRED bits.
(In reply to comment #12) > No. The availability macro (WK_AVAILABLE) is unrelated to what code actually > builds, and is just an annotation. You only want the availability macro, not > the #if __IPHONE_OS_VERSION_MIN_REQUIRED bits. Got it. But why in my previous patch, I only have the (WK_AVAILABLE) macro, but it gave me the error: property 'alwaysUserScalable' not found on object of type 'WKWebView *' on the mac port?
(In reply to comment #13) > (In reply to comment #12) > > No. The availability macro (WK_AVAILABLE) is unrelated to what code actually > > builds, and is just an annotation. You only want the availability macro, not > > the #if __IPHONE_OS_VERSION_MIN_REQUIRED bits. > > Got it. But why in my previous patch, I only have the (WK_AVAILABLE) macro, > but it gave me the error: property 'alwaysUserScalable' not found on object > of type 'WKWebView *' on the mac port? If your property only exists/works on iOS you may need some #if PLATFORM(IOS) guards, but not the version guards.
(In reply to comment #14) > If your property only exists/works on iOS you may need some #if > PLATFORM(IOS) guards, but not the version guards. Ok, will remove the #if __IPHONE_OS_VERSION_MIN_REQUIRED
Created attachment 280860 [details] patch - Addressed review comments. - removed the __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 check - Not sure what a good name would be for the property. Suggestions welcome!
Comment on attachment 280860 [details] patch Attachment 280860 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1469705 New failing tests: editing/selection/selection-in-iframe-removed-crash.html
Created attachment 280867 [details] Archive of layout-test-results from ews114 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #17) > Comment on attachment 280860 [details] > patch > > Attachment 280860 [details] did not pass mac-debug-ews (mac): > Output: http://webkit-queues.webkit.org/results/1469705 > > New failing tests: > editing/selection/selection-in-iframe-removed-crash.html Nan is this failure related to this patch
(In reply to comment #19) > (In reply to comment #17) > > Comment on attachment 280860 [details] > > patch > > > > Attachment 280860 [details] did not pass mac-debug-ews (mac): > > Output: http://webkit-queues.webkit.org/results/1469705 > > > > New failing tests: > > editing/selection/selection-in-iframe-removed-crash.html > > Nan is this failure related to this patch I don't think so.
Comment on attachment 280860 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=280860&action=review > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.h:247 > +@property (nonatomic) BOOL alwaysUserScalable WK_AVAILABLE(NA, WK_IOS_TBA); How do people feel about "ignoresUserScalable"
Created attachment 283048 [details] Patch - Changed the property name to ignoresViewportScaleLimits - Moved the property to WKWebViewConfiguration - Added a test option so that we can set the configuration in test.
Comment on attachment 283048 [details] Patch Instead of having anything on the WKWebView, just add a boolean to WebPageCreationParameters and pipe it through from the WKWebView initializer. I think having a setting on the Internals object for testing is better than having an injected bundle function.
Created attachment 283255 [details] patch Based on the review: - Added a boolean in WebPageCreationParameters and removed logics in WKWebView - Since the viewport configuration is in WebKit2 and Internals is in WebCore, there's no easy way to communicate between. So I kept the injected bundle function.
Created attachment 283256 [details] patch fixed a small typo
Comment on attachment 283256 [details] patch Attachment 283256 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1652076 New failing tests: fast/scrolling/ios/scrollTo-at-page-load.html
Created attachment 283257 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Created attachment 283262 [details] patch The failure test passed on my local machine. Let's try again.
Comment on attachment 283262 [details] patch Clearing flags on attachment: 283262 Committed r203075: <http://trac.webkit.org/changeset/203075>
All reviewed patches have been landed. Closing bug.