Regression(r196770): Unable to use HipChat Mac app

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

Unable to use HipChat after <http://trac.webkit.org/changeset/196770>. It basically fails to connect.

[Chris Dumez]

rdar://problem/24931959

[Chris Dumez]

Created attachment 272817 [details]
Patch

[Chris Dumez]

Created attachment 272819 [details]
Patch

[Darin Adler]

Comment on attachment 272819 [details]
Patch

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

> Source/JavaScriptCore/runtime/PutPropertySlot.h:97
> +    void setIsStrictMode(bool value)

I think this can be named just setStrictMode.

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:51
> +static String& applicationBundleIdentifier()

I find the way this function works to be tricky and subtle. The way it works in all processes is a mix of multiple techniques. I think we might need a why comment or two explaining how things work.

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:59
> +#if USE(CF)
> +    if (identifier.get().isNull())
> +        identifier.get() = String(mainBundleIdentifier());
> +#endif

Inelegant to use a null check. Can we do this in a cleaner way? If it wasn’t for the #if it could be:

    static NeverDestroyed<String> identifier(mainBundleIdentifier());

Can we figure out a way to make it something closer to that?

> Source/WebCore/platform/RuntimeApplicationChecks.cpp:165
> +    static const bool isHipChat = applicationBundleIdentifier() == "com.hipchat.HipChat";

I don’t think the const adds anything here.

We might consider renaming mainBundleIsEqualTo to applicationBundleIsEqualTo and handle these additional cases for all the apps. I suppose safer not to do that. I think the distinction is currently unclear. The idea is that these other quirks are all only for WebKit 1 so it’s good to confine these quirks to the main process. On the other hand, this quirk is for WebKit 2 so we want it across all processes. But that is subtle and confusing with the term "main bundle" and "application bundle" being precise about this difference, I suppose, but subtle. Seems like it would be better to just have a single technique or to use more explicit language.

It’s also tricky that setApplicationBundleIdentifier must be called before any of the other functions. Otherwise, they will permanently, inaccurately return false.

[Chris Dumez]

Comment on attachment 272819 [details]
Patch

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

>> Source/JavaScriptCore/runtime/PutPropertySlot.h:97
>> +    void setIsStrictMode(bool value)
> 
> I think this can be named just setStrictMode.

Will rename before landing.

>> Source/WebCore/platform/RuntimeApplicationChecks.cpp:51
>> +static String& applicationBundleIdentifier()
> 
> I find the way this function works to be tricky and subtle. The way it works in all processes is a mix of multiple techniques. I think we might need a why comment or two explaining how things work.

Will add a comment before landing.

>> Source/WebCore/platform/RuntimeApplicationChecks.cpp:59
>> +#endif
> 
> Inelegant to use a null check. Can we do this in a cleaner way? If it wasn’t for the #if it could be:
> 
>     static NeverDestroyed<String> identifier(mainBundleIdentifier());
> 
> Can we figure out a way to make it something closer to that?

Proposing this:
#if USE(CF)
    static NeverDestroyed<String> identifier(mainBundleIdentifier());
#else
    static NeverDestroyed<String> identifier;
#endif

>> Source/WebCore/platform/RuntimeApplicationChecks.cpp:165
>> +    static const bool isHipChat = applicationBundleIdentifier() == "com.hipchat.HipChat";
> 
> I don’t think the const adds anything here.
> 
> We might consider renaming mainBundleIsEqualTo to applicationBundleIsEqualTo and handle these additional cases for all the apps. I suppose safer not to do that. I think the distinction is currently unclear. The idea is that these other quirks are all only for WebKit 1 so it’s good to confine these quirks to the main process. On the other hand, this quirk is for WebKit 2 so we want it across all processes. But that is subtle and confusing with the term "main bundle" and "application bundle" being precise about this difference, I suppose, but subtle. Seems like it would be better to just have a single technique or to use more explicit language.
> 
> It’s also tricky that setApplicationBundleIdentifier must be called before any of the other functions. Otherwise, they will permanently, inaccurately return false.

1. Will drop the const.
2. I agree about the refactoring so that everything would use the applicationBundle. However, I did not want to do it in this patch. I will follow up on this.
3. I will add an assertion to protect against applicationIsHipChat() getting called before setApplicationBundleIdentifier().

[Darin Adler]

All sounds good.

[Chris Dumez]

Created attachment 272829 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 272829 [details]
Patch

Clearing flags on attachment: 272829

Committed r197548: <http://trac.webkit.org/changeset/197548>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 272829 [details]
Patch

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

> Source/WebCore/bindings/js/JSLocationCustom.cpp:91
> +    if (applicationIsHipChat())
> +        slot.setStrictMode(false);

This appears to match how we do it currently, however I'm not sure if I like the new approach better than the old one.

We used to have such policy checks centralized and configured via semantically meaningful preferences. See e.g. _needsPreHTML5ParserQuirks in WebView.mm or Settings::setNeedsKeyboardEventDisambiguationQuirks.

Also, we used to only add quirks for versions linked against old WebKit. The upside was that application authors had to eventually fix their code, and the downside was of course that this was an incredibly passive aggressive way to communicate. I'm not sure whether this was a good thing overall.

[Darin Adler]

(In reply to comment #10)
> Also, we used to only add quirks for versions linked against old WebKit. The
> upside was that application authors had to eventually fix their code, and
> the downside was of course that this was an incredibly passive aggressive
> way to communicate.

I’d like to quibble with something here: Using a linked-on-or-after check is not a "way to communicate". It's a way to safely make an important change without affecting software already in the hands of users. We often make important changes that are not developer-specific in this safer way, not just ones that are app-specific, for example when we added main thread exceptions to WebKit.

It only qualifies "an incredibly passive aggressive way to communicate" if we expect WebKit itself to be our communication tool. We have other tools such as email, iOS and OS X release notes and other documentation, the WebKit blog and more.