Bug 200605 – [iOS] Add a quirk for gmail.com messages on iPhone iOS13

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

On iPhone iOS 13, the images in gmail.com messages are not rendered. The user agent for iOS 13 causes gmail.com to send all the images encapsulated in <iframe> elements whose dimensions are { width: 2px; height: 2px }. The bug was reported to Google. But till it is fixed, we are going to add a quirk which sets the user agent of iPhone iOS 12 for gmail messages only on iPhone iOS 13.

[Said Abou-Hallawa]

Created attachment 375997 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/52545742>

[Ryosuke Niwa]

Comment on attachment 375997 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:354
> +bool Quirks::isGoogleMail() const

The pattern of quirks is to define method like shouldAvoidUsingIOS13ForGmail here,
and have the main logic for computing the actual UA where it's used.

> Source/WebCore/page/Quirks.cpp:362
> +    if (!isGoogleMail())

Please add a check for if (!needsQuirks()) first.
This is important so that Google developers, etc... can disable this quirk via Web Inspector.

> Source/WebCore/page/Quirks.cpp:365
> +    static const NeverDestroyed<String> iPhoneOS13 = MAKE_STATIC_STRING_IMPL("iPhone OS 13_1");

All of this logic should really appear in WebPage::platformUserAgent instead.

> Source/WebCore/page/Quirks.h:77
> +#if PLATFORM(IOS_FAMILY)

Member functions of Quirk should never have if-def like this.
If-def should appear in the implementation of this function and should always return false for #else~#endif.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3788
> +    auto document = m_page->focusController().focusedOrMainFrame().document();

Please use m_frame->coreFrame()'s document instead.

[Said Abou-Hallawa]

Created attachment 376004 [details]
Patch

[Said Abou-Hallawa]

Created attachment 376009 [details]
Patch

[Said Abou-Hallawa]

Created attachment 376010 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 375997 [details]
Patch

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

>> Source/WebCore/page/Quirks.cpp:354
>> +bool Quirks::isGoogleMail() const
> 
> The pattern of quirks is to define method like shouldAvoidUsingIOS13ForGmail here,
> and have the main logic for computing the actual UA where it's used.

shouldAvoidUsingIOS13ForGmail() was added to Quirks. And the logic for changing the user agent was moved to WebPage::platformUserAgent() in WebPageIOS.mm.

>> Source/WebCore/page/Quirks.cpp:362
>> +    if (!isGoogleMail())
> 
> Please add a check for if (!needsQuirks()) first.
> This is important so that Google developers, etc... can disable this quirk via Web Inspector.

The check was added to Quirks::shouldAvoidUsingIOS13ForGmail().

>> Source/WebCore/page/Quirks.h:77
>> +#if PLATFORM(IOS_FAMILY)
> 
> Member functions of Quirk should never have if-def like this.
> If-def should appear in the implementation of this function and should always return false for #else~#endif.

Done in Quirks::shouldAvoidUsingIOS13ForGmail().

>> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:3788
>> +    auto document = m_page->focusController().focusedOrMainFrame().document();
> 
> Please use m_frame->coreFrame()'s document instead.

This was changed to:
    auto document = m_mainFrame->coreFrame()->document();

[Maciej Stachowiak]

Comment on attachment 376010 [details]
Patch

Is there a way to add a test case? (I'm not sure if quirks are testable). The code change looks ok and it seems like all the comments from the previous round of review have been addressed.

[Said Abou-Hallawa]

(In reply to Maciej Stachowiak from comment #8)
> Comment on attachment 376010 [details]
> Patch
> 
> Is there a way to add a test case? (I'm not sure if quirks are testable).
> The code change looks ok and it seems like all the comments from the
> previous round of review have been addressed.

I am not aware of a way for doing that especially gmail needs log-in credentials and navigating to a message with images.

I will ask around and if there a way for doing that I will post a follow up patch.

[Maciej Stachowiak]

(In reply to Said Abou-Hallawa from comment #9)
>
> I will ask around and if there a way for doing that I will post a follow up
> patch.

Sounds good.

[WebKit Commit Bot]

Comment on attachment 376010 [details]
Patch

Clearing flags on attachment: 376010

Committed r248501: <https://trac.webkit.org/changeset/248501>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.