Bug 125746 – [iOS] Upstream PLATFORM(IOS) changes to Source/WebKit/

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Andy Estes]

[iOS] Upstream PLATFORM(IOS) changes to Source/WebKit/

[Andy Estes]

Created attachment 219265 [details]
[iOS] Upstream PLATFORM(IOS) changes to Source/WebKit/

[Daniel Bates]

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

[David Kilzer (:ddkilzer)]

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.  :)

[Andy Estes]

(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!

[Andy Estes]

(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?

[Andy Estes]

Committed r161043: <http://trac.webkit.org/changeset/161043>

[Andy Estes]

I rolled this out in http://trac.webkit.org/changeset/161049 because it broke the 32-bit Mac build. Reopening to investigate.

[Andy Estes]

Committed r161185: <http://trac.webkit.org/changeset/161185>