Created attachment 158604 [details] Patch Proposed patch.

Comment on attachment 158604 [details] Patch I'm confused. This method walks backwards through styles until it finds colors for each of the 3 types of decoration. They can be different colors. Your code looks like it only works with one color total?

(In reply to comment #2) > (From update of attachment 158604 [details]) > I'm confused. This method walks backwards through styles until it finds colors for each of the 3 types of decoration. They can be different colors. Your code looks like it only works with one color total? The method walks backwards through styles, indeed :) but the color value is the same because all that decorationColor() takes as param is styleToUse, which is the same for all colors on the loop run. So each style (underline, overline, line-through) continues to obtain separated color values as the layout tests runs demonstrates on cr-linux. In resume: the behavior is not changed at all, only optimized.

Created attachment 158625 [details] Patch Moved current object is anonymous block check to preserve behavior.

Comment on attachment 158625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158625&action=review While I like the goal of your change, I think the current form of your patch introduces a potential crash. I think you should stay more like the original code in layout, but incorporate the constant lifting you correctly identified. I'm curious if you did any profiling that identified this code as a bottleneck, or if you ran across this from code inspection? > Source/WebCore/rendering/RenderObject.cpp:2616 > + Color resultColor = decorationColor(styleToUse); I think you should just declare these variables here, and assign in the top of the do loop, as was done previously. This avoids a potential null dereference later. > Source/WebCore/rendering/RenderObject.cpp:-2617 > - int currDecs = styleToUse->textDecoration(); Move assignments here (but keep the change from int to ETextDecoration) > Source/WebCore/rendering/RenderObject.cpp:-2636 > - curr = toRenderBlock(curr)->continuation(); I think you should keep this stanza as-is, rather than the new block below. > Source/WebCore/rendering/RenderObject.cpp:2637 > + styleToUse = curr->style(firstlineStyle); You might have just reassigned curr from the call to continuation in the line above. You should null-check again to avoid faulting here. I think a better flow would be to follow the original logic as outlined in the earlier comments I made.

Comment on attachment 158625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=158625&action=review While I like the goal of your change, I think the current form of your patch introduces a potential crash. I think you should stay more like the original code in layout, but incorporate the constant lifting you correctly identified. I'm curious if you did any profiling that identified this code as a bottleneck, or if you ran across this from code inspection? > Source/WebCore/rendering/RenderObject.cpp:2616 > + Color resultColor = decorationColor(styleToUse); I think you should just declare these variables here, and assign in the top of the do loop, as was done previously. This avoids a potential null dereference later. > Source/WebCore/rendering/RenderObject.cpp:-2617 > - int currDecs = styleToUse->textDecoration(); Move assignments here (but keep the change from int to ETextDecoration) > Source/WebCore/rendering/RenderObject.cpp:-2636 > - curr = toRenderBlock(curr)->continuation(); I think you should keep this stanza as-is, rather than the new block below. > Source/WebCore/rendering/RenderObject.cpp:2637 > + styleToUse = curr->style(firstlineStyle); You might have just reassigned curr from the call to continuation in the line above. You should null-check again to avoid faulting here. I think a better flow would be to follow the original logic as outlined in the earlier comments I made.

I really like the goal of this change, but I think the new code would introduce a potential crash. Please revise as I outline. For this reason, I am setting this r-.

(In reply to comment #7) > I really like the goal of this change, but I think the new code would introduce a potential crash. Please revise as I outline. > > For this reason, I am setting this r-. Hi Brent, thank you for the valuable review, I just got back from vacations and I'll update the patch with your recommendations as soon as possible.

Created attachment 177493 [details] Patch Updated version based on Brent's review.

Comment on attachment 177493 [details] Patch Thanks for revising the code. Looks good! r=me.

Comment on attachment 177493 [details] Patch Rejecting attachment 177493 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: ripts/update-webkit line 152. Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2 Updating OpenSource From http://git.chromium.org/external/Webkit 16b6329..69df7e9 HEAD -> origin/HEAD error: Ref refs/remotes/origin/master is at 69df7e90d827085e08064fe2060b71e2a596ead5 but expected 16b6329a5796e11a418c6e68e9effd8d7e1537bf ! 16b6329..69df7e9 master -> origin/master (unable to update local ref) Died at Tools/Scripts/update-webkit line 152. Full output: http://queues.webkit.org/results/15138245

Comment on attachment 177493 [details] Patch Thanks for the review Brent. The above error sounds like a buildbot internal issue, so trying again.

Comment on attachment 177493 [details] Patch Clearing flags on attachment: 177493 Committed r136617: <http://trac.webkit.org/changeset/136617>

All reviewed patches have been landed. Closing bug.