| Summary: | Migrate FontCascade's expansion functions to use StringView iterators | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||
| Status: | NEW --- | ||||||||
| Severity: | Normal | CC: | benjamin, cmarcelo, commit-queue | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Myles C. Maxfield
2015-04-03 14:54:57 PDT
Created attachment 250102 [details]
Patch
Comment on attachment 250102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250102&action=review Not sure why compilation is failing. Please don’t land until you resolve that! > Source/WTF/wtf/text/StringView.h:74 > + // Note that these four iterators do not support double increments or double dereferences. These four things that follow are not iterators. These are sort of “collections” that you can then call begin/end on to get iterators. I don’t think the terms “double increments” and “double dereferences” are clear. What you are saying is that these are a certain type of iterator. I believe that what you are saying is that these are input iterators http://en.cppreference.com/w/cpp/concept/InputIterator but further they are even more restricted because you must always dereference and then increment before dereferencing again. I don’t think the phrase “do not support double increments” says that to me. > Source/WTF/wtf/text/StringView.h:515 > +// Iterators I don’t think this is a helpful comment, especially since the classes below are not all iterators. > Source/WTF/wtf/text/StringView.h:516 > +class StringView::CodeUnits { I suggest putting all the class definitions first, and then all the inline functions later. Comment on attachment 250102 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250102&action=review >> Source/WTF/wtf/text/StringView.h:74 >> + // Note that these four iterators do not support double increments or double dereferences. > > These four things that follow are not iterators. These are sort of “collections” that you can then call begin/end on to get iterators. > > I don’t think the terms “double increments” and “double dereferences” are clear. What you are saying is that these are a certain type of iterator. I believe that what you are saying is that these are input iterators http://en.cppreference.com/w/cpp/concept/InputIterator but further they are even more restricted because you must always dereference and then increment before dereferencing again. I don’t think the phrase “do not support double increments” says that to me. Done. >> Source/WTF/wtf/text/StringView.h:515 >> +// Iterators > > I don’t think this is a helpful comment, especially since the classes below are not all iterators. Done. >> Source/WTF/wtf/text/StringView.h:516 >> +class StringView::CodeUnits { > > I suggest putting all the class definitions first, and then all the inline functions later. Done. Out of curiosity, why is this preferred? Created attachment 250111 [details]
Patch for landing
Looks like this patch regresses Layout/line-layout-simple.html about 0.5% |