Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9111042:
Fix RTL and complex script title in print preview header/footer. (Closed)
|
Created:
8 years, 11 months ago by arthurhsu Modified:
8 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org, kmadhusu, xji, Alexei Svitkine (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix RTL and complex script title in print preview header/footer.
Mac fixes is not there yet since RenderText does not support Mac for now.
BUG=108599
TEST=print preview pages in different languages
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135750
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use RenderText instead #
Total comments: 3
Patch Set 3 : User RenderText, Windows only solution #
Total comments: 20
Patch Set 4 : Update per code review #
Total comments: 6
Patch Set 5 : Update per code review #
Total comments: 6
Patch Set 6 : Update per code review #
Total comments: 4
Patch Set 7 : Update per code review #Messages
Total messages: 27 (0 generated)
arthurhsu: By looking at the code, I think vandebo@ is the better reviewer to review this CL. (He reviewed the initial header footer text CL and he is more familiar with skia code). -kmadhusu@ from reviewer list. +vandebo@ to reviewer list. Thanks.
I would be surprised if there wasn't already code to do this.
On 2012/01/06 18:21:35, vandebo wrote: > I would be surprised if there wasn't already code to do this. I've found two places implementing similar functionality but not really reusable. First is render_text.h. I had a version to reuse it but it simply error out when I call RenderText::CreateRenderText(). I was quite surprised and become more surprised when codesearch showed nobody using that code. The other route is use WebKit to render it but I wondered its feasibility. It requires creating a GraphicsContext to draw the text, and I'm not too sure we can do that or not.
http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.cc:227: std::vector<uint16_t> glyphs; Just using an array of glyphs does not work well because the glyph position is also critical. Moreover, the input string may not be rendered with a single font. In that case, each glyph id has to be associated with a font as well. As you wrote offline, I think using RenderText is a way to go (after resolving issues you came across while trying to use it). http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper.h (right): http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper.h:103: std::vector<uint16_t>* glyphs); As I wrote elsewhere, just getting an array of glyph is not sufficient. Positions (xoffset and yoffset) and fonts (when multiple fonts are necessary for drawing the input string) are necessary as well. http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:338: SCRIPT_ANALYSIS PrintWebViewHelper::g_script_analysis_; In the past, we used to have UniscribeHelper class in chrome's tree instead of Webkit expecting this kind of 'non-Webkit' usage. A few years ago, we moved inside Webkit. I wonder if we have to take it out again (eventually). @xji: what's your plan for RenderText on Windows? http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:421: After calling ScriptShape, ScriptPlace (?) has to be called as well.
http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... File chrome/renderer/print_web_view_helper_win.cc (right): http://codereview.chromium.org/9111042/diff/1/chrome/renderer/print_web_view_... chrome/renderer/print_web_view_helper_win.cc:338: SCRIPT_ANALYSIS PrintWebViewHelper::g_script_analysis_; On 2012/01/06 22:38:45, Jungshik Shin wrote: > In the past, we used to have UniscribeHelper class in chrome's tree instead of > Webkit expecting this kind of 'non-Webkit' usage. A few years ago, we moved > inside Webkit. > > I wonder if we have to take it out again (eventually). > > @xji: what's your plan for RenderText on Windows? Mike (msw@) implemented RenderText in Windows. The ConvertTextToGlyphs is very similar to RenderTextWin::EnsureLayout(). It would be nice and the way to go to use RenderText instead of duplicating the code for Win and Linux. (We do not have similar implementation in Mac though). I will need to check why it fails in Windows.
I've updated the implementation to use RenderText instead, so please treat Patch Set #2 as a completely new review. I did not close this issue because I'd like to keep comments on Patch #1.
http://codereview.chromium.org/9111042/diff/11001/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/11001/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:408: gfx::Rect rect(0, y, device_size.width(), height); As far as I can tell, this moves where the headers and footers are drawn and doesn't take the webkit scale factor into consideration. Grab me to explain if I'm misunderstanding. http://codereview.chromium.org/9111042/diff/11001/ui/gfx/render_text_win.cc File ui/gfx/render_text_win.cc (right): http://codereview.chromium.org/9111042/diff/11001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:126: LOG(WARNING) << "ScriptRecordDigitSubstitution failed " << hr Is it because of the sandbox? Does that mean this code isn't safe to run in the sandbox? A Log isn't a good solution to the known. http://codereview.chromium.org/9111042/diff/11001/ui/gfx/render_text_win.cc#n... ui/gfx/render_text_win.cc:131: hr = ScriptApplyDigitSubstitution(&digit_substitute_, Why is is ok to not so this in the sandbox?
For this CL, we will be focus on Windows only solution. I've already filed other issues to break down the CL. See crbug.com/125659 Make RenderText to be able to run in sandbox on Linux crbug.com/125660 RenderText does not draw Korean characters correctly in renderer crbug.com/125661 RenderText has different painting behavior cross platform crbug.com/125664 RenderText support for Mac
http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:429: scoped_ptr<gfx::Canvas> gfx_canvas(new gfx::Canvas(canvas)); I'm wondering whether you've tried using Canvas::DrawString() directly, which (as of M20), uses RenderText. Might save you the trouble of managing your own RenderText instance.
http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:429: scoped_ptr<gfx::Canvas> gfx_canvas(new gfx::Canvas(canvas)); Actually, I tried not to use it for debugging purposes (since I can easily watch my own var in debugger, not going deep into statics/other object's members). As mentioned in e-mail, we have painting issues on Linux and I tend to have this as-is for now. On 2012/05/01 14:56:39, Alexei Svitkine wrote: > I'm wondering whether you've tried using Canvas::DrawString() directly, which > (as of M20), uses RenderText. Might save you the trouble of managing your own > RenderText instance.
On 2012/05/01 17:26:37, arthurhsu wrote: > http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... > File chrome/renderer/print_web_view_helper.cc (right): > > http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... > chrome/renderer/print_web_view_helper.cc:429: scoped_ptr<gfx::Canvas> > gfx_canvas(new gfx::Canvas(canvas)); > Actually, I tried not to use it for debugging purposes (since I can easily watch > my own var in debugger, not going deep into statics/other object's members). As > mentioned in e-mail, we have painting issues on Linux and I tend to have this > as-is for now. > > On 2012/05/01 14:56:39, Alexei Svitkine wrote: > > I'm wondering whether you've tried using Canvas::DrawString() directly, which > > (as of M20), uses RenderText. Might save you the trouble of managing your own > > RenderText instance. Drive-by recommendation to try following Alexei's advice. It seems like abstraction here and fixing any legit bugs with Canvas/RenderText would be a better approach (as long as it's not prohibitively difficult).
On 2012/05/01 17:50:27, msw wrote: > On 2012/05/01 17:26:37, arthurhsu wrote: > > > http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... > > File chrome/renderer/print_web_view_helper.cc (right): > > > > > http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... > > chrome/renderer/print_web_view_helper.cc:429: scoped_ptr<gfx::Canvas> > > gfx_canvas(new gfx::Canvas(canvas)); > > Actually, I tried not to use it for debugging purposes (since I can easily > watch > > my own var in debugger, not going deep into statics/other object's members). > As > > mentioned in e-mail, we have painting issues on Linux and I tend to have this > > as-is for now. > > > > On 2012/05/01 14:56:39, Alexei Svitkine wrote: > > > I'm wondering whether you've tried using Canvas::DrawString() directly, > which > > > (as of M20), uses RenderText. Might save you the trouble of managing your > own > > > RenderText instance. > > Drive-by recommendation to try following Alexei's advice. It seems like > abstraction here and fixing any legit bugs with Canvas/RenderText would be a > better approach (as long as it's not prohibitively difficult). Take a second look into this code (sorry it was done a week ago and I had serious memory leak), it actually relies on RenderText to provide width of the string to be painted in order to calculate the exact coordinates of the DisplayRect that will be passed in later. The gfx::Canvas::DrawStringInt() does not offer similar functionalities. I need to calculate the coordinates using existing code to make sure it paints at the same (or closest) coordinates across platforms/configurations.
On Tue, May 1, 2012 at 5:59 PM, <arthurhsu@chromium.org> wrote: > On 2012/05/01 17:50:27, msw wrote: >> >> On 2012/05/01 17:26:37, arthurhsu wrote: >> > > > > http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... >> >> > File chrome/renderer/print_web_view_helper.cc (right): >> > >> > > > > http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... >> >> > chrome/renderer/print_web_view_helper.cc:429: scoped_ptr<gfx::Canvas> >> > gfx_canvas(new gfx::Canvas(canvas)); >> > Actually, I tried not to use it for debugging purposes (since I can >> > easily >> watch >> > my own var in debugger, not going deep into statics/other object's >> > members). > > >> As >> > mentioned in e-mail, we have painting issues on Linux and I tend to have > > this >> >> > as-is for now. >> > >> > On 2012/05/01 14:56:39, Alexei Svitkine wrote: >> > > I'm wondering whether you've tried using Canvas::DrawString() >> > > directly, >> which >> > > (as of M20), uses RenderText. Might save you the trouble of managing >> > > your >> own >> > > RenderText instance. > > >> Drive-by recommendation to try following Alexei's advice. It seems like >> abstraction here and fixing any legit bugs with Canvas/RenderText would be >> a >> better approach (as long as it's not prohibitively difficult). > > > Take a second look into this code (sorry it was done a week ago and I had > serious memory leak), it actually relies on RenderText to provide width of > the string to be painted in order to calculate the exact coordinates of the > DisplayRect that will be passed in later. Okay, that's reasonable reason to use RenderText. I just wanted to make sure you're not taking a complicated approach when a simple one would suffice. I can see that the simple one doesn't suffice here. ;)
Here are my initial comments. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:413: #if defined(OS_WIN) It might be a good idea to refactor this to a separate function and call it from PrintHeaderFooterText(). Otherwise, this is making PrintHeaderFooterText() quite messy. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:417: DCHECK(render_text.get()); What does this DCHECK() buy you? If it's NULL, then the next line will just crash - which you'll notice just as well, if not better than a DCHECK(). http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:421: int font_height = render_text->GetStringSize().height() / webkit_scale_factor; It would be more accurate to name this |text_height|, since there may be multiple fonts involved in a single line of text (multiple runs using different fonts). http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:429: scoped_ptr<gfx::Canvas> gfx_canvas(new gfx::Canvas(canvas)); I suggest moving the gfx_canvas declaration to right before where you actually use it (after all the calculations - just above the draw call. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:430: int text_width = render_text->GetStringSize().width() / webkit_scale_factor; Suggest just calling GetStringSize() once and storing the Size returned as a local variable. (You're calling it here and above.) http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:450: gfx_canvas.reset(); Is it necessary to explicitly reset it here? You're using a scoped_ptr already, so it will be freed when going out of scope. Looking at the gfx::Canvas destructor, it's empty - so I don't think there's any reason to explicitly reset it.
http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:421: int font_height = render_text->GetStringSize().height() / webkit_scale_factor; Can you explain the webkit_scale_factor? It seems to me you're getting the metrics from RenderText and modifying them by this factor. But then, when you go to draw the render text, I don't see this factor being used. So it seems incorrect to me - since you're drawing in a way that doesn't take the factor into account, but calculating metrics in a way that does. Maybe I'm missing something here - can you explain this? http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:438: if (point.y() - y_offset + font_height < margin_top + 1) { I think this logic can be written in a way that's less confusing. It looks like what you're testing for is that the text bottom is outside the margin. Can you express that logic in a more readable way? (i.e. keeping local variables that are well named - e.g. text_bottom = point.y() - y_offset + text_height - although perhaps you should apply y_offset to point.y() before this, then you can have text_bottom = point.y() + text_height).
http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:421: int font_height = render_text->GetStringSize().height() / webkit_scale_factor; On 2012/05/02 15:23:58, Alexei Svitkine wrote: > Can you explain the webkit_scale_factor? > > It seems to me you're getting the metrics from RenderText and modifying them by > this factor. But then, when you go to draw the render text, I don't see this > factor being used. it re-SetFontSize() based on this factor on line 420.
http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:413: #if defined(OS_WIN) On 2012/05/02 15:04:05, Alexei Svitkine wrote: > It might be a good idea to refactor this to a separate function and call it from > PrintHeaderFooterText(). Otherwise, this is making PrintHeaderFooterText() quite > messy. Done. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:417: DCHECK(render_text.get()); On 2012/05/02 15:04:05, Alexei Svitkine wrote: > What does this DCHECK() buy you? If it's NULL, then the next line will just > crash - which you'll notice just as well, if not better than a DCHECK(). Done. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:421: int font_height = render_text->GetStringSize().height() / webkit_scale_factor; On 2012/05/02 15:04:05, Alexei Svitkine wrote: > It would be more accurate to name this |text_height|, since there may be > multiple fonts involved in a single line of text (multiple runs using different > fonts). Done. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:429: scoped_ptr<gfx::Canvas> gfx_canvas(new gfx::Canvas(canvas)); On 2012/05/02 15:04:05, Alexei Svitkine wrote: > I suggest moving the gfx_canvas declaration to right before where you actually > use it (after all the calculations - just above the draw call. Done. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:430: int text_width = render_text->GetStringSize().width() / webkit_scale_factor; On 2012/05/02 15:04:05, Alexei Svitkine wrote: > Suggest just calling GetStringSize() once and storing the Size returned as a > local variable. (You're calling it here and above.) Done. http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:438: if (point.y() - y_offset + font_height < margin_top + 1) { I've added more comments on this. On 2012/05/02 15:23:58, Alexei Svitkine wrote: > I think this logic can be written in a way that's less confusing. > > It looks like what you're testing for is that the text bottom is outside the > margin. Can you express that logic in a more readable way? (i.e. keeping local > variables that are well named - e.g. text_bottom = point.y() - y_offset + > text_height - although perhaps you should apply y_offset to point.y() before > this, then you can have text_bottom = point.y() + text_height). http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:450: gfx_canvas.reset(); I do not wish to assume that dtor of gfx::Canvas does nothing in the future (say, it can call restoreToCount()). Force releasing it guarantees that my subsequent restoreToCount() does not have consequences. On 2012/05/02 15:04:05, Alexei Svitkine wrote: > Is it necessary to explicitly reset it here? You're using a scoped_ptr already, > so it will be freed when going out of scope. > > Looking at the gfx::Canvas destructor, it's empty - so I don't think there's any > reason to explicitly reset it.
http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:450: gfx_canvas.reset(); On 2012/05/03 00:28:44, arthurhsu wrote: > I do not wish to assume that dtor of gfx::Canvas does nothing in the future > (say, it can call restoreToCount()). Force releasing it guarantees that my > subsequent restoreToCount() does not have consequences. If you want to be that paranoid, then I suggest using an inner scope. Also, you should just stack allocate gfx::Canvas. e.g.: { gfx::Canvas gfx_canvas(canvas); render_text->Draw(&gfx_canvas); } http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:418: int text_height = text_size.height() / webkit_scale_factor; The scaling still looks incorrect to me. You take "printing::kSettingHeaderFooterFontSize" and divide it by webkit_scale_factor and set that as the font size. Then you get the text_size from render_text and divide the height from that again by webkit_scale_factor. That looks wrong - you're dividing both the input and the output by it - essentially it seems like you're dividing it twice. Did you mean to multiply the output instead? http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:425: canvas->translate(-margin_left, -margin_top); Move these two canvas operations further down, just before actually drawing the text. http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:436: if (point.y() - y_offset + text_height < margin_top + 1) { I still think this can made clearer by the using of aptly named intermediate variables (see my previous comment).
http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/17002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:450: gfx_canvas.reset(); On 2012/05/03 19:04:41, Alexei Svitkine wrote: > On 2012/05/03 00:28:44, arthurhsu wrote: > > I do not wish to assume that dtor of gfx::Canvas does nothing in the future > > (say, it can call restoreToCount()). Force releasing it guarantees that my > > subsequent restoreToCount() does not have consequences. > > If you want to be that paranoid, then I suggest using an inner scope. Also, you > should just stack allocate gfx::Canvas. e.g.: > > { > gfx::Canvas gfx_canvas(canvas); > render_text->Draw(&gfx_canvas); > } Done. http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:418: int text_height = text_size.height() / webkit_scale_factor; Ah yeah nice catch. The division by webkit_scale_factor is to transform context coordinates to skia coordinates, so the text_size got from RenderText does not need to be transformed again since it's already in skia coordinates. On 2012/05/03 19:04:41, Alexei Svitkine wrote: > The scaling still looks incorrect to me. > > You take "printing::kSettingHeaderFooterFontSize" and divide it by > webkit_scale_factor and set that as the font size. > > Then you get the text_size from render_text and divide the height from that > again by webkit_scale_factor. > > That looks wrong - you're dividing both the input and the output by it - > essentially it seems like you're dividing it twice. Did you mean to multiply the > output instead? http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:425: canvas->translate(-margin_left, -margin_top); On 2012/05/03 19:04:41, Alexei Svitkine wrote: > Move these two canvas operations further down, just before actually drawing the > text. Done. http://codereview.chromium.org/9111042/diff/29002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:436: if (point.y() - y_offset + text_height < margin_top + 1) { On 2012/05/03 19:04:41, Alexei Svitkine wrote: > I still think this can made clearer by the using of aptly named intermediate > variables (see my previous comment). Done.
Thanks, looking much better. Can you also add manual test instructions to the TEST= line? http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:428: SkScalarToDouble(text_width)); Looking at other callers of this function, I don't think text_width that's passed into this function should be divided by webkit_scale_factor either. http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:448: gfx::Rect rect(point.x(), point.y() - y_offset, text_width, text_height); The text_width passed into here shouldn't be scaled either, I think. http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:451: render_text->SetDisplayRect(rect); Nit: move this line right after the gfx::Rect declaration.
http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:428: SkScalarToDouble(text_width)); On 2012/05/07 17:48:18, Alexei Svitkine wrote: > Looking at other callers of this function, I don't think text_width that's > passed into this function should be divided by webkit_scale_factor either. Done. http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:448: gfx::Rect rect(point.x(), point.y() - y_offset, text_width, text_height); On 2012/05/07 17:48:18, Alexei Svitkine wrote: > The text_width passed into here shouldn't be scaled either, I think. Done. http://codereview.chromium.org/9111042/diff/36002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:451: render_text->SetDisplayRect(rect); On 2012/05/07 17:48:18, Alexei Svitkine wrote: > Nit: move this line right after the gfx::Rect declaration. Done.
http://codereview.chromium.org/9111042/diff/43002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/43002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:448: gfx::Rect rect(point.x(), point.y() - y_offset, text_width, text_height); (Move it below this line.) http://codereview.chromium.org/9111042/diff/43002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:453: render_text->SetDisplayRect(rect); I was suggesting you move it to after the gfx::Rect declaration above.
http://codereview.chromium.org/9111042/diff/43002/chrome/renderer/print_web_v... File chrome/renderer/print_web_view_helper.cc (right): http://codereview.chromium.org/9111042/diff/43002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:448: gfx::Rect rect(point.x(), point.y() - y_offset, text_width, text_height); On 2012/05/07 18:22:43, Alexei Svitkine wrote: > (Move it below this line.) Done. http://codereview.chromium.org/9111042/diff/43002/chrome/renderer/print_web_v... chrome/renderer/print_web_view_helper.cc:453: render_text->SetDisplayRect(rect); On 2012/05/07 18:22:43, Alexei Svitkine wrote: > I was suggesting you move it to after the gfx::Rect declaration above. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/arthurhsu@chromium.org/9111042/44005
Change committed as 135750 |
