Bug 42800 – Upstream Android auto hyphenation implemenation to webkit

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[sw]

Upstream Android auto hyphenation implemenation to webkit

[sw]

Created attachment 62323 [details]
Implment auto hyphenation interface for Android.

[Darin Adler]

Comment on attachment 62323 [details]
Implment auto hyphenation interface for Android.

There's a more elegant way to deal with the single hyphenation dictionary.

    static HyphenDict* dictionary = loadHyphenationDictionary();

Since you're only using it at one call site, you can use this there, and not have a "get" function. For one thing we don’t use "get" in our simple getter function names in WebKit. For another, there's no need for a separate boolean. If you did want a get function, it can be written like this.

    static HyphenDict* hyphenationDictionary()
    {
        static HyphenDict* staticDictionary = loadHyphenationDictionary();
        return staticDictionary;
    }

There's no reason for the boolean and to nest the entire function inside an if statement. It's also possibly more efficient to have the one time code out of line in a separate function.

r=me, though, as is.

[sw]

Thanks Darin for the review approval.  Shall I ask you or Steveblock to land the patch?

[Steve Block]

> Thanks Darin for the review approval.  Shall I ask you or Steveblock to land the patch?
I'm happy to cq+ the patch for you. It would be great if you could address Darin's comments first though.

[sw]

I think his comments is for nice-to-have.  So I think I'll keep it as is now.

Comment on attachment 62323 [details]
Implment auto hyphenation interface for Android.

> +#include "NotImplemented.h"

Is this needed?

[sw]

it's needed for the android logging logw/logd.  thanks.

[Steve Block]

Comment on attachment 62323 [details]
Implment auto hyphenation interface for Android.

(In reply to comment #7)
> it's needed for the android logging logw/logd.  thanks.
I'm not sure that we should rely on NotImplemented.h to provide the logging macros for Android, especially as this is not present upstream. WebCore doesn't typically use debug logging like this, but if it's really required, you should probably include the necessary headers directly.

WebCore/ChangeLog:9
 +          output may be different in Android.
Are you suggesting that we need Android-specific expected results for the layout tests? Is it possible to submit those as part of this patch?

[sw]

Created attachment 62610 [details]
Implment auto hyphenation interface for Android.

hi, Steve/Darin/Mitz,

I address all your comments in the new patch.  I created android platform specific results.

Thanks,

[sw]

Created attachment 62611 [details]
Implement auto hyphenation interface for Android.

fixed a style issue for the LayoutTests/ChangeLog in the latest patch.

[WebKit Review Bot]

Attachment 62610 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
LayoutTests/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 62611 [details]
Implement auto hyphenation interface for Android.

> +        No new tests. (OOPS!)

You need to remove this, or we can't land it. The only OOPS is on the reviewer line, because the landing script edits that line. Any other OOPS will prevent the patch from landing.

Patch otherwise seems fine. r=me, but you’ll need to submit a new patch if you want the commit-queue to work.

Comment on attachment 62611 [details]
Implement auto hyphenation interface for Android.

> +    if (!hnj_hyphen_hyphenate(dict, word, length, hyphens))
> +        for (size_t i = beforeIndex - 1; i > 0; --i) {
> +            if (hyphens[i] & 1)
> +                return i + 1;
> +        }

There should be braces around the multi-line if block.

[sw]

Created attachment 62621 [details]
Implement auto hyphenation interface for Android.

Thanks Darin for review.  Removed the test OOPS.

[sw]

Created attachment 62622 [details]
Implement auto hyphenation interface for Android.

added braces per Mitz' review.  Thanks Darin and Mitz for reviewing.

[WebKit Commit Bot]

Comment on attachment 62622 [details]
Implement auto hyphenation interface for Android.

Clearing flags on attachment: 62622

Committed r64087: <http://trac.webkit.org/changeset/64087>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.