Bug 215329

<rdar://problem/59824469>

Created attachment 406317 [details]
Patch

Comment on attachment 406317 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:501
> +    auto& url = m_document->topDocument().url();
> +    auto host = url.host().toString();
> +    if (host.startsWithIgnoringASCIICase("mail.") && topPrivatelyControlledDomain(host).startsWith("yahoo."))
> +        return true;
> +
> +    return false;

Can do this more efficiently like so:

    auto host = m_document->topDocument().url().host();
    return startsWithLettersIgnoringASCIICase(host, "mail.") && topPrivatelyControlledDomain(host.toString()).startsWith("yahoo.");

Also, I think we should write an isYahooMail helper function so we can share this logic with Quirks::shouldAvoidPastingImagesAsWebContent. Also, that uses a data member to cache the result. Should we be doing that here too? Or sharing a single one?

Comment on attachment 406317 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        <https://mail.yahoo.com/> serves the mobile site even in desktop browsing "mode".

There's a bit of missing context here: that the mobile site doesn't work well with mouse events in a particular case.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:570
> +#if ENABLE(IOS_TOUCH_EVENTS)
> +    if (mouseEventPolicy == MouseEventPolicy::Default && document->quirks().shouldSynthesizeTouchEvents())
> +        mouseEventPolicy = MouseEventPolicy::SynthesizeTouchEvents;
> +#endif

Is there a reason we can't tuck this away in the DocumentLoader mouseEventPolicy() getter instead?

Comment on attachment 406317 [details]
Patch

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

>> Source/WebCore/page/Quirks.cpp:501
>> +    return false;
> 
> Can do this more efficiently like so:
> 
>     auto host = m_document->topDocument().url().host();
>     return startsWithLettersIgnoringASCIICase(host, "mail.") && topPrivatelyControlledDomain(host.toString()).startsWith("yahoo.");
> 
> Also, I think we should write an isYahooMail helper function so we can share this logic with Quirks::shouldAvoidPastingImagesAsWebContent. Also, that uses a data member to cache the result. Should we be doing that here too? Or sharing a single one?

A shared function is a good idea I'll do that.

As far as saving this to a variable, I was thinking that this is only called during top-level/frame navigations so it wouldn't happen that often, but perhaps caching it is a good idea.

>> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:570
>> +#endif
> 
> Is there a reason we can't tuck this away in the DocumentLoader mouseEventPolicy() getter instead?

I was trying to follow what `DocumentLoader::simulatedMouseEventsDispatchPolicy` does, but I think your idea is probably cleaner.  I'll change it.

Created attachment 406342 [details]
Patch

Created attachment 406351 [details]
Patch

fix win build

Committed r265485: <https://trac.webkit.org/changeset/265485>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406351 [details].