Bug 154999 - Regression(r196770): Unable to use HipChat Mac app
Summary: Regression(r196770): Unable to use HipChat Mac app
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac Unspecified
: P1 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 154396
  Show dependency treegraph
 
Reported: 2016-03-03 18:42 PST by Chris Dumez
Modified: 2016-03-05 17:52 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.82 KB, patch)
2016-03-03 19:14 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.85 KB, patch)
2016-03-03 19:34 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.47 KB, patch)
2016-03-03 21:11 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-03-03 18:42:26 PST
Unable to use HipChat after <http://trac.webkit.org/changeset/196770>. It basically fails to connect.
Comment 1 Chris Dumez 2016-03-03 18:42:45 PST
rdar://problem/24931959
Comment 2 Chris Dumez 2016-03-03 19:14:36 PST
Created attachment 272817 [details]
Patch
Comment 3 Chris Dumez 2016-03-03 19:34:20 PST
Created attachment 272819 [details]
Patch
Comment 4 Darin Adler 2016-03-03 20:23:19 PST
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.
Comment 5 Chris Dumez 2016-03-03 20:53:15 PST
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().
Comment 6 Darin Adler 2016-03-03 21:04:08 PST
All sounds good.
Comment 7 Chris Dumez 2016-03-03 21:11:45 PST
Created attachment 272829 [details]
Patch
Comment 8 WebKit Commit Bot 2016-03-03 21:59:53 PST
Comment on attachment 272829 [details]
Patch

Clearing flags on attachment: 272829

Committed r197548: <http://trac.webkit.org/changeset/197548>
Comment 9 WebKit Commit Bot 2016-03-03 21:59:59 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Alexey Proskuryakov 2016-03-05 15:06:11 PST
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.
Comment 11 Darin Adler 2016-03-05 17:52:38 PST
(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.