[iOS] Upstream PLATFORM(IOS) changes to Source/WebKit/
Created attachment 219265 [details] [iOS] Upstream PLATFORM(IOS) changes to Source/WebKit/
Comment on attachment 219265 [details] [iOS] Upstream PLATFORM(IOS) changes to Source/WebKit/ View in context: https://bugs.webkit.org/attachment.cgi?id=219265&action=review Briefly read through some of this patch. When I have a moment, I'll look to finish reading through this patch and be more thorough. > Source/WebKit/mac/DOM/WebDOMOperations.mm:110 > + if (![self isKindOfClass:[DOMHTMLInputElement class]] && Nit: && should be moved to the start of the next line. > Source/WebKit/mac/DOM/WebDOMOperations.mm:121 > + if (![self isKindOfClass:[DOMHTMLInputElement class]] && Ditto. > Source/WebKit/mac/DOM/WebDOMOperations.mm:125 > + Node *node = core(self); Nit: This variable is used exactly once (below) and its name doesn't improve the readability of this code. I suggest we inline its value into the line below. > Source/WebKit/mac/History/WebBackForwardList.mm:181 > + HistoryItemVector::iterator it = historyItems.begin(); Nit: We should use the keyword auto here. > Source/WebKit/mac/History/WebBackForwardList.mm:182 > + while (it != historyItems.end()) { Nit: We should look to convert this to a C++11 range-based for loop. > Source/WebKit/mac/History/WebBackForwardList.mm:184 > + it++; Nit: Ideally we would use an C++11 range-based for loop (see above comment). Otherwise, I would use a pre increment operator here since we aren't using the return value of the post-increment operator. > Source/WebKit/mac/History/WebBackForwardList.mm:189 > + [NSNumber numberWithUnsignedInt:coreBFList->current()], WebBackForwardListDictionaryCurrentKey, Nit: We should look to expose a better interface to BackForwardList so that we don't need to directly access its internal index (via BackForwardList::current()) as such a detail seems to violate encapsulation. Consider adding a FIXME comment. > Source/WebKit/mac/History/WebBackForwardList.mm:214 > + size_t listSize = coreBFList->entries().size(); > + if (currentIndex >= listSize) > + currentIndex = listSize - 1; > + coreBFList->setCurrent(currentIndex); Nit: We should look to expose a better interface to BackForwardList so that we don't need to manage its size and index. Such details seem to violate the encapsulation of the BackForwardList. Consider adding a FIXME comment. > Source/WebKit/mac/History/WebHistory.mm:362 > + WebHistoryItem *otherEntry= [_entriesByURL objectForKey:URLString]; Nit: Missing a space on the left side of the '='. > Source/WebKit/mac/History/WebHistory.mm:367 > + if ([otherEntry lastVisitedTimeInterval]<[entry lastVisitedTimeInterval]) { Nit: Missing a space on both sides of the '<'. > Source/WebKit/mac/History/WebHistory.mm:377 > + Nit: Line begins with extraneous whitespace. > Source/WebKit/mac/History/WebHistory.mm:382 > + // Special case for merges when new items may be older than pre-existing entries. > + else Nit: The else should be on the same line as the '}'. Consider moving the comment on line 381 to be inline on line 382. > Source/WebKit/mac/History/WebHistoryItem.mm:481 > +#if PLATFORM(IOS) > - (NSDictionary *)dictionaryRepresentation > { > + return [self dictionaryRepresentationIncludingChildren:YES]; > +} > + > +- (NSDictionary *)dictionaryRepresentationIncludingChildren:(BOOL)includesChildren > +#else > +- (NSDictionary *)dictionaryRepresentation > +#endif We should look to clean this up after we upstream iOS. One way to clean this up is to keep the dictionaryRepresentationIncludingChildren() function and compile it without guards then update the Mac callers to pass NO. You may want to consider adding a FIXME comment to the code to help increase the visibility of this effort. > Source/WebKit/mac/History/WebHistoryItem.mm:555 > + if (viewportArguments) { > + [dict setObject:viewportArguments forKey:@"WebViewportArguments"]; > + } Nit: The curly braces aren't necessary. > Source/WebKit/mac/Misc/WebCache.mm:202 > + WebCore::SecurityOrigin* topOrigin = 0; Nit: Use nullptr instead of 0. > Source/WebKit/mac/Misc/WebCache.mm:219 > + WebCore::SecurityOrigin* topOrigin = 0; Ditto. > Source/WebKit/mac/Misc/WebCache.mm:230 > + return 0; Ditto. > Source/WebKit/mac/Misc/WebCache.mm:234 > + return 0; Ditto. > Source/WebKit/mac/Misc/WebCache.mm:237 > + return 0; Ditto. > Source/WebKit/mac/Misc/WebNSURLExtras.mm:299 > + Nit: This line has extraneous whitespace. > Source/WebKit/mac/Misc/WebNSURLExtras.mm:523 > + if (queryLength == 0) Nit: !queryLength instead of queryLength == 0 > Source/WebKit/mac/Misc/WebNSURLExtras.mm:529 > + while (equalSearchRange.location < queryLength - 1 && equalSearchRange.length != 0) { Nit: The "!= 0" isn't necessary as NSRange.length is an unsigned integer. > Source/WebKit/mac/Misc/WebNSURLExtras.mm:555 > + if (keyRange.length != 0 && valueRange.length != 0) { Ditto. > Source/WebKit/mac/Misc/WebNSURLExtras.mm:560 > + if ([key length] != 0 && [value length] != 0) Nit: Similarly the "!= 0" isn't necessary as NSString.length is an unsigned datatype. > Source/WebKit/mac/Plugins/WebPluginController.mm:108 > + static bool audioSessionWasInitialized = false; Nit: It's unnecessary to explicitly initialize this variable to false. The C and C++ standard guarantee that variables of static storage duration are zero initialized. Also, we tend to follow the Cocoa naming convention and place the verb at the beginning of the variable name (e.g. wasAudioSessionInitialized). > Source/WebKit/mac/Plugins/WebPluginController.mm:215 > + FrameView* coreView = coreFrame ? coreFrame->view() : 0; Nit: 0 => nullptr. > Source/WebKit/mac/Plugins/WebPluginController.mm:318 > + for (NSUInteger i = 0; i < viewsCount; i++) Nit: i++ => ++i (since we aren't using the return value; although the compiler is likely smart enough to realize this). > Source/WebKit/mac/Plugins/WebPluginController.mm:323 > + for (NSUInteger i = 0; i < viewsNotInDocumentCount; i++) Ditto. > Source/WebKit/mac/Plugins/WebPluginController.mm:338 > + for (NSUInteger i = 0; i < viewsCount; i++) Ditto. > Source/WebKit/mac/Plugins/WebPluginController.mm:343 > + for (NSUInteger i = 0; i < viewsNotInDocumentCount; i++) Ditto. > Source/WebKit/mac/Storage/WebDatabaseManager.mm:204 > + deletedDatabaseFileCount++; Nit:: deletedDatabaseFileCount++ => ++deletedDatabaseFileCount > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:910 > + return 0; Nit: 0 => nullptr > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:919 > + return 0; Ditto. > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:807 > + m_selectionNotificationSuppressions--; Nit: m_selectionNotificationSuppressions-- => --m_selectionNotificationSuppressions > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1880 > + return 0; Nit: 0 => nullptr > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2159 > + return 0; Nit: 0 => nullptr > Source/WebKit/mac/WebCoreSupport/WebOpenPanelResultListener.mm:107 > + for (NSUInteger i = 0; i < count; i++) Nit: i++ => ++i > Source/WebKit/mac/WebCoreSupport/WebOpenPanelResultListener.mm:110 > + _chooser->deref(); Nit: It's sad we have to manage this ourselves. > Source/WebKit/mac/WebCoreSupport/WebOpenPanelResultListener.mm:111 > + _chooser = 0; 0 => nullptr
Comment on attachment 219265 [details] [iOS] Upstream PLATFORM(IOS) changes to Source/WebKit/ View in context: https://bugs.webkit.org/attachment.cgi?id=219265&action=review r=me > Source/WebKit/mac/Configurations/DebugRelease.xcconfig:33 > -MACOSX_DEPLOYMENT_TARGET_iphoneos = 10.5; > -MACOSX_DEPLOYMENT_TARGET_iphonesimulator = 10.5; > +MACOSX_DEPLOYMENT_TARGET_iphoneos = 10.6; > +MACOSX_DEPLOYMENT_TARGET_iphonesimulator = 10.6; Lies! > Source/WebKit/mac/Configurations/Version.xcconfig:35 > -SYSTEM_VERSION_PREFIX_iphoneos = 6; // iPhone OS is most like SnowLeopard currently. > +SYSTEM_VERSION_PREFIX_iphoneos = 6; // iOS is most like SnowLeopard currently. More lies! > Source/WebKit/mac/Configurations/WebKit.xcconfig:64 > -HEADER_SEARCH_PATHS = $(WEBKITSYSTEMINTERFACE_STATIC_LIBRARY_HEADERS_FOLDER_PATH) $(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders $(WEBCORE_PRIVATE_HEADERS_DIR)/icu "${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit" $(HEADER_SEARCH_PATHS); > +HEADER_SEARCH_PATHS = $(WEBKITSYSTEMINTERFACE_STATIC_LIBRARY_HEADERS_FOLDER_PATH) $(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders $(WEBCORE_PRIVATE_HEADERS_DIR)/icu "${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit" "$(BUILT_PRODUCTS_DIR)/usr/local/include" $(HEADER_SEARCH_PATHS); This typo exited in open source, but the curly brackets around "${BUILT_PRODUCTS_DIR} " should be parenthesis instead "$(BUILT_PRODUCTS_DIR)". > Source/WebKit/mac/History/WebBackForwardList.mm:110 > + (void)initialize > { > +#if !PLATFORM(IOS) > JSC::initializeThreading(); > WTF::initializeMainThreadToProcessMainThread(); > RunLoop::initializeMainRunLoop(); > +#endif All code like this needs to turn into a runtime check eventually. (Please file a bug.) > Source/WebKit/mac/WebView/WebFrame.mm:1751 > +// WebCoreFrameBridge methods used by iOS applications and frameworks Oh. The. Horror. WebCoreFrameBride is long gone. :)
(In reply to comment #2) > (From update of attachment 219265 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219265&action=review > > Briefly read through some of this patch. When I have a moment, I'll look to finish reading through this patch and be more thorough. > > > Source/WebKit/mac/DOM/WebDOMOperations.mm:110 > > + if (![self isKindOfClass:[DOMHTMLInputElement class]] && > > Nit: && should be moved to the start of the next line. Done. > > > Source/WebKit/mac/DOM/WebDOMOperations.mm:121 > > + if (![self isKindOfClass:[DOMHTMLInputElement class]] && > > Ditto. > > > Source/WebKit/mac/DOM/WebDOMOperations.mm:125 > > + Node *node = core(self); > > Nit: This variable is used exactly once (below) and its name doesn't improve the readability of this code. I suggest we inline its value into the line below. Done. > > > Source/WebKit/mac/History/WebBackForwardList.mm:181 > > + HistoryItemVector::iterator it = historyItems.begin(); > > Nit: We should use the keyword auto here. Done. > > > Source/WebKit/mac/History/WebBackForwardList.mm:182 > > + while (it != historyItems.end()) { > > Nit: We should look to convert this to a C++11 range-based for loop. Done. > > > Source/WebKit/mac/History/WebBackForwardList.mm:184 > > + it++; > > Nit: Ideally we would use an C++11 range-based for loop (see above comment). Otherwise, I would use a pre increment operator here since we aren't using the return value of the post-increment operator. I went with the range-based for loop. > > > Source/WebKit/mac/History/WebBackForwardList.mm:189 > > + [NSNumber numberWithUnsignedInt:coreBFList->current()], WebBackForwardListDictionaryCurrentKey, > > Nit: We should look to expose a better interface to BackForwardList so that we don't need to directly access its internal index (via BackForwardList::current()) as such a detail seems to violate encapsulation. Consider adding a FIXME comment. // FIXME: Move into WebCore the code that deals directly with WebCore::BackForwardList. > > > Source/WebKit/mac/History/WebBackForwardList.mm:214 > > + size_t listSize = coreBFList->entries().size(); > > + if (currentIndex >= listSize) > > + currentIndex = listSize - 1; > > + coreBFList->setCurrent(currentIndex); > > Nit: We should look to expose a better interface to BackForwardList so that we don't need to manage its size and index. Such details seem to violate the encapsulation of the BackForwardList. Consider adding a FIXME comment. > > > Source/WebKit/mac/History/WebHistory.mm:362 > > + WebHistoryItem *otherEntry= [_entriesByURL objectForKey:URLString]; > > Nit: Missing a space on the left side of the '='. Fixed. > > > Source/WebKit/mac/History/WebHistory.mm:367 > > + if ([otherEntry lastVisitedTimeInterval]<[entry lastVisitedTimeInterval]) { > > Nit: Missing a space on both sides of the '<'. Fixed. > > > Source/WebKit/mac/History/WebHistory.mm:377 > > + > > Nit: Line begins with extraneous whitespace. Fixed. > > > Source/WebKit/mac/History/WebHistory.mm:382 > > + // Special case for merges when new items may be older than pre-existing entries. > > + else > > Nit: The else should be on the same line as the '}'. Consider moving the comment on line 381 to be inline on line 382. Fixed. > > > Source/WebKit/mac/History/WebHistoryItem.mm:481 > > +#if PLATFORM(IOS) > > - (NSDictionary *)dictionaryRepresentation > > { > > + return [self dictionaryRepresentationIncludingChildren:YES]; > > +} > > + > > +- (NSDictionary *)dictionaryRepresentationIncludingChildren:(BOOL)includesChildren > > +#else > > +- (NSDictionary *)dictionaryRepresentation > > +#endif > > We should look to clean this up after we upstream iOS. One way to clean this up is to keep the dictionaryRepresentationIncludingChildren() function and compile it without guards then update the Mac callers to pass NO. You may want to consider adding a FIXME comment to the code to help increase the visibility of this effort. // FIXME: The only iOS difference here should be whether YES or NO is passed to dictionaryRepresentationIncludingChildren: > > > Source/WebKit/mac/History/WebHistoryItem.mm:555 > > + if (viewportArguments) { > > + [dict setObject:viewportArguments forKey:@"WebViewportArguments"]; > > + } > > Nit: The curly braces aren't necessary. Fixed. > > > Source/WebKit/mac/Misc/WebCache.mm:202 > > + WebCore::SecurityOrigin* topOrigin = 0; > > Nit: Use nullptr instead of 0. Fixed. > > > Source/WebKit/mac/Misc/WebCache.mm:219 > > + WebCore::SecurityOrigin* topOrigin = 0; > > Ditto. > > > Source/WebKit/mac/Misc/WebCache.mm:230 > > + return 0; > > Ditto. > > > Source/WebKit/mac/Misc/WebCache.mm:234 > > + return 0; > > Ditto. > > > Source/WebKit/mac/Misc/WebCache.mm:237 > > + return 0; > > Ditto. > > > Source/WebKit/mac/Misc/WebNSURLExtras.mm:299 > > + > > Nit: This line has extraneous whitespace. Fixed. > > > Source/WebKit/mac/Misc/WebNSURLExtras.mm:523 > > + if (queryLength == 0) > > Nit: !queryLength instead of queryLength == 0 Fixed. > > > Source/WebKit/mac/Misc/WebNSURLExtras.mm:529 > > + while (equalSearchRange.location < queryLength - 1 && equalSearchRange.length != 0) { > > Nit: The "!= 0" isn't necessary as NSRange.length is an unsigned integer. Fixed. > > > Source/WebKit/mac/Misc/WebNSURLExtras.mm:555 > > + if (keyRange.length != 0 && valueRange.length != 0) { > > Ditto. > > > Source/WebKit/mac/Misc/WebNSURLExtras.mm:560 > > + if ([key length] != 0 && [value length] != 0) > > Nit: Similarly the "!= 0" isn't necessary as NSString.length is an unsigned datatype. Fixed. > > > Source/WebKit/mac/Plugins/WebPluginController.mm:108 > > + static bool audioSessionWasInitialized = false; > > Nit: It's unnecessary to explicitly initialize this variable to false. The C and C++ standard guarantee that variables of static storage duration are zero initialized. > > Also, we tend to follow the Cocoa naming convention and place the verb at the beginning of the variable name (e.g. wasAudioSessionInitialized). Fixed. > > > Source/WebKit/mac/Plugins/WebPluginController.mm:215 > > + FrameView* coreView = coreFrame ? coreFrame->view() : 0; > > Nit: 0 => nullptr. Fixed. > > > Source/WebKit/mac/Plugins/WebPluginController.mm:318 > > + for (NSUInteger i = 0; i < viewsCount; i++) > > Nit: i++ => ++i (since we aren't using the return value; although the compiler is likely smart enough to realize this). Fixed. > > > Source/WebKit/mac/Plugins/WebPluginController.mm:323 > > + for (NSUInteger i = 0; i < viewsNotInDocumentCount; i++) > > Ditto. > > > Source/WebKit/mac/Plugins/WebPluginController.mm:338 > > + for (NSUInteger i = 0; i < viewsCount; i++) > > Ditto. > > > Source/WebKit/mac/Plugins/WebPluginController.mm:343 > > + for (NSUInteger i = 0; i < viewsNotInDocumentCount; i++) > > Ditto. > > > Source/WebKit/mac/Storage/WebDatabaseManager.mm:204 > > + deletedDatabaseFileCount++; > > Nit:: deletedDatabaseFileCount++ => ++deletedDatabaseFileCount Fixed. > > > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:910 > > + return 0; > > Nit: 0 => nullptr Fixed. > > > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:919 > > + return 0; > > Ditto. > > > Source/WebKit/mac/WebCoreSupport/WebEditorClient.mm:807 > > + m_selectionNotificationSuppressions--; > > Nit: m_selectionNotificationSuppressions-- => --m_selectionNotificationSuppressions Fixed. > > > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:1880 > > + return 0; > > Nit: 0 => nullptr Fixed. > > > Source/WebKit/mac/WebCoreSupport/WebFrameLoaderClient.mm:2159 > > + return 0; > > Nit: 0 => nullptr Fixed. > > > Source/WebKit/mac/WebCoreSupport/WebOpenPanelResultListener.mm:107 > > + for (NSUInteger i = 0; i < count; i++) > > Nit: i++ => ++i Fixed. > > > Source/WebKit/mac/WebCoreSupport/WebOpenPanelResultListener.mm:110 > > + _chooser->deref(); > > Nit: It's sad we have to manage this ourselves. // FIXME: we shouldn't be manually deref()'ing here. > > > Source/WebKit/mac/WebCoreSupport/WebOpenPanelResultListener.mm:111 > > + _chooser = 0; > > 0 => nullptr Fixed. Thanks for the feedback!
(In reply to comment #3) > (From update of attachment 219265 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=219265&action=review > > r=me Thanks! > > > Source/WebKit/mac/Configurations/DebugRelease.xcconfig:33 > > -MACOSX_DEPLOYMENT_TARGET_iphoneos = 10.5; > > -MACOSX_DEPLOYMENT_TARGET_iphonesimulator = 10.5; > > +MACOSX_DEPLOYMENT_TARGET_iphoneos = 10.6; > > +MACOSX_DEPLOYMENT_TARGET_iphonesimulator = 10.6; > > Lies! Updated to 10.8. > > > Source/WebKit/mac/Configurations/Version.xcconfig:35 > > -SYSTEM_VERSION_PREFIX_iphoneos = 6; // iPhone OS is most like SnowLeopard currently. > > +SYSTEM_VERSION_PREFIX_iphoneos = 6; // iOS is most like SnowLeopard currently. > > More lies! Ditto. > > > Source/WebKit/mac/Configurations/WebKit.xcconfig:64 > > -HEADER_SEARCH_PATHS = $(WEBKITSYSTEMINTERFACE_STATIC_LIBRARY_HEADERS_FOLDER_PATH) $(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders $(WEBCORE_PRIVATE_HEADERS_DIR)/icu "${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit" $(HEADER_SEARCH_PATHS); > > +HEADER_SEARCH_PATHS = $(WEBKITSYSTEMINTERFACE_STATIC_LIBRARY_HEADERS_FOLDER_PATH) $(WEBCORE_PRIVATE_HEADERS_DIR)/ForwardingHeaders $(WEBCORE_PRIVATE_HEADERS_DIR)/icu "${BUILT_PRODUCTS_DIR}/DerivedSources/WebKit" "$(BUILT_PRODUCTS_DIR)/usr/local/include" $(HEADER_SEARCH_PATHS); > > This typo exited in open source, but the curly brackets around "${BUILT_PRODUCTS_DIR} " should be parenthesis instead "$(BUILT_PRODUCTS_DIR)". Fixed. > > > Source/WebKit/mac/History/WebBackForwardList.mm:110 > > + (void)initialize > > { > > +#if !PLATFORM(IOS) > > JSC::initializeThreading(); > > WTF::initializeMainThreadToProcessMainThread(); > > RunLoop::initializeMainRunLoop(); > > +#endif > > All code like this needs to turn into a runtime check eventually. (Please file a bug.) Filed https://bugs.webkit.org/show_bug.cgi?id=126197 > > > Source/WebKit/mac/WebView/WebFrame.mm:1751 > > +// WebCoreFrameBridge methods used by iOS applications and frameworks > > Oh. The. Horror. WebCoreFrameBride is long gone. :) // FIXME: WebCoreFrameBridge is long gone. Can we remove these methods?
Committed r161043: <http://trac.webkit.org/changeset/161043>
I rolled this out in http://trac.webkit.org/changeset/161049 because it broke the 32-bit Mac build. Reopening to investigate.
Committed r161185: <http://trac.webkit.org/changeset/161185>