Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
207553007:
Apply the correct style to first-letter pseudo elements composed of different renderers (Closed)
|
Created:
6 years, 9 months ago by mario.prada Modified:
6 years, 8 months ago CC:
blink-reviews, bemjb+rendering_chromium.org, dsinclair, zoltan1, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, pdr., rune+blink, esprehn Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionApply the correct style to first-letter pseudo elements composed of different renderers
Traverse (in preorder) the whole subtree under a render block with a first-letter selector when
looking for the text renderers which would need to get the apropriate style applied, instead of
only considering the first renderer only, which might not contain all the text that should be
considered as a "first letter" (leading and trailing punctuation should be included too).
The present patch also considers leading spaces, if any, as part of the first-letter element,
to match the behaviour of other renderers such as IE and Opera (Firefox does not do that).
R=eae@chromium.org, leviw@chromium.org
BUG=354403
TEST=Add new test to check first-letter elements composed of different renderers.
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170677
Patch Set 1 #
Total comments: 2
Patch Set 2 : Addressed comments from Eric: refactored code + more testing #
Total comments: 14
Patch Set 3 : Added early return for empty strings + fixed comments #
Total comments: 4
Patch Set 4 : Use a helper struct FirstLetterSearchState to have a cleaner implementation #
Total comments: 16
Patch Set 5 : Use a class FirstLetterFinder and a state machine to process the text renderers #
Total comments: 8
Patch Set 6 : Address comments from the latest review #Messages
Total messages: 24 (0 generated)
Finally, I managed to attach a patch that seems to fix the issue while not introducing any regression in debug or release builds (at least in the Linux environment I tried it). The current patch basically merges behaviour from firstLetterLength() and updateFirstLetter() into a new function findTextRenderersForFirstLetterBlock(), which traverse the subtree looking for all the renderers that should become part of the first-letter pseudoelement, and adding them to a Vector passed from updateFirstLetter, which will be later on traverse to generate the apropriate RenderObjects that will replace the original ones as needed. Last, I also provided a new reftest for this that should cover these use cases, and also updated a couple of test expectations as needed. Please review, thanks!
See an screenshot of blink rendering the newly added test here: https://code.google.com/p/chromium/issues/detail?id=354403#c3
On 2014/03/25 17:27:28, mario.prada wrote: > See an screenshot of blink rendering the newly added test here: > https://code.google.com/p/chromium/issues/detail?id=354403#c3 I don't know this code well enough to review this, Levi or Eric would be more suitable.
On 2014/03/25 19:15:28, eae wrote: > On 2014/03/25 17:27:28, mario.prada wrote: > > See an screenshot of blink rendering the newly added test here: > > https://code.google.com/p/chromium/issues/detail?id=354403#c3 > > I don't know this code well enough to review this, Levi or Eric would be more suitable. Thanks for the feedback anyway, and for suggesting other reviewers for this patch. I understand it's not an easy one to review so I will appreciate any input I might be able to get from anyone that is familiar with this code.
I think we need more testing of first-letter. we've found several bugs in this code.
I'm OK with the idea of being spec-compliant here, but I don't believe we have sufficient testing. It would be nice to have a test which attempted to test all the vaious ifs you're adding. https://codereview.chromium.org/207553007/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBlock.cpp:4189: text = getRendererTextForFirstLetter(currChild); Please break this block out into its own function. 100line functions are hard to read.
https://codereview.chromium.org/207553007/diff/1/Source/core/rendering/Render... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/1/Source/core/rendering/Render... Source/core/rendering/RenderBlock.cpp:4154: static String getRendererTextForFirstLetter(RenderObject* renderer) We don't normally use "get" for getters in Blink style.
Uploaded a new patch set trying to address Eric's comments. See some notes about this new patch below... https://codereview.chromium.org/207553007/diff/130001/LayoutTests/editing/sel... File LayoutTests/editing/selection/extend-by-word-002.html (right): https://codereview.chromium.org/207553007/diff/130001/LayoutTests/editing/sel... LayoutTests/editing/selection/extend-by-word-002.html:68: shouldBe('selection.focusOffset', '5'); This needs to be changed now because the starting point to calculate the offset is no longer inside of a generated RenderText element containing "teak..." (after splitting the original one into 2: "S" + "teak..."), but inside a RenderText element containing the whole "Steak & Ribs" text. Therefore, the expected offset now is 5, not 4 https://codereview.chromium.org/207553007/diff/130001/LayoutTests/fast/css-ge... File LayoutTests/fast/css-generated-content/quote-first-letter-expected.html (right): https://codereview.chromium.org/207553007/diff/130001/LayoutTests/fast/css-ge... LayoutTests/fast/css-generated-content/quote-first-letter-expected.html:3: .line:first-letter { color: red; } I needed to update this to line and remove the <span class="quote"> below because now "'S" will become red because of the first-letter pseudo element used in the test over the following <span> element (and not just the quote): <span> <q>Should not crash or assert and all four quotes should be displayed.</q> </span> https://codereview.chromium.org/207553007/diff/130001/LayoutTests/fast/css/fi... File LayoutTests/fast/css/first-letter-punctuation.html (right): https://codereview.chromium.org/207553007/diff/130001/LayoutTests/fast/css/fi... LayoutTests/fast/css/first-letter-punctuation.html:8: font-size: 26px; I reduced the font size here to make sure the whole get page gets rendered without scrolling https://codereview.chromium.org/207553007/diff/130001/LayoutTests/fast/css/fi... LayoutTests/fast/css/first-letter-punctuation.html:26: <div><q>Test</q></div> I added some more cases in this test too to make sure the code behaves properly when trailing punctuation should not be taken into account, and also with elements that will translate into plain text characters later on (<q>)
It feels very odd to me that "first-letter" doesn't actually just mean one letter, but i guess that's what the spec says. https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4172: if ((leadingSpacesChecked && isSpaceForFirstLetter(text[0])) What if text is empty? https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4176: // Now start looking for valid characters for the firs-letter pseudo element. "firs-letter" https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4184: // We now we finished checking if there are still more characters. We now we? https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4304: // Don't recur. recurse?
https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4163: if (!result && !textRenderer->isBR() && !textRenderer->isWordBreak()) Are you sure !result does what you want? That's checking null, are these empty or null? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4163: if (!result && !textRenderer->isBR() && !textRenderer->isWordBreak()) On 2014/03/27 21:48:11, eseidel wrote: > Are you sure !result does what you want? That's checking null, are these empty > or null? > My intention here was to check null and, if that's the case, fallback to text(). But if I'm getting an empty string after calling originalText() then I believe I should no call text() because that would mean that I valid string was found, despite of it happening to be an empty one (but of course, I might be wrong) https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4172: if ((leadingSpacesChecked && isSpaceForFirstLetter(text[0])) On 2014/03/27 21:45:30, eseidel wrote: > What if text is empty? I think in that case the flow will continue all the way down until the end of this block. Anyway, I agree that perhaps an early return would be a good thing here, so I will add one. https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4176: // Now start looking for valid characters for the firs-letter pseudo element. On 2014/03/27 21:45:30, eseidel wrote: > "firs-letter" English is not my mother tongue, so I won't discuss this :). I'll change it https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4184: // We now we finished checking if there are still more characters. On 2014/03/27 21:45:30, eseidel wrote: > We now we? :) Ok. I'll fix this too, no problem https://codereview.chromium.org/207553007/diff/130001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4304: // Don't recur. On 2014/03/27 21:45:30, eseidel wrote: > recurse? That's not from my patch, I've just moved it around. Anyway, I will change that (to be honest, I don't understand what 'recur' means either)
https://codereview.chromium.org/207553007/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4169: static bool shouldConsiderTextForFirstLetter(const String& text, unsigned& length, bool& leadingSpacesChecked, bool& leadingPunctuationChecked, bool& firstLetterRenderersFound) These three bools combined could be a struct: struct FirstLetterSearchState { FirstLetterSearchState() : length(0), , leadingSpacesChecked(false) , leadingPunctuationChecked(false) , firstLetterRenderersFound(false) {} unsigned length; bool leadingSpacesChecked; bool leadingPunctuationChecked; bool firstLetterRenderersFound; } Might make your bookkeeping slightly easier. https://codereview.chromium.org/207553007/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4233: bool leadingSpacesChecked = false; Is this a state machine? Do we progress through 3 states in the search or can an arbitrary combination of tehse be set at one time?
Thank you very much for the review. I will upload a new patch soon addressing the last comments, and will look for continuing the conversation after that. https://codereview.chromium.org/207553007/diff/150001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4169: static bool shouldConsiderTextForFirstLetter(const String& text, unsigned& length, bool& leadingSpacesChecked, bool& leadingPunctuationChecked, bool& firstLetterRenderersFound) On 2014/03/31 16:12:52, eseidel wrote: > These three bools combined could be a struct: > > struct FirstLetterSearchState { > FirstLetterSearchState() > : length(0), > , leadingSpacesChecked(false) > , leadingPunctuationChecked(false) > , firstLetterRenderersFound(false) > {} > > unsigned length; > bool leadingSpacesChecked; > bool leadingPunctuationChecked; > bool firstLetterRenderersFound; > } > > Might make your bookkeeping slightly easier. Good suggestion. Will do it in the next patch https://codereview.chromium.org/207553007/diff/150001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4233: bool leadingSpacesChecked = false; On 2014/03/31 16:12:52, eseidel wrote: > Is this a state machine? Do we progress through 3 states in the search or can > an arbitrary combination of tehse be set at one time? It's pretty much a state machine, indeed. In a successful scenario (first-letter text found), states would be as follows: S1. You haven't found anything yet (but spaces, maybe) S2. You have found some leading punctuation. Spaces are no allowed from this point on S3. After traversing all the chars considered as punctuation, you found a valid "letter" right after them (no spaces in between). This means that you already could return and consider part of the text found as a first-letter (firstLetterFound would be true at this point) S4. You keep looking in case there is some valid trailing punctuation you would need to include as part of the first-letter element. S5. END state: you are done looking for characters in text renderers, regardless you have found a first-letter element or not So yes, it certainly looks like a state machine with clear transitions :)
If its truly a state machine, an enum would be better than bools. :)
We're going to need to go another round here. You don't have to do the class refactor, but we do need to find more ways to simplify this code. This is super security sensitive code due to teh fact that it breaks the assumption that the rendering tree cannot be modified during layout(). https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4163: if (!result && !textRenderer->isBR() && !textRenderer->isWordBreak()) This is bad that we now have this fragile blacklist in two places! https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4243: static unsigned findTextRenderersForFirstLetterBlock(RenderObject*& firstLetterBlock, Vector<RenderObject*>& renderers) This is starting to feel like a member function on a FirstLetterFinder class, but that can be a later refactoring too. :) https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4252: previousLength = searchState.length; This is a bit awkward, and probably just belongs as a member on state, especially if it becomes a class. Should this search just be producing a vector of objects and a vector of lengths and we're just looking at lengths[-1] when we need it later? https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4254: // shouldConsiderTextForFirstLetter() will set the right values for the 'length' variable and those booleans. Probably needs adjustment now that it takes a struct instead of bools? https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4265: if (!searchState.length || searchState.length < text.length()) Should this be searcher.shouldSearchMore(text)? https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4277: // If found a floating/out-of-flow element with the first_letter I'm not sur eyou mean "first_letter"? https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4320: // FIXME: We need to destroy the first-letter object if it is no longer the first child. Need to find What are the consequences of this FIXME? Leaks? Two renderers thinking they're first letter? Can we just check renderer->previousSibling()? :first-letter has been a non-ending source of cluzerfuzz crashes over the years. :( Any time we touch this code we likely introduce security bugs, so we need to be very careful. It's one of very few places where we modify the rendering tree during layout (which is generally not allowed!) https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4347: if (!currentRenderer->isText() || currentRenderer->isBR() || textRenderer->isWordBreak()) We need to share this code with the other caller. we cna't copy-paste this already bad code.
Thanks a lot for the careful review. I will try to address all your comments today, and will also consider the previous comment about using an enum for the state machine. https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4163: if (!result && !textRenderer->isBR() && !textRenderer->isWordBreak()) On 2014/04/01 06:05:43, eseidel wrote: > This is bad that we now have this fragile blacklist in two places! True and sorry. I can certainly take those checks out to a helper function (e.g. isTextRendererValidForFirstLetter()) to help this under control, if that helps. While I agree that does not fix the issue, I guess it's the best I can do without knowing the real reasoning behind the original blacklist. Perhaps you have any hint? https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4243: static unsigned findTextRenderersForFirstLetterBlock(RenderObject*& firstLetterBlock, Vector<RenderObject*>& renderers) On 2014/04/01 06:05:43, eseidel wrote: > This is starting to feel like a member function on a FirstLetterFinder class, > but that can be a later refactoring too. :) Yes :). However, I'd rather keep it simpler for now if possible, as I guess that should probably help to make the patch smaller and hopefully easier to review. It will make it easier to port it to WebKit too, which is a plus for me as well (even though I understand that might not be a key point to consider) https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4252: previousLength = searchState.length; On 2014/04/01 06:05:43, eseidel wrote: > This is a bit awkward, and probably just belongs as a member on state, > especially if it becomes a class. Should this search just be producing a vector > of objects and a vector of lengths and we're just looking at lengths[-1] when we > need it later? I can't do anything but agreeing with you one more time here. This is the result of "just" applying your suggestion of creating a struct quickly, without going into deeper thinking about it. I'll re-do my patch today considering all the comments here and will hopefully provide a better patch by the end of the day https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4254: // shouldConsiderTextForFirstLetter() will set the right values for the 'length' variable and those booleans. On 2014/04/01 06:05:43, eseidel wrote: > Probably needs adjustment now that it takes a struct instead of bools? Sure https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4265: if (!searchState.length || searchState.length < text.length()) On 2014/04/01 06:05:43, eseidel wrote: > Should this be searcher.shouldSearchMore(text)? Could be. I'll consider it for my next patch https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4277: // If found a floating/out-of-flow element with the first_letter On 2014/04/01 06:05:43, eseidel wrote: > I'm not sur eyou mean "first_letter"? It's a typo, sorry. Will fix it https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4320: // FIXME: We need to destroy the first-letter object if it is no longer the first child. Need to find On 2014/04/01 06:05:43, eseidel wrote: > What are the consequences of this FIXME? Leaks? Two renderers thinking they're > first letter? > > Can we just check renderer->previousSibling()? > > :first-letter has been a non-ending source of cluzerfuzz crashes over the years. > :( Any time we touch this code we likely introduce security bugs, so we need to > be very careful. It's one of very few places where we modify the rendering tree > during layout (which is generally not allowed!) This specific chunk is not new in my patch, so I'm not really sure about how to answer that question right now. What I can do, though is to try your suggestion (which makes sense to me too, btw) and run all the tests to see if it impacts them in any way or not. https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4347: if (!currentRenderer->isText() || currentRenderer->isBR() || textRenderer->isWordBreak()) On 2014/04/01 06:05:43, eseidel wrote: > We need to share this code with the other caller. we cna't copy-paste this > already bad code. I'll do. See my comment above
I'll be uploading a new patch in a couple of minutes, but let me first make a couple of comments over my previous answers, since I have new info now: On 2014/04/01 12:56:08, mario.prada wrote: > [...] > https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... > Source/core/rendering/RenderBlock.cpp:4243: static unsigned > findTextRenderersForFirstLetterBlock(RenderObject*& firstLetterBlock, > Vector<RenderObject*>& renderers) > On 2014/04/01 06:05:43, eseidel wrote: > > This is starting to feel like a member function on a FirstLetterFinder class, > > but that can be a later refactoring too. :) > > Yes :). However, I'd rather keep it simpler for now if possible, as I guess that > should probably help to make the patch smaller and hopefully easier to review. > > It will make it easier to port it to WebKit too, which is a plus for me as well > (even though I understand that might not be a key point to consider) > In the end, I couldn't help but doing this refactor now, since it did feel like much cleaner indeed, and hopefully won't make it any harder (without me expecting it to be easy either) to eventually port the patch to WebKit. You'll see the change in the next patch. > https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... > Source/core/rendering/RenderBlock.cpp:4252: previousLength = searchState.length; > On 2014/04/01 06:05:43, eseidel wrote: > > This is a bit awkward, and probably just belongs as a member on state, > > especially if it becomes a class. Should this search just be producing a > vector > > of objects and a vector of lengths and we're just looking at lengths[-1] when > we > > need it later? > > I can't do anything but agreeing with you one more time here. This is the result > of "just" applying your suggestion of creating a struct quickly, without going > into deeper thinking about it. > > I'll re-do my patch today considering all the comments here and will hopefully > provide a better patch by the end of the day > In the new patch, I got rid of this weird thing by better encapsulating the functionality inside a FirstLetterFinder class, and also by storing a pair<RenderObject*, unsigned> in that Vector (containing the text renderer found and the length of the associated text), so we do not need to calculate the text length for any element in the Vector later on but for the last one, for which we would be using the last length stored during the search. I do believe that, while we'd be storing and additional unsigned for each rendered found that could be calculated in most cases, the code is much cleaner and self-contained now. That, combined with the fact that I don't think we will normally see a big number of elements in that Vector, made me think this option would be better than returning a Vector of Render Objects + the length of the text for the last element (more confusing, IMHO). Please let me know what you think. > [...] > https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... > Source/core/rendering/RenderBlock.cpp:4265: if (!searchState.length || > searchState.length < text.length()) > On 2014/04/01 06:05:43, eseidel wrote: > > Should this be searcher.shouldSearchMore(text)? > > Could be. I'll consider it for my next patch > I think in the end it's not needed since the comment should be fine and now everything is more self contained inside that class. But I will happily change it if you don't agree, of course. > [...] > https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... > Source/core/rendering/RenderBlock.cpp:4320: // FIXME: We need to destroy the > first-letter object if it is no longer the first child. Need to find > On 2014/04/01 06:05:43, eseidel wrote: > > What are the consequences of this FIXME? Leaks? Two renderers thinking > they're > > first letter? > > > > Can we just check renderer->previousSibling()? > > > > :first-letter has been a non-ending source of cluzerfuzz crashes over the > years. > > :( Any time we touch this code we likely introduce security bugs, so we need > to > > be very careful. It's one of very few places where we modify the rendering > tree > > during layout (which is generally not allowed!) > > This specific chunk is not new in my patch, so I'm not really sure about how to > answer that question right now. What I can do, though is to try your suggestion > (which makes sense to me too, btw) and run all the tests to see if it impacts > them in any way or not. > I've tried this and makes a whole bunch of tests to fail. In any case, we still need the firstLetterBlock element returned by findFirstLetterBlock(), so I don't think replacing that with previousSibling() would be correct anyway. > https://codereview.chromium.org/207553007/diff/170001/Source/core/rendering/R... > Source/core/rendering/RenderBlock.cpp:4347: if (!currentRenderer->isText() || > currentRenderer->isBR() || textRenderer->isWordBreak()) > On 2014/04/01 06:05:43, eseidel wrote: > > We need to share this code with the other caller. we cna't copy-paste this > > already bad code. > > I'll do. See my comment above Done
lgtm I think this is OK. I'm sure we could iterate more here. https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4285: while (length < textLength && isPunctuationForFirstLetter(text[length])) This is a "skipWhile<foo>()" kind of helper which we use in other parsers: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4288: if (length < textLength) Is it always true that if length < textLength at the end of the while loop we set m_state += 1? https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4313: default: You can remove this, then the compiler will enforce that all cases are handled. https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4318: return length; Do we need to ASSERT(length <= textLength); here? Presumably other parts of the code will blow up if that's not true?
On 2014/04/01 17:50:37, eseidel wrote: > lgtm > > I think this is OK. I'm sure we could iterate more here. > Thanks a lot for the quick and helpful review, I'll address all those comments in the next patch set. https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... File Source/core/rendering/RenderBlock.cpp (right): https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4285: while (length < textLength && isPunctuationForFirstLetter(text[length])) On 2014/04/01 17:50:37, eseidel wrote: > This is a "skipWhile<foo>()" kind of helper which we use in other parsers: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Thanks for the suggestion. Will re-write that part in that way in my next patch https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4288: if (length < textLength) On 2014/04/01 17:50:37, eseidel wrote: > Is it always true that if length < textLength at the end of the while loop we > set m_state += 1? Yes, although in the third state it's not as explicit as in the previous ones, since there's no while loop before updating the state, but length will be obviously less than textLength. Otherwise, the main while loop would not have continued after the previous iteration. To be honest, I thought of refactoring this in some way in this patch (e.g. doing the state update after the switch statement), but in the end I thought it would be clearer this way, so that's why I left it. That said, I don't mind changing it if you think it would be clearer in the other way. https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4313: default: On 2014/04/01 17:50:37, eseidel wrote: > You can remove this, then the compiler will enforce that all cases are handled. Ok. https://codereview.chromium.org/207553007/diff/190001/Source/core/rendering/R... Source/core/rendering/RenderBlock.cpp:4318: return length; On 2014/04/01 17:50:37, eseidel wrote: > Do we need to ASSERT(length <= textLength); here? Presumably other parts of the > code will blow up if that's not true? Right, I will add it in the next patch.
I think we should try this and iterate from here. lgtm.
On 2014/04/02 17:17:40, eseidel wrote: > I think we should try this and iterate from here. lgtm. Thanks. I will keep a look on this code after landing, will file new bugs if I find any
The CQ bit was checked by mario.prada@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mario.prada@samsung.com/207553007/200001
Message was sent while issue was closed.
Change committed as 170677 |
