Bug 168749 – Switch back to the Twitter API for the Tweet widget

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Website

[Jon Davis]

Switch back to using the Twitter API, with the push-tweet-listener as a trigger for updates and fallback if the API fails.

[Jon Davis]

Created attachment 302451 [details]
Patch

[Matt Baker]

Comment on attachment 302451 [details]
Patch

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

r=me, with many a nit!

This is largely a style review and rubber-stamp, as I'm not familiar with the Twitter API. And since PHP is C-like, most of our WebKit coding standards are applicable and should be followed, at least:

- $camelCaseIdentifiers
- No spaces after/before open/close parenthesis in expressions, function signatures, etc
- Function body open brace should start on a newline

> Websites/webkit.org/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

A brief description of the change would be nice.

> Websites/webkit.org/wp-content/themes/webkit/style.css:672
> +    

Style: remove blank line.

> Websites/webkit.org/wp-content/themes/webkit/style.css:682
> +    width: auto;

Style: move width above height, to match the order of the max-width/height pair below.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:28
> +        

Style: remove whitespace.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:35
> +        //     return $this->follow_markup($options);

Remove commented code.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:50
> +            if ( preg_match('!webkit.org/.+?!', $entry->expanded_url) == 1 ) {

Style: no need for spaces after open parenthesis/before close parenthesis.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:60
>  

Style: remove blank line.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:85
> +                <li><a href="https://twitter.com/webkit" target="twitter-modal"><span class="twitter-icon">Twitter</span>@webkit</a></li>

Nice housekeeping.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:121
> +        $PushedTweet = false;

Style: $pushedTweet

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:154
> +        $key = join('&', array_map('rawurlencode', array(

Should be all on one line.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:168
>          );

Style: add a blank line before "// Wrap..."

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:169
> +        // Wrap values in double-quotes

Style: end comments in a period

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:199
> +                && preg_match('!webkit.org/.+?!', $Tweet->entities->urls[0]->expanded_url) == 1 ); // links to webkit.org somewhere

This function could be rewritten to be more readable. My PHP is rusty, but I think `count` returns 0 for null arguments, so the `empty($tweet->entities->urls)` check can be eliminated:

function isWebKitLink($tweet) {
    if (empty($tweet->entities) || count($tweet->entities->urls != 1))
        return false;

    return preg_match('!webkit.org/.+?!', $tweet->entities->urls[0]->expanded_url) == 1;
}

The comments don't add much that isn't obvious by reading the code, and can be omitted.

[Matt Baker]

Mismatched parenthesis in my code above, should be:

if (empty($tweet->entities) || count($tweet->entities->urls) != 1)
    return false;

[Jon Davis]

Created attachment 304087 [details]
Patch

[Lucas Forschler]

Comment on attachment 304087 [details]
Patch

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

r=me after fixing the leading whitespace.

> Websites/webkit.org/wp-content/themes/webkit/widgets/twitter.php:29
> +        

looks like some extra whitespace here

[Jon Davis]

Created attachment 304090 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304090 [details]
Patch

Clearing flags on attachment: 304090

Committed r213740: <http://trac.webkit.org/changeset/213740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.